Re: [Ace] AD review of draft-ietf-ace-oauth-params-05

[Benjamin Kaduk <[e-mail]>]

One more update on this one: I cancelled the last-call request, because my
discussions with Ludwig about the core framework indicated a strong chance
that we'll want to tweak how rs_cnf works, and we should of course reflect
any change to rs_cnf in this document as well.

Further updates (hopefully) in the framework's thread.

-Ben

On Mon, Nov 18, 2019 at 09:42:41PM -0800, Benjamin Kaduk wrote:
> On Sun, Nov 17, 2019 at 04:45:04AM +0100, Ludwig Seitz wrote:
> > On 15/11/2019 13:14, Benjamin Kaduk wrote:
> > > Hi all,
> > > 
> > > I'm mostly just nitpicking in the following comments; the actual content
> > > here is in good shape.  (But some of these are popular things that
> > > directorate reviewers like to complain about, so fixing them preemptively
> > > seems wise.)
> > > 
> > > Section 1
> > > 
> > >     access tokens.  These parameters and claims can also be used in other
> > >     contexts, and may need to be updated to align them with ongoing OAuth
> > >     work.  Therefore they have been split out into this document, which
> > >     can be used and updated independently of [I-D.ietf-ace-oauth-authz].
> > > 
> > > I expect that we'll get some reviewers who want to wordsmith this last
> > > sentence.  It's fine to wait and see what suggestions come in (if any),
> > > but if you want to try to forestall those, my suggestion would be:
> > > 
> > > % Therefore, these parameters and claims have been put into a dedicated
> > > % document, to facilitate their use and any potential updates in a manner
> > > % independent of [I-D.ace-oauth-authz].
> > > 
> > Done.
> > 
> > > Section 3.1
> > > 
> > >     req_cnf
> > >        OPTIONAL.  This field contains information about the key the
> > >        client would like to bind to the access token for proof-of-
> > >        possession.  It is RECOMMENDED that an AS reject a request
> > >        containing a symmetric key value in the 'req_cnf' field, since the
> > >        AS is expected to be able to generate better symmetric keys than a
> > >        potentially constrained client.  The AS MUST verify that the
> > > 
> > > nit: I'd wordsmith this a bit, since the idea is that the AS-generated
> > > key will be better than one generated by a constrained client, but a
> > > non-constrained client can probably do just fine at keygen.  So,
> > > I'd consider dropping "potentially", as the rest of the sentence does
> > > not have anything to imply that all clients using this claim are
> > > constrained.
> > > 
> > Done.
> > 
> > >     Payload:
> > >     {
> > >        "req_cnf" : {
> > >           "COSE_Key" : {
> > >              "kty" : "EC",
> > >              "kid" : h'11',
> > >              "crv" : "P-256",
> > >              "x" : b64'usWxHK2PmfnHKwXPS54m0kTcGJ90UiglWiGahtagnv8',
> > >              "y" : b64'IBOL+C3BttVivg+lSreASjpkttcsz+1rb7btKLv8EX4'
> > >           }
> > >        }
> > >      }
> > > 
> > >     Figure 1: Example request for an access token bound to an asymmetric
> > >                                     key.
> > > 
> > > Shouldn't an access token request have an authorization code?
> > > The other parameters from Section 4.1.3 of RFC 6749 have conditions
> > > attached to their REQUIRED status, but not "code".
> > >
> > In draft-ietf-ace-oauth-authz section 5.6.1. (Client-to-AS Request) we 
> > specify that when the "grant_type" parameter is missing, then 
> > "client_credentials" is implied, which in turn means that the client 
> > does not need an authorization code.  Section 4.1.3 of RFC 6749 is about 
> > the authorization code grant, you are looking at the ACE-ified version 
> > of Section 4.4
> 
> Ah, thanks.  I remembered that we had a default "grant_type" but was too
> fast when trying to check the other (sometimes-)required parameters.
> 
> > > Section 3.2
> > > 
> > >     cnf
> > >        REQUIRED if the token type is "pop" and a symmetric key is used.
> > > 
> > > Just to check, this means that if the client includes a symmetric 'cnf'
> > > key and the AS ignores the prior recommendation to ignore such a
> > > symmetric confirmation key in the request, the AS still has to echo
> > > back the symmetric key that is in use?  (There's also potentially some
> > > interesting interactions if we use a 'kid'.)
> > 
> > Indeed as the drafts are written right now, that would be the expected
> > AS behaviour. This is not entirely intentional, and my only objection to 
> > changing it at this time is that writing specification text handling 
> > interactions we are recommending against (i.e. accepting 
> > client-suggested symmetric keys) seems icky.
> > As for 'kid' I'd just expect the AS to echo back the kid, not the actual 
> > key.
> 
> I don't see any obvious problems from this behavior, so leaving it as-is
> seems like the right thing to do for now.
> 
> > > 
> > >        MAY be present for asymmetric proof-of-possession keys.  This
> > >        field contains the proof-of-possession key that the AS selected
> > >        for the token.  Values of this parameter follow the syntax of the
> > > 
> > > And to check here again, the omitted 'cnf' in the response means that he
> > > AS accepted the client's requested asymmetric key?  (There wouldn't be a
> > > case where the AS rejects an offered asymmetric key and replaces it with
> > > a new one, right?)
> > Indeed that is correct. Note that there could be cases where an AS 
> > rejects an offered asymmetric key and replaces it by another one. This 
> > would be if the AS knows that the RS doesn't support this key format,
> > and also knows that the client holds another public key which is 
> > actually supported by the RS.
> 
> Thanks for pointing out that scenario :)
> 
> > > 
> > >     rs_cnf
> > >        OPTIONAL if the token type is "pop" and asymmetric keys are used.
> > >        MUST NOT be present otherwise.  This field contains information
> > >        about the public key used by the RS to authenticate.  If this
> > >        parameter is absent, either the RS does not use a public key or
> > >        the AS assumes that the client already knows the public key of the
> > >        RS.  Values of this parameter follow the syntax of the "cnf" claim
> > > 
> > > I'd prefer to not word this as "the AS assumes", as that implies that we
> > > will end up with a fragile overall system.  Can we say something more
> > > along the lines of "when the AS knows that the RS can authenticate
> > > itself to the client without additional information"?  (I expect that
> > > less awkward wordings than the above are possible...)
> > >
> > Done.
> > 
> > 
> > >        from section 3.1 of [I-D.ietf-ace-cwt-proof-of-possession].  See
> > >        Section 5 for details on the use of this parameter.
> > > 
> > > Also (both here and above), I'd suggest s/details on the use of this
> > > parameter/additional discussion of the usage of this parameter/; we do not
> > > go into a huge amount of detail in Section 5.
> > Done.
> > 
> > > 
> > > I don't think the caption for Figure 3 is quite right...
> > Fixed.
> > 
> > > 
> > > Also, Figures 2 and 3 are using the same "SlAV32hkKG ..." access token
> > > snippet that we changed in the core spec, so we should probably change
> > > it here as well.
> > I changed the one in figure 3 to the same snippet I used which an 
> > asymmetric pop key in the core spec. The snippet in figure 2 is still in 
> > the core spec, so I left it here as well.
> > 
> > > 
> > > Section 3.3
> > > 
> > > In the symmetric-key case this basically degenerates to
> > > Needham-Schroeder (but I also don't expect anyone to use this with
> > > symmetric keys).
> > > However, for asymmetric keys, we might assume that the AS is not
> > > creating a new asymmetric keypair for the RS to use, and instead
> > > referring to one already known to the RS, e.g., just via the public
> > > part.  I can't quite decide whether the RS should always blindly trust
> > > the AS in this regard, or might want to have an additional policy layer
> > > that would allow for "I do not want to use this public key with this
> > > specific client".  Any such benefit from that would be subtle, as
> > > presumably the AS has already told the client the same key, and so the
> > > RS is limited to merely not presenting cryptographic evidence to the
> > > client that it holds the private key (as opposed to the client trusting
> > > the AS as trusted-third-party that the RS has that key).
> > > I guess that suggests that there's no need for additional policy on the
> > > RS, and we don't need any more text here.
> > > 
> > Actually I had asymmetric keys in mind when I created this claim.
> > Since one could imagine protocols using different symmetric keys for 
> > client and RS though, so I wouldn't want to restrict this to
> > asymmetric-only.
> > Also: yes, the idea was that the AS just uses the public part of an 
> > asymmetric key, of which it knows that the RS has the corresponding 
> > private part.
> > Do you want me to make this more explicit?
> 
> This seems like a case where more text would not provide enough value to
> justify its presence.  I don't think there's really anything to be confused
> about that requires clarification; there are just some cases where the AS
> might do something that's a little weird but is not harmful.
> 
> > > Section 5
> > > 
> > >     o  "req_cnf" in the access token request C -> AS, OPTIONAL to
> > >        indicate the client's raw public key, or the key-identifier of a
> > >        previously established key between C and RS that the client wishes
> > >        to use for proof-of-possession of the access token.
> > > 
> > > There's potentially an interesting question here about whether the AS
> > > has to, can, or shouldn't assign semantics to key identifiers.  The part
> > > most needed for interoperability is, of course, that the RS knows to map
> > > an incoming kid to an actual key, and the parallel that the client knows
> > > that the (kid, key, RS) tuple is usable.  I don't see a reason why the
> > > AS needs to have semantics to a given kid for interoperability, though
> > > perhaps there will be some scenarios where the AS manages the kid
> > > namespace at a given RS as opposed to the RS doing so.  But, on the
> > > other hand, having the AS not give semantics to 'kid' turns the AS into
> > > a dumb transport, which tends to be the sort of thing that merits
> > > additional analysis.
> > 
> > Basically the current expectation is that when the client requests a kid 
> > to be bound to an access token, it "knows what it is doing" i.e. either 
> > it has already sent a token using this pop key to the RS previously; or 
> > the client and the RS have been commissioned to have this shared key 
> > bound to the kid.
> > Do you want me to make this more explicit?
> 
> We already say "previously established key between C and RS", so I don't
> think we need to repeat "the client knows what it's doing".
> If we were to add text it would be relating to whether or what policy the
> AS might enforce relating to 'kid'.  But, I don't think we have any real
> thoughts on this matter, so there isn't anything to say at this point.
> 
> > > 
> > >     o  "cnf" in the token response AS -> C, OPTIONAL if using an
> > >        asymmetric key or a key that the client requested via a key
> > >        identifier in the request.  REQUIRED if the client didn't specify
> > >        a "req_cnf" and symmetric keys are used.  Used to indicate the
> > > 
> > > This seems to make no statement (REQUIRED/OPTIONAL) about this claim for
> > > the case where the client didn't specify a "req_cnf" and asymmetric keys
> > > are used.  This situation, of course, can't happen (at least not
> > > currently), as the absence of "req_cnf" is used as a signal for server
> > > (symmetric) keygen, but I suggest that we only mention "client didn't
> > > specify a 'req_cnf'" so we don't have to keep explaining this to
> > > reviewers.
> > 
> > So should I remove the "and symmetric keys are used." part?
> 
> Yes, I think that's best.
> Since that's the only comment left on the -06, I'm happy to just send the
> -06 out for IETF LC and lump it in with any comments from the last call.
> 
> > 
> > > 
> > >     o  "rs_cnf" in the token response AS -> C, OPTIONAL to indicate the
> > >        public key of the RS, if it uses one to authenticate to the
> > >        client.
> > > 
> > > nit(?): I'd consider adding a phrase about "and the binding between key
> > > and RS identiyt is not established through other means".
> > Done.
> > 
> > > 
> > >     o  "rs_cnf" in the introspection response AS -> RS, OPTIONAL,
> > >        contains the public key that the RS should use for authenticating
> > >        to the client (e.g. if the RS has several different public keys).
> > > 
> > > nit(?): I'd consider adding a phrase about "and there may be ambiguity
> > > as to which key to use" or similar, though I'm failing to come up with a
> > > non-awkward phrasing.
> > > 
> > Done. I'll go with slightly awkward, but more correct.
> > 
> > 
> > >     Note that the COSE_Key structure in a confirmation claim or parameter
> > >     may contain an "alg" or "key_ops" parameter.  If such parameters are
> > >     present, a client MUST NOT use a key that is not compatible with the
> > >     profile or proof-of-possession algorithm according to those
> > > 
> > > nit(?): s/not compatible/incompatible/ would avoid a "double 'not'", if
> > > that's seen as desirable.
> > Fixed.
> > 
> > > 
> > > Section 6
> > > 
> > > I thought we had set up separate map key namespaces for (e.g.) token
> > > requests, token responses, and introspection responses, so that
> > > attempting to use the same key value for all the uses of these
> > > parameters defined by this document is something of a type error.
> > > E.g., they end up in different registries, as we do in Section 9.2, 9.5,
> > > and 9.6.
> > > 
> > That is correct. I have added separate entries and a column
> > specifying the different uses. I will leave the values to be the same 
> > though.
> 
> Great; thank you!
> 
> > > Section 9.1, 9.2, 9.4
> > > 
> > > I note that the distance between "authenticate to the client" and
> > > "authenticate the client" is small enough that I expect it to be a
> > > common misreading, and also that the other descriptions at
> > > https://www.iana.org/assignments/jwt/jwt.xhtml#claims are a bit more
> > > pithy than what we use here.  Perhaps, "public key used by RS to
> > > authenticate itself to client"?
> > > 
> > Done.
> > 
> > > Section 9.5
> > > 
> > >     This section registers the following parameter mappings in the "Token
> > >     Endpoint CBOR Mappings" registry established in section 8.9. of
> > >     [I-D.ietf-ace-oauth-authz].
> > > 
> > > It looks like this has been renamed to the "OAuth Parameters CBOR
> > > Mappings" registry?
> > Correct. Fixed.
> > 
> > > 
> > > Section 9.6
> > > 
> > >     This section registers the following parameter mappings in the
> > >     "Introspection Endpoint CBOR Mappings" registry established in
> > >     section 8.11. of [I-D.ietf-ace-oauth-authz].
> > > 
> > > Similarly, this has been renamed to "OAuth Token Introspection Response
> > > CBOR Mappings".
> > > 
> > Fixed.
> > 
> > > Section 11.1
> > > 
> > > It's not entirely clear that RFC 7252 needs to be a normative reference;
> > > we don't do much with CoAP directly in this document.
> > > 
> > Agree. I moved it.
> > 
> > > Appendix A
> > > 
> > > We might want to wordsmith this some if it's to be kept for the final
> > > RFC (depending on what the OAuth work looks like at that point).  I'm
> > > not sure that there are any useful changes to make to it right now,
> > > though.
> > 
> > It seems the OAuth draft I'm talking about here is not going anywhere 
> > fast. We might consider removing this in the final edition.
> 
> Sure.  Let's plan to revisit that after IETF LC (where hopefully we will
> not get many comments; I think this one is in good shape).
> We could also ask the authors here in Singapore what they see its prospects
> as.
> 
> Thanks for the updates,
> 
> Ben