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

[Benjamin Kaduk <[e-mail]>]

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

>     raw public keys, it needs to determine which key to use. The
>     authorization server can help with this decision by including a
>     `cnf` parameter in the access token that is associated with this
>     communication.  In this case, the resource server MUST use the
>     information from the `cnf` field to select the proper keying
>     material.
> 
> > In Section 3.3:
> >
> >    To convey the same secret to the resource server, the authorization
> >    server either includes it directly in the access token by means of
> >    the "cnf" claim or it provides sufficient information to enable the
> >    resource server to derive the key from the access token using key
> >    derivation.
> >
> > This text could be read as prohibiting using token introspection to
> > obtain the C/RS shared key (if I remember correctly about "claim" being
> > a CWT thing and not a more generic term for a parameter associated with
> > a token).  I don't think that's the intent, so perhaps a wording tweak
> > is in order.
> 
> This now has been changed to:
> 
> NEW:
> 
>     To convey the same secret to the resource server, the
>     authorization server can include it directly in the access token
>     by means of the `cnf` claim or provide sufficient information to
>     enable the resource server to derive the shared secret from the
>     access token. As an alternative, the resource server MAY use token
>     introspection to retrieve the keying material for this access
>     token directly from the authorization server.
> 
> > In Section 3.3.1:
> >
> >    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.
> >
> > The way this was reworded seems to imply that any "psk_identity" with
> > positive length is a full access token (as would have been uploaded to
> > the authz-info endpoint), which prevents the case previously described
> > where the identity is just a COSE_Key with the 'kid' from "rs_cnf".  So
> > I think we still need some kind of description of two potential
> > interpretations of the "psk_identity".
> 
> Yes, this is misleading. We have changed it as follows to indicate the
> two (non-exclusive) processing steps:
> 
> 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?

>       * If the data comprises additional CWT information, this
>         information must be stored as access token for this DTLS

nit: "as an"

>         association before continuing with the DTLS handshake.
> 
>    If the contents of the `psk_identity` do not yield sufficient
>    information to select a valid access token for the requesting
>    client, the resource server aborts the DTLS handshake with an
>    `illegal_parameter` alert.
> 
> > In section 3.4, we updated the text about "a common understanding"
> > between RS and AS about how access tokens are ordered, and reworded the
> > text about checking authorization on every request.  I think we could
> > say a little bit more about how there is not a strict guarantee of
> > processing order for requests and authorization updates, so requests
> > that occur around the time of an authorization update might be processed
> > using either the old or new authorization rules.  Even just referencing
> > Section 7.2 would be enough.
> 
> This now has been clarified as follows, introducing another forward
> reference:
> 
> OLD:
>     The client MUST ascertain that its keying material is still valid
>     before sending a request or processing a response.  If the client
>     gets an error response
> 
> NEW:
> 
>     The client MUST ascertain that its keying material is still valid
>     before sending a request or processing a response. If the client
>     recently has updated the access token (see Section 4), it must be
>     prepared that its request is still handled according to the
>     previous authorization rules as there is no strict ordering
>     between access token uploads and resource access messages. See
>     also Section 7.2 for a discussion of access token
>     processing.
> 
> > (more inline)
> >
> >> > On Tue, Apr 14, 2020 at 06:20:38PM +0200, Olaf Bergmann wrote:
> >> >> Ben,
> >> >> 
> >> >> Thanks again for the thorough review of the ACE DTLS profile and my
> >> >> sincere apologies for the long time it took to process. Please find our
> >> >> comments inline. As this comprises a lot of changes, the proposed text
> >> >> already has been included in the Editor's copy [1].  Our proposals for
> >> >> new text are in the section-by-section comments.
> >> >> 
> >> >> [1] https://ace-wg.github.io/ace-dtls-profile/
> >> >> 
> >> >> Benjamin Kaduk <[e-mail]> writes:
> >> >> 
> >> >> > Hi all,
> >> >> >
> >> >> > Some high-level remarks before delving into the section-by-section
> >> >> > comments:
> >> >> >
> >> >> > This document is pretty clearly DTLS 1.2-specific -- it talks about
> >> >> > particular protocol messages, message fields, and cipher suites that
> >> >> > simply do not apply to DTLS 1.3.  In order to use this profile with DTLS
> >> >> > 1.3, we'd need to specify the analogous behavior/requirements to these
> >> >> > (including standalone signature schemes and key-exchange groups, which
> >> >> > are now both separately negotiated from the cipher suite).
> >> >> > Given that DTLS 1.3 is past WGLC and only a few documents behind this
> >> >> > one in my review queue, it seems fairly prudent to spend some time and
> >> >> > cover how DTLS 1.3 would work here, since otherwise this document will
> >> >> > become stale shortly after (or even before!) its publication as an RFC.
> >
> > Is anyone working on a DTLS 1.3 version?
> 
> Steffi and I have been in the process of starting to take a closer look :-)

I'm happy to hear it :)

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

> >> >> NEW:
> >> >> 
> >> >>     The authorization server must ascertain that the keying material
> >> >>     for the client that it provides to the resource server actually is
> >> >>     associated with this client.  Malicious clients may hand over
> >> >>     access tokens containing their own access permissions to other
> >> >>     entities. This problem cannot be completely
> >> >>     eliminated. Nevertheless, in RPK mode it should not be possible
> >> >>     for clients to request access tokens for arbitrary public keys,
> >> >>     since that would allow the client to relay tokens without the need
> >> >>     to share its own credentials with others. The authorization server
> >> >
> >> > I think this sentence could probably be wordsmithed some more, though I
> >> > don't have a concrete proposal at the moment.  It does seem like we might
> >> > want to discuss this scenario more in terms of "identity misbinding" than
> >> > "relay tokens", though.  (We had a SAAG talk at IETF 104 about misbinding
> >> > attacks on device pairing, which is not quite the same, but shares some of
> >> > the concepts.)
> >> 
> >> Okay. This is the only comment I did not yet address in the text. We
> >> might want to keep this in mind for -11.
> >
> > Looking at it again, perhaps:
> >
> > % eliminated. Nevertheless, in RPK mode it should not be possible
> > % for clients to request access tokens for arbitrary public keys: if the
> > % client can cause the AS to issue a token for a public key without
> > % proving possession of the corresponding private key, this allows for
> > % identity misbinding attacks where the issued token is usable by an
> > % entity other than the intended one.  [...]
> 
> Very good, thank you.
> 
> >> >> OLD:
> >> >> 
> >> >>    The response also contains an access token and an "rs_cnf"
> >> >>    parameter containing information about the public key that is used
> >> >>    by the resource server.  AS MUST ascertain that the RPK specified
> >> >>    in "rs_cnf" belongs to the resource server that C wants to
> >> >>    communicate with.
> >> >> 
> >> >> NEW:
> >> >> 
> >> >>    The response also contains an access token with information about
> >> >>    the public key that is used by the resource server. The
> >> >
> >> > This sounds like "the information[...]" is part of the access token; maybe
> >> > "along with"?
> >> 
> >> What is meant here is that the information is contained in the access
> >> token. The misleading part is that the AT contains information about the
> >> client's RPK that the resource server uses to recognize the client in
> >> the subsequent communication.
> >> 
> >> I have changed this sentence to:
> >> 
> >> NEW:
> >> 
> >>   The response also contains an access token with information for the
> >>   resource server about the client's public key.
> >
> > That should help a lot; thanks!
> >
> >> >> >    The fields in the context information "COSE_KDF_Context"
> >> >> >    (Section 11.2 of [RFC8152]) have the following values:
> >> >> >
> >> >> >    o  AlgorithmID = "ACE-CoAP-DTLS-key-derivation"
> >> >> >
> >> >> > RFC 8152 says that these AlgorithmID values come from the COSE
> >> >> > Algorithms registry, which this does not (unless we register it there,
> >> >> > which we don't currently).  Also, since the output is used directly for
> >> >> > DTLS PSK, I don't think the "key-derivation" portion is needed.
> >> >> >
> >> >> >    o  PartyUInfo = PartyVInfo = ( null, null, null )
> >> >> >
> >> >> > (8152 seems to prefer "nil" over "null".)
> >> >> >
> >> >> >    o  keyDataLength needs to be defined by the application
> >> >> >
> >> >> >    o  protected MUST be a zero length bstr
> >> >> >
> >> >> >    o  other is a zero length bstr
> >> >> >
> >> >> > (These three are all members of the "SuppPubInfo" field, which is not
> >> >> > very clear from how we include them in the same list as top-level
> >> >> > fields.)
> >> >> 
> >> >> The example now has updated and rephrased according to HKDF usage.
> >> >
> >> > The formatting/discussion looks good, but I'll need to cross-check the data
> >> > structures once the new revision is uploaded.
> >
> > I do like the presentation with HKDF and the structured input to it.
> >
> > However, I think we may have over-corrected w.r.t. key-id uniqueness:
> >
> >    In order to generate a new symmetric key to be used by client and
> >    resource server, the authorization server generates a new key
> >    identifier which MUST be unique among all key identifiers used by the
> >    authorization server.  The authorization server then uses the key
> >
> > I think this requirement for uniqueness global to the AS may be too
> > strong; couldn't key identifiers be reused for different RSes?
> > Having the kid be part of the input to HKDF keyed by a unique AS/RS
> > shared key also helps a great deal to avoid accidental key collisions.
> 
> True, uniqueness for each individual RS would be sufficient:
> 
> OLD:
>    MUST be unique among all key identifiers used by the authorization
>    server.
> 
> NEW:
>    MUST be unique among all key identifiers used by the authorization
>    server for this resource server.
> 
> >    To ensure uniqueness of the derived shared secret, the authorization
> >    server SHOULD generate a sufficiently random kid value and include
> >    the claims "iat" and either "exp" or "exi" in the access token.
> >
> > I don't think I understand the motivation for all these statements (and
> > in fact that they may not be entirely correct).  It is permitted to use
> > the 'info' parameter to HKDF for domain separation, so even having the
> > unique kid mentioned above is enough to ensure a unique derived shared
> > secret (and this holds even if the kid uniqueness requirement is
> > weakendt to be per-RS and not per-AS).  Including "iat", "exp", and/or
> > "exi" also helps to ensure uniqueness and should be recommended, but the
> > interrelationships are not stated well here.  Perhaps:
> >
> > % Use of a unique (per RS) "kid" and the use of a unique per-AS/RS key
> > % derivation IKM as specified above will ensure that the derived key is
> > % not shared across multiple clients.  However, to additionally provide
> > % variation in the derived key across different tokens used by the same
> > % client, it is additionally RECOMMENDED to include the "iat" claim and
> > % either the "exp" or "exi" claims in the access token.
> 
> Okay, changed as suggested.
> 
> >> > 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.)

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

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/

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

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.

Section 7.1

nit: s/renogiation/renegotiation/

-Ben