-
Notifications
You must be signed in to change notification settings - Fork 44
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
Restriction to only respond to authorized with Allow et al #353
Conversation
I would guess that the spec is written this way due to security concerns; Should an unauthorised user/consumer even be able to tell what methods may be allowable for a resource if they are not authenticated? Could knowing allowable methods reveal private or confidential information about what resources may exist? e.g., if something doesn't exist or isn't public, I'd argue it should just be responded to with a 404 or 401 (respectively), and no other information should be revealed. |
Actually, I had exactly the same reaction the first time I implemented a system with an In the case where the resource does not exist, or you do not wish to reveal its existence, the server will respond with a I think the intention of HTTP in this area is that if you have no intention of supporting for example I believe this sentence brings us out of compliance with HTTP, and while this is likely a corner case, actual enforcement of authorization should happen with an authorization mechanism, which we have, and |
Could the |
I completely agree with @ThisIsMissEm in that we don't want this to be an avenue where we leak data. From RFC 7231:
If @kjetilk's point is about returning this header on 405 responses, then I agree with that direction. I would not expect the |
Yes, this is certainly not on other 4xx errors. However, it is also of note that there will always be only a very small number of verbs, and they will all have normative statements about them in the spec. We most certainly don't want to attach any security related to being obscure about it, as an attacker can just guess, try out different methods. If we have a vulnerability, they will find it, |
But to the substance of this change: by removing this clause, you are effectively saying that 405 responses take precedence over 401/403 responses. This is dangerous because it gives out information that an agent would otherwise not be able to access. This is what @ThisIsMissEm is also arguing, if I am not mistaken. Consider the following: an unauthorized agent would like to know if a certain non-container is RDF (includes Accept-PATCH). If that agent receives a 405 response instead of a 401/403 response, then information will have been leaked about the resource. I would strongly advocate for leaving the text as in its current form (i.e., close this PR), because, I would argue, authorization should occur before disclosing any information about a particular resource. |
To be accurate, this change leaves the interpretation to be HTTP compliant, which indeed is what does.
Alright, so your concern isn't with If there are no acceptable representations, the corresponding error is Therefore, one resolution to this problem is to just move the requirement to authorize, so that it does not pertain to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the Allow header need not be coupled with authorization. As I see it, the Protocol specifies the HTTP methods that can be used when making requests to resources (containers, non-containers..). Nevertheless, a server can always be configured to return 405 for any resource, e.g., URI space can't be used to allocate resources due to a (e.g., persistence) policy. The URI owner instructs the server to not accept certain requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This review is not to be mixed with the approval of the PR in review: #353 (review)
This PR should instead modify ED/protocol.html
.
The suggested change is to have OPTIONS also indicate support for methods in Allow.
@@ -681,11 +681,9 @@ <h3 property="schema:name">Reading Resources</h3> | |||
<div datatype="rdf:HTML" property="schema:description"> | |||
<p><span about="" id="server-safe-methods" rel="spec:requirement" resource="#server-safe-methods"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> support the HTTP <code>GET</code>, <code>HEAD</code> and <code>OPTIONS</code> methods [<cite><a class="bibref" href="#bib-rfc7231">RFC7231</a></cite>] for clients to read resources or to determine communication options.</span></span> [<a href="https://github.com/solid/specification/issues/39#issuecomment-538017667" rel="cito:citesAsSourceDocument">Source</a>]</p> | |||
|
|||
<p>When responding to authorized requests:</p> | |||
|
|||
<p><span about="" id="server-allow-methods" rel="spec:requirement" resource="#server-allow-methods"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> indicate their support for HTTP Methods by responding to HTTP <code>GET</code> and <code>HEAD</code> requests for the target resource with the HTTP Method tokens in the HTTP response header <code>Allow</code>.</span></span></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't require OPTIONS for specific cases such as Allow or Accept-* headers ( if #369 is accepted which also depends on this PR), then there is nothing besides the CORS preflight request that needs OPTIONS ( #server-cors-options ). In any case, one reason may be sufficient to make the following change:
<p><span about="" id="server-allow-methods" rel="spec:requirement" resource="#server-allow-methods"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> indicate their support for HTTP Methods by responding to HTTP <code>GET</code> and <code>HEAD</code> requests for the target resource with the HTTP Method tokens in the HTTP response header <code>Allow</code>.</span></span></p> | |
<p><span about="" id="server-allow-methods" rel="spec:requirement" resource="#server-allow-methods"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> indicate their support for HTTP Methods by responding to HTTP <code>GET</code>, <code>HEAD</code> and <code>OPTIONS</code> requests for the target resource with the HTTP Method tokens in the HTTP response header <code>Allow</code>.</span></span></p> |
Put differently, it doesn't seem sensible to require OPTIONS but not have it include the Allow header in the response.
Two scenarios with public access:
|
Yes, it does, that is my interpretation of HTTP.
In that case, it will see |
To further help clarify the purpose of 405 and where the Allow header is typically used, note the following alongside 501:
501 and 405 are concerned about whether server recognises the method supported on target resource. Think of it as whether the request method is an itself is legitimate/useful (as opposed to a bad request) and potentially allowed. Not whether it is actually approved against the request target in any way:
Here is another example from the Solid Protocol:
This is not about authorization. It is a requirement that the protocol sets to protect server's root and the default access controls. No amount of access privilege will grant anyone - including the storage owner! - to have the server to process the DELETE request. (Yes, an agent can wipe off the Authorization rules while not deleting the ACL resource but that's beside the point here.) Another example: if a server were to be configured to not support PUT requests targeting a container to "replace" the container - because say it finds it to be a minefield to preserve containment statements - it can respond with 405 including the Allow header and listing the supported methods. With some special cases aside (for example with POST), typically the server will support GET, HEAD, OPTIONS, PUT, POST, PATCH, DELETE targeting containers, resources (non-containers), and auxiliary resources. |
I don't understand the additional restriction in Section 5.2 that
Allow
headers and variousAccept-*
headers should only be in authorized requests. This PR removes it, but that's mainly because it was as easy to remove it as to submit only an issue.I don't find anything in HTTP that says that this is something that should be in only authorized requests. Also, you could imagine a server that allows for example append for totally unauthorized clients.
I suppose you could argue that in Solid, every request is an authorized one, since you can always authorize requests with
foaf:Agent
, and in that case, that line is redundant. NSS returnsAllow
on a public resource, BTW.But it also goes into a further interpretation of
Allow
and405 Method not Allowed
. My interpretation is that either you put a method intoAllow
, or you respond to it with405
, i.e., authentication and authorization has nothing to do with it. It is about what methods you could potentially use on a resource if you had the right authorization.I also thought this was the rationale behind
WAC-Allow
i.e. a method that does depend on authorization. Which I agree with, I haven't foundAllow
terribly useful, for that reason.Anyway, low priority, I just wanted to register the concern while it was in my head.