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

[Brian Campbell <[e-mail]>]

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".



>
> 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"



>
>    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.



> 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.



>
>
>    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.



>
> 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>]."



>
>
>    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.



>
> 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.



>
> 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.



>
> 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.



>
> 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. .

-- 
_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._