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

[Olaf Bergmann <[e-mail]>]

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.

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?

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.

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

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

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

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

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

Grüße
Olaf