Re: [sipcore] WGLC draft-ietf-sipcore-sip-token-authnz

[Paul Kyzivat <[e-mail]>]

On 12/13/19 9:02 AM, Rifaat Shekh-Yusef wrote:
> 
> 
> On Sun, Dec 8, 2019 at 5:17 PM Paul Kyzivat <[e-mail] 
> <mailto:[e-mail]>> wrote:

>     I'm assuming responses to my other points will be coming later.
> 
> Christer provided feedback on the rest of you comments. Do you have any 
> further comment on Christer's responses?

Hmm. I can find no evidence that I received Christer's reply. But I have 
now found it in the archives. I will reply based on the text from the 
archive. I'm sorry that this isn't properly threaded into the replies.

On 12/13/19 9:02 AM, Christer Holmberg wrote:

> Hi Paul,
> 
> My comments on some of your SIP-related issues (I am not addressing your issues on Sections 2.1.1 and 2.1.2 in this reply). 
> 
>>    * Section 2.1.4:
>>    
>>    This section says that the UAC MUST include an Authorization header 
>>    field with the Bearer scheme. This is overly strong.
>>    
>>    This should be an explanation that *if* it has received a challenge 
>>    containing the Bearer scheme then to resolve that particular challenge 
>>    it needs to send a request with an Authorization header field containing 
>>    the response to that challenge, including the Bearer scheme.
>   
> I suggest that we expand the first paragraph in the section. Something like:
> 
> "The procedures in this section apply when the UAC has received a challenge that contains a Bearer scheme, 
> and the UAC has obtained a token as specified in section Section 2.1.1."

OK

>>    (But note that if there were multiple challenges with different schemes 
>>    then it maybe able to successfully retry the request using non-Bearer 
>>    credentials.)
>>    
>>    I don't understand what distinction you are trying to draw by 
>>    distinguishing the behavior for creating a binding from that for 
>>    refreshing a binding. AFAIK there should be no difference. And in fact 
>>    the UAC in general cannot know whether the request being send will 
>>    establish or refresh a binding.
>   
> It is important to point out that the same access token might not necessarily be used throughout the lifetime of the registration, if the previously included access token has expired.

Fine. It is good to cover that. But it shouldn't be conflated with 
creating vs. refreshing a binding. For instance, the UAC may have 
knowingly allowed a registration to expire and yet still cached the 
credentials (tokens). Then later it might want to reestablish a 
registration using cached credentials. At that time it can decide if its 
access token is still good, and if not use its refresh token to get a 
fresh one, and then either way use that preemptively in its register 
request.

> ---
>  
>>    * Section 2.1.5:
>>    
>>    This section has similar issues to section 2.1.4: The MUST is too 
>>    strong, and the reason for distinguishing between in-dialog requests 
>>    from others is unclear.
>   
> See my comments for 2.1.4.
> 
> ---
> 
>>    * Section 2.2:
>>    
>>    Again is overly strong in its use of MUST. I think it is evident that 
>>    the point being made is that a challenge containing Bearer scheme is the 
>>    way to indicate that is with a challenge with Bearer scheme, but the 
>>    text is at least awkward.
>>    
>>    How about:
>>    
>>        When a UAS or Registrar receives a request that fails to contain
>>        authorization credentials acceptable to it, it SHOULD challenge the
>>        request by sending a 401 (Unauthorized) response. To indicate that
>>        it is willing to accept an OAuth2 token as a credential the
>>        UAS/Registrar MUST include a Proxy-Authentication header field in the
>>        response, indicate "Bearer" scheme and include an address of an
>>        Authorization Server from which the originator can obtain an access
>>        token.
>   
> WFM.
>   
>>    In the second paragraph, please provide a reference:
>>    
>>        ... using the procedures from [RFC???] associated with the type of
>>        access token used.
>   
> Ok.
> 
> ---
>   
>>    * Section 2.3:
>>    
>>    Similar comment to section 2.2. How about the following for the first 
>>    paragraph:
>>    
>>        When a proxy receives a request that fails to contain
>>        authorization credentials acceptable to it, it SHOULD challenge the
>>        request by sending a 407 (Proxy Authentication Required) response.
>>        To indicate that it is willing to accept an OAuth2 token as a
>>        credential the proxy MUST include a Proxy-Authentication
>>        header field in the response, indicating "Bearer" scheme and
>>        including an address to an Authorization Server from which the
>>        originator can obtain an access token.
>   
> WFM.
> 
>>    I don't think the second paragraph should condition any behavior on 
>>    whether it has previously challenged. That requires that the proxy be 
>>    stateful, and it isn't a necessary condition. Instead, how about:
>>    
>>        When a proxy wishes to authenticate a received request, it MUST
>>        search the request for Proxy-Authorization header fields with 'realm'
>>        parameters that match its realm. It then MUST successfully validate
>>        the credentials from at least one Proxy-Authorization header field
>>        for its realm. When the scheme is Bearer the proxy MUST validate the
>>        access token, using the procedures from [RFC???] associated with the
>>        type of access token used.
>   
> WFM.
> 
> ---
> 
>>    * Section 3:
>>    
>>    The following:
>>    
>>        The realm and auth-param parameters are defined in [RFC3261].
>>    
>>    is incomplete, because it is missing 'challenge', 'realm', and 
>>    'https-URI. (Where is https-URI defined?) 
>>
>> Another way to do this is by  adding the following to the syntax:
>>    
>>            challenge = <defined in RFC3261>
>>            realm = <defined in RFC3261>
>>            auth-param = <defined in RFC3261>
>>            scope = <defined in RFC6749>
>>            error = <defined in RFC6749>
>>            https-URI = <where???>
>>    
>>    (This helps those who might try to formally validate the syntax because 
>>    it removes undefined parameters.)
> 
> The section does enhance 'challenge', so I guess we can add text saying that 'challenge' is defined in RFC 3261.
> 
> Regarding the other parameters, the text already says where they are defined.
> 
> Or, are you saying that you want the references in the syntax, rather than in the normative text?

I'm just *suggesting* that doing so in ABNF will make life easier for 
anybody who wants to process the abnf using tools.

>>    Also, 'scope' and 'error' are very generic rule names being used in a 
>>    very restricted context. While these are not defined in RFC3261, there 
>>    is the potential conflict with other extensions. I'd recommend using 
>>    more specific terms. E.g.,
>>    
>>            challenge  =/  ("Bearer" LWS bearer-cln *(COMMA bearer-cln))
>>            bearer-cln = realm / bearer-scope / authz-server / bearer-error /
>>                         auth-param
>>            authz-server = "authz_server" EQUAL authz-server-value
>>            authz-server-value = https-URI
>>
>>            challenge = <defined in RFC3261>
>>            realm = <defined in RFC3261>
>>            auth-param = <defined in RFC3261>
>>            bearer-scope = <defined as scope in RFC6749>
>>            bearer-error = <defined as error in RFC6749>
>>            https-URI = <where???>
> 
> The HTTP WWW-Authenticate header field uses 'error' and 'scope', and I think it would be good to be aligned.

The context in which abnf from HTTP is processed is distinct from that 
in which SIP abnf is processed.

Consider somebody who is trying to pull together complete abnf for sip 
and all the sip extensions that they implement. All the rule names newly 
defined in one sip extension need to be distinct from those defined in 
other sip extensions. (Except in cases of extensions to extensions.)

If the new rule names are sufficiently unique to effectively eliminate 
conflicts then all should be good. If common words are used as rule 
names, then multiple extensions may inadvertently conflict. We have no 
mechanisms for easily detecting such conflicts.

I'm just trying to suggest practices that help with this. I don't think 
that what I suggest above will confuse anyone that is familiar with the 
HTTP abnf.

>>    Regarding the scope parameter:
>>    
>>    This parameter is conveyed from the registrar to the UAC. But what is 
>>    the UAC to do with it? How does it have any control over the scope of 
>>    the token it gets from the AS? Is the UAC supposed to convey this to the 
>>    AS? If so, how?
>   
> The UAC will get the scope from the AS, and will forward it towards the UAS/registrar.

I'm confused. According to RFC6749 the client and user agent are passing 
the scope *to* the AS. Then (I think) that influences whether a token is 
returned and if so which one. In our context here, I gather that the 
registrar is conveying the scope for credentials it will accept. Then I 
guess the UAC is expected to pass this on to the AS.

I think the processing of scope needs more explanation.

>>    Regarding the error parameter:
>>    
>>    Is this just intended to be of help in debugging the UAC? Or is it 
>>    expected that the UAC might take some sort of pre-programmed action in 
>>    response? If the latter, how might that work? Is this information 
>>    suitable for presentation to the user?
>   
> I see no reason why it can't be presented to the user, and I guess the UAC actions depend on what the error code is.
> 
> However, as the parameter is described in RFC 6749, I don't think we need to repeat that in this draft.

I'm not sure I understand it there, but it appears to me that the error 
parameter is returned by the AS to the client in response to a request 
for a token. In that case I don't see why it will be present in a 
WWW-Authenticate header field.

Can you explain the flow that results in this being in WWW-Authenticate?

> ---
>   
>>    * Section 4:
>>    
>>    I agree with Dale - I don't see any value in this tag. I tried to search 
>>    for all discussion on the point. The discussion attributes me with 
>>    requesting it, but I don't think I did.
>   
> It was Olle who requested it.
> 
> However, I don't see any harm in having it. It allows the UAC to inform proxies/UAS/Registrar whether it supports the bearer scheme.

There is no particular harm other than added complexity and the 
likelihood that others will in the future ask about it.

> ---
> 
>>    * Missing:
>>    
>>    I find no syntax extensions for Authorization and Proxy-Authorization. 
>>    These need to be added.
>   
> Ok.

	Thanks,
	Paul