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. > * 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. > * 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. 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. _______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
