Subject: [OAUTH-WG] A token review of draft-parecki-oauth-v2-1-01

From: Brian Campbell <[email-masked]>
Date: Tue, 21 Apr 2020 16:48:29 -0600

Been working on this on and off for a while now (it's not exactly short at
80+ pages, various other priorities, etc.) but wanted to share my thoughts
from an initial review of the OAuth 2.1 draft before the interim next week
where it is on the agenda
https://datatracker.ietf.org/doc/agenda-interim-2020-oauth-06-oauth-01/.
So for better or worse, here's that review:

Abstract:
"replaces and obsoletes the OAuth 2.0 Authorization Framework described in
RFC 6749."
I think "replaces" is probably unnecessary here and, to be pedantic, is
arguably inaccurate because published RFCs don't ever go away or get
replaced.
Probably should also consider using the official "obsoletes" attribute
marker too for 6749 https://tools.ietf.org/html/rfc7991#section-2.45.8 and
probably also "updates"/"obsoletes" for others based on the scope of the
rest of the document (not sure how this even works with a BCP or if you can
or would want to update or obsolete a BCP) if this work proceeds. That
scope could be better described in the abstract too as discussed somewhat
in the thead
https://mailarchive.ietf.org/arch/msg/oauth/Ne4Q9erPP7SpC5051sSy6XnLDv0/
and also the 1st paragraph of
https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-12.
What is and isn't in scope is another larger question that I"m not even
sure how to ask. What's included vs. what's referenced? Should this doc be
incorporating bits of BCP 212 - OAuth 2.0 for Native Apps? Seems kinda
antithetical to what a BCP is supposed to be but it's also a bit hard to
tell how much was used. I mean, what happens if/when the BCP is updated?
And that kinda begs the question of if it should also incorporate parts of
or even replace the browser based apps draft? I guess that's a TBD circa
page 68. There was talk about the device grant being in scope but I'm not
seeing it (not saying it should or shouldn't be there but it was talked
about). I dunno exactly but those are some scope questions that come to
mind.
Speaking of obsoleting, I do want to ensure that existing extensions and
profiles of RFC 6749 that use legitimate extension points still present and
unchanged in OAuth 2.1 aren't inadvertently impacted by this effort. I'm
not sure how that should work in practice but want to be aware of it as/if
this work progresses.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-1
"is designed for use with HTTP ([RFC2616])."
I was momentarily proud of myself for knowing that RFC 2616 is obsolete but
then remembered that the nits tool can automate the check for other
obsolete or problematic references
https://tools.ietf.org/idnits?url=https://tools.ietf.org/id/draft-parecki-oauth-v2-1-01.txt
so take a look at those issues.
Probably should also check the errata of any documents this one intends to
update/obsolete or just borrowed a lot of text from to see if anything
should be applied.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-1.1
"The interaction between the authorization server and resource server is
beyond the scope of this specification."
Don't want to try and change the scope but perhaps a mention that things
like RFC 7662 Token Introspection and self-contained tokens a la JWT exist
here (or sec 1.4 or 7 or...) would be worthwhile.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-1.5
"   Steps (3), (4), (5), and (6) are outside the scope of this
   specification, as described in Section 7."
But Section 7 incorporates parts of RFC 6750 so being out of scope isn't
really true.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-1.8
Not too sure about this section in this document but seems that it should
at least reflect the fact that some things like RFC 8414  Authorization
Server Metadata do now exist.

https://tools.ietf.org/html/rfc6749?#section-1.9
All the cool drafts are doing BCP 14 [RFC2119] [RFC8174] these days.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-2
Mentioning the existence of RFC 7591 Dynamic Client Registration here seems
appropriate. And I think it's be super useful to say that even when RFC
7591 isn't being used directly, it (and registered extensions to it
https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml#client-metadata)
imply a common general data model for clients that's pretty widely used and
useful even with so called static registration that "typically involve
end-user interaction with an HTML registration form."

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-2.2
"Authorization servers SHOULD NOT allow clients to influence their
   "client_id" or "sub" value or any other claim if that can cause
   confusion with a genuine resource owner."
This text taken from draft-ietf-oauth-security-topic is out of context and
somewhere between meaningless and confusing here in this document. Removing
it or maybe something like this might be more appropriate:
  "Authorization servers SHOULD NOT allow clients to influence their
"client_id" value in such a way that it might cause confusion with the
identifier of a genuine resource owner during subsequent protocol
interactions."

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-2.3.1
"The client MAY omit
      the [client_secret] parameter if the client secret is an empty
string."
Let's remove that sentence. As Michael Peck noted in his review, it doesn't
even make sense in the context of this section describing authentication of
confidential clients using a password/client secret. And that sentence has
been the source of some other problems. More detail is in this thread
https://mailarchive.ietf.org/arch/msg/oauth/D1-FrLrSeWrg9M_ca9dbkYeruc4/

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-2.3.2
"Other Authorization Methods" should be "Other *Authentication* Methods"
It's probably worthwhile to note or reference the other client
authentication methods that have been specified since this text was written
in 6749 and/or point to the OAuth Token Endpoint Authentication Methods
registry
https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml#token-endpoint-auth-method
for them and also maybe mention that, despite the Token Endpoint in the
name, they are general methods of client auth to the AS when making direct
requests.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-3.1
"The means through which the client obtains the location of the
   authorization endpoint are beyond the scope of this specification,
   but the location is typically provided in the service documentation."
Maybe add something like "or in the authorization server's metadata
document [RFC8414]."
"Request and response parameters
   MUST NOT be included more than once."
please clarify that sentence to say:
"Request and response parameters
defined by this specification MUST NOT be included more than once."

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-3.1.2
"   The authorization server MUST compare the two URIs using simple
   string comparison as defined in [RFC3986], Section 6.2.1."
There's absolutely no context here for understanding what two URIs are
being compared.
Mandating full redirect_uri comparison is maybe more appropriate in 4.1.1
with the redirect_uri parameter somewhere. And 3.1.2.3. and 3.1.2.2 needs
some attention with respect to this too.
But also an exception (for the port part) to full redirect_uri comparison
is needed for loopback redirect_uris on native clients as in
https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-9.6.1 and
https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-9.2 and
https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-10.3.3

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-3.1.2.1
As Michael Peck noted in his review, it's probably okay now to just mandate
TLS for HTTP redirect URIs. Although custom scheme or loopback redirect
URIs for native apps wouldn't use or require TLS.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-3.1.2.2 and
https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-3.1.2.3
Seem to still allow for registration of partial redirection URIs. Which
isn't gonna work with an exact match requirement.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-3.2
"Request and response parameters
   MUST NOT be included more than once."
please clarify that sentence to say:
"Request and response parameters
defined by this specification MUST NOT be included more than once."
"   The means through which the client obtains the location of the token
   endpoint are beyond the scope of this specification, but the location
   is typically provided in the service documentation."
Maybe add something like "or in the authorization server's metadata
document [RFC8414]."

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-3.2.1
"Client authentication is critical
      when an authorization code is transmitted to the redirection
      endpoint over an insecure channel or when the redirection URI has
      not been registered in full."
Isn't full registration of redirection URI now required (other than maybe
the port for native apps) by virtue of requiring a full comparison be done
when validating the authz request?  And other than a native app using the
loopback, maybe it's time to move to require that redirection URIs be
accessed via secure channel?

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-4.1.1.2
"Clients are
   permitted to use "plain" only if they cannot support "S256" for some
   technical reason and know via out-of-band configuration that the
   server supports "plain"."
With code_challenge_methods_supported from Authorization Server Metadata /
RFC 8414 it doesn't have to be out-of-band anymore.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-4.1.2
 " Typically, the "code_challenge" and "code_challenge_method" values
   are stored in encrypted form in the "code" itself but could
   alternatively "
'Typically' - really?

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-4.1.3
"   *  ensure that the "redirect_uri" parameter is present if the
      "redirect_uri" parameter was included in the initial authorization
      request as described in Section 4.1.1, and if included ensure that
      their values are identical."
The reference should be to 4.1.1.3.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-4.1.4
"   An example successful response:
   HTTP/1.1 200 OK
   Content-Type: application/json;charset=UTF-8
   Cache-Control: no-store
   Pragma: no-cache"
Pretty sure application/json shouldn't have a charset (see note at end of
section https://tools.ietf.org/html/rfc8259#section-11)
and I've long thought that "Pragma: no-cache" shouldn't be there (see
https://mailarchive.ietf.org/arch/msg/oauth/9DdkE2P0RrUZMeZAbdf3NrMfy0w/ )
Note that these apply to most of the example responses in the draft.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-4.3
" specifying the grant type
   using an absolute URI (defined by the authorization server) as the
   value of the "grant_type" parameter"
The words in the parenthetical have led to questions in AD review of
documents making use of the grant type extension point (see
https://mozphab-ietf.devsvcdev.mozaws.net/D4278#inline-1581) and I think
"understood by the authorization server" might be better phrasing or even
removing the parenthetical all together.
RFC7522 is mentioned here as an example extension grant but maybe worth
also including mention of others like RFC7523, RFC8628, RFC8693, and maybe
even non IETF ones.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-6
 "  *  _Sender-constrained refresh tokens:_ the authorization server
      cryptographically binds the refresh token to a certain client
      instance by utilizing [I-D.ietf-oauth-token-binding] or [RFC8705]."
Given the relative immaturity of ways to do this, maybe something more open
ended would be appropriate? This reads like token-binding or MTLS are the
only ways allowed. I'd think wording that would allow for DPoP or some
yet-to-be-defined method would be better here. Also maybe drop the
token-binding reference all together (it's long expired and doesn't look
like that's gonna change).

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-7.4.4
The SHOULDs/RECOMMENDEDs in this section seem a little overzealous and/or
too specific.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-9.4
Seems to be a lot of overlap and duplication between 9.4. Access Tokens
(under 9. Security Considerations) and section 7.4. Access Token Security
Considerations, which could/should maybe be reconciled.
"(#pop_tokens)" hints that some text was copied from elsewhere but the
markdown references weren't fixed/updated

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-9.5
"(#refresh_token_protection)" same thing about markdown references not
fixed/updated

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-9.6
"(#insufficient_uri_validation)", "(#mix_up)",
"(#open_redirector_on_client)","(#csrf_countermeasures)", again
"(#mix_up)", and "(#redirect_307)"

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-9.7
"Attacker A4 in (#secmodel)"
"The use of PKCE is RECOMMENDED to this end."
PKCE is required elsewhere so this doesn't seem quite right. Similar
comments about text ijn
https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-9.14 that
talks as though PKCE might not be there.

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-9.17.1
"(#redir_uri_open_redir)"

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-9.20
"(#client_impersonating)"

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-12
"   *  Redirect URIs must be compared using exact string matching as per
      Section 4.1.3 of [I-D.ietf-oauth-security-topics]"
Should that maybe be qualified to cover dynamic ports on ephemeral local
http servers used for redirect URI with native clients?
BTW, does [I-D.ietf-oauth-security-topics] need to make a similar
allowance?

https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#appendix-C
"TBD"
Given the potentially high visibility of an OAuth 2.1 effort, I think it'd
be worthwhile to list organizational affiliations of individuals here in
the acknowledgements along with their names. Something like what was done
in the first part of
https://tools.ietf.org/html/draft-ietf-oauth-access-token-jwt-06#appendix-A.
This can help with visibility and justification of (sometimes not
insignificant) time spent on the work by non-authors/editors.

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

---

From: Dick Hardt <[email-masked]>
Date: Wed, 22 Apr 2020 16:35:53 -0700

Brian: many, many thanks for all the feedback!

I did a quick skim of your comments, and will address the first and last
ones.

On Tue, Apr 21, 2020 at 3:49 PM Brian Campbell <bcampbell=
[email-masked]> wrote:

> "replaces and obsoletes the OAuth 2..0 Authorization Framework described
> in RFC 6749."

I took this language from RFC 6749:

 This specification replaces and obsoletes the OAuth 1.0 protocol described
in RFC 5849.

And adapted it.

> marker too for 6749 https://tools.ietf.org/html/rfc7991#section-2.45.8
> and probably also "updates"/"obsoletes" for others based on the scope of
> the rest of the document (not sure how this even works with a BCP or if you
> can or would want to update or obsolete a BCP) if this work proceeds. That
> or even replace the browser based apps draft?
>

Our thinking was to include all the documents where the OAuth Security
Topics was obsoleting sections of documents so that it could stand on its
own, and roll up all the best practices.

If there are new best practices for OAuth 2.1, then that would be a new
BCP..

> I guess that's a TBD circa page 68. There was talk about the device grant
> being in scope but I'm not seeing it (not saying it should or shouldn't be
> there but it was talked about). I dunno exactly but those are some scope
> questions that come to mind.

Perhaps have a section in the document listing all existing documents that
work fine with 2.1?

<snip>

> insignificant) time spent on the work by non-authors/editors.

 Sure. Are you ok with being added there?

---

From: Brian Campbell <[email-masked]>
Date: Fri, 24 Apr 2020 14:36:24 -0600

> Brian: many, many thanks for all the feedback!

You are welcome. I won't lie - it was not a quick exercise :)

> [email-masked] <[email-masked]..org>>
> wrote:
>
>> Been working on this on and off for a while now (it's not exactly short
>> at 80+ pages, various other priorities, etc.) but wanted to share my
>> thoughts from an initial review of the OAuth 2.1 draft before the interim
>> next week where it is on the agenda
>  This specification replaces and obsoletes the OAuth 1.0 protocol
> described in RFC 5849.
>
>
> And adapted it.
>

Well then, I guess there's precedent for it. To me, it doesn't seem quite
right. But I won't argue further with precedent.

>> marker too for 6749 https://tools.ietf..org/html/rfc7991#section-2.45.8
>> <https://tools..ietf.org/html/rfc7991#section-2.45.8> and probably also
>> "updates"/"obsoletes" for others based on the scope of the rest of the
>> document (not sure how this even works with a BCP or if you can or would
>> want to update or obsolete a BCP) if this work proceeds. That scope could
>> be better described in the abstract too as discussed somewhat in the thead

Okay, thanks for the explanation. I think that generally makes sense.

I was thinking more about the other direction like what it would mean for
OAuth 2.1 to update or obsolete the existing BCP 212. But maybe that's not
even an issue.

> Perhaps have a section in the document listing all existing documents that

I'm honestly not sure if anything is needed or not. But I do want to try
and ensure that such a perception problem doesn't arise.

Perhaps have a small section that says more or less what I'd said above -
that existing extensions and profiles of RFC 6749 that use legitimate
extension points still present and unchanged in OAuth 2.1 are still okay as
they are now? And include a not-meant-to-be exhaustive list of such
documents as examples thereof.

>> Given the potentially high visibility of an OAuth 2.1 effort, I think
>> it'd be worthwhile to list organizational affiliations of individuals here
>> in the acknowledgements along with their names. Something like what was
>> done in the first part of

Yes, thank you. That was sort of implicit in my suggestion but perhaps I
should have been explicit.

---

From: Aaron Parecki <[email-masked]>
Date: Fri, 24 Apr 2020 16:47:49 -0700

Thanks for the detailed review Brian! I've made many of these edits to the
draft, but there are still a few things that need some discussion on the
list. My notes are inline below.

On Tue, Apr 21, 2020 at 3:49 PM Brian Campbell <bcampbell=
[email-masked]> wrote:

> I was momentarily proud of myself for knowing that RFC 2616 is obsolete
> but then remembered that the nits tool can automate the check for other
I've fixed a bunch of these references. It was "fun" to track down where
all the stuff in 2616 ended up as that was split into like 5 RFCs.

> here (or sec 1.4 or 7 or...) would be worthwhile.
I'm not sure how best to phrase this here since it's so early on in the
draft, but I'm generally in favor of something like this.

>
> https://tools.ietf..org/html/draft-parecki-oauth-v2-1-01#section-1.5
> <https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-1.5>
Agreed, I dropped that "out of scope" mention.

> Server Metadata do now exist.
This section was a last-minute add to the OAuth 2.0 spec and probably needs
to be entirely rethought for this update.

> All the cool drafts are doing BCP 14 [RFC2119] [RFC8174] these days.
Fixed.

> Mentioning the existence of RFC 7591 Dynamic Client Registration here
> seems appropriate. And I think it's be super useful to say that even when
> RFC 7591 isn't being used directly, it (and registered extensions to it
I've added some text mentioning the optional dynamic client registration.

> identifier of a genuine resource owner during subsequent protocol
I like it, I've incorporated this.

> Let's remove that sentence. As Michael Peck noted in his review, it
> doesn't even make sense in the context of this section describing
> authentication of confidential clients using a password/client secret. And
> that sentence has been the source of some other problems. More detail is in
> this thread
Agreed.

> name, they are general methods of client auth to the AS when making direct
Sounds reasonable to refer to that registry. I don't know if there is an
"official" way, but I've just mentioned it in the text for now.

> Maybe add something like "or in the authorization server's metadata

Sounds good.

> defined by this specification MUST NOT be included more than once."

Done.

> https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-10.3.3
This is a good point, but is too much for me to try to tackle right now.
Let's revisit this.

> As Michael Peck noted in his review, it's probably okay now to just
> mandate TLS for HTTP redirect URIs.. Although custom scheme or loopback
> redirect URIs for native apps wouldn't use or require TLS.
>
>
This is probably worth discussing on its own.

>
> https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-3.1.2.2
> <https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-3.1.2..2>
> and
After making the edits suggested by Michael Peck, I don't think this
section suggests partial registration anymore.

> defined by this specification MUST NOT be included more than once."

Done.

> Maybe add something like "or in the authorization server's metadata

Done.

> accessed via secure channel?

I've taken off the "not been registered in full" part for now.

> RFC 8414 it doesn't have to be out-of-band anymore.

I mentioned RFC8414 here.

>    are stored in encrypted form in the "code" itself but could

Agreed, I rephrased this. Though I am curious to do a proper survey of
implementations to see if anyone is doing this at all.

> Fixed.

> Note that these apply to most of the example responses in the draft.
I've removed the charset from the examples. I'm not sure about no-cache.

> also including mention of others like RFC7523, RFC8628, RFC8693, and maybe

Suggestions welcome here.

> Given the relative immaturity of ways to do this, maybe something more
> open ended would be appropriate? This reads like token-binding or MTLS are
> the only ways allowed. I'd think wording that would allow for DPoP or some

I'm generally supportive of something more open ended here. Would that also
suggest that the corresponding text in the Security BCP be updated as well?

> "(#mix_up)", and "(#redirect_307)"

I'm not sure how this happened but it'll take a bit to track these down.

> talks as though PKCE might not be there.

I believe this text was included since an extension such as OpenID Connect
can define other methods of authorization code injection. Also note that
this same sentence is in the Security BCP so if we update anything here
they should probably match.

> BTW, does [I-D.ietf-oauth-security-topics] need to make a similar

Sounds like there are a few things around redirect URIs that need to be
clarified in both documents.

> *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-masked]
> https://www.ietf.org/mailman/listinfo/oauth
>

---

From: Brian Campbell <[email-masked]>
Date: Mon, 27 Apr 2020 08:56:15 -0600

> list. My notes are inline below.

Thanks Aaron. I realize there was a lot there. I've followed up on just a
couple of the things below that seemed like they needed a response.

> I'm generally supportive of something more open ended here. Would that
> also suggest that the corresponding text in the Security BCP be updated as
> well?
>

For some reason, I'd been thinking that this stuff had been changed in the
Security draft. But I was apparently mistaken.  Anyway, yes, I think it
should be updated there as well.

>> <https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-9..7>
>> https://tools.ietf.org/html/draft-parecki-oauth-v2-1-01#section-9.14
>> that talks as though PKCE might not be there.
>>
>

I guess my understanding of the intended scope of the two documents was
that they were a little different. Like there's a security profile that
describes a few different ways of achieving something with the tools of
2.0, OIDC and PKCE. While 2.1 is saying PKCE is always required and there
are benefits to that. So the latter doesn't seem to need conditional
language around PKCE or alternatives. Maybe my understanding is flawed
though. Or maybe there's a larger question here about the value/need/cost
of having a lot of duplicative or overlapping content across multiple
documents.