As promised at the last meeting, I have been able to do a full review of the
OAuth for Browser Based Applications draft spec, and my notes are attached,
indexed by sections and paragraphs where possible.
Even though my notes are extensive, I do want to say that overall the document
is in great shape and I think it offers great guidance and necessary support to
application developers. Many of my comments focus around clarifying intent,
consistency in language, and addressing specific concerns like when the
document might be too opinionated (or not opinionated enough).
Thank you to the authors for this momentous work!
— Justin
OAuth for browser-based apps
Title and throughout: should we use "applications" instead of "apps"? The
document is not consistent.
§1:
¶1 should be specific here and elsewhere:
"implementing OAuth 2.0" -> "implementing OAuth 2.0 clients"
¶2 perhaps "is known as" or "is referred to" instead of nicknamed?
BCP for native apps should be the reference, not the RFC
"implement OAuth clients"
¶3 perhaps "This specification, OAuth 2.0 for Browser-Based Apps, ..."
"implementing OAuth <<clients as>> native apps and browser-based apps"
§2: nit, there's a BCP to reference here instead of 2119 directly
§3: if not expanding "app" to "application" throughout, perhaps call out here,
even though it's fairly obvious it doesn't hurt
¶4 does "this" refer to the phrase "JavaScript applications" or the document as
a whole and its recommendations?
§4:
¶1 the use of "were" is confusing here since OAuth 2.0 is considered one
protocol framework; perhaps rephrase:
"At the time that OAuth 2.0 was initially specified in [RFC6749] and
[RFC6750],"
"authorization code flow" -> "authorization code grant type" ... and throughout
the rest of the document, generally change "flow" to "grant type" when
referring to the official grant names. same applies to "implicit" flow and any
others
"this was one" -> "this limitation(?) was one"
¶3 add a reference for CORS?
exposure of tokens is not the problem for refresh tokens historically, it's
been the lack of client credentials in SPAs and poor storage mechanisms to go
with the refresh token
¶4 second sentence has many asides and parentheticals and is hard to follow,
suggest rewording
§5 does "JavaScript" here also refer to other browser-based execution
languages, or is this advice JavaScript only? The definition in §3 only calls
out "JavaScript application" specifically, not "JavaScript" being used as a
generic in general
¶1 remove commas from "vectors, such as ... files, give an"
¶2 "JS" -> "JavaScript" (and elsewhere), or expand and define on first use
is the bold formatting really necessary to make this point?
¶3 perhaps instead of "the first part" and "the second part", link sections
directly and number them specifically (5.1, 5.2) so as to prevent the reader
from tripping over what constitutes the first or second "part"
¶4 it feels odd to introduce what other sections will do in this section, but
I'm not sure where else to put this pointer, which is likely helpful to the
reader if they're going all the way through the document
§5.1 suggest removal of "range from trivial to sophisticated" as it implies
ranking even though the next sentence says there is no implied ranking; and
adversarial toolkits can sometimes make sophisticated attacks trivial to
execute in practice.
§5.1.1 why does this scenario underestimate the capabilities of an attacker? It
is real and can cause damage. I think what might be intended here is that if a
developer focuses only on thwarting this trivial attack they are
underestimating an attacker's ability, is that the case? In any event perhaps
such value judgement isn't helpful unless it's tied to specific guidance for
the reader.
§5.1.2 the attack in §5.1.1 does not depend on theft of token from the payload
of the token response but rather from storage; is this difference intended? or
is "the payload" meant to refer to the attacker's payload of malicious code?
¶2 I believe the second bullet list is supposed to have a sub-list, but this
was not rendered as such in the version I reviewed
§5.1.3
"Setup" -> "set up"
add reference for "Web Messaging"?
It should be noted that a precondition to step (3) is that it's only possible
if the authz code can be issued silently and in a frame context, or else the
payload would need to manipulate the AS into doing so; this is mentioned in the
closing paragraph but it felt out of place not to put it sooner
§5.2.1 nit, it's not impersonation of the user so much as impersonation of the
legitimate client application
§5.2.2 same nit as above, not the user but the client; any other place this
appears should also be aligned
¶2: suggest "considers the access token to be valid" -> "accepts the valid
access token".
¶5 the "additionally" doesn't follow from the preceding text, suggest pulling
it out or re-ordering the topics here
§5.2.3
¶2 CORS is expanded here but not above, and both times without a reference;
suggest add a reference and expand the first time, and stick to CORS elsewhere
throughout
suggest making it clear that it's the CORS configuration of the RS that
prevents calls from the client and adversary to that RS, and does not prevent
calls elsewhere that have an open policy (or other non-CORS-restricted methods)
§6.1
¶3 what does "Payload" refer to here? The sections above are not referred to as
"Payloads" in the titles
the "BFF is a confidential client", but that requirement is not stated until
all the way down in 6.1.3.1 where it's specifically under "security
considerations". If it's a precondition for the architecture pattern, then
state that.
Perhaps move all the preconditions up front as enumerated requirements?
§6.1.1
diagram nit: suggest using aasvg or similar tooling instead of plain ASCII
diagram nit: this might read better as a vertical time sequence diagram, with
concise labels on the arrows
There seem to be several holes in this process, and I think the text is more
opinionated about implementation of the pattern than it ought to be:
how is "an active session" communicated between the BFF and the javascript? in
other words, why do we expect that the BFF does not immediately detect the
scenario and go from (B) to (D) and instead waits for the javascript to
initiate (C)? While that's also a valid pattern I would like to understand the
reason behind the flow
Is there a need for the front end to handle the redirect in (E) and not have
the BFF process the redirect itself?
what is "relevant information" in step (G) and why does this pattern seem to
presume a cookie format?
why does the application fully reload itself in (H) instead of managing state
internally? It would be trivial for many applications to respond to (G) without
a full application reload, especially since the application would have reloaded
at the start of (E) on the tail end of the redirect anyway.
the request in step (J) need not be a direct proxy to the RS, as it could be
transformed by the BFF in ways beyond the simple credential swap between (J)
and (K)
§6.1.2.1 are these items "recommended" or "RECOMMENDED"? ie, is this
intentionally informative?
formatting/consistency nit, above the steps are listed as "(F)" but here they
are "step F" (no parens, include "step").
formatting/consistency nit: capitalization of Refresh Token is inconsistent
throughout this section (and likely elsewhere in the document); is this
significant?
¶3 what does "recover" mean in this context? continue to call the RS?
§6.1.2.2
¶1 suggest remove "traditional" unless you have an idea of a "nontraditional"
browser cookie :)
add reference for HTTP cookies RFC(s) (either here or earlier in the
description of the architecture)
¶2 "only expose" -> "expose only"
§6.1.2.3 it should be noted that if the frontend catches the redirect, then the
frontend ALSO gets the ID token -- even though it's implied that it should
ignore that, it doesn't need to (and importantly, an attacker in the frontend
will see it)
§6.1.3.1 here we finally see a requirement for being a confidential client,
while the past few sections are built around this assumption (which is pretty
much stated here but not earlier). Additionally, a major benefit is not
exposing access and refresh tokens to the browser environment, which has
nothing to do with the confidential client.
The SHOULD here is confusing -- is authz code w/o PKCE OK? is some other grant
OK? under what circumstances can the SHOULD be relaxed?
§6.1.3.2 I feel like more attention should be paid to the two different types
of cookie storage and their resulting requirements.
Why is AEAD the only encryption method called out here? What's particularly
special about this algorithm?
There are no requirements for key management. It's obvious to a seasoned
developer or security engineer that the BFF holds the decryption keys and
doesn't give them out but that's never talked about. Are there any other
considerations? Like, is it OK for the BFF to use the same encryption keys with
each client? Does the key need to be identified?
§6.1.3.3.1
¶3 remove the last sentence, it's unnecessary commentary and value judgement
("Technically, this attack...")
§6.1.3.3.2
¶5 I got lost as to which components are protected by the CORS methods. If the
system is using a BFF then the RS should never be called by the browser and
CORS doesn't matter there.
¶6 it is considered bad practice to create `X-` header field names, and
probably even worse practice to recommend making up an unregistered and ignored
header field name just to force CORS. Especially if the BFF is fielding all
requests to the RS in the first place (see previous point), I don't think this
is good and actionable advice and will likely run afoul of the HTTP directorate
review during publication. If this actually does need to be kept, perhaps seek
out advice from HTTP WG?
§6.1.3.4 it's not a matter of power so much as a matter of secure key
management and hopefully better control over the runtime environment of the
application code. Neither of these have to do with power, as I can guarantee
you that I've deployed web servers with less computational power than the
browser on my laptop has today. :)
§6.1.4 see previous notes about "Payload"
§6.1.4.1
¶5 suggest remove "unfortunately" as it's a value judgement
¶6 this aspect also makes the BFF a prime target for attackers. One can argue
that it's a harder target, fine -- but any proxy architecture has privacy and
security implications that are absolutely tradeoffs. I accept that attacks
against non-browser-systems are out of scope for this draft, but I feel like it
would be irresponsible to at least mention this and place it out of scope.
Basically, tell people that the BFF needs to be extra secure and that just
having one doesn't solve security
§6.1.4.2
¶5 suggest "Because of the nature of the BFF" -> "Since refresh and access
tokens are managed by the BFF not exposed to the browser,"
§6.1.4.3
¶1 this isn't a summary, this is a new point being made here for the first
time. Suggest remove "To summarize" from this paragraph.
§6.2.1 same notes on diagrams and step-label formatting as above
same note as above on returning the code through the front end instead of
directly to the backend
same note as above on relevant information (this seems to presume storing state
in the cookie as the solution)
same note as above on reloading the application
question, is the "Editor's note" intended to survive to the full RFC? it seems
useful reference if sufficiently caveated
§6.2.2.1 same comment as before about assuming a confidential client but not
requiring it in the text until much later (§6.2.3.1 here)
same comment as above on "recover"
same comment as above on non-normative "recommended" vs "RECOMMENDED" intent
here
§6.2.2.3 same note as above on "traditional" and cookie reference
§6.2.2.5 the patterns do affect the relationship, though, as it helps determine
whether the token proxy needs to have CORS or not and how that should be
deployed
§6.2.3.1 an additional major benefit is not exposing refresh tokens to the
browser environment
§6.2.3.4 sure it CAN, but is that a recommendation or requirement for proper
use of this pattern? Possibility does not imply action.
§6.2.4.3.1 this "note" is pretty serious and deserves more than a note in terms
of attention
§6.2.4.3.2 DPoP could also be solved by having the backend deliver a keypair
alongside the token to the front end, which would come with its own security
issues. Should this be covered here?
§6.2.4.4 similar note as above on "summary"
suggest remove aphorism "go the extra mile", and the advice is questionable as
the complexities and downsides of a full proxied system are significant and
should not be ignored by implementors
§6.3.1 same comments as above on diagrams and step labeling
in-browser applications are fully capable of doing dynamic client registration
per RFC7591. There are some pretty severe management tradeoffs when doing this,
but it's disingenuous to say "There's no way". A second pattern, which I
haven't seen in the wild and only in a PoC, is to programmatically register an
instance of the SPA and pack credentials in at the time of download. In both of
these cases, though, malicious code still has the ability to get access tokens
as if it were the client. Yet another option is the in-line registration method
used in OIDC Federation.
§6.3.2.1 suggest remove the two "in summary" as they aren't summarizing
anything from before.
suggest splitting the inner bullet list into three items instead of 2, and
suggest stating that the techniques can be safely and effectively used in
combination, as is done in §6.3.2.6
the advice on refresh token expiration seems confusing at first read,
particularly if my lifetime is based on use in the first place. What is this
advice in the last bullet trying to prevent?
§6.3.2.2
¶1 again this restriction to public clients predates several specs that remove
it and the advice shouldn't be parroted here. better to say that they usually
are going to be public because of the problem of registering a client ID and
storing a secret of any kind at configuration time associated with that client
ID.
¶2 the "or more" seems like it is asking for trouble with this kind of client.
§6.3.2.3 again "cannot contain" is strictly too strong a statement. Also, this
repeats text in the previous section without adding.
§6.2.3.5
¶1 the last sentence currently reads as though it's saying "to avoid page
refreshes or to avoid run silent frame-based flows" which I don't think is the
intent, suggest adding the infinitive with "to run frame-based flows silently"
or something similar.
§6.2.3.2 details of what, exactly, should the reader be looking for there?
§6.3.2.7 I would expect a reader at this point of the document to not need an
introduction to what a refresh token is and what it does. Maybe skip the intro
sentence or two?
aren't these the same requirements as above in §6.3.2.1? Does it need to be
listed as requirements in both places? Are they identical?
§6.3.2.8
¶2 what is a "proper CORS configuration" as per this spec? I don't think we
have quite enough details to require that as such.
§6.3.3.2.1 and §6.3.3.2.2 add references for local storage, web worker,
indexdb, etc
§6.3.3.2.2 the note on dpop should also note that since the attacker can
control which key the new token is bound to, the attacker can count on being
able to exfiltrate the token to their own system that also has a copy of the
key and use it outside of the original application context
§7 ¶1 who is "our", the authors, the readers, the oauth community, someone else?
§7.2
the whole of ¶2 seems out of place as first we're discussing the implicit flow
but then give requirements, in that context, that contradict what the flow
requires one to do. At first it seems like we're reading requirements FOR the
implicit flow, not to prevent its use. Perhaps this can be introduced to make
the requirements less jarring, such as "The security properties of the implicit
flow make it no longer a recommended best practice. To effectively prevent the
implicit flow, the as MUST NOT ..."
§7.2.1
¶1 as I recall, the fragment was used primarily to prevent the access token
from leaking to whatever static backend was serving the SPA. If it's in query
parameters it lands in the back end. If it's in the fragment, it is not sent.
this is a problem that remains with the code flow and browser-based apps, but I
don't see that mentioned in this document (especially since this document
recommends serving the redirect URI from the front end in all architectures).
§7.2.3.x suggest removing "Threat:" from headings (it's not used elsewhere in
the doc)
§7.2.4
¶1 remove "standard" before "authorization code" since implicit is also just as
standard per OAuth 2.0
§7.4.1.1
suggest change "Just note the that the service worker" -> "The service
worker..." and combine last two sentences into one.
§8
¶1 previously the token-mediated pattern is said to not require storage of
tokens b/c the frontend can just go get a new token from the mediator, this
seems to contradict with what's stated here
§8.2
¶3 is it true that the service worker obtains tokens FROM the application, or
should this be FOR the application? or is it the application obtains them FROM
the service worker, instead? I might be missing the intent of this advice
§8.4 this section seems explicitly JavaScript specific, is that the case?
§8.5 provide references for each storage type's specification
§9 security considerations sections should be about "why" and really shouldn't
have normative requirements, please remove them from the subsequent sections
§9.1 recommend removing "straightforward"
§9.2
last sentence of ¶2 repeats the first sentence of ¶3, suggest removing one or
combining both
§9+ this draft should add privacy considerations, particularly for BFF
pattern's proxy architecture.e
§A it feels wrong to have normative requirements in an appendix (point 7), and
points 2 and 3 both have a weird uppercase NOT which isn't a keyword
_______________________________________________
OAuth mailing list
OAuth@ietf.org
https://www.ietf.org/mailman/listinfo/oauth
