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

[Rifaat Shekh-Yusef <[e-mail]>]

Inline...

On Thu, Apr 23, 2020 at 5:04 PM Christer Holmberg <
[e-mail]> 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.
>
> Sure.


> ---
>
> >    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.
>

I answered in this the reply to the DISCUSS


>
> >       *  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.
>
> Sure.



> >       *  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.
>
> How about "opaque random string"?



> >    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.
>
> This is explicitly mentioned in the text, but I agree that it would be
helpful to include it in the figure.



> >       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.
>
> >    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.
>
> Again, this is explicitly mentioned in the text, but I agree that it would
be helpful to include it in the figure.


> ----
>
> >    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"?
>
> >       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.
>
> As I mentioned in the reply to the DISCUSS, we will clarify this.


> >       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.
>
> As I mentioned in the reply to the DISCUSS, we will clarify this.



> >       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.
>
> Ok


> >       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'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...".
>
> >    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.
>
> I would not rely only on an IP address, but the answer is yes.



> >    Also, is it clear what will be done (new token request) when the MAY
> is not
> >    heeded?
>
> I'll leave this for Rifaat.
>
>
The Client would need to obtain a new access token. I will clarify this.



> >    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.
>
> Sure.


>    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..."
>
> >       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.
>
>
I do not think that an IP address is appropriate, and what I have in mind
is a DNS name.
I will clarify it.


>       [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.
>
> Looks good to me too.

Regards,
 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]."
>
> >           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."
>
> >    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.
>
> ---
>
> Regards,
>
> Christer
>
>
>
>
>
>