[OAUTH-WG] Comments on draft-ietf-oauth-v2-1-00 (The OAuth 2.1 Authorization Framework)

[Karsten Meyer zu Selhausen <[e-mail]>]

Hi all,

great to see that the OAuth 2.1 draft was adopted by the WG.
We appreciate the efforts to create a single document which contains the
current state of the art in terms of OAuth and all security best
practices very much. It will make it a lot easier for developers and
others to get started with OAuth.

Christian and I reviewed the current draft
(https://www.ietf.org/id/draft-ietf-oauth-v2-1-00.html) and would like
give some feedback.

You can find our comments below; they are sorted based on the section
they affect. I divided the comments into two parts: The first one
consists of suggestions and improvements to generally clarify the draft
and increase the security of deployments implementing OAuth 2.1. The
second part contains typos and other minor mistakes. There should be no
need for further discussion on the list regarding these comments.

We are happy to hear your thoughts on our suggestions.

Best regards,
Karsten

Comments:
Part 1: Suggestions and Improvements

Section 1. Introduction
- 1.4: "The string is opaque to the client, but depending on the
authorization server, may be parseable by the resource server."
    - What does opaque mean in this context? The client is technically
able to decode an AT if it is, for example, a JWT.
    - For refresh tokens the wording is not as strong: "The string is
**usually** opaque to the client." (see section 1.5)
- 1.5: In step 2 and 8 the AS "authenticates the client" but in step 7
the client provides "client authentication if it has been issued
credentials". Should the possibility of public clients and credentialed
clients (which cannot be authenticated) not be taken into account in
step 2 and 8, as well?
- 1.7 (and in general): HTTP status code 303 shoudl be used instead of
302 in this section and generally in all examples due to the
recommendation to use 303 in section 9.7.2.

Section 2. Client Registration
- 2.3 (and in general): Is "client authentication" the right term for
credentialed clients? We think the term "authentication" is confusing
because their credentials should be considered to be public. Therefore,
the AS can not be sure about their identity when they present their
client credentials.
- 2.3.1: An AS MUST NOT accept requests with ambiguous client
information (both HTTP Basic Authentication and client_id/client_secret
body parameters).
    - Different application logics could extract and use a different
client_id from the request otherwise. For example, the HTTP header is
validated to pass client authentication but the client_id parameter is
used to check if code/RT was issued for this client.
    - Should also be considered in 4.1.3.
    - Implicitly included in 5.2 Error Response: ""invalid_request": The
request ... includes multiple credentials, utilizes more than one
mechanism for authenticating the client …"

Section 3. Protocol Endpoints
- 3.1.2: "The authorization server MUST compare the two URIs using
simple string comparison as defined in [RFC3986], Section 6.2.1." We
think the term "two URIs" is ambiguous and suggest to name the
parameters here directly.
- 3.1.2.4: "If an authorization request fails validation due to a
missing, invalid, or mismatching redirect URI, the authorization server
SHOULD inform the resource owner of the error and MUST NOT automatically
redirect the user-agent to the invalid redirect URI." We think the term
"missing" is inaccurate because it might indicate that a redirect URI
should always be present. Maybe the paragraph could be changed to
something like "If an authorization request fails validation due to **an
invalid, a mismatching, or a missing (if it was required**) redirect
URI, the authorization server SHOULD inform the resource owner of the
error and MUST NOT automatically redirect the user-agent to the invalid
redirect URI.".

Section 4. Obtaining Authorization
- 4.1.2.1: ""invalid_request": The request is missing a required
parameter, includes an invalid parameter value, includes a parameter
more than once" Is this only relevant for the parameters defined in this
RFC or also for extensions? For example, RFC8707 (Resource Indicators)
allows to use multiple "resource" parameters in the authorization
request ("Multiple resource parameters MAY be used to indicate that the
requested token is intended to be used at multiple resources.",
https://www.rfc-editor.org/rfc/rfc8707.html#section-2-2.2).
- 4.2.2: Combine "The client MUST authenticate with the authorization
server as described in Section 3.2.1." and "The authorization server
MUST authenticate the client." below the example. The second sentence
looks oddly placed below the example.

Section 6. Refreshing an Access Token
- 6.1: Is token binding supported by any major browser? AFAIK it has
been removed recently, so it could be removed here as well.

Section 7. Accessing Protected Resources
- 7.2.1: The RS MUST NOT accept resource requests which use more than
one method to transmit the access token. This should be stated more
clearly here and not be included only in 7.2.3 Error Codes
"invalid_request".
- 7.4.2:
    - "As a further defense against token disclosure, the client MUST
validate the TLS certificate chain when making requests to protected
resources, including checking the Certificate Revocation List (CRL)
[RFC5280]." The paragraph should also mention OCSP [RFC 6960] as an
alternative to CRLs.
    - "Bearer tokens MUST NOT be stored in cookies that can be sent in
the clear". Maybe state explicitly that only cookies with secure flag
are allowed?
- 7.4.3.4: Maybe add recommended cookie flags (secure, httpOnly,
sameSite) with note "or equivalent measures" (to ensure that future
cookie flags / alternatives are allowed). This is also discussed in this
thread:
https://mailarchive.ietf.org/arch/msg/oauth/f18vynrMXC4dj4tqck7SZIl4xp4/

Section 9. Security Considerations
- 9.2: "Authorization servers MUST require clients to register" should
be clarified to "Authorization servers MUST require **native app**
clients to register" although this is already present in the headline of
the section.
- 9.7: Explicitly state that code_challenge / nonce must be bound to
user agent, as well?
- 9.15: Adjust text according to CSRF part in 9.7.
- 9.21: Remove the section as the only present recommendation is
redundant and already covered in 9.6.

Appendix:
- Appendix C:
    - Sort Extensions either alphabetically or depending on the RFC
number (drafts below the RFCs).
    - Should RFC7592 (Dynamic Client Management) really be included as
an "well-established extension"? It is only categorized as
"experimental" RFC.

General Comments:
- Unify the order of client types: In 2.1 and 9 it is web application,
browser-based application, native application, but the main section
about native applications (10) is in front of the main section about
browser-based applications (11). This should also be taken into account
when sections are added to 9, 9.1, or 9.3. Currently 9.1.1, 9.2, and
9.3.1 are about native applications and sections about browser-based
applications should be added in front of them (if there will be sections
specifically about browser-based applications).
- Is there any statement that it is allowed to add additional parameters
in the authorization/token request/response?


Part 2: Minor Mistakes (typos etc.)

Section 1. Introduction
- 1.5: "If the authorization server issues a refresh token, it is
included when issuing an access token (i.e., step (4) in Figure 2)." ->
"If the authorization server issues a refresh token, it is included when
issuing an access token (i.e., step (**2**) in Figure 2)."

Section 4. Obtaining Authorization
- 4.1.2: Note "(with extra line breaks for display purposes only)" is
missing in front of example.
- 4.2.2: Note "(with extra line breaks for display purposes only)"
present but there are no extra line breaks. The note is not present in
4.1.2.1, 4.1.4, and 4.2.3, for example.

Section 5. Issuing an Access Token
- 5.2: "The provided authorization grant (e.g., authorization code,
resource owner credentials)" -> "The provided authorization grant (e.g.,
authorization code, **client** credentials)"

Section 6. Refreshing an Access Token
- 6: Note "(with extra line breaks for display purposes only)" present
but there are no extra line breaks.

Section 7. Accessing Protected Resources
- 7.2.2: Note "(with extra line breaks for display purposes only)" is
missing in front of last example.

Section 9. Security Considerations
- 9.1.1: "public native app**s** clients" -> "public native app clients"
- 9.4.2: Missing reference to "(#pop_tokens)".
- 9.5: Missing reference to "(#refresh_token_protection)".
- 9.7:
    - Missing references to "(#insufficient_uri_validation)",
"(#open_redirector_on_client)", "(#csrf_countermeasures)", and
"(#mix_up)" (twice).
    - "Clients MUST store the authorization server they sent an
authorization request to and bind this information to the user agent and
check that the authorization request was received from the correct
authorization server." -> "Clients MUST store the authorization server
they sent an authorization request to and bind this information to the
user agent and check that the authorization **response** was received
from the correct authorization server."
- 9.7.2 (and in general): Replace "user agent" (used 18 times) with
"user-agent" (used 64 times).
- 9.8: Missing reference to "(#secmodel)".
- 9.18.1: Missing reference to "(#redir_uri_open_redir)".
- 9.21: Missing reference to "(#client_impersonating)".

-- 
Karsten Meyer zu Selhausen
IT Security Consultant
Phone:	+49 (0)234 / 54456499
Web:	https://hackmanit.de | IT Security Consulting, Penetration Testing, Security Training

Unsere nächste Live Online-Schulung zur Sicherheit von OAuth und OpenID Connect am 24.09 + 25.09:
https://hackmanit.de/de/schulungen/109-live-online-schulung-single-sign-on-sicherheit-oauth-openid-connect-am-24-und-25-09-2020

Hackmanit GmbH
Universitätsstraße 60 (Exzenterhaus)
44789 Bochum

Registergericht: Amtsgericht Bochum, HRB 14896
Geschäftsführer: Prof. Dr. Jörg Schwenk, Prof. Dr. Juraj Somorovsky, Dr. Christian Mainka, Dr. Marcus Niemietz