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

Restriction to only respond to authorized with Allow et al #353

Closed
wants to merge 2 commits into from

Conversation

kjetilk
Copy link
Member

@kjetilk kjetilk commented Nov 18, 2021

I don't understand the additional restriction in Section 5.2 that Allow headers and various Accept-* 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 returns Allow on a public resource, BTW.

But it also goes into a further interpretation of Allow and 405 Method not Allowed. My interpretation is that either you put a method into Allow, or you respond to it with 405, 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 found Allow terribly useful, for that reason.

Anyway, low priority, I just wanted to register the concern while it was in my head.

@ThisIsMissEm
Copy link

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.

@kjetilk
Copy link
Member Author

kjetilk commented Nov 20, 2021

Actually, I had exactly the same reaction the first time I implemented a system with an Allow header. It just struck me as really odd to do this. I guess I should explain it more carefully what I have come to understand about it:

In the case where the resource does not exist, or you do not wish to reveal its existence, the server will respond with a 404. That is not what this is about.

I think the intention of HTTP in this area is that if you have no intention of supporting for example POST on a certain resource, you should say so in the Allow header, and respond with a 405 on that resource, so that a client doesn't take the extra roundtrip to authenticate and get authorized in vain, as it is wasted effort both for the client and the server.

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 401 and 403 is for that purpose. 405 is for a different and orthogonal purpose.

@ThisIsMissEm
Copy link

Could the Allow header not vary based on authentication? e.g., an authenticated and authorized user could make a POST as they're able to know that the resource even exists in the first place, but for an unauthenticated user, they would get a 404 and no Allow header, as to not leak that there is actually a resource existing at the given path?

@acoburn
Copy link
Member

acoburn commented Nov 20, 2021

I completely agree with @ThisIsMissEm in that we don't want this to be an avenue where we leak data.

From RFC 7231:

The actual set of allowed methods is defined by the origin server at
the time of each request. An origin server MUST generate an Allow
field in a 405 (Method Not Allowed) response and MAY do so in any
other response.

If @kjetilk's point is about returning this header on 405 responses, then I agree with that direction. I would not expect the Allow* headers to appear on other 4xx category errors, especially not with 401 or 403.

@kjetilk
Copy link
Member Author

kjetilk commented Nov 20, 2021

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, Allow or not. I therefore see no value at all at being obscure about it, but I do see value in not wasting cycles on running through auth* that will not make a difference.

@acoburn
Copy link
Member

acoburn commented Nov 20, 2021

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.

@kjetilk
Copy link
Member Author

kjetilk commented Nov 22, 2021

But to the substance of this change: by removing this clause, you are effectively saying that 405 responses take precedence over 401/403 responses.

To be accurate, this change leaves the interpretation to be HTTP compliant, which indeed is what does.

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.

Alright, so your concern isn't with Allow, but with the consequences it has for Accept-*? That can be reconciled, because this section puts together two things that in HTTP are different, one thing is the allowable methods on a resource, the other thing is acceptable representations. If clients use a disallowed method, they will receive a 405 and since good practice dictates an error should be followed with a way to resolve it, the Allow header must go with it, but that does not mean that the Accept-* header must go with it, there's nothing in HTTP to indicate that that shouldn't be subject to authorization, just Allow.

If there are no acceptable representations, the corresponding error is 406, to which the Accept-* headers must follow, but that can well be after authorization.

Therefore, one resolution to this problem is to just move the requirement to authorize, so that it does not pertain to Allow but does to the Accept-* headers. I've committed an update that does this. Alternatively, we could say that Accept-* headers should only follow 406 and successful requests, and leave it to implementation to decide whether they want to authorize first.

Copy link
Member

@csarven csarven left a 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.

@kjetilk kjetilk requested a review from a team December 17, 2021 22:46
@kjetilk kjetilk added this to the Something for 0.9.x milestone Dec 23, 2021
Copy link
Member

@csarven csarven left a 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>
Copy link
Member

@csarven csarven Dec 27, 2021

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:

Suggested 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.

@timbl
Copy link
Contributor

timbl commented Jan 12, 2022

Two scenarios with public access:

  1. If a resource has public append permissions, and supports public POST say, then the app needs to know that that is possible. Does it get Allow: POST in the first GET it does, even though unauthenticated?
  2. If a resource has public read only but private Append, then the app will do what -- read unauthenticated and get Allow GET, then prompt the user to log in, to a head, and then check that it has Allow: POST ?

@kjetilk
Copy link
Member Author

kjetilk commented Jan 12, 2022

Two scenarios with public access:

1. If a resource has public append permissions, and supports public POST say, then the app needs to know that that is possible.  Does it get Allow: POST in the first GET it does, even though unauthenticated?

Yes, it does, that is my interpretation of HTTP.

2. If a resource has public read only but private Append, then the app will do what -- read unauthenticated and get Allow GET, then prompt the user to  log in, to a head, and then check that it has Allow: POST ?

In that case, it will see Allow: POST in both cases, which indeed isn't terribly useful (and I haven't found Allow to be very useful), but in that case, the app will find WAC-Allow more useful, right? Isn't the intention of WAC-Allow to address exactly that kind of problem?

@csarven
Copy link
Member

csarven commented Feb 7, 2022

To further help clarify the purpose of 405 and where the Allow header is typically used, note the following alongside 501:

When a request method is received
that is unrecognized or not implemented by an origin server, the
origin server SHOULD respond with the 501 (Not Implemented) status
code. When a request method is received that is known by an origin
server but not allowed for the target resource, the origin server
SHOULD respond with the 405 (Method Not Allowed) status code.

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:

The purpose of this field is
strictly to inform the recipient of valid request methods associated
with the resource.

Here is another example from the Solid Protocol:

When a DELETE request targets storage’s root container or its associated ACL resource, the server MUST respond with the 405 status code. Server MUST exclude the DELETE method in the HTTP response header Allow in response to requests to these resources [RFC7231].

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants