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

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

Brian,

Thanks for all the hard work that you put into this important document.
We do not mind it when you are vibrant; we know that you always have good
intentions 😀

Regards,
 Rifaat



On Wed, Jul 27, 2022 at 6:44 PM Brian Campbell <[e-mail]>
wrote:

> Thanks Rifaat and others for the vibrant* discussions about the DPoP draft
> in the side meeting yesterday.
>
> I thought it'd be appropriate to share/reiterate the three action items
> we'd agreed on during the meeting (as I remember anyway):
>
>    1. Justin to review the text about why we have the AT hash and either
>    create a PR adding additional motivations or say that what we have is
>    already sufficient
>    2. Danial to add some text to further explain decisions with respect
>    to PAR
>    3. Brian (aka me) to add a parenthetical remark to the Signature
>    Algorithms subsection listing 'ES256'
>
> PR's for the latter two are here
> <https://github.com/danielfett/draft-dpop/pull/165> and here
> <https://github.com/danielfett/draft-dpop/pull/166> respectively.  And
> yes, this message is, at least in part, a passive-aggressive reminder to
> Justin about #1.
>
> The slides that I used to try and help guide the discussions are attached.
> They are admittedly rather suboptimal but I'm including them for the sake
> of transparency (and because they have a couple of photos).
>
>
> * my apologies for being overly vibrant at times
>
>
>
>
> On Wed, Jul 6, 2022 at 4:32 PM Brian Campbell <[e-mail]>
> wrote:
>
>> 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.*