Re: [OAUTH-WG] DPoP - Document Shepherd Review

[Rifaat Shekh-Yusef <[e-mail]>]

Thanks Brian!
See my replies inline below.


On Thu, Jun 30, 2022 at 6:52 PM Brian Campbell <[e-mail]>
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 <
> [e-mail]> 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.*