Re: [Ace] AD review of draft-ietf-ace-oauth-authz-24

[Benjamin Kaduk <[e-mail]>]

I guess I'll be responding to the thread in backwards order, to try and
catch any topics that have received later updates...

On Tue, Oct 15, 2019 at 06:20:51PM -0700, Jim Schaad wrote:
> I have trimmed down to the issues that I wanted to respond to.
> 
> Jim
> 
> 
> -----Original Message-----
> From: Ludwig Seitz <[e-mail]> 
> Sent: Tuesday, October 15, 2019 7:08 AM
> To: Benjamin Kaduk <[e-mail]>; [e-mail]
> Cc: [e-mail]
> Subject: Re: AD review of draft-ietf-ace-oauth-authz-24
> 
> Hello Ben,
> 
> thank you for your thorough review.
> 
> I have taken the liberty to add numbers to your comments in order to 
> refer to them in a easier way.
> 
> I have fixed 93 your 113 and there are 20 left where I am asking for 
> clarifications. These are:
> 
>   6.), 12.), 16.), 19.), 34.), 39.), 41.), 45.), 52.), 57.), 58.), 65.), 
> 66.), 68.), 76.), 78.), 79.), 88.), 99.), 111.)
> 
> Note that 39.) Requires input from OAuth experts. Hannes?
> 
> If you whish to inspect the changes made on other review issues, please 
> consult the commits here:
> https://github.com/ace-wg/ace-oauth/commits/master
> 
> 
> Detailed comments below.
> 
> /Ludwig
> 
> > 
> > Hi all,
> > 
> > The length of this review notwithstanding, this document is generally in
> > good shape -- there's a bunch of localized items to tighten up, and we
> > can flesh out the security considerations some more, but nothing too
> > drastic should be needed.
> > 
> 
> 
> > 12.)
> >       A refresh token in OAuth 2.0 is a string representing the
> >       authorization granted to the client by the resource owner.  The
> >       string is usually opaque to the client.  The token denotes an
> > 
> > Apparently I'd forgotten that this couldn't be binary.
> 
> [LS] From RFC 6749, section 1.5: "A refresh token is a string ..."
> No qualifiers such as "binary" so I interpret this to mean a text-string.
> Do you want us to add some clarification?
> 
> [JLS] I think we may want to change this at some point in the future, but I don't know how much people are going to use refresh tokens so I don't think I would worry about it.

I don't think we need to do anything right now.

> > 32.)
> > Section 5.6.1
> > 
> >    The client sends a POST request to the token endpoint at the AS.  The
> >    profile MUST specify how the communication is protected.  The content
> > 
> > In the previous section we said that maybe even other transports than
> > coaps or https would be possible; are we limited to those that have POST
> > verbs?
> > Also, a similar comment as above about what attributes the protection
> > entails seems to apply.
> 
> [LS] This will need a major rephrasing of the text.
> I see two options here:
> 
> 1.) We rewrite all parts to use a neutral language in general and specify
> POST/GET etc. for transports that have these verbs.
> 
> 2.) We state in the beginning that transports that do not use RESTful verbs
> should use the best equivalent.
> 
> Option 1. would get a bit cluncky, while option 2. might be a bit confusing
> Do you have a specific preference?
> 
> [JLS] I would suggest that this could fall under the punt to the new transport that does not have the same as these verbs in it to explain how they would map.  

I agree that (1) is more effort than it's likely to be worth.  If we can
finagle a single-sentence disclaimer like "transports that do not use these
verbs will need to specify the requisite behavior" that would be great; if
not, then we can consider whether to just ignore it or do something more
complicated.

> > 39.)
> >    Refresh tokens are typically not stored as securely as proof-of-
> >    possession keys in requesting clients.  Proof-of-possession based
> >    refresh token requests MUST NOT request different proof-of-possession
> >    keys or different audiences in token requests.  Refresh token
> >    requests can only use to request access tokens bound to the same
> >    proof-of-possession key and the same audience as access tokens issued
> >    in the initial token request.
> > 
> > This is perhaps something of a philosophical question, but if a refresh
> > token is only usable at the token endpoint, in some sense its audience
> > is definitionally the AS.  So there's a little bit of a mismatch in
> > treating it as having the audience value that the access tokens issued
> > from it will have.  I don't know the background for audience-restriced
> > refresh tokens in regular OAuth 2.0, though, so hopefully someone can
> > educate me.
> 
> [LS] I'm equally confused. I suggest that Hannes or one of the other OAuth
> experts give us a hint on that one.
> 
> [JLS]  My understanding is that the refresh token is completely opaque to anybody but the AS so how it is stored is not as critical.  The client then does a normal request with that as a parameter.  The refresh token allows for the AS to avoid doing some of the authorization steps and potentially re-use the same key for the new token.  As such, the newly requested token needs to headed to the same audience as the original POP token.  This is the reason for needing to keep audience with it on the client.  

I agree with the behavior, yes.  It's just interesting/surprising to have
something associated with the refresh token that uses the term "audience"
but that does not apply to the actual usage of the refresh token.
We probably don't need any text changes here, on second look (I note that
the text is "same audience as *access* tokens issued in the initial token
request).

> > 41.)
> >    token_type:
> >       This parameter is OPTIONAL, as opposed to 'required' in [RFC6749].
> >       By default implementations of this framework SHOULD assume that
> >       the token_type is "pop".  If a specific use case requires another
> >       token_type (e.g., "Bearer") to be used then this parameter is
> >       REQUIRED.
> > 
> > When we are "weakening" the formal requirements of the parent spec like
> > this, we should be careful about what happens when someone is trying to
> > use the ACE stuff but ends up needing to go over HTTPS like traditional
> > OAuth -- do they have to fall back to the OAuth required behavior in
> > that case or are we trying to preempt that as well?
> 
> [LS] The use case here is a constrained connection between client and AS
> (e.g. due to a constrained client), where we try to save the bytes for
> sending a token_type by defining a default. I would guess that a HTTPS based
> connection would not have such constraints and would therefore be able 
> to send
> the token_type = "pop" as required by RFC6749. Should we explain this 
> somewhere
> and describe the cases where RFC6749 behaviour is warranted?
> 
> [JLS] Given that it would be identified as being application/ace+json, I don't think there is a problem with having the default value.  The fact that this is ACE is identifiable from the media-type of the message sent over HTTPS.

Okay, you convinced me.

> > 52.)
> > Section 5.7.4
> > 
> > "scope" is showing up with the key 9 a lot; I have mixed feelings about
> > having the CBOR map key value be aliased across different contexts (but
> > it's probably too late to change without disruption anyway).
> 
> [LS] Scope is used in 4 places:
> 
> I. AS Request Creation Hints
> II. /token request and response parameter
> III. CWT claim
> IV. /introspection response parameter
> 
> I claim that in all 4 cases we expect the same content with the same
> formatting and semantics:  I. tells the client what to put into II.
> which in turn informs the AS about the requested claim value in III.
> and IV. is supposed to reflect that claim value. I therefore don't feel
> hesitant to use the same abbreviation for all four cases.
> Is that acceptable or do you whish us to perform any action on this?
> 
> [JLS]  I could have wished you said something sooner.  I kept trying to argue that there was no need to be consistent on the mapping, but that is how it is.  I will note that sections 8.6, 8.9, 8.11 as well as 8.13 are all separate registries for doing the mapping.  This means that while today there is consistency across all of the registries, there is no guarantee that this will be true going forward.

Sorry for being slow, and agreed on all the rest.
It seems unlikely that any gain from changing things now will be worth the
pain of churn.

-Ben