OAuth 2.0 Demonstrating Proof of Possession (DPoP)
draft-ietf-oauth-dpop-16

Thanks for the clear specification.

While I agree with Ben Schwartz comment in the secdir review that the term "nonce" is wrong in the document, and that it should really be called "cookie", I think it is too late in the game to change this.

Thank you for the work put into this document. 

Please find below some non-blocking COMMENT points, and some nits.

Special thanks to Rifaat Shekh-Yusef for the shepherd's detailed write-up including the WG consensus (and the author count) even if the justification of the intended status is rather light. 

I hope that this review helps to improve the document,

Regards,

-éric

# COMMENTS (non blocking)

## Section 1

Should there be a reference to OAuth ?

s/The mechanism described herein /The mechanism specified herein / ? as it is proposed standard

Adding a short description of SPA would be useful, or simply remove this reference ?

# NITS (non blocking / cosmetic)

## Section 2

` Properly audience restricting access tokens can prevent such misuse` is difficult to parse

## Section 4.1

s/repeated below for ease of reference/repeated below in figure 3 for ease of reference/ ?

## Section 4.2

s/MUST NOT be none or an identifier for a symmetric algorithm (MAC)/MUST NOT be 'none' or an identifier for a symmetric algorithm/

## Section 6.1

`JSON Web Tokens (JWT)` the JWT acronym has already been defined.

# GEN AD review of draft-ietf-oauth-dpop-14

CC @larseggert

## Comments

### Section 9, paragraph 5
```
     only at the issuing server.  Developers should also take care to not
     confuse DPoP nonces with the OpenID Connect [OpenID.Core] ID Token
     nonce.
```
Could this ambiguity not be avoided by using a different term/claim?

### Too many authors

The document has six authors, which exceeds the recommended author limit. Has
the sponsoring AD agreed that this is appropriate?

### Missing references

No reference entries found for these items, which were mentioned in the text:
`[IANA.OAuth.Parameters]`.

### DOWNREFs

DOWNREF `[RFC8792]` from this Proposed Standard to Informational `RFC8792`.
(For IESG discussion. It seems this DOWNREF was not mentioned in the Last Call
and also seems to not appear in the DOWNREF registry.)

### Inclusive language

Found terminology that should be reviewed for inclusivity; see
https://www.rfc-editor.org/part2/#inclusive_language for background and more
guidance:

 * Term `native`; alternatives might be `built-in`, `fundamental`, `ingrained`,
   `intrinsic`, `original`
 * Term `blindly`; alternatives might be `visually impaired`, `unmindful of`,
   `unconcerned about`, `negligent of`, `unaware`, `uncomprehending`,
   `unaware`, `uncritical`, `unthinking`, `hasty`, `blocked`, `opaque`

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.


### JSON

```

    {
     "error": "use_dpop_nonce"
     ^ Expecting ',' delimiter
     "error_description":
    }```

### Outdated references

Document references `draft-ietf-oauth-security-topics-21`, but `-22` is the
latest available revision.

### URLs

These URLs in the document can probably be converted to HTTPS:

 * http://www.iana.org/assignments/jwt
 * http://openid.net/specs/openid-connect-core-1_0.html

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool

Most of the SHOULDs here seem unsupported to me, in the sense that I'm not clear what interoperability breaks if I decide not to do what it says.  Some prose about that would be helpful to include.

I know this isn't the first OAUTH document I've reviewed, but I still find it strange that claim names are not quoted or in all-caps or something.  In prose, they just look like typos to me.

Hi,

Thanks for your work on this document.  I found it pleasant to read and informative.

I have no substantive comments.

Regards,
Rob

Thank you for writing this; I found it a fascinating and informative read.

I don't have any particularly substantive comments, but I do have some nits and similar to hopefully further improve the document.

1: "These stolen artifacts can later be used together independent of the client application to access protected resources." -- I found this really hard to parse. I think that some of it is the "used together independent" formulation - adding a comma would help, but I think just dropping "together" works even better (it does say "artifacts" in plural, so that's already covered?)

2: "Properly audience restricting access tokens can prevent such misuse" - I think that it would be helpful to reword this, or find a reference for "audience restricting"

3: Might it be worth adding a reference for XSS? I'm guessing that the audience will already be familiar, but if not, https://owasp.org/www-community/attacks/xss/ ?

4: Question: Why is the Nonce optional? Perhaps I missed it, but I was unable to find any discussion (I was expecting something in Sec 8,9 or 10) providing some reason why a server might not use a nonce (the closest I found was "The logic through which the server
   makes that determination is out of scope of this document.", so I'm guessing that there *is* a reason, but... )

Thanks for working on this specification. My review of this document did not identified any transport protocol related issues.