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

[Olaf Bergmann <[e-mail]>]

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

      * If the data comprises additional CWT information, this
        information must be stored as access token for this DTLS
        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 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?

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

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

Grüße
Olaf