I concur with Brian's evaluation of these proposed changes. -- Mike
________________________________ From: OAuth <oauth-boun...@ietf.org> on behalf of Brian Campbell <bcampbell=40pingidentity....@dmarc.ietf.org> Sent: Wednesday, July 6, 2022 1:53:17 PM To: Spencer Balogh <spbal...@gmail.com> Cc: oauth@ietf.org <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 <spbal...@gmail.com<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<https://github.com/danielfett/draft-dpop/pull/160>, I can definitely understand the concerns around the private key and <Signature>, 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 <bcampb...@pingidentity.com<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<https://github.com/danielfett/draft-dpop/pull/157> 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<https://github.com/danielfett/draft-dpop/pull/158> seems reasonable. Barring any objections from the WG, this can probably be incorporated as well. The changes in # PR Fully specified examples<https://github.com/danielfett/draft-dpop/pull/160> 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<https://github.com/danielfett/draft-dpop/pull/159> 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 <spbal...@gmail.com<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 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<mailto:OAuth@ietf.org> https://www.ietf.org/mailman/listinfo/oauth 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.
_______________________________________________ OAuth mailing list OAuth@ietf.org https://www.ietf.org/mailman/listinfo/oauth
