Re: [Gen-art] Gen-ART Last Call review of draft-ietf-ace-dtls-authorize-12

[Paul Kyzivat <[e-mail]>]

Hi Ben,

Please find my comments inline.

On 7/27/20 3:10 PM, Benjamin Kaduk wrote:
> Hi Paul,
> 
> Just a couple notes in addition to what Jim already mentioned.
> 
> On Sun, Jul 19, 2020 at 04:23:46PM -0400, Paul Kyzivat wrote:
>> I am the assigned Gen-ART reviewer for this draft. The General Area
>> Review Team (Gen-ART) reviews all IETF documents being processed
>> by the IESG for the IETF Chair.  Please treat these comments just
>> like any other last call comments.
>>
>> For more information, please see the FAQ at
>>
>> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
>>
>> Document: draft-ietf-ace-dtls-authorize-12
>> Reviewer: Paul Kyzivat
>> Review Date: 2020-07-19
>> IETF LC End Date: 2020-07-20
>> IESG Telechat date: ?
>>
>> Summary:
>>
>> This draft is on the right track but has open issues, described in the
>> review.
>>
>> General:
>>
>> TBD
>>
>> Issues:
>>
>> Major: 2
>> Minor: 1
>> Nits:  1
>>
>> 1) MAJOR: Management of token storage in RS
>>
>> There seems to be an expectation that when the client uploads an access
>> token that the RS will retain it until the client attempts to establish
>> a DTLS association. This seems to require some sort of management of
>> token lifetime in the RS. The only discussion I can find of this issue
>> is the following in section 7:
>>
>>      ... A similar issue exists with the
>>      unprotected authorization information endpoint where the resource
>>      server needs to keep valid access tokens until their expiry.
>>      Adversaries can fill up the constrained resource server's internal
>>      storage for a very long time with interjected or otherwise retrieved
>>      valid access tokens.
>>
>> This seems to imply a normative requirement to keep tokens until their
>> expiry. But I find no supporting normative requirements about this. And,
>> this section only presents it as a DoS attack, rather than a potential
>> problem with valid usage.
>>
>> ISTM that there is an implied requirement that the RS have the capacity
>> to store one access token for every PoP key of every authorized client.
>> If so, that ought to be stated. If not, then some other way of bounding
>> storage ought to be discussed.
>>
>> 2) MAJOR: Missing normative language
>>
>> I found several places where the text seems to suggest required behavior
>> but fails to do so using normative language:
>>
>> * In section 3.3:
>>
>>      ... Instead of
>>      providing the keying material in the access token, the authorization
>>      server includes the key identifier in the "kid" parameter, see
>>      Figure 7.  This key identifier enables the resource server to
>>      calculate the symmetric key used for the communication with the
>>      client using the key derivation key and a KDF to be defined by the
>>      application, for example HKDF-SHA-256.  The key identifier picked by
>>      the authorization server needs to be unique for each access token
>>      where a unique symmetric key is required.
>>      ...
>>      Use of a unique (per resource server) "kid" and the use of a key
>>      derivation IKM that is unique per authorization server/resource
>>      server pair as specified above will ensure that the derived key is
>>      not shared across multiple clients.
>>
>> The uniqueness seems to be a requirement. Perhaps "needs to be unique"
>> should be "MUST be unique". (And something similar for the IKM.)
>>
>> * Also in section 3.3:
>>
>>      All CBOR data types are encoded in CBOR using preferred serialization
>>      and deterministic encoding as specified in Section 4 of
>>      [I-D.ietf-cbor-7049bis].  This implies in particular that the "type"
>>      and "L" components use the minimum length encoding.  The content of
>>      the "access_token" field is treated as opaque data for the purpose of
>>      key derivation.
>>
>> IIUC the type of serialization and encoding is a requirement. Will need
>> some rewording to make it so.
> 
> Both this and the previous item are scoped by the following text:
> 
>     The method for how the resource server determines the symmetric key
>     from an access token containing only a key identifier is application-
>     specific; the remainder of this section provides one example.
> 
> So it's not entirely clear that normative language is appropriate in the
> discussion of the example behavior.

OK. That makes sense.

>> * In section 3.3.1:
>>
>>      ... To
>>      be consistent with the recommendations in [RFC7252] a client is
>>      expected to offer at least the ciphersuite TLS_PSK_WITH_AES_128_CCM_8
>>      [RFC6655] to the resource server.
>>
>> I think "is expected" should be "MUST".
>>
>> * Also in section 3.3.1:
>>
>>      ... This
>>      specification assumes that the access token is a PoP token as
>>      described in [I-D.ietf-ace-oauth-authz] unless specifically stated
>>      otherwise.
>>
>> I think "assumes ... unless" should be "MUST ... unless".
> 
> My understanding is that this is just talking about the text in the
> document itself.  But as far as I remember we always require PoP tokens, so
> this could just be removed.

It gets simpler if you always require PoP tokens. Does it state that 
normatively somewhere?

The "unless" construct opens a can of worms about how things are to work 
in that case.

>> * Also in section 3.3.1:
>>
>>      ... New access tokens issued by the
>>      authorization server are supposed to replace previously issued access
>>      tokens for the respective client.
>>
>> Is this normative? Should "are supposed to" be "MUST"?
> 
> I don't think this is a "MUST"; it refers to some behavior in the core
> framework that is merely suggested and not mandatory.

I commented on this in my reply to Jim.

	Thanks,
	Paul

> Thanks for the review; it brought up some good points!
> 
> -Ben
> 
>> 3) MINOR: Insufficient specification
>>
>> (I'm waffling whether this is minor or major.)
>>
>> There are a couple of places where what seem to be requirements are
>> stated too vaguely to be implemented consistently:
>>
>> * In the previously mentioned paragraph in 3.3.1:
>>
>>      ... This
>>      specification assumes that the access token is a PoP token as
>>      described in [I-D.ietf-ace-oauth-authz] unless specifically stated
>>      otherwise.
>>
>> The "unless specifically stated otherwise" is too vague to be normative.
>> How would the alternative be indicated? Is this an escape hatch for
>> future extensions? If so, it needs more work to make that clear and to
>> open a path for that future work.
>>
>> * Also in section 3.3.1:
>>
>>      ... The resource server therefore must
>>      have a common understanding with the authorization server how access
>>      tokens are ordered.
>>
>> The last statement ("must have a common understanding") is mysterious.
>> IIUC section 4 is covering the same topic in a less mysterious way.
>>
>> 4) NIT: Subsection organization
>>
>> Both 3.2 and 3.3 share a common structure:
>>
>> * The section begins with discussion of the interaction between the
>> client and the AS.
>>
>> * it is followed by a subsection discussing the interaction between the
>> client and the RS.
>>
>> It is odd to have a section with a single subsection. And the structure
>> isn't easily discerned from the TOC.
>>
>> I suggest it would be clearer if each of these sections had *two*
>> subsections, one covering the AS interactions and the other the RS
>> interactions. IOW, put the material covering the AS interactions into a
>> new subsection.