Issues from Justin Richer's review that need to be addressed
Details
§5 is the bold formatting really necessary to make this point?
Comment: Leave it for now, see what happens during review
§6.1 what does "Payload" refer to here? The sections above are not referred to as "Payloads" in the titles
[x] Update refs "Payload Section XXX" to the name of the actual attack scenario with a simple reference to the section (Without any text content)
[x] Refer to them as "attack scenarios" instead of payloads throughout the document
[x] Also look for "attack scenarios" and make them consistent
diagram nit: suggest using aasvg or similar tooling instead of plain ASCII
[x] Create an issue for Aaron
§6.1 diagram nit: this might read better as a vertical time sequence diagram, with concise labels on the arrows
Comment: The diagrams show the deployment quite well, so we decided to keep them as is.
§6.1.2.1 are these items "recommended" or "RECOMMENDED"? ie, is this intentionally informative?
[x] Refer to 4.14 from the security bcp
[x] Rephrase to avoid giving a recommendation on refresh tokens
[x] Apply the same changes to 6.2.2.1.
§6.1.3.2
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?
[x] Avoid the algo language
[x] Update language to make clear cookie is not being prevented from being extracted
[x] Also mention that the stolen cookie can still be used
§6.1.3.3.2 ¶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. 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?
[x] Add language that states the BFF has to check the static header
[x] Rename the custom header to My-Static-Header and make very clear this is just an example
§6.2.1 question, is the "Editor's note" intended to survive to the full RFC? it seems useful reference if sufficiently caveated
[x] Keep it in, but rephrase this as "an early draft documenting this concept"
§6.2.4.3.1 this "note" is pretty serious and deserves more than a note in terms of attention
I have reworded this slightly, but this is quite inherent to the nature of the pattern. Not sure how to put more emphasis on it and whether it is needed.
[x] Add more language to make explicit that the malicious JS code can get it from the endpoint. This should be the same language as for a browser-basaed app with a new flow
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?
Do you think this sentence needs rewording: "when issuing a rotated refresh token, MUST NOT extend the lifetime of the new refresh token beyond the lifetime of the original refresh token if the refresh token has a preestablished expiration time"?
[x] There should be an implementation details (6.3.2)
[x] First sentence of 6.3.2.1 moves up as the intro. The rest of 6.3.2.1 is removed (but not the subsections)
[x] CSRF and RT go to implementation details
[x] Security considerations become 6.3.2.3, 6.3.2.4, 6.3.2.5, CORS
[x] Move the sentence with "limiting the overall..." above the example in the text
§6.3.2.2
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.
[x] Drop 6.3.2.2
§6.3.2.7
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?
Comment: Addressed by the restructuring above
§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.
[x] Replace "proper" with "necessary"
[x] Replace "allows" with "enable"
§9 security considerations sections should be about "why" and really shouldn't have normative requirements, please remove them from the subsequent sections
[x] Everything is fine, except for mixup attacks. Only keep first and last sentence with the ref to the BCP and we are good
§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
[x] Drop appendix A
§9+ this draft should add privacy considerations, particularly for BFF pattern's proxy architecture.e
I think it does warrant mentioning, because the main assumptions about an spa are that everything goes from the browser to the api itself. It might be surprising to a user or even a naive developer that every request goes through another party as a black box. Even if it's all first party abd deployed together, that model should be called out by the draft as an assumption for privacy. After all, this section is for considerations - things you should think about that might not be obvious.
[x] Add a new item under security considerations of the BFF to discuss prviacy considerations. Mainly relevant for off the shelf BFF libraries / components (e.g. offered by Cloudlfare) that may now see all customer data. A third party could observe all the data
philippederyck
commented
7 months ago
Issues from Justin Richer's review that need to be addressed
Details
Comment: Leave it for now, see what happens during review
Comment: The diagrams show the deployment quite well, so we decided to keep them as is.
My-Static-Headerand make very clear this is just an exampleI have reworded this slightly, but this is quite inherent to the nature of the pattern. Not sure how to put more emphasis on it and whether it is needed.
Do you think this sentence needs rewording: "when issuing a rotated refresh token, MUST NOT extend the lifetime of the new refresh token beyond the lifetime of the original refresh token if the refresh token has a preestablished expiration time"?
Comment: Addressed by the restructuring above