Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache-Control should also include 'public' for public document GET requests #189

Open
DougReeder opened this issue Mar 7, 2024 · 4 comments

Comments

@DougReeder
Copy link
Contributor

without private (in addition to no-cache in the Cache-Control header for GET requests, server-side caches are allowed to cache responses, creating a large security hole. That is, a server-side cache would happily serve RS content to anyone on the internet, without the authentication of the RS server.

The HTTP spec says HEAD requests MUST send the same headers as GET requests, the spec should probably include HEAD requests in that sentence.

@raucao
Copy link
Member

raucao commented Mar 9, 2024

AFAIK, shared caches generally do not happily serve content that was requested with an Authorization header. If you use Cache-Control: public instead of no-cache, then that would intentionally override said restriction:

A cache MUST NOT store a response to a request unless:

  • if the cache is shared: the Authorization header field is not present in the request [...] or a response directive is present that explicitly allows shared caching

https://www.rfc-editor.org/rfc/rfc9111#name-storing-responses-in-caches

I agree that adding private would make it even more explicit, but in that case we should probably also specify that private should not be added to responses for /public/ URLs, because caching popular public documents can meaningfully decrease server load for providers, and guard against DDoS attacks as well. But since shared caches are not allowed to cache authorized responses in the first place, I'm not sure the extra complexity is worth it.

@DougReeder Did you find a specific cache implementation that has this problem?

@DougReeder
Copy link
Contributor Author

Good catch on the Authorization header preventing caching - that's probably why we haven't observed a problem in practice.

draft 18 states "If is null, the client will not have a way to obtain
an access token, and SHOULD send all requests without Authorization
header, and rely on Kerberos [KERBEROS] instead for requests that
would normally be sent with a bearer token" so perhaps all we need to add to the spec is to note that if a non-public response doesn't have an Authorization header, it should also specify Cache-Control: private.

@raucao
Copy link
Member

raucao commented Mar 9, 2024

Forgot about Kerberos. So yes, the addition should go somewhere around here I think:

If <auth-dialog> is null, the client will not have a way to obtain
an access token, and SHOULD send all requests without Authorization
header, and rely on Kerberos [KERBEROS] instead for requests that
would normally be sent with a bearer token, but servers SHOULD NOT
impose any such access barriers for resources that would normally
not require an access token.

@DougReeder DougReeder changed the title Cache-Control should also include 'private' for GET requests Cache-Control should also include 'public' for public document GET requests Mar 18, 2024
@DougReeder
Copy link
Contributor Author

As @raucao noted, request with credentials are not normally cacheable. An unauthenticated request would evoke a 401 status, which is also not cacheable, which closes off a theoretical DOS attack. Adding Cache-Control: private allows a client to DOS itself (I think) but doesn't help in any scenario I currently know of, so I withdraw the recommendation that Cache-Control: private be added.

So, it appears to me that implementations should add Cache-Control: public to public documents (but not public directories), along with the no-cache for all documents and directories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants