Re: [sipcore] Benjamin Kaduk's Discuss on draft-ietf-sipcore-sip-token-authnz-13: (with DISCUSS and COMMENT) - COMMENT

[Benjamin Kaduk <[e-mail]>]

Hi Christer,

On Thu, Apr 23, 2020 at 09:03:53PM +0000, Christer Holmberg wrote:
> Hi Benjamin,
> 
> Thank You for the review! In this reply I will address some of your COMMENT issues, and indicate the ones that I want Rifaat to address. Your DISCUSS will be addressed in a separate realy.
> 
>     ----------------------------------------------------------------------
>     COMMENT:
>     ----------------------------------------------------------------------
>     
> >    Section 1
> >    
> >       The OpenID Connect 1.0 specification [OPENID] defines a simple
> >       identity layer on top of the OAuth 2.0 protocol, which enables
> >       clients to verify the identity of the user based on the
> >    
> >    Please make it clear that these are OAuth/OIDC clients, not SIP clients.
>   
> I'll leave this for Rifaat.
> 
> ---
>   
> >    Section 1.3
> >    
> >    OAuth 2.0 doesn't require the issuance of a Refresh token, but the
> >    discussion here implies that for the SIP case there will always be a refresh
> >    token.  If we're profiling 6749 in this manner, we should be more explicit
> >    about the requirement to issue refresh tokens.
> 
> I am not sure whether the intention is to say that there will always be a refresh token. But, I'll leave this for Rifaat.
> 
> >       *  ID Token: this token contains the SIP URI and other user-specific
> >          details that will be consumed by the UAC.
> >    
> >    nit: I don't think we should use the definite article in "the SIP URI" since
> >    we haven't discussed any such SIP URI usage yet.  That is, we should say
> >    what it's used for, e.g., "a SIP URI associated with the user".
>     
> I'll leave this for Rifaat.
> 
> >       *  Structured Token: a token that consists of a structured object
> >          that contains the claims associated with the token, e.g.  JWT as
> >          defined in [RFC7519].
> >    
> >    nit: comma after "e.g.".
>   
> Will be fixed.
>   
> >       *  Reference Token: a token that consists of a random string that is
> >          used to obtain the details of the token and its associated claims,
> >          as defined in [RFC6749].
> >    
> >    It doesn't technically have to be random (though in practice it should
> >    contain signficant random elements); "opaque" might be better wording.
>   
> I'll leave this for Rifaat.
>   
> >    Section 1.4.1
> >    
> >    Perhaps Figure 1 could include some indication that steps 5 and 6 are
> >    optional/do not always occur (in the case when the access token is a
> >    self-contained JWT)?
>   
> I'll leave this for Rifaat.
>   
> >       In step [2], the registrar challenges the UA, by sending a SIP 401
> >       (Unauthorized) response to the REGISTER request.  In the response,
> >       the registrar includes information about the AS to contact in order
> >       to obtain a token.
> >    
> >    The UAC needs to have a preconfigured whitelist of acceptable ASes in order
> >    to avoid directing the user's credentials to malicious sites.
>   
> This is related to your DISCUSS, so it will be addressed as part of that.
> 
> >       The registrar validates the access token.  If the access token is a
> >       reference token, the registrar MAY perform an introspection
> >       [RFC7662], as in steps [5] and [6], in order to obtain more
> >       information about the access token and its scope, per [RFC7662].
> >    
> >    I think we could tighten up the normative language a bit here.
> >    In particular, the registrar MUST validate the token in some fashion; for
> >    reference tokens, the main ways to do that are either inspection or
> >    (essentially) being the party that issued the token in the first place.
> >  
> >       Otherwise, after the registrar validates the token to make sure it
> >       was signed by a trusted entity, it inspects its claims and acts upon
> >       it.
> >    
> >    I think we can also be more specific than "a trusted entity" -- in this
> >    flow, it looks like the registrar should know exactly which entity should
> >    have signed the token in question (for the user in question), and should not
> >    accept a signed token from a different entity that happens to be trusted to
> >    issue other tokens.
>     
> The text in Section 2.2. mandates the validation:
> 
>    "When a UAS/Registrar receives a SIP request that contains an
>    Authorization header field with an access token, the UAS/Registrar
>    MUST validate the access token,..."
> 
> I don't think we should put too much normative text into the example flows.

Perhaps we should just say "validates the token" (and not "to <do thing
X>") in this example?

> >    Section 1.4.2
> >    
> >    (Similarly, Figure 2 could note that steps 3 and 4 do not always occur, and
> >    the text about token validation should remain parallel between examples.)
>     
> I'll leave this for Rifaat.
> 
> ----
> 
> >    Section 2
> >    
> >    I note that RFC 3261 explicitly disclaims any definition of authorization
> >    systems, which is not true for this document, given that OAuth is an
> >    authorization system :)
> 
> RFC 3261 only says that the RFC does not recommend or discuss any authorization system.
> 
> >    Section 2.1.1
> >    
> >       Required) response with a Proxy-Authenticate header field.  If the
> >       WWW-Authenticate or Proxy-Authenticate header field indicates
> >       "Bearer" scheme authentication and contains an address to an
> >       authorization server, the UAC contacts the authorization server in
> >       order to obtain tokens, and includes the requested scopes, based on a
> >       local configuration (Figure 1).
> >    
> >    [whitelist of allowed ASes, again]
>   
> Will be addressed when the DISCUSS is addressed.
> 
> >       The detailed OAuth2 procedure to authenticate the user and obtain
> >       these tokens is out of scope of this document.  The address of the
> >       authorization server might already be known to the UAC via
> >       configuration.  In which case, the UAC can contact the authorization
> >       server for tokens before it sends a SIP request (Figure 2).
> >    
> >    nit: I think that "in which case" functions as a conjunction, which makes
> >    this an incomplete sentence.
>   
> The text was suggested by one of the reviewers, but I agree it sounds incomplete.
> 
> Perhaps "In such cases"?

I think that works.

> >       The tokens returned to the UAC depend on the type of authorization
> >       server (AS): an OAuth AS provides an access token and refresh token
> >       [RFC6749].  The UAC provides the access token to the SIP servers to
> >    
> >    Again, this implies that refresh tokens are always issued; RFC 6749 is quite
> >    clear that "issuing a refresh token is optional at the discretion of the
> >    authorization server".
>     
> I'll leave this for Rifaat.
> 
> >       authorize UAC's access to the service.  The UAC uses the refresh
> >       token only with the AS to get a new access token and refresh token
> >       before the expiry of the current access token (see [RFC6749], section
> >    
> >    (New refresh token is also optional.)
>   
> I'll leave this for Rifaat.
> 
> >       1.5 Refresh Token for details).  An OpenID Connect server returns an
> >       additional ID-Token containing the SIP URI and other user-specific
> >       details that will be consumed by the UAC.
> >    
> >    [same comment as above about "the SIP URI".]
>   
> I'll leave this for Rifaat.
> 
> >       If the UAC receives a 401/407 response with multiple WWW-
> >       Authenticate/Proxy-Authenticate header fields, providing challenges
> >       using different authentication schemes for the same realm, the UAC
> >       selects one or more of the provided schemes (based on local policy)
> >       and provides credentials for those schemes.
> >    
> >    RFC 3261 seems to be written in a world that assumed that Basic and Digest
> >    are the only defined HTTP authentication schemes, and since it prohibits
> >    Basic, that there would only be one authentication challenge (type)
> >    possible.  This text righly acknowledges that we are opening the field up
> >    and could have cases where multiple authentication schemes are possible;
> >    however, it goes even further and admits the possibility of using them
> >    simultaneously.  It seems like this places an onus on us to give some
> >    indication of the semantics when multiple schemes are used at the same time.
> >    Do the authenticated identities need to match?  Or are we asserting that
> >    both/all identities are consenting to the request in question?  (If we don't
> >    have a concrete use case in mind, perhaps the "or more" is just opening a
> >    box that we don't need to open right now.)
> 
> I can modify as suggested.
> 
> (Personally, I have never seen multiple schemes in a deployment.)
>     
> >    Section 2.1.2
> >    
> >       access to it.  Endpoints that support this specification MUST support
> >       encrypted JSON Web Tokens (JWT) [RFC7519] for encoding and protecting
> >       access tokens when they are included in SIP requests, unless some
> >       other mechanism is used to guarantee that only authorized SIP
> >       endpoints have access to the access token.
> >    
> >    I think we may want "MUST support" (always) and "MUST use, unless some other
> >    mechanism".
>   
> Based on another review, I suggested to change to "MUST use". I am not sure whether we need to keep "MUST support".

I think "MUST use" implies "MUST support", yes.

> >    I'd also suggest a quick note that TLS remains fine for protecting the
> >    UAC/AS interaction where the refresh token is used.
>   
> I can do that.
> 
> I could also say "*SIP* endpoints that support this specification...".

Either sounds good.

> >    Whatever is done, please harmonize with Section 5 that has essentially the
> >    same text.
>   
> Will do.
> 
> >    Section 2.1.3
> >    
> >       Based on local policy, the UAC MAY include an access token that has
> >       been used for another binding associated with the same Address Of
> >       Record (AOR) in the request.
> >    
> >    Just to check: there's no real privacy considerations with such use, as its
> >    the same parties interacting and there are other identifiers (e.g., IP
> >    addresses) to confirm that it's the same client communicating to the server?
>     
> I'll leave this for Rifaat.
> 
> >    Also, is it clear what will be done (new token request) when the MAY is not
> >    heeded?
>   
> I'll leave this for Rifaat.
> 
> >    Section 2.1.4
> >    
> >    The text here just talks about "a valid access token" and similar, without
> >    saying a whole lot about it being related in any way to the specifics of the
> >    authentication challenge.  Is there something useful to say about, e.g., the
> >    token in question having been issued by the authorization server indicated
> >    in the challenge?
>   
> I'll leave this for Rifaat.
> 
> >    Section 2.2
> >    
> >       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 access token as a credential, the UAS/
> >       Registrar MUST include a Proxy-Authentication header field in the
> >       response that indicates "Bearer" scheme and includes an address of an
> >    
> >    nit(?): I'd consider just making this a declarative statement, a la "the
> >    UAS/registrar includes a Proxy-Authentication header field with the "Bearer"
> >    scheme to indicate that it is willing to accept an access token as a
> >    credential"  (but that wording is incomplete and would need to be fleshed
> >    out a bit more).
> 
> I think that would be weird. Because, first we say that the UAS/Registrar SHOULD challenge, and with your suggestion the text would say that the UAS/Registrar MUST include a Proxy-Authentication header field even if it does NOT challenge the request.
> 
> Perhaps:
> 
> "If the UAS/Registrar chooses to challenge the request, and is willing to accept an access token as a credential, the UAS/Registrar MUST include a..."

This version does get rid of the confusion about whether the MUST is
conditional that I wanted to address, thanks.  (I see I didn't actually say
why I proposed the change I did, whoops.)

> >       authorization server from which the originator can obtain an access
> >       token.
> >    
> >    nit: "address of" an AS, does that mean bare IP address only or is a DNS
> >    name okay?
>   
> I'll leave this for Rifaat.
>   
> >       [RFC7519].  If the token provided is an expired access token, then
> >       the UAS MUST reply with 401 Unauthorized, as defined in section 3 of
> >       [RFC6750].  If the validation is successful, the UAS/Registrar can
> >       continue to process the request using normal SIP procedures.  If the
> >       validation fails, the UAS/Registrar MUST reject the request.
> >    
> >    Is "expired" the only case that should get a 401 vs. outright rejection, as
> >    stated here?
> 
> 401 is sent also in the case where the validation fails. I will clarify that.
>   
> >    Section 2.3
> >    
> >       sending a 407 (Proxy Authentication Required) response.  To indicate
> >       that it is willing to accept an access token as a credential, the
> >       proxy MUST include a Proxy-Authentication header field in the
> >       response that indicates "Bearer" scheme and includes an address to an
> >    
> >    [same comment as above about normative vs. declarative statement]
> >    Also, please keep the text parallel between sections -- this copy has at
> >    least one nit ("address to an" vs. "address of an").
>   
> I will fix this in the same way.
> 
> >    Section 3
> >    
> >       If an access token is encoded as a JWT, it might contain a list of
> >       claims [RFC7519], some registered and some application-specific.  The
> >    
> >    I don't think there's a question of whether a JWT will contain a list of
> >    claims :)
> 
> Fair enough :)
> 
> >    Maybe "If the access token is encoded as a JWT, it will contain a list of
> >    claims [RFC7519], which may include both registered and application-specific
> >    claims"?
>   
> Looks good to me, but I'll leave this for Rifaat.
> 
> ----
> 
> >    Section 4
> >    
> >    This section claims to cover WWW-Authenticate.  What specification will the
> >    SIP use of Bearer for Authorization operate under?
>   
> RFC 6750.
> 
> Section 2.1.3 says:
> 
>    "The UAC sends a REGISTER request with an Authorization header field
>    containing the response to the challenge, including the Bearer scheme
>    carrying a valid access token in the request, as specified in
>    [RFC6750]."

Okay, thanks for the reminder.

> >           challenge  =/  ("Bearer" LWS bearer-cln *(COMMA bearer-cln))
> >    
> >    side note: is there a mnemonic for the "cln" in "bearer-cln"?
> 
> RFC 3261 uses "cln" for digest.
> 
> >           bearer-cln = realm / scope / authz-server / error /
> >                        auth-param
> >    
> >    nit: realm is already included in auth-param, though I don't see a harm in
> >    calling it out explicitly.
> 
> auth-param is used to allow possible new parameters in the future.  
> 
> >           realm = <defined in RFC3261>
> >           auth-param = <defined in RFC3261>
> >    
> >    RFC 3261 defers to RFC 2617 (now obsoleted by 7235) for both of these; we
> >    could perhaps short-circuit the chain and say "defined in RFC 7235".
>   
> We discussed this in the WG, and the outcome was to keep the same references as RFC 3261, since that is what people are implementing.
> 
> Then, as a separate task, someone could update RFC 3261 with the new references.
> 
> ---
>   
> >    Section 5
> >    
> >    We should probably note that SIP makes much heavier use of proxies than is
> >    common in the web world of OAuth.
> 
> Maybe something like:
> 
> "However, SIP makes have use of intermediary SIP proxies, and TLS only guarantees hop-to-hop protection when used to
>    protect SIP signaling."

Sure, that should work.

> >    I'm happy to see that some privacy considerations are being added in
> >    response to Barry's review.
>   
> Good :)
> 
> ---
> 
> >    Section 8
> >    
> >    I think RFCs 8252 and 8414 could be just informative.
>   
> I can fix that.

Thanks for the updates, and I'll look for the additional threads.

-Ben