Thanks for your review Orie. Responses inline. You can also see the diff with the changes here:
https://github.com/oauth-wg/oauth-browser-based-apps/pull/109/files > ---------------------------------------------------------------------- > COMMENT: > ---------------------------------------------------------------------- > # Orie Steele, ART AD, comments for draft-ietf-oauth-browser-based-apps-24 > CC @OR13 > ## Comments > Thanks to Marc Blanchet for the ART review. > ### 1. Introduction > The ordering of the introduction feels a bit awkward, you might consider > starting with: > ``` > This document focuses on JavaScript frontend applications acting as the OAuth > client,... ``` > Then relate to "OAuth 2.0 for Native Apps" and then end with the comment about > OpenID Connect: > ``` > Such a scenario, (which only uses OAuth 2.0 as the underlying specification of > OpenID Connect), is not within scope of this specification. ``` Agreed, I've rearranged this accordingly. > I was not familiar with the term "first-party frontend" before. > Having read the full document, I might try to more clearly distinguish this > case from BFF up front. I changed this to "common domain" and added a reference to section 7.1 where this is discussed further. > ### Revoking of refresh tokens > ``` > 530 The attack is only stopped when the authorization server refuses a > 531 refresh token because it has expired or rotated, or when the > refresh > 532 token is revoked. In a typical browser-based OAuth client, it is > not > 533 uncommon for a refresh token to remain valid for multiple hours, or > 534 even days. > ``` > Given the comments about refresh token reuse, is it common to automatically > revoke a refresh token where reuse is detected or attempted? Yes, described in Section 4.14.2 of RFC 9700. > ### Reference for session fixation > ``` > 576 sending requests to backend systems. Alternatively, the attacker > can > 577 also abuse their access to the application to launch additional > 578 attacks, such as tricking the client into acting on behalf of the > 579 attacker using an attack such as session fixation. > ``` > Consider a reference for session fixation, possibly OWASP. Thanks, added. > ### confidential client > Consider a reference or definition for "confidential client". Added. > ``` > 861 The main benefit of using a BFF is the BFF's ability to act as a > 862 confidential client. Therefore, the BFF MUST act as a confidential > 863 client. Furthermore, the BFF MUST use the OAuth 2.0 Authorization > 864 Code grant as described in Section 2.1.1 of [RFC9700] to initiate a > 865 request for an access token. > ``` > Making it easier to implement this MUST, I'm not sure what is required to act > as a confidential client. I added "by establishing credentials with the authorization server" to the end of that sentence. > ### Cookie security, why NOT MUST > ``` > 867 6.1.3.2. Cookie Security > ``` > This section contains several SHOULDs and SHOULD NOTs. > While it is nice that the guidance is so compact, it raises the question of > why > these are not MUSTs, and under which conditions they ought to be required. > Consider providing a reference if the answer lies elsewhere, or a brief > comment > after the compact list. The paragraph below the list goes in to some of this detail. I tried to make this more explicit by adding a sentence in between. > ## Nits > ### refresh tokens and correlation of requests > ``` > 367 reduce the scope and lifetime of the token. For refresh tokens, > the > 368 use of refresh token rotation offers a detection and correction > 369 mechanism. Sender-constrained tokens (Section 9.2) offer an > ``` > A few extra words here, or a pointer might make it clearer what recommendation > is being made wrt refresh tokens. Refresh token rotation is defined in RFC9700, I've added a reference to that. > Are you recommending a specific refresh token rotation policy here ? No. > Later you have: > ``` > 410 is not sufficient to prevent abuse of a refresh token. An attacker > 411 can easily wait until the user closes the application or their > 412 browser goes offline before using the latest refresh token, thereby > 413 ensuring that the latest refresh token is not reused. > ``` > Consider forshadowing this comment. This paragraph has been rephrased based on some of the other feedback. > ### Confused Deputy > ``` > 921 Site Request Forgery (CSRF) attacks. A successful CSRF attack > could > 922 transform the BFF into a confused deputy, allowing the attacker's > 923 request to the BFF to trigger outgoing calls to a protected > resource > 924 on behalf of the user. > ``` > Consider providing a reference for this term. I simplified the sentence by removing the reference entirely: "A successful CSRF attack could allow the attacker's request to the BFF to trigger outgoing calls to a protected resource." > ### Extracting CryptoKeyPair > ``` > 1851 extractable [CryptoKeyPair] is stored using [W3C.IndexedDB]). As a > 1852 result, the use of DPoP effectively prevents scenarios where the > 1853 attacker exfiltrates the application's tokens (See Section 5.1.1 > and > 1854 Section 5.1.2). > ``` > IIRC, the non extractable crypto key pair could potentially still be extracted > by other processes running on the same machine. The context makes it clear you > are referring to attackers with the ability to execute javascript in the > browser, but there are other kinds of attackers. > I see you comment on this later in Section 8.6 & 9.2, consider if you want to > make the text here a little more narrowly focused on attackers with XSS > capabilities. The "attack scenarios" and "additional defenses" headers preceding this text already scope the discussion to XSS attackers. Just for good measure I updated this to "where the XSS attacker" as a reminder. --- Aaron Parecki _______________________________________________ OAuth mailing list -- oauth@ietf.org To unsubscribe send an email to oauth-le...@ietf.org
