Thank you for the thorough review! My responses and feedback is below. I've gone ahead and published a new draft based on this feedback so that the edits can be read in context. It is now available here:
https://tools.ietf.org/html/draft-ietf-oauth-browser-based-apps-05 I've made all "EDITORIAL" changes as suggested with the exception of the below: > Authorization Server Mix-Up Mitigation – Please also reference > draft-ietf-oauth-mix-up-mitigation here. It appears that draft expired in January 2017, so it feels strange to reference an outdated draft. If that document is picked up again and shows signs that it is on track to publication I'd be happy to reference it. I'm going to avoid getting into the "ADDITIONAL OVERALL ISSUES" in this thread to keep it manageable. I will follow up with those specific points in a new thread later. As for the "SUBSTANTIVE" list, I've made most of the changes as suggested, with a few exceptions. These are noted below. > 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. Changed "running entirely in a browser" to "executing in a browser". > 2. Overview – Change “This was the principal motivation” to “This was one of > the motivations”. Done > 3. Terminology – In the “OAuth” definition, also reference RFC 6750. Done > 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. This feels too broad. It ends up sounding like it's okay to use the implicit flow to get an access token if you're also "using OpenID Connect". I would rather reference a specific grant type / response mode in OpenID Connect similar to how this sentence references specifically authorization code with PKCE. Any suggestions? > 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”. Done > 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. The intent is to prevent dynamic redirect URIs such as varying the query string component. I will update this to: "Register one or more redirect URIs, and use only exact registered redirect URIs in authorization requests" > 4. Overview – Change “Support the PKCE extension” to “Support the PKCE > extension or OpenID Connect”. The Security BCP already requires that authorization servers MUST support PKCE. I don't think it makes sense for this BCP to allow supporting OpenID Connect *instead of* PKCE, which is what the text above would do. > 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. The intent here is to require a redirect-based flow rather than for example the password grant. I will update this to: "To conform to this best practice, first-party applications using OAuth or OpenID Connect MUST use a redirect-based flow (such as the OAuth Authorization Code flow) as described later in this document." > 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. I've updated this text to: "The Application Server stores this access token itself and establishes its own cookie-based session with the Browser application. The Application Server can store the access token either server-side, or in the cookie itself." > 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. I thought I remembered seeing an attack described where the user can extract an access token from an application and use it for things the developers didn't intend. I can't find a reference to that right now though. Anyone have an idea of where that might be? If we can't come up with something to point to about that, I'm inclined to drop this sentence. > 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.) This phrasing could use some work, but the point still stands. An application deployed this way has no mechanism to securely authenticate with the authorization server, which is the RFC6749 definition of a confidential client. Dynamic client registration doesn't solve this, because the registration endpoint would have to be publicly available and then anyone could register a client so there's no way to tell whether this particular javascript application is the "real" one. I don't see any way that an application in this architecture could be treated as a confidential client. I will rephrase this to the below: "In this architecture, the JavaScript code is first loaded from a static web host into the browser (A), and the application then runs in the browser. This application is considered a public client, since there is no way to issue it a client secret and there is no other secure client authentication mechanism available in the browser." > 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. Again, saying "use OpenID Connect" is not specific enough. One of the things this BCP is trying to capture is the advice of only issuing access tokens at the token endpoint with an authorization code. Since OpenID Connect defines a few additional ways of issuing access tokens, it would need to get more specific about what part of OpenID Connect is okay here. Also, in the case that an application is not using OpenID Connect at all, only using OAuth, it needs to be clear still that PKCE is required, and that simply adding some unspecified part of OpenID Connect doesn't magically solve the problems with the implicit flow. Can you provide more specific guidance on when it's okay to not use PKCE? For example using certain response type or response mode combinations, and what parts of ID token validation must be followed in order to safely ignore PKCE? > 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. I've changed this paragraph to the below to better match the Security BCP, and to match what was discussed during IETF 106: "Browser-based apps MUST prevent CSRF attacks against their redirect URI. This can be accomplished by any of the below: * using PKCE, and confirming that the authorization server supports PKCE * if the application is using OpenID Connect, by using the OpenID Connect "nonce" parameter * using a unique value for the OAuth 2.0 "state" parameter" This section then references the Security BCP. > 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. Do we know of anyone doing this in the wild with SPAs right now? This feels like it has limited use at best, since dynamic registration doesn't actually authenticate the client for public clients. You could use the client secret instead of PKCE to prevent stolen authorization codes, but since PKCE also solves that problem, it doesn't seem very useful to recommend this. > 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. I am not familiar with how the id_token_hint provides assurance of the client identity. If the id_token was obtained via any front-channel mechanism, then it is at risk of being stolen, and then it can't be considered client authentication. Additionally, merely using the id_token in a GET request means it's susceptible to all the same problems we are avoiding with exposing access tokens in URLs, so again it doesn't seem useful as client authentication. > 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.) Correct, this was supposed to say that wildcard values for any of those three components are not allowed. I will rephrase it to: "...a fixed set of absolute HTTPS URIs, preventing the use of wildcard domains, wildcard paths, or wildcard query string components..." > 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. I've replaced this section with the below: "Clients MUST prevent Cross-Site Request Forgery (CSRF) attacks against their redirect URI. Clients can accomplish this by either ensuring the authorization server supports PKCE and relying on the CSRF protection that PKCE provides, or using the "state" parameter to carry one-time-use CSRF tokens as described in {{auth_code_request}}, or if the client is also an OpenID Connect client, using the OpenID Connect "nonce" parameter. See Section 2.1 of [oauth-security-topics] for additional details." > 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. The following sections with headers beginning with "Threat" describe the attacks, I've updated the text to hopefully clarify that. I've also added a new header structure here which should help make that relationship between the paragraphs more apparent. > 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. I've updated this section along with the change above. > 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. This does not match what the Security BCP says: "Authorization servers MUST support PKCE" ---- Aaron Parecki aaronparecki.com @aaronpk On Fri, Feb 21, 2020 at 4:48 PM Mike Jones <[email protected]> wrote: > > I did a cover-to-cover read of 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. > > 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. > 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 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. > 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. > 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 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
