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

[Ludwig Seitz <[e-mail]>]

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

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

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

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

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

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


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

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

/Ludwig

-- 
Ludwig Seitz, PhD
Security Lab, RISE
Phone [phone]