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 <r...@cert.org> 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 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. > On review I think "strongly" and "strong" should just be removed. > ** 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. > Okay > > ** Section 10.1. Typo. s/distingush/distinguish/ > Not sure how we've (or spellcheck) missed that... > > ** 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, 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. > HTTP requests to the same URI for which the jti value has been seen before > would be declined. > Appreciate the recommended wording. Will use. > > ** 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. > Appreciate the recommended wording here too. Will use. ** Section 11.1. Editorial. > > OLD > If jti is enforced unique for > the lifetime of the nonce, there is no additional risk of token > replay. > > > NEW > As long as the jti value is unique for the lifetime of the nonce, there is > no additional risk of token > replay. > The OLD wording is hard to follow (and maybe problematic) but I think that NEW wording subtly misses the intended meaning. How about the following? NEW NEW As long as the jti value is tracked for the lifetime of the nonce, there is no additional risk of token replay. Maybe I'm off in the weeds and hung up on the meaning of one word here though... > > ** Section 11.2. Editorial. Wouldn't it be more precise to mention that > the DPoP proofs are tied to endpoints > > OLD > An attacker in control of the client can pre-generate DPoP proofs for > use arbitrarily far into the future by choosing the iat value in the > DPoP proof to be signed by the proof-of-possession key. > > NEW > An attacker in control of the client can pre-generate DPoP proofs for > specific end points to use arbitrarily far into the future by choosing the > iat value in the DPoP proof to be signed by the proof-of-possession key. > Can include that. > > ** Section 11.5. In additional saying that the typ field should be > checked, perhaps add that the value it must be set to -- "dpop+jwt" > Can add that. > > ** Section 12.17.1. I understand the intent of the change. I am going to > need to consult with IANA on how to handle a modification to a registry > entry for which the IETF is not the change controller. > Sorry about the bureaucratic complications here. Hopefully it can be as simple as IANA just asking the current change controller? I do know the current change controller will be okay with it. Some more info/context in this thread https://mailarchive.ietf.org/arch/msg/oauth/guEDCdAzKO0GRLMiYo3VNU2l0TU/ but basically it's all the same people involved so there shouldn't be any objections. > > ** Section 12.8. Why is the "Permanent Message Header Field Name" > registry being used instead of the Hypertext Transfer Protocol (HTTP) Field > Name Registry" at > https://www.iana.org/assignments/http-fields/http-fields.xhtml. See > Section 5.1 of RFC9110. > I suspect it's because I used an older (or maybe wrong) RFC as a template when doing that section and missed it even when updating other refs in the draft for RFC9110's publication. I'll update to the new/correct way via RFC9110. > > ** Section 11.8. I'd like to have a deeper conversation about the > algorithm agility for the three data elements - ath, jkt and dpop_jkt - > which have SHA256 hard-coded. > > Regardless if more deeper changes are made, dpop_jkt needs to be mentioned > in this section. Yeah, dpop_jkt was a much later addition to the draft and apparently the corresponding updates to this section got missed. Although the next section 11.9 does kinda mention it saying the same considerations apply. > Additionally, the guidance of how to use a different algorithm would > required a bit more text. In addition to specifying and registering the > claim, some document would also have to update this document to allow it's > use and the explain the new guidance for ath and jkt. > > Stepping back, given that ath and jkt are being invented by this spec, > what is the argument for hardcoding the hash algorithm? Why not > future-proof them now by redefining ath/jkt to be a more complicated data > structure with a hash algorithm identifier + the hash value? What would be > the penalty for doing that? I do see a long list of implementations in the > shepherd report. > Maybe we can have a face-to-face conversation next week (though I know it's super busy for you) because expressing some of the history and reasoning behind this isn't easy in writing (for me anyway). But my argument/thinking has basically been that future-proofing isn't entirely free and that going with a single hash can avoid added complexity, MTI and/or agreement, algorithm identifiers or registrations, potential downgrade issues, etc., while SHA-256 is likely okay in this context well into the foreseeable future and some modicum of agility is still possible in the future with a new cnf/claim/parameter name. We did SHA-256 only in rfc8705 for the x5t#S256 confirmation method member. And followed that approach when doing the jkt confirmation method member in DPoP, which was the only hashed thing in the draft for some time. The ath claim and dpop_jkt parameter were later additions to the draft and used SHA-256 for consistency and simplicity. However, to your point, the steps and effort needed to roll out new claim and parameter names in terms of protocol impact and document updates are more onerous. The document shepherd also questioned the current approach in his review FWIW. We discussed it at a side meeting at 114 and managed to get the okay though. But I am admittedly now somewhat rethinking my resolve on the question. The long list of implementations in the shepherd report is a consideration, however, we could come up with something that isn't breaking for existing implementations. Like maybe the encoded hash value could be optionally prefixed with a hash name from the Named Information Hash Algorithm Registry <https://www.iana.org/assignments/named-information/named-information.xhtml> and some delimiter but SHA-256 is the default and used/implied when there's no prefix on the value. > Thanks, > Roman > > _______________________________________________ > OAuth mailing list > OAuth@ietf.org > https://www.ietf.org/mailman/listinfo/oauth > -- _CONFIDENTIALITY NOTICE: This email may contain confidential and privileged material for the sole use of the intended recipient(s). Any review, use, distribution or disclosure by others is strictly prohibited. If you have received this communication in error, please notify the sender immediately by e-mail and delete the message and any file attachments from your computer. Thank you._
_______________________________________________ OAuth mailing list OAuth@ietf.org https://www.ietf.org/mailman/listinfo/oauth
