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

[Christer Holmberg <[e-mail]>]

Hi Benjamin,

    ----------------------------------------------------------------------
             COMMENT:
    ----------------------------------------------------------------------
    >>     
    >>     ...
    >>   
    >>     Section 1.4.1
    >>     ...
    >> 
    >>     >>>       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?
    >>  
    >>     So, something like this:
    >> 
    >> OLD:
    >> 
    >>    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].
    >>    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.
    >> 
    >> NEW:
    >>
    >>    In step [4] and [5], the registrar validates the access token.
    >> 
    >>
    > That might be trimming down too far -- we lose the explanation of steps [5]
    > and [6].  I was thinking more like:
    >
    > % 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].
    > % Otherwise, after the registrar validates the token, it inspects its
    > % claims and acts upon it.
    >
    > This level of detail seems okay to me because the dichotomy between
    > "reference token" and "self-contained claims" is established earlier in the
    > document.
    
    I realized that my suggested text took away too much, and was going to suggest the following:

        "The registrar validates the access token. In steps [5] and [6], the registrar
        performs an introspection [RFC7662]. The introspection procedure is optional, and might
        be performed by the registrar if the access token is a reference token."

    But, I think the text you suggest is better :)

    In addition, we were thinking about also indicate the optionality in the flow itself:
 
  |                               | [3] HTTP POST /introspect     |
  |                               |     {access_token}                     |
  |                               |       (OPTIONAL)                        |
  |                               |------------------------------------->|
  |                               |                                                    |
  |                               | [4] 200 OK {metadata}           |
  |                               |       (OPTIONAL)                        |
  |                               |<-------------------------------------|

    ...
         
    >>     >>>    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.
    >>     
    >>    My suggestion was to do both changes :) Something like:
    >> 
    >>    "SIP endpoints that support this specification MUST use 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. TLS can still be used for protecting traffic between
    >>     SIP endpoints and the AS."
    >> 
    >
    > Ah.  Looks good!
    
   Good :)

    Regards,

    Christer






    
    >  
    >     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.)
    > 
    >    ...
    > 
    >     >>>       [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 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.
    > 
    >     ---
    > 
    >     > >    Section 8
    >     > >    
    >     > >    I think RFCs 8252 and 8414 could be just informative.
    >     >   
    >     > I can fix that.
    >     
    >     ---
    > 
    > The changes done so far based on your IESG review can be found in the following pull request commit:
    > 
    > https://protect2.fireeye.com/v1/url?k=8e6297cc-d2e8b503-8e62d757-0cc47ad93ea4-de384e701a508192&q=1&e=e601bbb4-7695-4c82-a11c-34b68b5109df&u=https%3A%2F%2Fgithub.com%2Frifaat-ietf%2Fdraft-ietf-sipcore-sip-token-authnz%2Fpull%2F7%2Fcommits%2F0fc655719d7e3cefc7417b382e7662c1d9c12dbb
    > 
    > 
    > Regards,
    > 
    > Christer
    > 
    >     
    >