Re: [OAUTH-WG] DPoP - Document Shepherd Review

[Daniel Fett <[e-mail]>]

Apologies accepted! :-)

Am 28.07.22 um 01:11 schrieb Brian Campbell:
> I need to make one more apology - this time for the incorrect spelling 
> of Dr. Fett's name (should be Daniel not Danial). My apologies.
>
> On Wed, Jul 27, 2022 at 6:43 PM Brian Campbell 
> <[e-mail]> wrote:
>
>     Thanks Rifaat and others for the vibrant* discussions about the
>     DPoP draft in the side meeting yesterday.
>
>     I thought it'd be appropriate to share/reiterate the three action
>     items we'd agreed on during the meeting (as I remember anyway):
>
>      1. Justin to review the text about why we have the AT hash and
>         either create a PR adding additional motivations or say that
>         what we have is already sufficient
>      2. Danial to add some text to further explain decisions with
>         respect to PAR
>      3. Brian (aka me) to add a parenthetical remark to the Signature
>         Algorithms subsection listing 'ES256'
>
>     PR's for the latter two are here
>     <https://github.com/danielfett/draft-dpop/pull/165> and here
>     <https://github.com/danielfett/draft-dpop/pull/166> respectively. 
>     And yes, this message is, at least in part, a passive-aggressive
>     reminder to Justin about #1.
>
>     The slides that I used to try and help guide the discussions are
>     attached. They are admittedly rather suboptimal but I'm including
>     them for the sake of transparency (and because they have a couple
>     of photos).
>
>
>     * my apologies for being overly vibrant at times
>
>
>
>
>     On Wed, Jul 6, 2022 at 4:32 PM Brian Campbell
>     <[e-mail]> wrote:
>
>         Thanks Rifaat!
>         I will make those changes in the document source and come to
>         Philly prepared to discuss the other items. One of the side
>         meetings seems like a good forum for that, good idea.
>
>         On Tue, Jul 5, 2022 at 11:14 AM Rifaat Shekh-Yusef
>         <[e-mail]> wrote:
>
>             Thanks Brian!
>             See my replies inline below.
>
>
>             On Thu, Jun 30, 2022 at 6:52 PM Brian Campbell
>             <[e-mail]> wrote:
>
>                 Thanks for shepherding Rifaat. And apologies for the
>                 slow reply. My attempts at answering questions and
>                 responding to comments are inline below.
>
>
>                 On Fri, Jun 3, 2022 at 11:55 AM Rifaat Shekh-Yusef
>                 <[e-mail]> wrote:
>
>                     The following is my review as a document shepherd:
>
>                     Section 4.3
>
>                     Last sentence
>
>                     Since the document uses “SHOULD”, this implies
>                     that there are some valid cases where this is not
>                     needed.
>
>                     Should a text be added to explain when this is not
>                     needed?
>
>
>
>                 What about giving a bit more context about why they
>                 should? Changing that sentence to say, "To reduce the
>                 likelihood of false negatives, servers SHOULD employ
>                 Syntax-Based Normalization (Section 6.2.2
>                 <https://rfc-editor.org/rfc/rfc3986> of [RFC3986]) and
>                 Scheme-Based Normalization (Section 6.2.2
>                 <https://rfc-editor.org/rfc/rfc3986> of [RFC3986])
>                 before comparing the |htu| claim." And also maybe
>                 changing it to a little "should".
>
>             Yes, that works.
>             I suggest keeping it as "SHOULD" to encourage implementers
>             to use this, unless they have a really good reason not to.
>
>
>                     Section 6.1
>
>                     1.
>
>                         First sentence - what is the reason for using
>                         “SHOULD”, instead of “MUST” in this case?
>
>
>
>                 Good question. I think it was a bit of carryover from
>                 OAuth in general not strictly defining access token
>                 format or content. And wanting to not encroach on
>                 that. But that's kinda covered/allowed for in general
>                 by Section 6 already. And Section 6.2 is effectively
>                 the same as 6.1 but for introspection and it doesn't
>                 use "SHOULD". I think the “SHOULD” in the first
>                 sentence of 6.1 should be removed thereby making it an
>                 implicit must - like "when using JWT, this is how it
>                 is". That would align with the way it's described for
>                 introspection. Also leaves some room for hash
>                 algorithm agility via a new confirmation method member
>                 (described in
>                 https://www.ietf.org/archive/id/draft-ietf-oauth-dpop-09.html#name-access-token-and-public-key)
>                 without going against a "MUST"
>
>             I am fine with removing the "SHOULD" to make it an
>             implicit must.
>
>
>
>
>                     2.
>
>                         The DPoP Proof contains a hash of the Access
>                         Token, and the Access Token contains a hash of
>                         the public key in the DPoP Proof.
>
>                     Why do you need both? Would one of these be
>                     sufficient?
>
>
>
>                 The latter (AT containing a hash of the public key in
>                 the DPoP Proof) is needed and largely sufficient for
>                 the main goals of binding the AT to a key held by the
>                 client. The former (DPoP Proof containing a hash of
>                 the AT) was added later via very rough WG consensus -
>                 it can prevent some esoteric swapping of tokens that I
>                 never really understood to be honest and also limits
>                 the impact of using maliciously precomputed and
>                 exfiltrated proofs
>                 (https://www.ietf.org/archive/id/draft-ietf-oauth-dpop-09.html#section-2-6
>                 talks about it a bit). Use of the nonce mechanism,
>                 which was added to the draft even later, also (and
>                 better) protects against precomputed and exfiltrated
>                 proofs. The value of the AT hash in the proof seems
>                 somewhat questionable. To me anyway. But removing it
>                 at this point is potentially problematic due to
>                 inertia, existing implementations/deployments, rough
>                 WG consensus, and more.
>
>             I think that at least a text is needed to justify this,
>             and explain the "it can prevent some esoteric swapping of
>             tokens" issue.
>             Maybe we can discuss this during one of the side meetings
>             in Philly.
>
>
>                     Section 7.1
>
>                     1.
>
>                         “if the request does not include valid
>                         credentials or does not contain an access
>                         token sufficient for access, the server
>                         canrespond with a challenge to the client to
>                         provide DPoP authentication information.”
>
>
>                     Should the “can” be replaced with a “SHOULD”?
>
>
>
>                 FWIW, there was some discussion around that sentence
>                 that included some pushback on dropping the "can".
>                 https://github.com/danielfett/draft-dpop/issues/119
>                 and https://github.com/danielfett/draft-dpop/pull/122
>                 have the conversation. I'm rather hesitant to try and
>                 change it after all that.
>
>             Ok
>
>
>                     2.
>
>                         Also, I think it would be clearer if you can
>                         explicitly state what the authorization server
>                         should do when it does not challenge the
>                         client, which I am assuming will be something
>                         along the lines of: “the authorization server
>                         issues an error response per Section 5.2 of
>                         RFC6749“
>
>
>
>                 The section in question is about protected resource
>                 access so anything about the authorization server
>                 wouldn't be appropriate there. The protected resource
>                 / RS always has the option to simply fail the request
>                 and can do that however it sees fit. I'm not sure how
>                 to state that in the document text. Or if anything
>                 should be stated, honestly.
>
>             Ok
>
>
>                     Section 7.2
>
>                     1.
>
>                         “Specifically, such a protected resource MUST
>                         reject a DPoP-bound access token received as a
>                         bearer token per [RFC6750
>                         <https://www.ietf.org/archive/id/draft-ietf-oauth-dpop-08.html#RFC6750>].”
>
>
>                     I think that I understand what you are trying to
>                     say with this sentence, but the way the sentence
>                     reads is confusing to me.
>
>                     I am assuming what you are trying to say is
>                     something along the lines of “a dpop protected
>                     resource must reject a request that provides a
>                     bearer token”. Is that correct? If so, can you
>                     please rephrase the sentence to make it clearer?
>
>
>
>                 That's not quite correct. That paragraph
>                 <https://www.ietf.org/archive/id/draft-ietf-oauth-dpop-09.html#section-7.2-1>
>                 (copied below) is attempting say that a protected
>                 resource that will accept either "Authorization:
>                 Bearer <bearer token>" or "Authorization: DPoP
>                 <dpop-bound token>" is required to reject a request
>                 that uses the Bearer scheme with a DPoP-bound access
>                 token. This is to prevent downgraded usage of a bound
>                 access token without demonstrating possession of the
>                 key to which it is bound.
>                 "Protected resources simultaneously supporting both
>                 the |DPoP| and |Bearer| schemes need to update how
>                 evaluation of bearer tokens is performed to prevent
>                 downgraded usage of a DPoP-bound access token.
>                 Specifically, such a protected resource MUST reject a
>                 DPoP-bound access token received as a bearer token per
>                 [RFC6750
>                 <https://www.ietf.org/archive/id/draft-ietf-oauth-dpop-09.html#RFC6750>]."
>
>             Got it.
>
>
>                     2.
>
>                         “A protected resource that supports only
>                         [RFC6750
>                         <https://www.ietf.org/archive/id/draft-ietf-oauth-dpop-08.html#RFC6750>]
>                         and is unaware of DPoP would most presumably
>                         accept a DPoP-bound access token as a bearer
>                         token”
>
>
>                     Wouldn't such a resource server check the value of
>                     the WWW-Authenticate header to make sure it
>                     contains the Bearer scheme, which means that the
>                     request is most likely to be declined?
>
>
>
>                 What that is trying to say is that a protected
>                 resource that only does or knows about the RFC6750
>                 Bearer scheme ("Authorization: Bearer <token>") will
>                 almost certainly accept a bound access token sent via
>                 the Bearer scheme.
>
>             Ok
>
>
>                     Section 10.1
>
>                     Why define two different mechanisms to achieve the
>                     same thing?
>
>                     This seems to add complexity without an obvious
>                     benefit.
>
>
>
>                 This is a bit of a tricky area. The benefit with PAR
>                 is the direct request from client to AS, which allows
>                 for an actual DPoP proof to be used for the eventual
>                 binding of the authorization code to the key. Also the
>                 client doesn't have to do the JWK hash in that case.
>                 Whereas the normal authorization request is indirect
>                 via the browser and just a hash of the key is given
>                 for the code binding with the dpop_jkt parameter. And
>                 the client has to compute the hash. But PAR is just an
>                 alternative way to pass the authorization parameters
>                 (like dpop_jkt) so it's kinda awkward to use things
>                 together like this.
>                 https://github.com/danielfett/draft-dpop/issues/103
>                 and https://github.com/danielfett/draft-dpop/pull/111
>                 have some discussion around this but there was some in
>                 person talk too so that's not complete.
>
>                 I don't love that there's two different mechanisms
>                 here. But it's what we were able to come up with given
>                 all the factors. Certainly open to considering
>                 improvements but am pretty much at a loss of what that
>                 might be.
>
>             Let's discuss this during one of the side meetings in Philly
>
>
>                     Section 11.6
>
>                     Should the algorithms be explicitly called out? Or
>                     at least reference a document that calls out such
>                     algorithms?
>
>
>
>                 There isn't a single such document and it's not
>                 necessarily a static list of algorithms. I was about
>                 to say we could point to the JOSE alg registry but
>                 glancing again at it
>                 https://www.iana.org/assignments/jose/jose.xhtml#web-signature-encryption-algorithms
>                 and I suspect that'd confuse more than help. We could
>                 perhaps list some/many of the algs with the
>                 qualification that it's not an exclusive or complete
>                 list? But I'm not sure how useful that would be, to be
>                 honest.
>
>             If you do not specify any algorithm, how do you ensure
>             interop?
>             I think this is worth a discussion in Philly
>
>
>                     Section 11.7
>
>                     Why is OAuth Token Binding included?
>
>
>
>                 Yeah, that doesn't make sense to encourage its use
>                 because it's not a viable thing to use. OAuth Token
>                 Binding should be removed from Section 11.7. Was that
>                 what you were getting at? That particular text in the
>                 paragraph that mentions token binding has been in the
>                 draft for a long time and honestly never made a lot of
>                 sense to me. So I could envision removing more. But
>                 that's maybe more than you were aiming for.
>
>             I am actually in favor of removing it
>
>
>
>                     Section 11.8
>
>                     Why not include algorithm agility to make sure the
>                     mechanism is ready to allow for more secure
>                     algorithms in the future?
>
>
>                 Algorithm agility is a whole can of worms that can be
>                 accomplished in different ways with different amounts
>                 of added complexity and potential vulnerabilities and
>                 issues of interop and MTI. Section 11.8 describes how
>                 DPoP allows for algorithm agility (without using the
>                 exact words) by suggesting that new dpop binding cnf
>                 method and/or AT hash claim be defined using a
>                 "better" hash algorithm if/when the need arises (OAuth
>                 2.0 Mutual-TLS takes a similar approach FWIW). The
>                 intent of doing it that way was to keep things as
>                 simple as possible in the spec right now without
>                 completely closing the door on future needs. .
>
>             This is another topic that is worth a discussion in Philly.
>
>             Regards,
>              Rifaat
>
>
>                 /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
> [e-mail]
> https://www.ietf.org/mailman/listinfo/oauth