Re: [OAUTH-WG] AD Review of draft-ietf-oauth-dpop-11
Thank you for the detailed review, Roman. I've endeavored to reply to each item inline below. On Thu, Oct 27, 2022 at 7:50 PM Roman Danyliw wrote: > Hi! > > I performed an AD review on draft-ietf-oauth-dpop-11. Thanks for this > document. Comments below. > > ** The document has 6 listed authors. Could this rationale for this be > explained on the list and captured in the shepherd write-up. > The rationale is basically that the six of us were together at the inception of the idea to create the initial draft. The individual level of contribution to the work from each hasn't exactly been equal since that time but there's not been an obvious route to get down to five (or fewer) authors. Is some version of that sufficient for the shepherd write-up? We did discuss this during the WG meeting at IETF 113 but didn't really get to the details of the rationale. It is admittedly/unfortunately a source of some anxiety for me. > ** Section 2. > (CRIME, >BREACH, Heartbleed, and the Cloudflare parser bug are some examples). > There have also been numerous published token theft attacks on OAuth >implementations themselves > > Good pointers. Could informative references be provided for them. > Yup, I'll look to provide reasonably appropriate and stable references as informational. > > ** Section 4.2. > > alg: a digital signature algorithm identifier such as per > [RFC7518]. > > Shouldn't this text constraint the algorithm identifier as coming from a > registry pointed to in RFC7518 (i.e., > https://www.iana.org/assignments/jose/jose.xhtml#web-signature-encryption-algorithms > )? > I think that makes sense. Although unregistered values are allowed in JWS per https://www.rfc-editor.org/rfc/rfc7515#section-4.1.1 they don't seem useful or needed in this context. And I've been hesitant about pointing to the registry because it has a lot of entries that are not signature algorithms and I wasn't sure it'd be a particularly helpful reference. But that said, it's probably the right reference so I'll adjust accordingly. > > ** Section 4.1. > > htm: The HTTP method of the request to which the JWT is attached, > as defined in [RFC9110]. > > Shouldn't the values in this field come from the registry HTTP methods > registry: > https://www.iana.org/assignments/http-methods/http-methods.xhtml? > RFC9110 specifies the semantics of the fields, not the values. > I'm honestly not sure. HTTP seems to me like the right reference because it's saying use the value of this method thing that HTTP defines. The registry has the registered values but it's the method thing that's being talked about here. Would this be better? "htm: The value of the HTTP method (Section 9.1 of [RFC9110]) of the request to which the JWT is attached." Or am I missing the point and going in the wrong direction with this? > ** Section 4.1. Shouldn't these text read like the description of htm: > > OLD > The HTTP target URI (Section 7.1 of [RFC9110]), without query > and fragment parts. > > NEW > The HTTP target URI (Section 7.1 of [RFC9110]), without query and fragment > parts of the request to which the JWT is attached. > Will change. > > ** Section 4.2. Editorial. > > OLD > But that it be a minimal subset of the HTTP data so as to >avoid the substantial difficulties inherent in attempting to >normalize HTTP messages. > > NEW > This design approach of using only a minimal subset of the HTTP header > data is to avoid the substantial difficulties inherent in attempting to > normalize HTTP messages. > That reads much better, thank you. Will change. > ** Section 4.3. Editorial. Clarifying. > > OLD > the header field value is a well-formed JWT, > > NEW > the DPoP HTTP request header field value is a well-formed JWT, > Okay, makes sense, will change. ** Section 4.3. > > the alg JOSE header parameter indicates an asymmetric digital > signature algorithm, is not none, is supported by the application, > and is deemed secure, > > -- check feedback already provided in Section 4.2 about this field > pointing to a value in a JOSE registry > -- "is deemed secure" seems open ended. Perhaps "and is acceptable per > local policy" > Yeah, that seems a bit less open ended. Will update. > ** Section 5. Normative language > > OLD > The client must discard the response in this case, > > NEW > The client MUST ... > Will change. > > ** Section 5.1. Cite the relevant registry which would the source of JWS > alg values in the dpop_signing_alg_values_supported array > Will do. > > ** Section 7 > >Binding the token value to the proof in this way prevents a >calculated proof to be used with multiple different access token >values across different requests. > > What makes a proof "calculated"? > I'm not sure, to be honest. I think that's an extraneous word that should just be removed. > > ** Section 7. > > its inclusion strongly >binds the access token value to the holder of the
[OAUTH-WG] AD Review of draft-ietf-oauth-dpop-11
Hi! I performed an AD review on draft-ietf-oauth-dpop-11. Thanks for this document. Comments below. ** The document has 6 listed authors. Could this rationale for this be explained on the list and captured in the shepherd write-up. ** Section 2. (CRIME, BREACH, Heartbleed, and the Cloudflare parser bug are some examples). There have also been numerous published token theft attacks on OAuth implementations themselves Good pointers. Could informative references be provided for them. ** Section 4.2. alg: a digital signature algorithm identifier such as per [RFC7518]. Shouldn't this text constraint the algorithm identifier as coming from a registry pointed to in RFC7518 (i.e., https://www.iana.org/assignments/jose/jose.xhtml#web-signature-encryption-algorithms)? ** Section 4.1. htm: The HTTP method of the request to which the JWT is attached, as defined in [RFC9110]. Shouldn't the values in this field come from the registry HTTP methods registry: https://www.iana.org/assignments/http-methods/http-methods.xhtml? RFC9110 specifies the semantics of the fields, not the values. ** Section 4.1. Shouldn't these text read like the description of htm: OLD The HTTP target URI (Section 7.1 of [RFC9110]), without query and fragment parts. NEW The HTTP target URI (Section 7.1 of [RFC9110]), without query and fragment parts of the request to which the JWT is attached. ** Section 4.2. Editorial. OLD But that it be a minimal subset of the HTTP data so as to avoid the substantial difficulties inherent in attempting to normalize HTTP messages. NEW This design approach of using only a minimal subset of the HTTP header data is to avoid the substantial difficulties inherent in attempting to normalize HTTP messages. ** Section 4.3. Editorial. Clarifying. OLD the header field value is a well-formed JWT, NEW the DPoP HTTP request header field value is a well-formed JWT, ** Section 4.3. the alg JOSE header parameter indicates an asymmetric digital signature algorithm, is not none, is supported by the application, and is deemed secure, -- check feedback already provided in Section 4.2 about this field pointing to a value in a JOSE registry -- "is deemed secure" seems open ended. Perhaps "and is acceptable per local policy" ** Section 5. Normative language OLD The client must discard the response in this case, NEW The client MUST ... ** Section 5.1. Cite the relevant registry which would the source of JWS alg values in the dpop_signing_alg_values_supported array ** Section 7 Binding the token value to the proof in this way prevents a calculated proof to be used with multiple different access token values across different requests. What makes a proof "calculated"? ** Section 7. its inclusion strongly binds the access token value to the holder of the key used to generate the signature. Is "strongly binds" the same as "cryptographically binds"? "Strong" is used in the next paragraph too. Please review it. ** Section 10.1. Editorial. OLD The dpop_jkt parameter can be used as described above to bind the issued authorization code to a specific key. NEW The dpop_jkt parameter can be used as described in Section 10 to bind the issued authorization code to a specific key. ** Section 10.1. Typo. s/distingush/distinguish/ ** Section 11.1. Editorial. Recommend unpacking this dense sentence. OLD To prevent multiple uses of the same DPoP proof servers can store, in the context of the target URI, the jti value of each DPoP proof for the time window in which the respective DPoP proof JWT would be accepted and decline HTTP requests to the same URI for which the jti value has been seen before. NEW To prevent multiple uses of the same DPoP proof, servers can store, inthe context of the target URI, the jti value of each DPoP proof forthe time window in which the respective DPoP proof JWT would beaccepted. HTTP requests to the same URI for which the jti value has been seen before would be declined. ** Section 11.1. Recommend unpacking this dense sentence. OLD Because clock skews between servers and clients may be large, servers may choose to limit DPoP proof lifetimes by using server-provided nonce values containing the time at the server rather than comparing the client-supplied iat time to the time at the server, yielding intended results even in the face of arbitrarily large clock skews. NEW Because clock skews between servers and clients may be large, servers MAY limit DPoP proof lifetimes by using server-provided nonce values containing the time at the server rather than comparing the client-supplied iat time to the time at the server. Nonces created in this way yield the same result even in the face of arbitrarily large clock skews. ** Section 11.1. Editorial. OLD If jti is enforced unique for the lifetime of the nonce, there is no additional
