Re: [OAUTH-WG] Feedback for draft-ietf-oauth-dpop-09

[Mike Jones]

I concur with Brian's evaluation of these proposed changes.

-- Mike


From: OAuth  on behalf of Brian Campbell 

Sent: Wednesday, July 6, 2022 1:53:17 PM
To: Spencer Balogh 
Cc: oauth@ietf.org 
Subject: Re: [OAUTH-WG] Feedback for draft-ietf-oauth-dpop-09

Thanks Spencer,

As you've likely surmised by now, my appetite for changes is fairly small at 
this point. But I can see the value in adding the corresponding decoded content 
in those spots for readability. So would definitely welcome a PR that is scoped 
to just that.

On Sun, Jul 3, 2022 at 7:27 PM Spencer Balogh 
mailto:spbal...@gmail.com>> wrote:
Hi Brian,

I'd definitely be happy to get the nonce and client considerations changes in, 
and I'm happy to propose more text tweaks if there's still any ambiguity there. 
Those were some of my largest concerns as a potential server or client 
implementer of this spec.

With respect to # PR Fully specified 
examples, I can definitely 
understand the concerns around the private key and , and I was 
feeling mixed on that in the first place. Would there be more appetite for this 
with the private keys and signatures removed, or at least adding a 
corresponding decoded content figure to `Figure 4: Token Request for a DPoP 
sender-constrained token using an authorization code`, and `Figure 6: Token 
Request for a DPoP-bound Token using a Refresh Token`, similar to `Figure 12: 
DPoP Protected Resource Request`'s corresponding `Figure 13: Decoded Content of 
the DPoP Proof JWT in Figure 12`. I think the decoded messages in particular 
help readability a good bit. I suppose this is getting into my preferences as 
well though. :)

Also understood on the layer violation for `dpop_signing_alg_values_supported` 
in validation. I'll go ahead and close that one.

Let me know what you think.

Thanks,
Spencer Balogh


On Fri, Jul 1, 2022 at 3:26 PM Brian Campbell 
mailto:bcampb...@pingidentity.com>> wrote:
Hi Spencer,

Thanks for the interest and feedback, even if it's a little late in the game.

I think the proposed changes in # PR Nonce clarifications, more complete 
documentation, and guidance 
are a nice improvement that closes the disconnect you pointed out. We'll need 
to produce a new -10 draft revision with updates from the Shepherd Review and 
I'd like to incorporate the content of this PR in that as well. Unless there 
are meaningful objections from the WG. I don't think that's overstepping things 
too much process-wise and I assume the Chairs will speak up if it is.

The new subsection in # PR Proposal Section 7.2 Client 
Considerations seems 
reasonable. Barring any objections from the WG, this can probably be 
incorporated as well.

The changes in # PR Fully specified 
examples are too much at 
this stage. Some of the JSON is invalid. There are stylistic and aesthetic 
differences and then inconsistencies. I think the private key does hurt in this 
context because it's a potential distraction and might lead to readers trying 
to reproduce the proofs (which won't work b/c ES256 isn't deterministic). There 
may be other objections but even this has involved a not insignificant amount 
of time.  Admittedly some of this is subjective or preference based but 
hopefully you can understand that there's risk and cost involved. And it's late 
in the process.

The  # PR Checking DPoP Proofs 
tweaks change is not 
workable because it pulls an authorization server specific construct as a 
normative requirement into a section that applies to both authorization server 
and resource server. It's kind of like a compile time error for a spec, if 
there was such a thing. The note about order not being implied could be added 
tho. Or the list could be changed to use bullets and not have numbering. I 
don't recall there being any real reason for the numbering.





On Thu, Jun 30, 2022 at 10:12 AM Spencer Balogh 
mailto:spbal...@gmail.com>> wrote:
Hi all,

I'm new to all of this, please forgive me if I'm missing any expectations or 
conventions here.

I have a bit of feedback for OAuth 2.0 Demonstrating Proof-of-Possession at the 
Application Layer
(DPoP) draft-ietf-oauth-dpop-09 
(https://datatracker.ietf.org/doc/html/draft-ietf-oauth-dpop). I'm aware that 
you're already after the Working Group Last Call, so I'm trying to come in with 
mostly non-normative changes, and with draft suggestions rather than just open 
questions. I've opened several pull requests against the RFC repo so that 
needed changes can be easily incorporated, and not get blocked by changes that 
are more my preference.

I also wanted to follow up here to provide a little bit more context on the 
change proposals.

# PR Nonce clarifications, more complete documentation, and 

Re: [OAUTH-WG] Feedback for draft-ietf-oauth-dpop-09

[Brian Campbell]

Thanks Spencer,

As you've likely surmised by now, my appetite for changes is fairly small
at this point. But I can see the value in adding the corresponding decoded
content in those spots for readability. So would definitely welcome a PR
that is scoped to just that.

On Sun, Jul 3, 2022 at 7:27 PM Spencer Balogh  wrote:

> Hi Brian,
>
> I'd definitely be happy to get the nonce and client considerations changes
> in, and I'm happy to propose more text tweaks if there's still any
> ambiguity there. Those were some of my largest concerns as a potential
> server or client implementer of this spec.
>
> With respect to # PR Fully specified examples
> , I can definitely
> understand the concerns around the private key and , and I was
> feeling mixed on that in the first place. Would there be more appetite for
> this with the private keys and signatures removed, or at least adding a
> corresponding decoded content figure to `Figure 4: Token Request for a DPoP
> sender-constrained token using an authorization code`, and `Figure 6: Token
> Request for a DPoP-bound Token using a Refresh Token`, similar to `Figure
> 12: DPoP Protected Resource Request`'s corresponding `Figure 13: Decoded
> Content of the DPoP Proof JWT in Figure 12`. I think the decoded messages
> in particular help readability a good bit. I suppose this is getting into
> my preferences as well though. :)
>
> Also understood on the layer violation for
> `dpop_signing_alg_values_supported` in validation. I'll go ahead and close
> that one.
>
> Let me know what you think.
>
> Thanks,
> Spencer Balogh
>
>
> On Fri, Jul 1, 2022 at 3:26 PM Brian Campbell 
> wrote:
>
>> Hi Spencer,
>>
>> Thanks for the interest and feedback, even if it's a little late in the
>> game.
>>
>> I think the proposed changes in # PR Nonce clarifications, more complete
>> documentation, and guidance
>>  are a nice
>> improvement that closes the disconnect you pointed out. We'll need to
>> produce a new -10 draft revision with updates from the Shepherd Review and
>> I'd like to incorporate the content of this PR in that as well. Unless
>> there are meaningful objections from the WG. I don't think that's
>> overstepping things too much process-wise and I assume the Chairs will
>> speak up if it is.
>>
>> The new subsection in # PR Proposal Section 7.2 Client Considerations
>>  seems reasonable.
>> Barring any objections from the WG, this can probably be incorporated as
>> well.
>>
>> The changes in # PR Fully specified examples
>>  are too much at this
>> stage. Some of the JSON is invalid. There are stylistic and aesthetic
>> differences and then inconsistencies. I think the private key does hurt in
>> this context because it's a potential distraction and might lead to readers
>> trying to reproduce the proofs (which won't work b/c ES256 isn't
>> deterministic). There may be other objections but even this has involved a
>> not insignificant amount of time.  Admittedly some of this is subjective or
>> preference based but hopefully you can understand that there's risk and
>> cost involved. And it's late in the process.
>>
>> The  # PR Checking DPoP Proofs tweaks
>>  change is not
>> workable because it pulls an authorization server specific construct as a
>> normative requirement into a section that applies to both authorization
>> server and resource server. It's kind of like a compile time error for a
>> spec, if there was such a thing. The note about order not being implied
>> could be added tho. Or the list could be changed to use bullets and not
>> have numbering. I don't recall there being any real reason for the
>> numbering.
>>
>>
>>
>>
>>
>> On Thu, Jun 30, 2022 at 10:12 AM Spencer Balogh 
>> wrote:
>>
>>> Hi all,
>>>
>>> I'm new to all of this, please forgive me if I'm missing any
>>> expectations or conventions here.
>>>
>>> I have a bit of feedback for OAuth 2.0 Demonstrating Proof-of-Possession
>>> at the Application Layer
>>> (DPoP) draft-ietf-oauth-dpop-09 (
>>> https://datatracker.ietf.org/doc/html/draft-ietf-oauth-dpop). I'm aware
>>> that you're already after the Working Group Last Call, so I'm trying to
>>> come in with mostly non-normative changes, and with draft suggestions
>>> rather than just open questions. I've opened several pull requests against
>>> the RFC repo so that needed changes can be easily incorporated, and not get
>>> blocked by changes that are more my preference.
>>>
>>> I also wanted to follow up here to provide a little bit more context on
>>> the change proposals.
>>>
>>> # PR Nonce clarifications, more complete documentation, and guidance
>>>
>>> https://github.com/danielfett/draft-dpop/pull/157
>>>
>>> It seems like there might be a little bit of a disconnect between what
>>> has been 

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

[Brian Campbell]

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 
wrote:

> Thanks Brian!
> See my replies inline below.
>
>
> On Thu, Jun 30, 2022 at 6:52 PM Brian Campbell 
> 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
>>  of [RFC3986]) and Scheme-Based
>> Normalization (Section 6.2.2  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