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

Which permissions are required for deleting an ACL document? #145

Closed
RubenVerborgh opened this issue Jun 23, 2019 · 17 comments
Closed

Which permissions are required for deleting an ACL document? #145

RubenVerborgh opened this issue Jun 23, 2019 · 17 comments

Comments

@RubenVerborgh
Copy link
Contributor

RubenVerborgh commented Jun 23, 2019

No description provided.

@michielbdejong
Copy link
Contributor

michielbdejong commented Jun 23, 2019

Something else. When deleting an ACL document R about a target T, we require Control on T.

This is explained in https://github.com/solid/web-access-control-spec#referring-to-the-acl-resource-itself

We should probably move this issue to the WAC repo, btw.

@RubenVerborgh
Copy link
Contributor Author

Thx, mainly for consistency with solid/solid-spec#195

@csarven
Copy link
Member

csarven commented Dec 2, 2019

R = ACL document.

P: R of root C can't be deleted.

P: If R can occur in containment triples, then Write permission on C is required ( if based on https://github.com/solid/solid-spec/issues/195#issuecomment-559799154 ). If R can't occur in containment triples (see also #116 ) then Write permission on C is not required.

P: R is an RDF Source. R can be created, updated and deleted using the same methods.

P: Deleting a resource removes associated R (not including inherited R).

Taking this as a whole, Write on resource or associated R is the only requirement to delete R.

@kjetilk kjetilk transferred this issue from solid/solid-spec Feb 5, 2020
@kjetilk
Copy link
Member

kjetilk commented Feb 5, 2020

I transferred this issue, since it is a dependency for #41 , in which the rough consensus is indeed as @michielbdejong 's notes (editing a bit, to (perhaps) add consistency within my comments):

When deleting an ACL resource A about a target R, we require Control on R.

Then, it is also understood that (follows from consensus in #126):

When deleting an resource R, we require Write on parent container C. If R has an ACL Document A, it will be deleted as a side-effect.

Also note that ACL documents aren't independently contained, so the resource R may be a container.

@csarven
Copy link
Member

csarven commented Feb 6, 2020

The premises that I've stated earlier was tied to common understanding or rough consensus based on available data. As this issue re-questioned the actual required permissions, my conclusion here:

Taking this as a whole, Write on resource or associated R is the only requirement to delete R.

was based on the minimum requirement by building up from the premises. I think it turned out to be Write. I have no objection to requiring Control on R in order to delete the associated ACL. It was just what was previously spec'd or understood wasn't clear.


Also note that ACL documents aren't independently contained, so the resource R may be a container.

Are you saying that when an ACL is created (probably via PUT or PATCH), it will not be added to containment? If so, where did you derive that from?

The following are equivalent:

When deleting an resource R, we require Write on parent container C.

P: If R can occur in containment triples, then Write permission on C is required ( if based on solid/solid-spec#195 (comment) )

But note:

If R can't occur in containment triples (see also #116 ) then Write permission on C is not required.

I think it makes sense to exclude resources like ACL in containment but I don't think we have fully worked that out (as to why or why not).


The following two statements are essentially describing the same thing (note that I've used 'R' as that was the initial term used in this issue):

If R has an ACL Document A, it will be deleted as a side-effect.

P: Deleting a resource removes associated R (not including inherited R).

This may however raise an issue in that other resources may depend on the same ACL. So, what's currently not specified is the shape of the ACL - IIRC, this is looked into in the interop panel - in that all authorization policies is for the same target resource. Or at the very least acknowledging that an ACL of a resource can only be referred by that resource (rel=acl). That will ensure that once a resource is deleted and its ACL with it (provided eg. Control on the resource, and Write on container it is in), then there is no issue about other resources suddenly losing their ACL. Perhaps this warrants a separate issue (unless one exists that escapes me) in that resources should have a dedicated ACL that's not shared by other resources.

@kjetilk
Copy link
Member

kjetilk commented Feb 6, 2020

Also note that ACL documents aren't independently contained, so the resource R may be a container.

Are you saying that when an ACL is created (probably via PUT or PATCH), it will not be added to containment?

Yes.

If so, where did you derive that from?

Merely stating current behaviour.

I think it makes sense to exclude resources like ACL in containment but I don't think we have fully worked that out (as to why or why not).

Yes, that's a valid point. Especially as the interop panel is generalizing to other metadata types, it would be nice to have a coherent explanation around this. Meanwhile, I don't think it falls within our mandate to change it.

This may however raise an issue in that other resources may depend on the same ACL. So, what's currently not specified is the shape of the ACL - IIRC, this is looked into in the interop panel - in that all authorization policies is for the same target resource.

Good point!

Or at the very least acknowledging that an ACL of a resource can only be referred by that resource (rel=acl). That will ensure that once a resource is deleted and its ACL with it (provided eg. Control on the resource, and Write on container it is in), then there is no issue about other resources suddenly losing their ACL. Perhaps this warrants a separate issue (unless one exists that escapes me) in that resources should have a dedicated ACL that's not shared by other resources.

Yes, or make the lifecycle expectations clear, that a certain type of metadata resource are tied to the lifecycle of their resource, and if they need to free themselves from that, they need to define how authorization is managed, possibly through their own ACL resources. Yes, it sounds like it should be its own issue.

@justinwb
Copy link
Member

justinwb commented Feb 7, 2020

It is not clear to me where we arrived at a rough consensus that you require control permission on resource R to remove the ACL related to it. This means that you either cannot delete a resource without control permission (despite having write), or if you delete a resource, the ACL is left to hang around and possibly cause problems later. If anything the WAC spec implies the opposite interpretation, by citing DELETE in the class of operations supported by acl:Write.

We provide some clarity around this in the draft resource metadata text in progress at solid/data-interoperability-panel#32.

An ACL metadata resource MUST be deleted by the Solid server when the resource it is directly associated with is deleted and the Solid server is authoritative for both resources.

@kjetilk
Copy link
Member

kjetilk commented Feb 7, 2020

It is not clear to me where we arrived at a rough consensus that you require control permission on resource R to remove the ACL related to it.

No, that is not the rough consensus, what I proposed is that if you delete only the ACL, you need Control, if you delete the resource itself, the ACL will be removed as a side effect, and thus only require Write, so we wouldn't be in the situation you described.

@csarven
Copy link
Member

csarven commented Feb 10, 2020

P: An agent may only need Write to a resource but not necessarily Control its ACL.

P: Deleting a resource's ACL may effectively change its authorization policy.

In order to prevent a chaos-ensuing policy change, Control on the resource is a reasonable requirement in order to allow direct delete of its ACL.

However, it is trivial to get around that:

An agent with Write on resource can re-create the resource with their preferred policies: delete resource (and ACL as a side-effect), create resource (with the same name and content), and then create ACL.


Edit: The bottom line is that given:

P: Write is an update in that information can be created or removed

P: Deleting a resource removes associated ACL (not including inherited ACL).

(modified variable in quote)

As long as an agent has Write on resource, the requirement to use Control on resource as a way to restrict the deletion of its ACL directly is inadequate for the general scenario. It may make it slightly inconvenient but doesn't prevent in the end. Interestingly, as I've mentioned earlier, Write on resource may still be the minimum requirement to delete an ACL.

@kjetilk

This comment has been minimized.

@kjetilk
Copy link
Member

kjetilk commented Feb 10, 2020

Thinking about it, I made a proposal in #46.

However, I think you convinced me that we could go with just acl:Write for now, and provide an optional security feature on this later, as I proposed in #46.

@csarven
Copy link
Member

csarven commented Feb 12, 2020

We've primarily focused on two ways to delete an ACL resource, ie. either by deleting the resource that makes an association to an ACL or the ACL resource directly.

For the sake of completeness, I think we should also acknowledge the possibility to limit the interaction by forbidding delete on ACL resource.

P: An ACL resource can only be deleted as a side-effect by the removal of the resource that is associated to.

We can weigh the ways forward in the draft ie. if and how an ACL can be deleted directly (as any RDF Source).

@justinwb

This comment has been minimized.

@csarven
Copy link
Member

csarven commented Apr 24, 2020

Correction/clarification to:

An agent with Write on resource can re-create the resource with their preferred policies: delete resource (and ACL as a side-effect), create resource (with the same name and content), and then create ACL.

Creating an ACL requires Control access on the resource to which access is being granted ( #42 (comment) ). While having Write access would allow deleting of the resource and effectively the associated ACL (if exists), it doesn't follow that a new ACL can be created that is associated with the resource after the resource is reinstated.

The general security issue is still valid in that the access control on the reinstated resource can change given the ACL inheritance algorithm:

  1. agent-a has write to /foo, agent-b has read to / but doesn't have read to /foo
  2. agent-a deletes /foo (/foo.acl is deleted)
  3. agent-a creates /foo (/foo.acl is linked to but has no representation)
  4. agent-b can read /foo (because of read to /)

@csarven
Copy link
Member

csarven commented May 26, 2021

Reviewing myself in #145 (comment) and recording with fresh eyes. There are some assumptions that should be clarified. I don't see a security issue in the scenario.

It is true that access controls on reinstated resources can "change" in comparison to the controls that was set on the ACL resource of the deleted resource if there is a default permission on the container. In the example scenario, we are assuming that agent-a and agent-b don't have Control on anything. (If agent-a had Control and didn't want agent-b to read /foo, it can block step 4) It is agent-c that has Control on / and gave it a default Read, which allows step 4 to happen. agent-c disallowed agent-b to Read /foo while /foo existed. That's all. Permissions are applicable to what exits and they are removed once a resource no longer exists. Aside: reinstating resources and what that entails/good practice is a separate matter and this is already documented in the Protocol as per AWWW.

@kjetilk
Copy link
Member

kjetilk commented May 26, 2021

Should we re-open?

@csarven
Copy link
Member

csarven commented May 26, 2021

No need. The Protocol currently includes what's required/accurate. It will actually be covered by the WAC spec, and soon removed from the Protocol.

@csarven csarven moved this to Done in Specification Sep 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

6 participants