Re: [OAUTH-WG] Detailed review of OAuth2.1

[[e-mail]]

Thank you, I am so glad you think so!

 

I hear you on the id_token abuse. That would be easily solved by appending a “provided that the resulting id_token is not abused by using it as access token”, in fact that would explicitly address one of the most common abuses we witness in this space by finally providing explicit language on the matter. I had frequent clashes with the Kubernetes crowd about it, and they required nuanced arguments, making them grok the concept of audience etc etc- all stuff that could have been avoided by having straightforward language along the lines of the above. We could argue whether that language belongs to the OIDC spec more than the OAuth2.1: my position is that we should take this opportunity to bring extra clarity, nothing prevents repeating that if the OIDC people will do their own updates in the future.

I also hear you on the open endorsement, however I suspect that just saying what you suggest without mentioning OIDC at all will not solve the problem of people thinking this deprecates those OIDC flows, too. Perhaps a compromise would make it explicit that the security considerations that led to the omission of implicit for the token response type in oauth2.1 do not apply to those flows in OIDC, provided that the id_token is not used as access token. So non an endorsement, but an explicit scoping statement 😊 would that sound more balanced?

 

From: Torsten Lodderstedt <[e-mail]> 
Sent: Saturday, December 12, 2020 4:20 AM
To: Dick Hardt <[e-mail]>
Cc: Vittorio Bertocci <[e-mail]>; [e-mail]; Aaron Parecki <[e-mail]>
Subject: Re: Detailed review of OAuth2.1

 

Thanks as lot Vittorio! You gave us a lot of homework but I think the draft will be improved a lot based on it.

 

Re OIDC implicit: I‘m reluctant to explicitly endorse use of OIDC implicit (response type „id_token“ or „code id_token“) as there are examples in the wild where the id_token is used as access token. Moreover, I‘m not aware of any systematic security threat analysis of those flows.

 

I‘m fine with pointing out to readers that omission of response type „token“ does not deprecate other extension response types.

 

WDYT?





Am 09.12.2020 um 01:55 schrieb Dick Hardt <[e-mail] <mailto:[e-mail]> >:



Thank you very much for your detailed feedback Vittorio!

  <https://mailfoogae.appspot.com/t?sender=aZGljay5oYXJkdEBnbWFpbC5jb20%3D&type=zerocontent&guid=97ba8439-d705-4f7d-88af-f5124e3a8225> ᐧ

 

On Tue, Dec 8, 2020 at 3:22 PM <[e-mail] <mailto:[e-mail]> > wrote:

Dear authors,

It took ages but I finally managed to go thru a full review of the current OAuth2.1 draft. Apologies for the delay.

Metacomments:

*	The VAST majority of the comments are suggestions for improving clarity, mostly on historical language coming from 2.0 that I found myself having to clarify to customers and colleagues over and over thru the years. None of those are critical.
*	There are a few places where 2.1 requires a MUST I believe to be unwarranted/too restrictive. For each of those I did my best to provide context and concrete examples.
*	A sizeable category of comments and disagreements on MUST come from treating mobile and desktop apps as largely equivalent under the “native app” umbrella, despite of the vast gulf that separates the two both in terms of security posture and user experience. Again, I tried to be as matter of fact as possible in there.
*	The main reason for which I spoke up during the IETF interim on oauth2.1 was the confusion the omission f the implicit grant caused among the devs using implicit in OIDC for obtaining ID_tokens. I suggested some language to pre-empt the issue, but I expect some iteration there.

Thanks,

V

 

§1

I wonder whether we should take the opportunity offered by OAuth2.1 to clarify frequent points of confusion about OAuth, by explicitly calling out in the introduction what is out of scope.

For example: OAuth is not an identity protocol, as it doesn’t concern itself with how resource owners are authenticated; OAuth isn’t meant to address 1st party scenarios, although the reader is free to use it in that context as well; and so on.

I believe there is value in adding this in the introduction rather than relegating it in some later considerations section, as the people who need this information the most rarely read past this point.

 

§1.1

In the RS definition, wondering whether including the word “API” would help to clarify what an RS is in practice.

 

§1.2

I always found this part extraordinarily difficult to decipher. I get that this is the first description and doesn’t have to be exhaustive and consider all cases (eg it’s ok if step 3 claims that the client authenticates w the AS even tho that’s only for confidential clients), but I think it could be much clearer than it is today.

Step 1 says

The client requests authorization from the resource owner.  The authorization request can be made directly to the resource owner (as shown), or preferably indirectly via the authorization server as an intermediary.

Besides the fact that “requests authorization” is a bit vague, this step and the corresponding diagram leg does not correspond at all to what normally happens- to get a code, the client does need to hit the AS and the mention in passing in the text isn’t enough to figure that out. Also, with the omission of ROPG there really isn’t any way of asking anything to the RO directly (the client creds doesn’t involve the RO).

I would recommend updating that diagram to be more descriptive of the canonical scenario.

Step 2

mentions the 2 grants defined in the spec, but only one of them represents the RO’s authorization. Claiming that the client itself is the RO is a formalism that doesn’t meet the reader’s intuition at this point.

Step 5 

The language here triggered multiple discussions, in particular on whether the AT can actually be used to ascertain the identity of the client – that isn’t the case for public clients, for example; besides, that’s not really the highest order bit of the AT. If it is, it seems that the spec should be more explicit about how client identification from the RS by means of an AT works. If it isn’t, perhaps we should change the language to omit authenticate.

The last paragraph is emblematic IMO – if the preferred method is very different from the diagram here, and if the abstraction presented here is not terribly useful (given that we no longer have multiple RO based grants, excluding the extension grants that are still too far at this point to warrant a cognitive downpayment for the reader) I wonder whether we’d be better off doing the authz code diagram directly (and mention that we also have the client creds grant separately).

 

§1.3

I understand that we can’t really change this because we inherit from OAuth 2 but I’ll mention it anyway- modeling clients as ROs is problematic, as it doesn’t often match what happens in practice. A confidential client might batch-read a user’s inbox searching for ad words, but the resource owner remains the user.

I know we straighten things up in 1.3.2, but the positioning here is confusing.

Also: isn’t the refresh token grant a core-specified grant as well? I know I am nitpicking.

§1.3.1

We don’t say anywhere here that the authorization code can be exchanged for an access token. It can be somewhat inferred from 1.2, but it’s a bit of an intelligence test (one needs to infer from authorization grant).

P2

“obtains authorization” could be more specific, to reinforce that we are doing a delegated flow. “Obtains” seem to suggest that we are talking about consent, rather than AS side rules. If that’s the case, calling it out might make the scenario clearer.

P3

Both the benefits listed apply to confidential clients only. Not sure whether calling it out here would help prevent confusion later on (eg people thinking that public clients can prove their identity) or would bring confusion on now (given that we didn’t differentiate between client types yet). Either ways, formally we are OK here; I am just thinking how to make things clearer. Perhaps defining client types before grants might help being clearer here.

 

§1.3.2

A concrete example of credential (eg shared secret) might help clarify things here. Also, the fact that client credentials indicate both a grant in itself and an artifact (which participates in other grants) is a well know source of confusion. Wondering if calling this out here might help.

 

§1.4

In general, we use “access token” and “token” interchangeably- perhaps pedantic, but I would suggest we always use “access token” to prevent confusion with refresh tokens later on, and other token types in other contexts (eg think ID tokens).

P1

The client should treat the AT string as opaque, but that doesn’t necessarily means it is: in some cases the client CAN see inside the token, and with the current language they might interpret it as “in this case, it’s OK to look- otherwise they would have made it opaque, per the spec”.

 

§1.5

The first phrase of P1 is wonderfully clear. We should have the equivalent in §1.3.1

Not having defined a mechanism for requesting a RT here, leaving it to ASes to decide when and where, created the situation in which some AS only issues RTs when they get the offline_access scope, with all the unfortunate consequences about RT lifetime vs session lifetime etc… I know we can’t really change this now as we don’t want to break existing AS implementations, but wondering if there’s anything we can say to further clarify/give readers a headsup about the ambiguity/diversity of behaviors they’ll encounter here.

P2

It’s odd that we say “usually opaque to the client”  for the RT while we decisively said opaque for the AT. Also, the client shouln’t do anything w the RT content hence I think the same considerations done for §1.4P2 apply here.

“The token denotes an identifier used to retrieve the authorization information” gets into the specifics of the implementation and it’s not universally true (some AS encrypt/sign the authz info in the RT itself and have no server state whatsoever.

Step 3

Should we add a reference to RFC6750 here?

 

§1.8

Should we say rich *delegated* authorization framework?

 

§2

“end-user interaction with an HTML registration form” is oddly specific 😊 in particular, I think “end user” might be misleading. We can either say “interactive” or refer “the client app developer” or equivalent.
Overkill but I’ll mention it anyway. Should we say that typically the client registration in the non-dynamic scenario occurs in authenticated settings? Not strictly necessary but might help the reader to tie what we say in this section with their concrete experience.
 
§2.1
P4
“Authorization servers SHOULD consider the level of confidence in a client's identity when deciding whether they allow such a client access to more critical functions, such as the Client Credentials grant type.” 
I don’t understand this sentence. Is the client credentials grant type a “more critical function”? Or is it a level of confidence? Either ways, I think it needs clarifying.
P5
IMPORTANT: this is going to break many OAuth implementations with significant adoption. Auth0 is fine (each client_id is tied to a single client type) but I know of others that will break.
I suggest softening to a SHOULD NOT.
“browser-based application”
I am not convinced this is so much easier than the original “user-agent-based”. I understand the advantages (dovetails w the BCP, more precise given that apps can be user agent as wells nowadays, more familiar) however the break w 2.0 terminology is jarring. I don’t feel very strongly about it but enough to type it.
 
§2.2
It’s a bit odd to define the client identifier like it’s something brand new when §2.1 already introduced it. This language from the original 2.0 might need to be revised to accommodate that change.
Wondering whether a warning against structured client_ids (eg identifiers assembled thru some string template, like developer name+region+serial) would be in order. Perhaps in the security considerations?
 
§2.3
P1
That sounds vaguely circular, given that being assigned credentials might be considered part of the “establish a client authentication method” task listed there. I’d simply say “if the client is confidential or credentialed”.
P2
I’d add “by the authorization server” for good measure.
P3
That sounds vague. Shouldn’t it be mandatory for the AS to require client auth for the client types who have creds? “if possible” seem to open the possibility of circumstances where that’s not he case.
P5
I think that this idea of identifying the client will need to be fleshed out more for people to fully understand it. Credentialed clients can prove that they are the same client instance across multiple transactions, which some might consider a weak form of identification. To rule that out, it has to be mentioned upfront IMO. If not here, in some of the considerations section… with a forward reference here.
P6
Do we say why anywhere? If yes, we should reference it. If not, perhaps we should.
 
§2.3.1
We no longer mentioned the empty client secret, but we don’t forbid it either. What’s our stance?
 
§2.3.2
In §2.3 we mention MTLS, private_key_jwt, but here we just point the reader to IANA. It looks like echoing those methods here might help clarity.
 
§3.1
Last paragraph
I have been in discussions where readers interpreted this as “you cannot send custom parameters to the authorization server”. To preempt that mistake, we mighr consider calling out that custom extensions _are_ permitted as long as the AS supports them. I know that’s what the current language says already.
 
§3.1.1
Wondering if referring to some specific, well known extensions (like OIDC) might help readers to better understand this point.
 
§3.1.2
RFC3986 6.2.1 talks about character by character comparison, but doesn’t mention case sensitivity. I am sure it does elsewhere in the spec, but for clarify and readability I recommend specifying the desired behavior directly here.
 
§3.1.2.1
Personally, I would advocate for a MUST here. True, lots of people won’t comply at development time, but I think that’s OK as long as they do use TLS when going in production. 
Also, SameSite changes are making the use of HTTPS at dev time more and more common. If OAuth2.1 is about picking the best of the security practices, this seems like a an obvious candidate.
 
§3.1.2.2
P3
“lack of requiring” doesn’t sound proper.
 
§3.2
P2
Should we also say that the spec doesn’t care about _when_ the client obtains the endpoint?
Last P
Same considerations as §3.1
 
§3.2.1
P1
That’s stricter than §2.3P3 – I think the language there should be tweaked to be coherent with the one here.
 
§3.3
Wondering if the “scope strings order does not matter” point should be somehow emphasized or clarified. I know of implementations who considered heuristics such as “if the scopes requested correspond to multiple resources, I’ll show consent for all byt the token eventually issued when redeeming the code will have as audience the resource corresponding to the FIRST requested scope”, which would violate the order invariant requirement. 
 
§4
Potentially VERY confusing. I would recommend to be more specific and state that “OAuth 2.1 defines two grant types”. 
 
§4.1
Diagram
Not critical. But I want to point it out. The first time I saw this diagram I found it confusing. The fact that the same numeral is assigned to multiple legs is just odd for anyone not already familiar with the flow, possibly still struggling to understand the client as a service side component.
Also, now that we have mighty SVG support, I would strongly advocate for a modern version of this diagram (there lines perhaps don’t need to be broken into segments).
Step 5
“optionally, a refresh token” is too vague IMO. I will look for opportunities to clarify later in the spec, given that this might not be the best place to go in details.
 
§4.1.1
Overall: a high summary of the steps in this preamble might help. The current denormalization in subsection can be pretty hard to follow for someone seeing this for the first time. 
Also: creating challenge and verifier BEFORE assembling the request seems profoundly counterintuitive to me, as it emphasizes a security measure over the core function of this leg of the flow. Unless there’s a crypto reason for this current sequencing that I can’t see, I recommend first creating the core request (what’s now 4.1.1.3) and then attaching challenge and verifier. Also, sending the message can be its own subsection rathe than being conflated with the last message composition subsection.
P1
“to begin” remains a bit suspended, given that there’s no obvious segue on what constitutes the steps after the beginning. 
P2
“later use with the authorization code” could be clearer, e.g. “at authorization code redemption time”. At this point that might still not be obvious for the reader.
Mentioning the provenance of properties (parameters?) code_challenge and code_verifier without first having introduced them might confuse people not already familiar with them and the request process in general, as their function will not be obvious not naturally map with the preceding sentencer.
P3
Imposing a MUST before knowing what those this are yet is not as clear as it would be if this would be stated after their use and function has been explained.
 
§4.1.1.3
On state. Given the change vs OAuth2, I think it might be helpful to call out the relevant section on the appendix about differences to help people familiar w 2.0 not to miss this important change and avoid doing work twice.
 
§4.1.2
P2
Should we say that the code should be opaque to the client, to discourage the use of structured code templates that can be partially manufactured?
P8
“he server MUST NOT  include the "code_challenge" value in client requests”, was that meant to be “responses”? 
Qualifying “other entities” (anyone but the AS?) might make this point clearer.
 
§4.3
We mentioned extension grants in passing, but I don’t recall seeing a definition/description of their function in the context of the framework. Even a short sentence to that effect here would help, given that the section title names them explicitly. Also, stressing that the device flow is just one example and other extensions might differ (for example in their logic to establish whether an access token request is valid and authorized) would go a long way in helping the reader put this section in better focus.
 
§5.1
On the access_token parameter. Given the discussions we had for the JWT AT profile draft, I am wondering whether it should be called out here that the AT recipient is the RS, that the client should not expect to be able to parse the access_token, and that the AS is under no obligation to use a consistent AT encoding outside of what is negotiated with the RS. I don’t feel very strongly about this, or about where in the spec this should be called out, but it sure would have made life easier in those discussions- hence the comment.
On the refresh_token parameter. The lack of details in how OAuth2 describes how/when an AS returns refresh tokens led to today’s complicated situation in which many implementations issue RTs only when OIDC’s offline_access is received in the scopes, as it was the only mention in public specs describing a concrete behavior. See the associated online_access discussion on the OIDC list, as RTs gain importance as session artifacts of sort for SPAs now that implicit is dead and ITP makes iframe renewals problematic.
Unfortunately it is too late to be prescriptive here, as we cannot break compatibility with whatever choices existing AS implementations made. However we can be more descriptive and give the reader a better idea of what’s the range of possibilities. Some nonnormative examples of how existing AS determine whether to issue an RT or not (eg as an option determined at client registration time, or any other heuristic you guys encountered in the wild) might help people to better understand their options and the intent of the specification here.
 
§5.2
It might help to remind the reader here that extensions to the core spec might specify or further specialize circumstances in which the errors mentioned here are returned (for example, see the validation errors in the JWT AT profile). There’s a mention of that in §7.3.1 but that’s pretty far, and having even brief language here might be handy for people reading the spec for reference rather than cover to cover.
 
§6
P1
I think the risk assessment is just one of the factors an AS might use to decide whether to issue an RT or not. The current language suggests risk is the only determinant in that decision and that doesn’t seem right.
Saying that one might refresh tokens using other grants seems odd. A new authorization code grant gets me a new token and offers me the opportunity to describe what token I want (scopes etc), the fact that I might choose to ask the exact same things I asked in the original request is expediency. I would rather phrase this as the fact that the client can simply repeat the original request, and external factors such as cookies, sessions and other auth method specific options may allow the client to do so without prompting the user.
P2
We might need to be more precise here. Do we mean the scopes consented by the RO in the request that led to the issuance of the RT being used? Just saying consented by the RO for the client does not exclude cases in which there are more instances of the client in operation. Say that I am running uber on phone 1 and I consent to read my google calendar, getting AT1 and RT1. Say that on phone 2 I also run the uber app, and this time I consent to write my google calendar, obtaining AT2 and RT2 on this new device. Now consider the various combinations here. Should RT2 allow me to get calendar:read too, given that it was already consented by RO for this client? Should RT1 allow me to get AT1’ containing calendar:write, given that RO consented for it when using a different instance of the same client? Whatever is the answer you want to the questions above, I think the spec should have language clear enough to unambiguously determine the desired behavior.
 
§6.1
It’s a bit confusing that half of the RT use requirements is in §6 (the requirement to authenticate confidential/credentialed clients) and half is in here, with the only differentiator being the nature of the client. This is pretty minor, but I think I would personally find clearer if all the requirements for the use of an RT would be consolidated in a single place. It’s true that the public client reqs are a SHOULD, but still.
Rotation.
Wondering whether it would be wise to advise the reader to have their AS revoke all the still valid ATs issued by the AS from the same session/family of RTs upon detection of a RT reuse. It is not uncommon for clients to request new ATs before their projected expiration.
P6
I think the “MAY” here might be confusing when applied to the rotation, as in either the AS does it, or the scenario won’t work. I understand this is formally correct, but perhaps explicitly calling out some cases in which the AS might decide to do otherwise and acknowledge that in that case the client will be stuck might help clarify. Also, if the public client protection measures were in §6 instead of here, there would be less opportunities for confusion as it would be easier to grok that this doesn’t apply to the rotation case only (now adjacent) but to other RT reissuance cases as well (eg sliding expiration).
On the identical scopes requirement. Say that after obtaining RT1, which includes scopes s1 and s2 for client c1, the RO revokes authorization for c1 to use s2. Should the AS fail the RT redemption, or return an AT with only s1 and a scopes parameter informing the client of the change? As developer I would prefer the latter, to preserve the experience: but if we are adamant about the current language, I think it might be useful to explicitly call out that any changes to the grant on the AS side should result in failure of the RT redemption.
P7
Calling out deprovisioning of RO might be useful as well.
On “the logout at the AS” event, that’s definitely a valid case but I worry about how presenting that alone might reinforce misunderstandings that equate RTs with sessions. There are certainly times where we want that (see mentions earlier of the online_access discussions) but there are also cases where the ability of a client to refresh ATs needs to survive session boundaries (offline_access) and confusing the two are problematic. I don’t have a clear solution here, just pointing out a potential point of confusion. Maybe there will be more opportunities to clarify later in the spec.
 
§7
P1
An important case to call out is the AS-RS colocation, where neither introspection nor token format agreements are necessary. I suggest mentioning it openly.
 
§7.2
“bearer tokens may be extended to include proof-of-possession  techniques by other specifications” sounds like an oxymoron. Wouldn’t PoP make the token no longer bearer by the very definition above?
It looks like we might need a term for simply “token”.
 
§7.2.1
Do we also want to forbid two tokens in the same request, using different methods? Current language only constraints the behavior of one token.
 
§7.2.3
The “insufficient_scope” description here is problematic. The privileges the AT carries/points to are not necessarily (or exclusively) represented by the included scopes (eg the RO might have granted document:read to the client, but RO might have no privileges for the particular document being requested in this particular call). It might be useful to specify that “invalid_scope” should be used for authorization errors that can be actually expressed in terms of delegated authorization, leaving to RS implementers the freedom to handle other authorization issues (eg user privileges, RBAC, etc) with a different error code. Or at least, we should be clear that authorization logic not expressed via scopes is out of scope (pun not intended) for this specification.
Note, this isn’t an abstract problem: there are SDKs out there that use “invalid_scope” for every permission issues. Very confusing.
 
§7.3.1
We rewrite portions of 6750 in oauth2.1, but here we refer to it as its own spec… that could be confusing as it’s unclear which parts of 6750 are overridden by oauth2.1 (eg no more querystring) and what parts remain normative. Perhaps we can call those things out in the sections meant to replace the corresponding sections in 6750.
 
§7.4.2
Pedantic: although the title of the section states it, wondering whether every instance of “token” here should be “access token” instead. Think of cases in which the spec is quoted in discussions and disputes, where snippets can be pasted and mentioned outside of the context of this section.
P2
Referencing the JWT AT profile as an example of extension providing the info out of scope for the core might help the reader grok a concrete example.
 
§7.4.3.5
“one hour or less” seems very arbitrary, and breaks step in respect to what the spec does elsewhere (eg we don’t give any indication of how long an AS should wait to invalidate an RT for inactivity, but we do say the AS should do so). I would actually not provide any reference value here.
 
§7.4.3.6
Another opportunity of referencing the JWT AT profile for a concrete example of detailed audience restriction guidance in ATs.
 
§7.4.3.7
Besides the indications given to clients here, should we also give guidance to an RS to ignore tokens passed that way?
 
§7.4.5
Along the same lines of the comments about delegated authorization earlier for §7.2.3. I think it would be useful to acknowledge here that ATs might carry, and RSs might expect, authorization information that go beyond the delegated authorization for 3rd party API case that is core to OAuth- and remind the reader that those mechanisms are out of scope for oauth hence they shouldn’t expect those aspects to be addressed/handled/regulated by this specification. 
 
§8
As mentioned earlier, it seems a potentially confusing to reference the section of a document being superseded. I do see an issue in redefining here something already established and in use, hence I am not expecting this to change. Just wondering whether we need to provide a more explicit map of the sections in 6749 that are being updated by oauth2.1.
 
§9
Should we also say something about the scenario being chiefly 3rd party clients? We know lots of people use OAuth2 for 1st party scenarios and some considerations might differ. This might be an opportunity to finally make that clear.
 
§9.1
P1
Well, the AS doesn’t really use client auth... the active part here is the client itself. Perhaps the AS can require it, when possible.
P2
Unclear. Are we saying that if it is possible to safely distribute keys to client, the AS MUST use client auth? That seems odd, there might be other reasons coming into play (cost, security posture) making that choice not viable. Or is the intent to say that the AS should not use client auth if the key distribution cannot be trusted? That sounds far more realistic, but then the language should be tweaked or the reader might pick the former interpretation.
P3
The AS can’t really PREVENT creds forwarding as the RO machine might still have funny business going on (eg DNS attacks). Some softer language might be more accurate there. 
P4
That sounds very abstract. What does it mean? That the AS should consider to issuing RTs to public clients? If that’s the case, we should just say so… tho without more details, I don’t know how actionable the guidance here will be. I can see BoFa requiring a user to reauth in their iPhone app after inactivity, but I don’t see Uber doing so for their app.. unless they produce long lived ATs, which isn’t what we want either.
P5 
This could be clearer. The dynamic client registration case just flesh out the confidence level an AS can have in its identity, but does not offer a corresponding privilege level to it – whereas the second case does mention assigned privileges explicitly. Also, pitting “dynamically registered client” vs “web application” might suggest the app type is a factor, whereas AFAIK 7591 can be used for registering web apps too (whether that s wise or not is immaterial here).
§9.2
Should we say something about whether native clients should be allowed to be “upgraded” as confidential clients in the future, or the other way round? I know of scenarios where people did that to preserve consent info, but that seem sketchy security wise.
P3
The SHOULD here refers to a requirement that in 10.3.1 is a MUST. I don’t think the MUST is warranted (more about that in the 10.3.1 comments) but if we do keep it, it looks like the level should be coherent here.
P4
That example is compatible with a SHOULD- works now, but would look odd if we’d upgrade P3 with a MUST for coherence with 10.3.1.
P5
Inclusion where/how? We should be precise IMO. If it’s just registration material (eg not part of the redirect URI), we should mention how we expect it to be used in the context of OAuth- and if we don’t know, perhaps we should not mention it here. 
 
§9.3
P1
I know it becomes clearer later on, but I think it would help here to explicitly call out confidential and credentialed client as the subject of this sentence. Those are the only client types with credentials, hence the current language is formally correct. This is just for clarity.
P2
I thought we require redirect URI registration in all cases? This makes it sound it’s only for public clients.
P3
I have been in various discussions where people were attempting to interpret what “explicit RO authentication” means in practice. Is it a full credential prompt regardless of whether one session already exists? A selection between existing sessions, if present?
P4
This is unclear. As it currently reads it seems to prohibit things like getting a new authz code silently via iframe (and prompt=none or equivalent UX suppressing mechanism, please ignore the ITP complications for the sake of argument). 
 
§9.3.1
P1
I don’t follow this sentence. The client identity cannot be proved for public clients (see also next P), and the native apps are public clients unless otherwise specified (eg credentialed).
P2
I find this misleading. Client side measures such as claimed schemes, domains etc might work to prevent an app impersonating another app on the same device/OS, but they aren’t guaranteed to be honored on other operating systems. The AS has no way of knowing whether those measures have been enforced on the client, hence it should not accept them as proof.  
 
§9.4.1
This is another place where a reference to the JWT AT profile would provide a concrete example of the conditions set forth here (eg RS guidance for audience validation).
Also, as mentioned earlier, it might be useful to remind the reader that the AS might include in the token authorization information that go beyond the delegated authz scenario OAuth2.x concerns itself with, and that those aspects are beyond the scope of this specification.  That would truly go a long way preventing people from abusing and overextending the spec on scenarios it is not meant to address.
And even for the canonical scenarios, it might be useful to remind the reader that the RS might have extra logic not described in this spec that determines whether the call will be authorized or otherwise- to dispel the notion that the AS is always the sole source of truth for authorization. 
 
§9.4.2
P2
Clarifying that MTLS is one instance of the sender constraint methods just mentioned would prevent some readers considering that an independent, additional constraint.
 
 §9.5
In §6.1 we give more details about protection, should we have a backward reference to that section here?
P2
Is it worth to specify that it doesn’t matter how the AS tracks the binding? (eg server side or embedded in the RT bits themselves).
 
§9.6
P1
This language makes the assumption that for client cred grants, the sub will take on the client_id value. That it certainly possible, but not a given. As such, the language here should be explicit abouty the fact that it’s what we are expecting to happen in this particular scenario.
P2
Referring to the client as the actor here is confusing as soon as you move beyond client_id. When you talk about rhe sub or any other value, it’s not as much the client as it is the developer who owns the client. The difference is subtle but might be a source of confusion.
 
§9.7
P3
I thought that we were going to make PKCE or nonce the only two mandatory alternatives? Nothing against supporting state as an accepted way to achieve this, just surprised as I recall people being quite adamant about pushing PKCE.
P4
Looks like the iss response parameter might make the distinct redirect URIs unnecessary?
 
§9.7.1
The guidance in this section isn’t likely to be widely followed, but I understand the rationale behind it.
 
§9.8
P2
That’s probably obvious, but I think we should specify that those multiple attempts should come with the expected code verifier as well for the revocation logic to be triggered- if it’s just the code without verifier, it doesn’t look like the leak went far enough to warrant that intervention.
P4
If this holds, then the remark about the use of state for CSRF in §9.7 P3 seems unnecessary.
P9
The use of MUST here seems incompatible with the concession of using nonce instead of PKCE. Either we allow it or we don’t…
 
§9.11
P1
Now that we no longer trade in RO passwords, should we mention them here? Sure, if the AS uses passwords they do need protection, but so do lots of other things the AS might use as part of auth that we don’t mention here for Occam’s razor. 
P3
What other means are we referring to? 
 
§9.12
“service providers” occurs for the first time in the document here. Tying the guidance to entities already mentioned in the doc (clients, AS) might make things clearer/more actionable.
 
§9.13
I understand that some of the comments in this area should be done on the BCP rather than here, but here we are. That said…
Blanket comment: this section seems to assume native app == mobile app, and that’s not strictly the case. Desktop apps have different characteristics and desktop OSes different capabilities. Would be clearer to make a distinction when relying on mobile OS capabilitesas we  appear to do here.
P2
Embedded user-agent != fake external user agent, but P2 uses that interchangeably. A desktop app might use an embedded browser for UX reasons and making no attempt whatsoever to disguise that as an external agent. And I would be pretty surprised to see a malware remover get rid of google drive for Mac, or Adobe products, or Office- all native apps using embedded user-agents for authentication.
P3
It’s unclear how the AS would do that, given that user agent strings can be faked. Google has its ML based secret sauce, but that might not be accessible by everyone.
P4
This is mobile specific, doesn’t apply to desktop apps as readily. Also unclear how that would change for the end user, given that pixel perfect replicas can easily include fake address bars. More details on how a user would detect an actual browser (presence of an existing session, access to bookmarks etc) might add enough color to help the reader truly understand the extent of the remedy power (or its true limits). 
 
§9.14
There’s no action for the reader here. If that reflects the actual situation (none of the roles described in this spec can do anything to mitigate or contain the damage, and solutions to prevent the situation lie outside its purview eg use MDM software on your corporate devices) we should explicitly say so.
 
§9.15
A bit odd that we used CSRF throughout the document assuming the reader was familiar with it, but here we attempt a definition. 
Still confused on how we admit state as a valid mechanism, as opposed to limiting to nonce and PKCE. Also note the potential discrepancy called out earlier in which at code redemption time we appear to require PKCE, in contrast to admitting nonce/state here.
 
§9.18.2
Wondering if here we should go as far as recommending the AS keeps dynamic client registration OFF when it’s not needed. That might provide good secure by default guidance for AS SDKs and product developers.
 
§9.19
I assume we’d add the “iss” response parameter as alternative to the requirements here?
 
§9.20
P1, P2, P3
The security properties described here do not apply as is to desktop apps using embedded browsers.
As mentioned earlier, on the desktop the separation between apps isn’t as stark as on mobile- the keystrokes message pump is potentially accessible at the user session level, encompassing multiple applications. Although process separation mechanisms are in place, the circumstances of the execution and the OS specific features in this respect determine the degree to which entering credentials in one process makes the data inaccessible by another. We should not claim advantages we cannot guarantee, and we should be explicit about what measures specific to dekstops should be considered to mitigate risks eg not executing apps as admins, enable and use UAC at the right level on Windows or equivalent on other OSes, use app sandboxing when the OS supports it, etc. Not suggesting we call out specific technologies like UAC, but at least point at the category of security measures.
P4
As mentioned above, the presence of the address bar can be easily faked by pixel perfect replicas- relying on those is security theater.
P5
This is a valid concern, but if we want to frame it in the context of the security considerations section we need to add more color (eg minimizing the situations in which one needs to enter creds is good security practice).
 
Another thing we might add as lowlight for embedded browsers: password managers might not work, making the user’s life difficult and possibly promoting insecure situations (eg placing a random pwd in the clipboard, where other apps might later steal it from).
§9.21
Do we need this here, given that we cover this in depth earlier?
 
§10
I find this subdivision confusing. We have native clients considerations scattered throughout the entire specification, but now we have a dedicated section that somewhat repeats some of the points already made while interleaving new ones. Perhaps having a more specific section title, one that better characterizes the content and intent of this section, would avoid giving the impression that if the reader is interested in native clients, this is the only section they need to read.
P2
See discussion so far on native vs desktop.
Also: I have been in ferocious discussions where people thought the external user agent HAD to be the browser, which we know isn’t the case (hero apps like the FB SDK or OS features like sign in w Apple work just as well). Some language here giving a nod to those non-browser external user agents might help clarify.
P4
See previous discussion on mobile!=desktop.
P5
I’d always qualify the “browser” with system, external, device etc given that some reader calls the embedded user-agent “embedded browser”, hence just saying “browser” without modifiers is ambiguous.
User authentication state.. on the device. I think specifying it will clarify what this really means.
 
§10.1
Unless you scope this statement to MOBILE apps, I think the MUST here should be a SHOULD. 
The security posture of the desktop is different enough, and the quality of the user experience when using a system browser bad and disomogeneous enough, that a MUST isn’t justified here.
 
§10.2
Beware of the native-mobile false equivalence here.
P3
One or two examples of non-browser external user agents might help.
P4
Text pasted here is technically not a best practice but core.
I see there are examples here, so no need to add them in P3.
It’s unclear why the external browser is RECOMMENDED here- if it’s because we can’t go in details of the behavior of non browser apps, it seems like we should say that much rather than making a recommendation. In other words, RECOMMENDED expresses a strong preference for it, but if I am on iOS and I want to sign in with Facebook I am actually better off using their SDK both for security and user experience reasons than to use the system browser.
 
§10.3
Requesting a MUST for all 3 methods seems restrictive… why not requesting at least one?
 
§10.3.1
P3
This seems unnecessarily restrictive. 3.8 in 7595 is mostly SHOULDs, and it refers to organizations rather than individuals. Not every developer owns a domain, not every app goes on an app store and is meant for general public consumption. An organization sideloading apps on managed devices should not be forced to follow those constraints if they control the environment and aren’t worried about other apps competing for the same schema, but placing a MUST here might compel SDK developers to embed validation checks that would make developers on those circumstances deal with complexity without any security upside. In fact, apps should not even be required to have internet access in the general case but this requirement does impose that. 
I think this is an important best practice that should be encouraged, RECOMMENDED or even SHOULDed , but it shouldn’t be a MUST in the core specification.
 
§10.3.2
P4
Like everything happening on the client, the AS cannot really take that as guarantee. What might be true for an app running on iOS might not apply if the requests are all manufactured via cURL on a Linux box. The scope of those measures is really limited to one particular device or devices sharing an OS, outside of that cohort there are no guarantees.
 
§10.3.3
This section should come with glaring warnings, anything actively listening on the client extends the attack surface- an app listening on the loopback might now get affected by exploits in the local HTTP driver/local network stack and taken over, executing with the same privileges as the user (remember https://www.rapid7.com/db/vulnerabilities/WINDOWS-HOTFIX-MS15-034/ <https://www.google.com/url?q=https://www.rapid7.com/db/vulnerabilities/WINDOWS-HOTFIX-MS15-034/&source=gmail-imap&ust=1608080153000000&usg=AOvVaw1MpuvulMp9M1cea-hULnKA> ). The fact that the loopback is only accessible locally only reduces the risk but doesn’t eliminate it, a local process with lower privileges might use that exploit for elevation of privileges for example. We should at least recommend that the app hosting the loopback adapter runs with low privileges, reiterate the “listen only when you must” and generally warn about the extra attack surface.
As AS I might not want to support this method at all, both for security reasons and for the extra logic the wildcard port entails at registration and request serving times, but right now the spec forces me to- more reasons for relaxing the MUST for all 3 methods as mentioned earlier..
 
§12
I am not exactly sure where to place the following- this section (or a subsection) might be the best fit.
As mentioned during the interim meeting, the omission of the implicit flow in OAuth2.1 has already caused a lot of people to interpret this as an indirect deprecation of the use of implicit flow by OpenID Connect for obtaining ID_tokens, either via traditional  response_modes or via form_post.
We already debated and concluded that the reasons that led to the omission of the implicit grant in 2.1 do not apply to ID_tokens, hence there’s no reason for people to stop using OpenID Connect that way. 
Formally we are in the clear, as OIDC is vased on 2.0 the omission of implicit here does not prevent it as extension grant in OIDC anyway- but the formal stance doesn’t help in preventing the confusion and assuaging the concerns of the developers who aren’t as well versed as the people on this list in all things specifications.
Because of this, I recommend we add some language here that prevents that confusion. Something like 
 
*  The Implicit grant ("response_type=token") is omitted from this
      specification as per Section 2.1.2 of[I-D.ietf-oauth-security-topics <https://www.google.com/url?q=https://tools.ietf.org/html/draft-ietf-oauth-v2-1-00%23ref-I-D.ietf-oauth-security-topics&source=gmail-imap&ust=1608080153000000&usg=AOvVaw3bx8w6IPAUi7Sa5ZtNQswD> ]
Please note: the omission of the implicit grant from this specification does not automatically imply that other extension grants obtaining credentials directly from the authorization endpoint should also be discarded. For example, the implicit flow defined in Section 3.2 of [OIDC <https://www.google.com/url?q=https://openid.net/specs/openid-connect-core-1_0.html%23ImplicitFlowAuth&source=gmail-imap&ust=1608080153000000&usg=AOvVaw0ESmfMieBxUOYjwVAdPjYB> ] remains valid for all the response_type values not including “token”. 
 
Although it might be a bit unusual to refer to details of specs from other entities, this spec already mentions OpenID 9 times even excluding §14.2- and as soon as the browser apps section will be included, that number is certain to rise. And the confusion on this point is truly widespread- adding language along the lines of the above directly in the core would go a long way to save a lot of grief.