Thanks Brian! See my replies inline below.
On Thu, Jun 30, 2022 at 6:52 PM Brian Campbell <bcampb...@pingidentity.com> wrote: > Thanks for shepherding Rifaat. And apologies for the slow reply. My > attempts at answering questions and responding to comments are inline > below. > > > On Fri, Jun 3, 2022 at 11:55 AM Rifaat Shekh-Yusef < > rifaat.s.i...@gmail.com> wrote: > >> The following is my review as a document shepherd: >> >> Section 4.3 >> >> Last sentence >> >> Since the document uses “SHOULD”, this implies that there are some valid >> cases where this is not needed. >> >> Should a text be added to explain when this is not needed? >> > > > What about giving a bit more context about why they should? Changing that > sentence to say, "To reduce the likelihood of false negatives, servers > SHOULD employ Syntax-Based Normalization (Section 6.2.2 > <https://rfc-editor.org/rfc/rfc3986> of [RFC3986]) and Scheme-Based > Normalization (Section 6.2.2 <https://rfc-editor.org/rfc/rfc3986> of [ > RFC3986]) before comparing the htu claim." And also maybe changing it to > a little "should". > > Yes, that works. I suggest keeping it as "SHOULD" to encourage implementers to use this, unless they have a really good reason not to. > > >> >> Section 6.1 >> >> 1. >> >> First sentence - what is the reason for using “SHOULD”, instead of >> “MUST” in this case? >> >> > > Good question. I think it was a bit of carryover from OAuth in general not > strictly defining access token format or content. And wanting to not > encroach on that. But that's kinda covered/allowed for in general by > Section 6 already. And Section 6.2 is effectively the same as 6.1 but for > introspection and it doesn't use "SHOULD". I think the “SHOULD” in the first > sentence of 6.1 should be removed thereby making it an implicit must - like > "when using JWT, this is how it is". That would align with the way it's > described for introspection. Also leaves some room for hash algorithm > agility via a new confirmation method member (described in > https://www.ietf.org/archive/id/draft-ietf-oauth-dpop-09.html#name-access-token-and-public-key) > without going against a "MUST" > > I am fine with removing the "SHOULD" to make it an implicit must. > > >> >> 1. >> >> The DPoP Proof contains a hash of the Access Token, and the Access >> Token contains a hash of the public key in the DPoP Proof. >> >> Why do you need both? Would one of these be sufficient? >> > > > The latter (AT containing a hash of the public key in the DPoP Proof) is > needed and largely sufficient for the main goals of binding the AT to a key > held by the client. The former (DPoP Proof containing a hash of the AT) > was added later via very rough WG consensus - it can prevent some esoteric > swapping of tokens that I never really understood to be honest and also > limits the impact of using maliciously precomputed and exfiltrated proofs > (https://www.ietf.org/archive/id/draft-ietf-oauth-dpop-09.html#section-2-6 > talks about it a bit). Use of the nonce mechanism, which was added to the > draft even later, also (and better) protects against precomputed and > exfiltrated proofs. The value of the AT hash in the proof seems somewhat > questionable. To me anyway. But removing it at this point is potentially > problematic due to inertia, existing implementations/deployments, rough WG > consensus, and more. > > I think that at least a text is needed to justify this, and explain the " it can prevent some esoteric swapping of tokens" issue. Maybe we can discuss this during one of the side meetings in Philly. > > >> Section 7.1 >> >> 1. >> >> “if the request does not include valid credentials or does not >> contain an access token sufficient for access, the server can respond >> with a challenge to the client to provide DPoP authentication >> information.” >> >> >> Should the “can” be replaced with a “SHOULD”? >> > > > FWIW, there was some discussion around that sentence that included some > pushback on dropping the "can". > https://github.com/danielfett/draft-dpop/issues/119 and > https://github.com/danielfett/draft-dpop/pull/122 have the conversation. > I'm rather hesitant to try and change it after all that. > > > Ok > >> >> 1. >> >> Also, I think it would be clearer if you can explicitly state what >> the authorization server should do when it does not challenge the client, >> which I am assuming will be something along the lines of: “the >> authorization server issues an error response per Section 5.2 of RFC6749“ >> >> > > The section in question is about protected resource access so anything > about the authorization server wouldn't be appropriate there. The protected > resource / RS always has the option to simply fail the request and can do > that however it sees fit. I'm not sure how to state that in the document > text. Or if anything should be stated, honestly. > > Ok > > >> >> Section 7.2 >> >> 1. >> >> “Specifically, such a protected resource MUST reject a DPoP-bound >> access token received as a bearer token per [RFC6750 >> <https://www.ietf.org/archive/id/draft-ietf-oauth-dpop-08.html#RFC6750> >> ].” >> >> >> I think that I understand what you are trying to say with this sentence, >> but the way the sentence reads is confusing to me. >> >> I am assuming what you are trying to say is something along the lines of >> “a dpop protected resource must reject a request that provides a bearer >> token”. Is that correct? If so, can you please rephrase the sentence to >> make it clearer? >> > > > That's not quite correct. That paragraph > <https://www.ietf.org/archive/id/draft-ietf-oauth-dpop-09.html#section-7.2-1> > (copied below) is attempting say that a protected resource that will > accept either "Authorization: Bearer <bearer token>" or "Authorization: > DPoP <dpop-bound token>" is required to reject a request that uses the > Bearer scheme with a DPoP-bound access token. This is to prevent > downgraded usage of a bound access token without demonstrating possession > of the key to which it is bound. > > "Protected resources simultaneously supporting both the DPoP and Bearer > schemes need to update how evaluation of bearer tokens is performed to > prevent downgraded usage of a DPoP-bound access token. Specifically, such a > protected resource MUST reject a DPoP-bound access token received as a > bearer token per [RFC6750 > <https://www.ietf.org/archive/id/draft-ietf-oauth-dpop-09.html#RFC6750>]." > > Got it. > > >> >> >> 1. >> >> “A protected resource that supports only [RFC6750 >> <https://www.ietf.org/archive/id/draft-ietf-oauth-dpop-08.html#RFC6750>] >> and is unaware of DPoP would most presumably accept a DPoP-bound access >> token as a bearer token” >> >> >> Wouldn't such a resource server check the value of the WWW-Authenticate >> header to make sure it contains the Bearer scheme, which means that the >> request is most likely to be declined? >> > > > What that is trying to say is that a protected resource that only does or > knows about the RFC6750 Bearer scheme ("Authorization: Bearer <token>") > will almost certainly accept a bound access token sent via the Bearer > scheme. > > Ok > > >> >> Section 10.1 >> >> Why define two different mechanisms to achieve the same thing? >> >> This seems to add complexity without an obvious benefit. >> > > > This is a bit of a tricky area. The benefit with PAR is the direct request > from client to AS, which allows for an actual DPoP proof to be used for the > eventual binding of the authorization code to the key. Also the client > doesn't have to do the JWK hash in that case. Whereas the normal > authorization request is indirect via the browser and just a hash of the > key is given for the code binding with the dpop_jkt parameter. And the > client has to compute the hash. But PAR is just an alternative way to pass > the authorization parameters (like dpop_jkt) so it's kinda awkward to use > things together like this. > https://github.com/danielfett/draft-dpop/issues/103 and > https://github.com/danielfett/draft-dpop/pull/111 have some discussion > around this but there was some in person talk too so that's not complete. > > I don't love that there's two different mechanisms here. But it's what we > were able to come up with given all the factors. Certainly open to > considering improvements but am pretty much at a loss of what that might > be. > > Let's discuss this during one of the side meetings in Philly > > >> >> Section 11.6 >> >> Should the algorithms be explicitly called out? Or at least reference a >> document that calls out such algorithms? >> > > > There isn't a single such document and it's not necessarily a static list > of algorithms. I was about to say we could point to the JOSE alg registry > but glancing again at it > https://www.iana.org/assignments/jose/jose.xhtml#web-signature-encryption-algorithms > and I suspect that'd confuse more than help. We could perhaps list > some/many of the algs with the qualification that it's not an exclusive or > complete list? But I'm not sure how useful that would be, to be honest. > > If you do not specify any algorithm, how do you ensure interop? I think this is worth a discussion in Philly > > >> >> Section 11.7 >> >> Why is OAuth Token Binding included? >> > > > Yeah, that doesn't make sense to encourage its use because it's not a > viable thing to use. OAuth Token Binding should be removed from Section > 11.7. Was that what you were getting at? That particular text in the > paragraph that mentions token binding has been in the draft for a long time > and honestly never made a lot of sense to me. So I could envision removing > more. But that's maybe more than you were aiming for. > > I am actually in favor of removing it > >> >> Section 11.8 >> >> Why not include algorithm agility to make sure the mechanism is ready to >> allow for more secure algorithms in the future? >> > > Algorithm agility is a whole can of worms that can be accomplished in > different ways with different amounts of added complexity and potential > vulnerabilities and issues of interop and MTI. Section 11.8 describes how > DPoP allows for algorithm agility (without using the exact words) by > suggesting that new dpop binding cnf method and/or AT hash claim be > defined using a "better" hash algorithm if/when the need arises (OAuth > 2.0 Mutual-TLS takes a similar approach FWIW). The intent of doing it > that way was to keep things as simple as possible in the spec right now > without completely closing the door on future needs. . > > This is another topic that is worth a discussion in Philly. Regards, Rifaat > > *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
