I did a cover-to-cover read of draft-ietf-oauth-browser-based-apps-04<https://tools.ietf.org/html/draft-ietf-oauth-browser-based-apps-04>. My review comments follow in three sections: substantive comments on the text, substantive overall issues to consider (provided by Microsoft product engineers), and editorial issues.
SUBSTANTIVE 1. Introduction - The first sentence says that this spec is about applications running entirely in a browser. Many browser-based applications also use helper services. Please broaden the scope to encompass both patterns. 2. Overview - Change "This was the principal motivation" to "This was one of the motivations". 3. Terminology - In the "OAuth" definition, also reference RFC 6750. 4. Overview - Change "the current best practice for browser-based applications is to use OAuth 2.0 authorization code flow with PKCE" to "the current best practice for browser-based applications is to use either OAuth 2.0 authorization code flow with PKCE or OpenID Connect <xref target="OpenID.Core"/>". This parallels the same recommendation in the forthcoming OAuth Security BCP specification. 4. Overview - Change "Use the OAuth 2.0 authorization code flow with the PKCE extension" to "Use either the OAuth 2.0 authorization code flow with the PKCE extension or OpenID Connect". 4. Overview - Change "Protect themselves against CSRF attacks by using the OAuth 2.0 state parameter to carry one-time use CSRF tokens, or by ensuring the authorization server supports PKCE" to "Protect themselves against CSRF attacks by using the OAuth 2.0 state parameter or the OpenID Connect nonce parameter to carry one-time use CSRF tokens or by ensuring the authorization server supports PKCE". 4. Overview - The intended meaning of this text isn't clear: "Register one or more redirect URIs, and not vary the redirect URI per authorization request". Do you mean to include a registered redirect_uri value in each authorization request? This needs to be clarified. 4. Overview - Change "Support the PKCE extension" to "Support the PKCE extension or OpenID Connect". 5. First-Party Applications - The third paragraph says that "first-party applications using OAuth or OpenID Connect MUST use the authorization code flow". OpenID Connect can be safely used with other response_type values than simply "code". For instance, the "code id_token" value is just as safe. Please generalize this language when describing the use of OpenID Connect to recognize that other response_type value may continue to be safely used.. 6.2. JavaScript Applications with a Backend - The sentence "The Application Server utilizes own session with the browser to store the access token" raises a question with no obvious answer: Where is the specification recommending or suggesting that the access token be stored in this case? Please clarify. 6.2. JavaScript Applications with a Backend - The last paragraph talks about exposing the access token to the end user but fails to describe the perceived risk or threat. If there are such risks or threats, please be explicit about what they are and their mitigations. 6.3. JavaScript Applications without a Backend - This sentence tells a false and incomplete story "The application then runs in the browser, and is considered a public client since it has no ability to be issued a client secret." Public clients can definitely be issued Client ID/Client Secret pairs using OAuth Dynamic Client Registration [RFC 7591]. Please revise this description accordingly. (Likewise, the following paragraph says that "The JavaScript app is then responsible for storing the access token securely using appropriate browser APIs. If this is safe, then storing the dynamically issued Client ID/Client Secret pair should be safe to do so in the same manner.) 7.1. Initiating the Authorization Request from a Browser-Based Application - As in other places where the text says that PKCE must be used, please revise this instance to say that either PKCE or OpenID Connect must be used - just as is done in the forthcoming Security BCP. 7.1. Initiating the Authorization Request from a Browser-Based Application - As in other places where the spec says that the "state" parameter must be used, revise this instance to say that either the OAuth "state" parameter or the OpenID Connect "nonce" parameter must be used to prevent the applicable attacks. 9.2. Client Authentication - The second paragraph talks about secrets that are statically included and warns against them, which is good. But please also add text describing how OAuth Dynamic Client Registration [RFC 7591] can be used to safely statically create these secrets. 9.3. Client Impersonation - Add text to the first paragraph describing how the use of the OpenID Connect "id_token_hint" parameter can be used to satisfy the requirement that the identity of the client can be assured. 9.3. Client Impersonation - The wording of this phrase is ambiguous: "If authorization servers restrict redirect URIs to a fixed set of absolute HTTPS URIs without wildcard domains, paths, or query string components..." It could be interpreted with the "without" applying to "paths or query string components", meaning that no path or query string components may be present at all. (I think you meant only that wildcard values not be present, but that's not the meaning of the phrase, as written.) 9.4. Cross-Site Request Forgery Protections - As in other places where the spec says that the "state" parameter must be used, revise this instance to say that either the OAuth "state" parameter or the OpenID Connect "nonce" parameter must be used to prevent the applicable attacks. 9.8. OAuth Implicit Grant Authorization Flow - The paragraph ends with "not all of which have sufficient mitigation strategies". It would be useful to be more specific here, saying which attacks do not have sufficient mitigation strategies. 9.8.5. Countermeasures - The phrase "using the authorization code with PKCE avoids these attacks" doesn't say what attacks are being referred to. Please revise so that the attacks in question are unambiguously referenced. Appendix A. Server Support Checklist - In bullet 3, revise to say that either PKCE or OpenID Connect is required, as per other locations in the specification. ADDITIONAL OVERALL ISSUES FROM MICROSOFT ENGINEERS These issues should be considered in the context of this document. For these, I'm requesting working group discussions - not specific resolutions at this time. 1. There seems to be a lot of focus on exact redirect URI validation for addressing the biggest risk (open redirect), but while certainly absolutely critical, it frankly ignores the reality of how apps integrate with authorization servers these days a bit. The pattern we are seeing is that a lot of applications have simply shifted to including their own redirect URIs in 'state' parameters. Even our own services use this pattern as cookie space is quite limited. We are seeing a lot of security issues and there are no real solutions in any of the BCP documents or RFCs. 2. In Section 4, the spec requires exact matching of redirect URIs. The OAuth Security BCP mentions that there are alternatives to exact redirect URI matching "As an alternative to exact redirect URI matching, the AS could also authenticate clients, e.g., using [I-D.ietf-oauth-jwsreq]." While I don't think client authentication is suitable for browser based apps, accepting signed requests via OpenID Connect's "request_uri" parameter<https://openid.net/specs/openid-connect-core-1_0.html#JWTRequests> would be a suitable alternative pattern to exact redirect URI checking. For consistency, I think both docs should have the same recommendation for redirect URI matching.. 3. Considering the above, we probably don't have many more good options for fragment flows. That's one of the reasons why I still think hybrid patterns have a place, but we need mechanisms to protect against code injection.. Unfortunately, the security-topics document dismisses solutions that we could make work (code-bound state, nonce) in favor of solutions that don't help (PKCE). This might be outside of this immediate discussion about the implicit flow, but standards need to evolve to better mitigate that risk. 4. The big problem with giving SPAs refresh tokens is that refresh tokens provide offline access - they live on after the user signs out - including when an attacker steals them. For JavaScript SPAs, where XSS is a big concern, and where access is only intended to be provided for an online user, these refresh tokens represent an unnecessary security liability. Disabling CORS on /token forces an implementation pattern without refresh tokens for web apps. So today, we have a tradeoff between using the implicit flow and accepting attacks around browser history and generically injected scripts with limited impact duration OR using the two legged flow and accept attacks with specifically injected scripts with unlimited duration, plus a little perf/cogs hit. EDITORIAL 1. Introduction - Change "native apps as well as browser-based apps" to "native apps and browser-based apps". 4. Overview - Change "OAuth 2.0 RFC 6749" to "OAuth 2.0 <xref target="RFC6749"/> <xref target="RFC6750"/>". 4. Overview - Change "the flow assures that" to "the flow ensures that". 5. First-Party Applications - The first sentence of the second paragraph has no verb. 5. First-Party Applications - The fifth paragraph starts with "By using the Authorization Code flow and redirecting the user to the authorization server". I believe that this is continuing the message in the (single-sentence) fourth paragraph. The exposition will be less confusing if these paragraphs are merged. 6. Application Architecture Patterns - The bulleted list is not in the same order as the corresponding subsections. Please reorder them to make them parallel. 6.2. JavaScript Applications with a Backend - This section references the OWASP Foundation as an organization. It would be far more useful to instead list the documents containing the specific applicable recommendations. Please revise accordingly. 9.5. Authorization Server Mix-Up Mitigation - Please also reference draft-ietf-oauth-mix-up-mitigation<https://tools.ietf.org/html/draft-ietf-oauth-mix-up-mitigation-01> here. 9.8.3. Threat: Manipulation of Scripts - Please change "is far greater" to "may be amplified" to avoid an overreaching statement. Likewise, delete the word "very" later in the paragraph. 9.9. Additional Security Considerations - This section references the OWASP Foundation as an organization. It would be far more useful to instead list the documents containing the specific applicable recommendations. Please revise accordingly. 11.1. Normative References - These references are missing URLs: CSP2, Fetch, oauth-security-topics. 11.1. Informative References - This reference is missing a URL: HTML. -- Mike
_______________________________________________ OAuth mailing list [email protected] https://www.ietf.org/mailman/listinfo/oauth
