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 <[email protected]> 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 <[email protected]> > 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 >> [email protected] >> 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.*
_______________________________________________ OAuth mailing list [email protected] https://www.ietf.org/mailman/listinfo/oauth
