Re: [Ace] AD review of draft-ietf-ace-dtls-authorize-09

[Benjamin Kaduk <[e-mail]>]

Hi Olaf,

On Tue, Jun 30, 2020 at 12:19:41PM +0200, Olaf Bergmann wrote:
> Hi Ben,
> 
> Thank you for the quick followup and another set of very useful
> comments. I have created a working copy with updates as listed
> inline. If you want me to, I can submit these as -12.

If it's easy for you, please go ahead.
Inline.

> Benjamin Kaduk <[e-mail]> writes:
> 
> > Hi Olaf,
> >
> > Thanks for the updated -11!
> > Some minor replies below, though in general the proposals look good.
> >
> > On Thu, Jun 18, 2020 at 02:38:32PM +0200, Olaf Bergmann wrote:
> >> Hi Benjamin,
> >> 
> >> Benjamin Kaduk <[e-mail]> writes:
> >> 
> >> > Thanks!  I think we will probably need an -11 fairly soon, though, given
> >> > the remaining comments I have.
> >> 
> >> We have just submitted -11 that addresses your comments. See below for
> >> detailed responses. I have trimmed-down the email thread context a bit
> >> to improved readability and focus on the recent comments.
> >> 
> >> > A couple general remarks looking at the -09-to-10 diff (not all new in
> >> > the -10, so I guess I missed it the first time around):
> >> >
> >> > In Section 3.2.1, I suggest applying:
> >> >
> >> > OLD:
> >> >   [...] If CBOR web tokens [RFC8392] are used as recommended in
> >> >   [I-D.ietf-ace-oauth-authz] [...]
> >> >
> >> > NEW:
> >> >   [...] If CBOR web tokens [RFC8392] are used (as recommended in
> >> >   [I-D.ietf-ace-oauth-authz]) [...]
> >> >
> >> > Making this a parenthetical statement makes it clear that the
> >> > conditional for "keys MUST be encoded" is "CWT are used" and separate
> >> > from the recommendation to do so.
> >> 
> >> Done
> >> 
> >> > Also in 3.2.1, we now have these two statements in fairly close
> >> > proximity:
> >> >
> >> >    [RFC8747].  The resource server MUST use its own raw public key in
> >> >    the DTLS handshake with the client.  If the resource server has
> >> >    several raw public keys, it must already know which key it is
> >> >    supposed to use with this client.  How this is achieved is not part
> >> >    of this profile.
> >> >
> >> >    The resource server MUST use the keying material that the
> >> >    authorizations server has specified in the "cnf" parameter in the
> >> >    access token for the DTLS handshake with the client.  Thus, the
> >> >
> >> > which seems a bit redundant and potentially in conflict.  Can we
> >> > consolidate this requirement a bit?
> >> 
> >> Yes, the wording is a bit confusing. The following proposal states
> >> the intention more clearly, I think:
> >> 
> >> NEW: 
> >> 
> >>     The raw public key used in the DTLS handshake with the client MUST
> >>     belong to the resource server. If the resource server has several
> >
> > (Someone might complain that this first sentence is tautological, but I
> > think we should leave it for now.)
> 
> Okay.
> 
> >> OLD:
> >>    If a resource server receives a ClientKeyExchange message that
> >>    contains a `psk_identity` with a length greater than zero, it MUST
> >>    process the contents of the `psk_identity` field as access token
> >>    that is stored with the authorization information endpoint, before
> >>    continuing the DTLS handshake. If the contents of the
> >>    `psk_identity` do not yield a valid access token for the requesting
> >>    client, the resource server aborts the DTLS handshake with an
> >>    `illegal_parameter` alert.
> >> 
> >> NEW:
> >> 
> >>    If a resource server receives a ClientKeyExchange message that
> >>    contains a `psk_identity` with a length greater than zero, it MUST
> >>    parse the contents of the `psk_identity` field as CBOR data
> >>    structure and process the contents as following:
> >> 
> >>       * If the data contains a `cnf` field with a `COSE_Key` structure
> >>         with a `kid`, the resource server continues the DTLS handshake
> >>         with the stored key associated with this kid.
> >
> > Maybe "corresponding" or "associated" stored key, since there is not
> > necessarily a unique kid/key relationship?

Rereading, my suggestion of "associated" doesn't make much sense, sorry.

> Hm, I was hoping that this is sufficiently expressed by "the stored
> key associated with this kid". My assumption is that we have a mapping
> from kids to keys where a kid is mapped to at most one key. (Usually,
> this would be a bijection but there might be cases where a kid is known
> and the key is not yet known or has expired.
> 
> How about this:
> 
> OLD:
> 
>      continues the DTLS handshake with the stored key associated with
>      this kid.
> 
> NEW:
> 
>      continues the DTLS handshake with the associated key that corresponds
>      to this kid.

I think this is better, thanks.

> >>       * If the data comprises additional CWT information, this
> >>         information must be stored as access token for this DTLS
> >
> > nit: "as an"
> 
> Fixed, thanks!
> 
> >> >> > I think we need to give some guidance about what the semantics are if/when
> >> >> > it happens.  My inclination would be to say that DTLS resumption can be
> >> >> > used to amortize the cost of a handshake, but DTLS 1.2 resumption does not
> >> >> > provide forward secrecy and also does not provide an opportunity for the
> >> >> > client to prove possession of the PoP key, so servers will want to
> >> >> > periodically force a full handshake according to their authentication
> >> >> > requirements.  I'd also be inclined to forbid renegotiation entirely.
> >> >> 
> >> >> True, especially because RFC 7925 requires resumption to be
> >> >> implemented. The security considerations now advise against session
> >> >> resumptions and renegotiation as you have suggested. The Security
> >> >> Considerations section now has been structured in sub-sections for
> >> >> clarity.
> >> >
> >> > The discussion in the -10 is reasonable (and the stated motivation of
> >> > having a fresh proof of possession is fairly compelling), though to be
> >> > clear, my initial "inclination" above was just to forbid renegotiation
> >> > but allow resumption.
> >> 
> >> Sorry, I had misinterpreted that. Actually, I am not entirely
> >> convinced by the "fresh PoP" argument anymore. When the access token
> >> made its way to the server through the client, there is no significant
> >> difference between keeping the DTLS session open and session
> >> resumption. Additional security against breaching the encryption of
> >> the (encrypted part of the) access token would be provided only by token
> >> introspection. But in that case, session resumption again would not
> >> harm security as we do not need to store precious information in the
> >> session ticket.
> >> 
> >> Are you in favor of allowing session resumption?
> >
> > I think it would be safe to allow resumption, but I personally am
> > indifferent about whether or not we should do so.
> 
> Okay. Currently, the section has a discussion that concludes in a NOT
> RECOMMENDED. If someone argues for generally allowing session resumption
> we would not have to do much more than just removing that conclusion.

Okay.

> >> >> > Up in Section 3.3 we talk about a case where the AS and RS are assumed to
> >> >> > share a key derivation key that's used to derive symmetric keys for the RS
> >> >> > to share with the client.  The KDF input is the IKM and the 'info' (since
> >> >> > 'salt' is empty), and the IKM is just that same "key derivation key".  So
> >> >> > the only input to HKDF that changes for different clients (and the same RS)
> >> >> > is the 'info'.  The 'info', in turn contains a string constant 'type' plus
> >> >> > the 'kid' and (presumably fixed) output length.  If I'm reading this
> >> >> > correctly, the 'kid' is the only HKDF input that varies between clients, so
> >> >> > if the 'kid' was reused for different clients, the AS and RS would derive
> >> >> > the same symmetric key for those distinct client/RS pairs.  Furthermore,
> >> >> > since the AS returns the actual symmetric key to the client in the token
> >> >> > response, that means that the symmetric key is shared by *three* parties
> >> >> > (vs. just client and RS, ignoring in the count the AS which knows
> >> >> > everything).  I guess technically this is not the scenario forbidden by the
> >> >> > core framework (which is to have the same symmetric key used at multiple
> >> >> > RSes, but it still is bad hygeine and would let one client impersonate the
> >> >> > other.
> >> >> >
> >> >> > In cases not using the KDF, the 'kid' is just a hint to finding the actual
> >> >> > key, and the actual key is assumed to be constructed uniquely by the AS
> >> >> > each time.
> >> >> 
> >> >> Thank you, this is absolutely true. I have now changed the input for the
> >> >> key derivation mechanism in the sense of what originally has been
> >> >> specified in draft-gerdes-ace-dcaf-authorize, i.e., including the access
> >> >> token as input to the KDF. This enables the authorization server to
> >> >> include a certain amount of unifying attributes in the info parameter,
> >> >> esp. with a randomly generated kid and the iat claim. I would have loved
> >> >> to have a claim type for carrying a nonce in the access token…
> >> >
> >> > Hmm, we say that "access_token is the decrypted access_token as
> >> > transferred from the AS to the RS", but it's supposed to be a CBOR map;
> >> > is this intended to just be the claims set from the CWT?  If so, we
> >> > should say that.
> >> 
> >> I do not see the difference: The access token is comprised of a CWT
> >> claim set which is represented on the wire as a CBOR map. 
> >
> > The Message of the CWT is a CBOR map, but the access token itself is not
> > just the Message, and "the decrypted access_token" is perhaps a bit
> > informal of language for what we wang.  (I think I was a bit confused when
> > I wrote the original comment, though, as for JWT the structure is a bit
> > different and the Claims Set a much more distinct object.)
> 
> Maybe this is a bit more specific without adding a more detailed
> explanation of how this is supposed to work below the bullet list:
> 
> OLD:
> 
>    access_token is the decrypted access_token as transferred from the
>    authorization server to the resource server.
> 
> NEW:
> 
>     access_token is the decrypted content of the access_token field
>     as transferred from the authorization server to the resource
>     server.

Okay.  (I note also the further discussion downthread regarding "innermost
COSE message" and such.)

> >> Unfortunately, this is not explained in draft-ietf-ace-oauth-params.
> >> 
> >> To avoid confusion here, the following clarification has been added:
> >> 
> >> NEW:
> >> 
> >>     The decrypted access token usually denotes a CWT claim set
> >>     represented as CBOR map.
> >
> > Thanks.
> >
> >> >> As there is no guidance so far on how permission sets in access tokens
> >> >> are to be combined properly it seems to be wise at least to advise
> >> >> against.
> >> >
> >> > I agree that's the right advice to give; what is less clear to me is
> >> > whether this document needs to be the place to give that advice (vs. the
> >> > framework doing so).  But I don't have a strong stance on this at the
> >> > moment, and am happy to move forward with the text as-is if you want.
> >> 
> >> Okay, to avoid last-minute changes to the framework document I leave
> >> it as it is.
> >> 
> >> >> >> > I think RFCs 7250, 7251, and 8422 need to be normative.
> >> >> >> 
> >> >> >> done
> >> >> 
> >> >> Actually, I had to revert this for the informational RFC 7251 as the
> >> >> idnits tool did not allow the downref from normative.
> >> >
> >> > idnits is just a tool to raise potential issues that a human can look
> >> > at and think about; in this case, idnits is wrong: 7251 is listed at
> >> > https://datatracker.ietf.org/doc/downref/ and is perfectly fine to use
> >> > as a normative reference.
> >> 
> >> Oh, sorry, I was not aware of that. This now has been reverted.
> >
> > Thanks for all the updates!
> >
> > I did have just a few nits from looking at the diff from -09 to -11; we can
> > probably address those along with other IETF LC feedback (though fixing
> > them earlier is not discouraged).
> 
> That sounds as if shipping -12 now would be okay :-)

Right :)

> > Section 3.2
> >
> >    An example access token response from the authorization to the client
> >    is depicted in Figure 4.  Here, the contents of the "access_token"
> >
> > nit: s/authorization/authorization server/
> 
> Fixed, thanks!
> 
> > Section 3.3
> >
> > I guess if we want the AS and RS to get the same key-derivation output we
> > need to say that the encoding for 'L' is the minimum-length encoding
> 
> Good point. As this is also true for the length encoding of the text
> string in the type field, I have added the following clarification:
> 
> NEW:
> 
>    All CBOR data types are encoded in canonical CBOR as defined in
>    Section 3.9 of {{RFC7049}}. This implies in particular that the
>    `type` and `L` components use the minimum length encoding.
> 
> 
> (This unfortunately imposes additional restrictions on the
> access_token which would not be required for key derivation. But IIRC
> there has been some implicit agreement that the access_token should be
> encoded in canonical CBOR as well.

[this is handled downthread]

> > Section 7
> >
> >    valid access tokens.  The protection of access tokens that are stored
> >    in the authorization information endpoint depends on the keying
> >    material that is used between the authorization server and the
> >    resource server: The resource server must ensure that it processes
> >    only access tokens that are encrypted and integrity-protected by an
> >    authorization server that is authorized to provide access tokens for
> >    the resource server.
> >
> > We may want to put "encrypted and" in parentheses, as we don't require the
> > use of encrypted CWTs.
> 
> Done. However, the only situation where encryption is not a MUST is in
> case of key derivation.
> 
> > Section 7.1
> >
> > nit: s/renogiation/renegotiation/
> 
> Thanks. I had to read this word several times to spot the difference :-)

:)

Thanks,

Ben