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

Allow ACL doc discovery without acl:Control #248

Closed
wants to merge 2 commits into from

Conversation

michielbdejong
Copy link
Contributor

This is a spec change proposal.

Practical Reason

Most current implementations of Solid allow ACL doc discovery without acl:Control.
For instance, NSS and CSS just add .acl and ESS adds ?ext=acl.
It would be a lot of work (including data migration on all existing servers) to change that.

Simplicity Reason

Using a predictable scheme makes the life of the server developer easier.
Also, reporting the Link header in for instance OPTIONS makes it easier
to discover if a server supports WAC at all, which is useful both to app
developers and to end-users of apps. You can then see "ah, there is an ACL
link but I don't have access", rather than being kept in the dark about it altogether.

Reasons against it

  • If the ACL doc URL is predictable, there is a real risk that app developers skip
    the discovery and just guess it. However, now that different servers already
    use different schemes, I don't think many app developers will do this anymore.

  • If the ACL doc URL is predictable, it might give an attacker a leg up (help them
    overcome the first barrier). The could then for instance try to brute-force their
    way into the ACL doc. However, if they can brute-force their way into acl:Control
    then they would maybe also be able to use that technique to break the barrier to
    discovery.

This is a spec change proposal.

### Practical Reason
Most current implementations of Solid allow ACL doc discovery without acl:Control.
For instance, NSS and CSS just add `.acl` and ESS adds `?ext=acl`.
It would be a lot of work (including data migration on all existing servers) to change that.

### Simplicity Reason
Using a predictable scheme makes the life of the server developer easier.
Also, reporting the `Link` header in for instance OPTIONS makes it easier
to discover if a server *supports* WAC at all, which is useful both to app
developers and to end-users of apps. You can then see "ah, there is an ACL
link but I don't have access", rather than being kept in the dark about it altogether.

### Reasons against it
* If the ACL doc URL is predictable, there is a real risk that app developers skip
the discovery and just guess it. However, now that different servers already
use different schemes, I don't think many app developers will do this anymore.

* If the ACL doc URL is predictable, it might give an attacker a leg up (help them
overcome the first barrier). The could then for instance try to brute-force their
way into the ACL doc. However, if they can brute-force their way into `acl:Control`
then they would maybe also be able to use that technique to break the barrier to
discovery.
@RubenVerborgh
Copy link
Contributor

There is no guarantee whatsoever that the URL is predictable.
It might be that ESS mints private URLs for specific resource for all we know.

-1 for me

Copy link
Contributor

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👎

@michielbdejong
Copy link
Contributor Author

There is no guarantee whatsoever that the URL is predictable.

How is that relevant? This is about whether the Link header should be hidden from sight or not for unauthenticated agents.

@michielbdejong
Copy link
Contributor Author

It might be that ESS mints private URLs for specific resource for all we know.

That is a good point, and good to leave that open. So the spec should say that whether or not discovery of the ACL doc URL requires acl:Control is up to the server.

Copy link
Contributor

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the spec should say that whether or not discovery of the ACL doc URL requires acl:Control is up to the server.

It should not; the existing text is just fine. Discovery requires Control permissions.
The existence of another channel to discover doesn't change that.

@csarven
Copy link
Member

csarven commented Mar 10, 2021

As far as I can tell, the rationale to limit "discovery" to agents with Control is not well-documented beyond it being introduced in early PRs for auxiliary resources. There is no "[Source]" link to the requirement. I know the requirement was mentioned/repeated in discussions but underlying reasons seems to be left out. There is much material/chat to process, but if anyone can link me to the background, I'd love to review.

As already mentioned, what's in the spec (re "discovery") is not based on existing server implementations. What that tells me is that while the decision to limit is done with good intentions (possibly intuitive or correct or preferred..), it wasn't based on observable practice. Again, this is one of the reasons why "Source" links for each spec requirement (at least in one of the drafts) is important/useful! "Open standards" based on open discussion and consensus. With receipts.

Thanks for raising this issue/PR @michielbdejong @ylebre . This PR can at least serve as a (repeat) review for the behaviour.


to discover if a server supports WAC at all

  • Protocol requires WAC...
  • Link rel=acl also discloses the location of the ACL resource.
  • When the WAC-Allow header is included in the response, that'd be sufficient for a client to know if WAC is supported. But even then I'm not sure if the client needs to know the URL of the ACL it can't access.

If the ACL doc URL is predictable, there is a real risk that app developers skip
the discovery and just guess it.

True. Always possible if one implementation is designed against another implementation (as opposed to the specification). The Protocol prescribes the required behaviour for ACL discovery. If developers are willing to ignore the spec, obviously it doesn't prevent them from developing an application against a specific server, and they do that with all the risks and limitations associated to it. Doesn't matter what the specs at this point.

Perhaps more of a concern is when attackers target a server implementation or even specific pods.

Limiting the discovery of ACL to agents with Control keeps the attack surface small(er) - cuts it in half? When a server is under a DDoS attack targeting the ACLs, it may also need to temporarily block access to the resources linking to the ACLs, or possibly just omit the Link header, to avoid further disturbance. That would be an interruption to legitimate use of the server - even if small. Consider the cases where Read is assigned to a resource for (un)authenticated agents/clients.. and the ACL URL is observable.

Having said that, servers generating ACL URIs based on a deterministic algorithm (eg. .acl, ?ext=acl) still have this as a potential issue. So, if the spec sticks to requiring Control in order to expose Link rel=acl, I propose to then introduce a security consideration for servers that use a deterministic algorithm when generating ACL URIs.


So the spec should say that whether or not discovery of the ACL doc URL requires acl:Control is up to the server.

I don't feel comfortable about allowing this sort of "Variability in Specs" ( #138 ) especially for access control. Prefer to have one reliable approach.


Having Read access on a resource grants one to view a representation. Container representations include statements about contained resources irrespective of agent having any access to the contained resources. The question is whether the exposure of the ACL resource from the associated resource is similar or can be seen to fit in the same category. In any case, we can still introduce exceptions ie. require Control on resource in order to include Link.


I'd be interested in reviewing more material before making any changes to the spec. The current requirement can be relaxed/removed at any time. Yes, in the meantime, some servers do not appear to conform on this requirement. So let's gather more data. That includes getting more feedback from developers about their current/future implementations.

@elf-pavlik
Copy link
Member

ACP doesn't use 'Control' and Access Control Resource include explicit policies for accessing the ACR itself. In that case discovery probably would depend on those policies granting (or not) Requesting Party access to the ACR. Maybe it would make sense to have mini spec just for Solid-WAC integration and separate one for Solid-ACP integration?

@csarven
Copy link
Member

csarven commented Mar 10, 2021

WAC is a way of setting Protocol's default behaviour and taking security considerations into account. If required, an explicit policy for discovering ACL resources can also be done.

@bblfish
Copy link
Member

bblfish commented Mar 11, 2021

I think the text is ok.
I want to make a proposal for ACLs to have ACLs that when enabled would allow servers to make their ACLs readable to a wider group, even public if needed. That proposal should be compatible with the current specification. Ie. when an acl does not have an acl, then the current behavior must be assumed.

@justinwb
Copy link
Member

justinwb commented Mar 11, 2021

So the spec should say that whether or not discovery of the ACL doc URL requires acl:Control is up to the server.

This shouldn't be left up to implementation, regardless of direction.

There is no "[Source]" link to the requirement. I know the requirement was mentioned/repeated in discussions but underlying reasons seems to be left out.

Since I did that submission - the motivation behind it starts with principle of least privilege - which is specifically relevant here given the role of the acl in the security posture of the system. To @csarven's point - it minimizes the attack surface. This is a general best practice we should continue to employ where we can. I think that as part of the drafting process we need to look for reasonable opportunities to improve security posture, even if existing implementations may need to be adjusted to conform. This should be done judiciously, but this seemed like a reasonable case (and still does).

@csarven
Copy link
Member

csarven commented Mar 12, 2021

As far as I can tell the removal of the text "discover" from the specification is appropriate and consistent with the Protocol. I'm in approval of the initial commit made to this PR ( 4639a5d ):


RFC3986 https://tools.ietf.org/html/rfc3986#section-7 states:

  1. Security Considerations

A URI does not in itself pose a security threat.


Introducing a security consideration for servers using a deterministic algorithm to generate a URI string does not fundamentally address the perceived concern on discovering the name of access control resources. Even if servers were to omit the link relation, it is trivial to look up the algorithm generating the URI string, especially in the case of open-source servers, and so server implementations and pods using them can be targeted en masse.

It is also worthwhile to consider scenarios where the access control source can be obtained in other ways eg. agent used to having Control access or having prior knowledge about the algorithm generating the string. (RFC 3986 blurb)

The only rule that can universally address this is by requiring servers to follow a certain URI Template for access control URIs eg. "Servers MUST use a non-deterministic (unguessable) algorithm to generate the URI string when allocating it to access control resources."

This adds unnecessary complexity to the system. Knowing an identifier != access. Similar to having Read access to a container and observing the containment statements does not entail having access to the contained resources. (RFC 3986 blurb)

The Solid Protocol must remain neutral and flexible about this by not requiring how servers name their resources. (Please do not bring up slash semantics into this discussion!)

Access control resources of WAC was never classified as "secret" - no documentation suggests this. Implementations simply followed this practice and at the very least did not deem them to be secret or a security threat. (RFC 3986 blurb)


Again, where needed, explicit policies controlling the discoverability of access control resources through HTTP headers (or anywhere) can still be prescribed/achieved per mechanism.

@bblfish
Copy link
Member

bblfish commented Mar 12, 2021

I agree

  1. with the original removal of "discover" from the first paragraph, since it could be guessable in any case, and making it unguessable is a lot of work for nothing: we all know that someone can edit some acl for a resource.
  2. I am not against addition of the second paragraph.

The specification should state what has to be, not what should not be, as that would be closing possibilities that are unknown. The same with tests. For example the proposal ACLs on ACLs for WAC would make tests that just check for :Control Access mode wrong on systems that allow ACLs to be read more widely.

<p>To discover, read, create, or modify an ACL auxiliary resource, an <code>acl:agent</code> MUST have <code>acl:Control</code> privileges per the <a href="https://github.com/solid/web-access-control-spec#acl-inheritance-algorithm">ACL inheritance algorithm</a> on the resource directly associated with it.</p>
<p>To read, create, or modify an ACL auxiliary resource, an <code>acl:agent</code> MUST have <code>acl:Control</code> privileges per the <a href="https://github.com/solid/web-access-control-spec#acl-inheritance-algorithm">ACL inheritance algorithm</a> on the resource directly associated with it.</p>

<p>Whether or not <code>acl:Control</code> is also required to *discover* an ACL auxiliary resource, is left up to the server implementer.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<p>Whether or not <code>acl:Control</code> is also required to *discover* an ACL auxiliary resource, is left up to the server implementer.</p>
<p>Whether <code>acl:Control</code> is required to <em>discover</em> an ACL auxiliary resource, is left to the implementation.</p>

@bblfish
Copy link
Member

bblfish commented Mar 15, 2021

I have added a detailed example on how ACLs on ACLs could allow protected resources to have public ACLs without leaking information. The example shows how Alice may find it useful to know that she has access to an<event> resource as member of a closed group.

What that shows is that acl discovery should be put in terms not of acl:Control but in terms of the visibility (especially: readability) of the ACL. If the ACL is only visible to members who have control then we are in the current situation. But for ACLs that are publicly readable, the ACL should of course be publicly discoverable. There are many intermediate possibilities too, with an acl only being visible to authenticated users, or to users who have shown they have a certain nationality, etc...

(There is in my view no problem either with ACLs being discoverable and yet not being readable, since we know logically that all resources have ACLs. Knowing the name of an ACL does not give any new information out. It may just be inconvenient as it suggests that the link can be followed, and so may result in another request with a 401)

@RubenVerborgh RubenVerborgh dismissed their stale review March 15, 2021 13:26

Following new updates to the thread

@bblfish
Copy link
Member

bblfish commented Mar 15, 2021

I opened up a complimentary PR that goes a little further than this one, taking into account use cases from the Authorization panel #250

@michielbdejong
Copy link
Contributor Author

Continued in #252

@michielbdejong
Copy link
Contributor Author

We can now consider this issue as resolved. A client needs (at most) acl:Read, not acl:Control, to see the rel="acl" link response header on a resource.

michielbdejong added a commit to solid-contrib/web-access-control-tests that referenced this pull request Mar 19, 2021
@michielbdejong michielbdejong deleted the patch-1 branch March 19, 2021 08:45
@bblfish
Copy link
Member

bblfish commented Mar 19, 2021

We can now consider this issue as resolved. A client needs (at most) acl:Read, not acl:Control, to see the rel="acl" link response header on a resource.

I agree. Is that written out somewhere? It seems I have only seen text being removed. :-)

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

Successfully merging this pull request may close these issues.

7 participants