Re: [OAUTH-WG] Fwd: New Version Notification for draft-ietf-oauth-dpop-01.txt

[Brian Campbell <[e-mail]>]

Thanks William and nice to see you pop up on the list again. Your
perspective and feedback is appreciated (and missed a little bit these
days). Attempts to respond to things that seemed to warrant a response are
inline below.

On Sun, May 3, 2020 at 5:54 PM William Denniss <wdenniss=
[e-mail]> wrote:

> Hi Brian, et. al,
>
> I reviewed the document. Overall I think the concept is clearly
> communicated, both the need, and how it is implemented. It was easy to
> read, and concise. Great work!
>
> Some comments and nits:
>
> § 1:
> (nit) "DPoP" should be defined on first use in the body of the document,
> in my opinion.
>

Yeah, you're probably right. Will make that change.



> § 3:
>
> I don't follow this why the sentence "For confidential clients, refresh
> tokens are bound to the "client_id", which is more flexible than binding it
> to a particular public key." belongs in this explanation of the diagram. Is
> it indicating that DPoP is not required for confidential clients? If the
> sentence was deleted / moved elsewhere in the doc, would it alter the
> meaning of the diagram?
>
> I wonder if the diagram would be simpler with less optionality (e.g.
> public / confidential separated into 2 diagrams, if they behave
> differently), or maybe just kept as 1 simple diagram, with any optionality
> described later. Generally I think these kind of overview diagrams (which
> are extremely helpful, and now I must go add one to my own draft) are best
> if they give an overview of the protocol without trying to document the
> entire specification, or justify why different decisions are made.
>

So the intended intent of the document is that refresh tokens be DPoP bound
only when issued to public clients. RFC6749 already requires that an AS
bind refresh tokens to the client to which they were issued and that
confidential clients (those having established authentication credentials
with the AS) authenticate to the AS when presenting a refresh token. Thus,
refresh tokens issued to confidential clients are already
sender-constrained to the client and its credentials.

Although the binding of refresh tokens only applies to public clients, it
seemed like a relevant enough piece of the overall DPoP picture to warrant
inclusion in the intro. And it doesn't change the overall flow so a
separate diagram would look the same :/

I take your point, however, and can work towards the "simple diagram, with
any optionality described later" type approach. Which, I think might just
end up being some tightening up of the language in the intro and better
explaining and justifying things later in the document.



§ 4.1 figure 2
>
> (nit) When showing a decoded JWT with the signature dropped, I think it's
> a good idea to first present the full encoded JWT. Someone who is new to
> JWT may not realize that this data has a signature (or even that it is
> encoded at all), since it was dropped already, while showing the
> before/after can make this clear. Not essential, I just think it would help
> readers. Having this also sets the scene for later when encoded JWTs are
> used in example request headers.
>

I'm always trying to find the right balance of including enough to be
useful but not too darn much in examples. In this case though I think you
might be right and that first presenting the full encoded JWT would be
useful. But also not overdoing it.


§ 4.1, below figure 2:
>
> Regarding "Note: To keep DPoP simple to implement, only the HTTP method
> and URI
>    are signed in DPoP proofs."
>
> I found this confusing as the entire JWT is signed, and the signature
> covers the whole JWT and not just those 2 claims. I get that since the JWT
> only meaningfully contains the HTTP method and URI that the statement is
> effectively true, but I would rephrase. Something along the lines of:
>
> "Of the HTTP headers in the request, only the HTTP method and URI are
> included in the DPoP JWT, and therefore only these 2 headers of the request
> are covered by the DPoP proof"
>

Agree this could be phrased in a better way.




> (nit) I would also drop the "To keep DPoP simple to implement" here, and
> maybe move any discussion about the design trade-offs to the security
> considerations (9.4).
>

Makes sense.


>
> § 5
>
> (nit) Expand "AS" on first use (or better: just say "authorization server"
> and skip the acronym in this case, as "AS" is insider lingo IMO)
>

Will just stick with "authorization server" here and throughout.



> § 5
>
> Regarding "If a refresh token is issued to a public client"...
>
> What about when it's issued to the confidential client? I think when
> specific behavior is documented for one client type, it's probably good to
> at least mention how the other types behave.
>
> I think this could be clarified here. In fact, I wonder if the text I
> highlighted earlier from § 3 might belong here.
>

Yeah, I think some of the more wordy bits from § 3 would be better moved
here. That could keep the intro context more concise and, to your point,
cover what happens here for both types of clients.


> § 8
>
> (nit) Suggest moving to an Appendix
>

Admittedly it doesn't flow particularly well here but it's still a
normative part of the spec so I don't think it belongs in an Appendix.


>
> § 9.1
>
> (nit) "up to a few seconds", I would just say "e.g. a few seconds in the
> future". Seems odd to specify an upper bound on a non-exact number.
>

Aggree.


>
> § 9.4
>
> " DPoP does not ensure the integrity of the payload or headers of
>    requests.  The signature of DPoP proofs only contains the HTTP URI
>    and method, but not, for example, the message body or other request
>    headers."
>
> Again, I find this an awkward phrase, "the signature of the DPoP proofs
> only contains the HTTP URI and method", and I think it' better to talk
> about the JWT only containing these claims, and the signature therefore
> only signing those claims. Also, (nit) does a proof "contain" something, or
> more accurately cover/sign something?
>

I'll work on the phrasing here too.


>
> William
>
>
>
>
> On Fri, May 1, 2020 at 12:04 PM Brian Campbell <bcampbell=
> [e-mail]> wrote:
>
>> I've pushed out a -01 revision of DPoP hopefully allowing folks enough
>> time to read it before the interim meeting on Monday (apologies that it
>> wasn't sooner but the edits took longer than expected or hoped). For ease
>> of reference the changes in this revision are summarized below. There are,
>> of course, still outstanding issues and discussion points that I hope to
>> make some progress on during the interim meeting on Monday.
>>
>>    -01
>>
>>    *  Editorial updates
>>    *  Attempt to more formally define the DPoP Authorization header
>>       scheme
>>    *  Define the 401/WWW-Authenticate challenge
>>    *  Added "invalid_dpop_proof" error code for DPoP errors in token
>>       request
>>    *  Fixed up and added to the IANA section
>>    *  Added "dpop_signing_alg_values_supported" authorization server
>>       metadata
>>    *  Moved the Acknowledgements into an Appendix and added a bunch of
>>       names (best effort)
>>
>> ---------- Forwarded message ---------
>> From: <internet-drafts@ietf..org>
>> Date: Fri, May 1, 2020 at 12:24 PM
>> Subject: New Version Notification for draft-ietf-oauth-dpop-01.txt
>> To: Torsten Lodderstedt <[e-mail]>, David Waite <
>> [e-mail]>, John Bradley <[e-mail]>, Brian
>> Campbell <[e-mail]>, Daniel Fett <[e-mail]>,
>> Michael Jones <[e-mail]>
>>
>>
>>
>> A new version of I-D, draft-ietf-oauth-dpop-01.txt
>> has been successfully submitted by Brian Campbell and posted to the
>> IETF repository.
>>
>> Name:           draft-ietf-oauth-dpop
>> Revision:       01
>> Title:          OAuth 2.0 Demonstration of Proof-of-Possession at the
>> Application Layer (DPoP)
>> Document date:  2020-05-01
>> Group:          oauth
>> Pages:          22
>> URL:
>> https://www.ietf.org/internet-drafts/draft-ietf-oauth-dpop-01.txt
>> Status:         https://datatracker.ietf.org/doc/draft-ietf-oauth-dpop/
>> Htmlized:       https://tools.ietf.org/html/draft-ietf-oauth-dpop-01
>> Htmlized:
>> https://datatracker.ietf.org/doc/html/draft-ietf-oauth-dpop
>> Diff:
>> https://www.ietf.org/rfcdiff?url2=draft-ietf-oauth-dpop-01
>>
>> Abstract:
>>    This document describes a mechanism for sender-constraining OAuth 2.0
>>    tokens via a proof-of-possession mechanism on the application level.
>>    This mechanism allows for the detection of replay attacks with access
>>    and refresh tokens.
>>
>>
>>
>>
>> Please note that it may take a couple of minutes from the time of
>> submission
>> until the htmlized version and diff are available at tools.ietf.org.
>>
>> The IETF Secretariat
>>
>>
>>
>> *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
>> [e-mail]
>> 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._