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

[Brian Campbell]

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



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

(copied below) is attempting say that a protected resource that 

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

[Spencer Balogh]

 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 discussed around nonces and what's actually present in the draft. I
saw that there's a previous edit suggesting a "recently supplied" nonce,
and that's more in line with my expectations than the previous draft, but
it's immediately followed by a paragraph starting with "The intent is that
both clients and servers need to keep only one nonce value for one another."

I made a couple changes to revise this statement to suggest the server
keeps a window of recent nonces, to add "nonce" to the syntax section of
the document, and to add a few more details to the nonce section of `DPoP
Proof Replay {#Token_Replay}`

# PR Proposal Section 7.2 Client Considerations

https://github.com/danielfett/draft-dpop/pull/158

Added a `Client Considerations` sub-section to `Protected Resource Access
{#protected-resource-access}`. I think there's a lot of potential to get
retry on idempotent requests wrong here going forward. This is regularly
handled at an HTTP client level, and that is likely to thrash for some
server implementations. I added a recommendation to generate a new DPoP
proof for retry, even if it's something they've previously considered a
transient error.

I don't really have guidance for improving this spec in environments where
there are frequent network errors, but it seems like a concern worth
considering as well for implementers.

# PR Fully specified examples

https://github.com/danielfett/draft-dpop/pull/160

Added nonce to the DPoP proof examples.

Added decoded DPoP proofs to examples. It's nice to not have to decode
while looking at examples.

Additionally, added references to the private keys used for signing and
repeated the requirement to redact private key material from requests a few
more times. I'm not sure if it's unconventional to include a private key in
a RFC, but it's a throwaway key, and reiterating private key redaction in
DPoP proofs a few more times seems like it couldn't hurt.

# PR Checking DPoP Proofs tweaks

https://github.com/danielfett/draft-dpop/pull/159

In `Checking DPoP Proofs {#checking}`:

`alg` checking seems vague, especially around "is deemed secure". Adding a
reference to `dpop_signing_alg_values_supported` here instead, and moving
the details of algorithm selection to that metadata section instead.

Clarifying that order is not implied by the numbering of this list.

Fixing link to RFC3986.

Please let me know if you have any suggestions for improving these change
proposals.

Thanks,
Spencer Balogh
___
OAuth mailing list
OAuth@ietf.org
https://www.ietf.org/mailman/listinfo/oauth