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

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

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.

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.

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.