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
_______________________________________________ OAuth mailing list OAuth@ietf.org https://www.ietf.org/mailman/listinfo/oauth
