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

[Brian Campbell <[e-mail]>]

Thanks Rifaat!
I will make those changes in the document source and come to Philly
prepared to discuss the other items. One of the side meetings seems like a
good forum for that, good idea.

On Tue, Jul 5, 2022 at 11:14 AM Rifaat Shekh-Yusef <[e-mail]>
wrote:

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

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