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

[Paul Kyzivat <[e-mail]>]

Jim,

Sorry - I forgot to respond sooner.

Please see comments inline.

	Thanks,
	Paul

On 7/19/20 6:29 PM, Jim Schaad wrote:
> 
> 
>> -----Original Message-----
>> From: Paul Kyzivat <[e-mail]>
>> Sent: Sunday, July 19, 2020 1:24 PM
>> To: [e-mail]
>> Cc: General Area Review Team <[e-mail]>
>> Subject: Gen-ART Last Call review of draft-ietf-ace-dtls-authorize-12
>>
>> 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.
> 
> In section 5.8.1 of draft-ietf-ace-oauth-authz is the sentence "The RS MUST
> be prepared to store at least one access token for future use."   When this
> was put in, this is exactly what we were discussing.  There is no
> requirement that an RS needs to store two access tokens for future use.  I
> think this means that there is a strongly bounded requirement on storage.

When I read that, it seems that the stated requirement is to store one 
token *in total*, yet the context implies that it ought to be one token 
*per* client.

The procedures defined aren't going to work unless there is one per client.

> Authors - It might be worthwhile to re-iterate this requirement in both of
> the profile documents.
> 
>>
>> 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.
>>
>> * 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".
> 
> The rule would be MUST implement not MUST offer.  A client could offer a
> completely different ciphersuite that is consistently used in the group and
> that would be fine.

That implies that the client be configured to *know* that the server 
supports something else. I guess that is ok.

>> * 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".
>>
>> * 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"?
>>
>> 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.
> 
> I am seeing this in section 3.4 - did I miss something?

I'm sorry. This issue should have referred to that passage in 3.4, not 
3.3.1.

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