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

Is Read required on top of Write for PATCH delete and where ? #220

Closed
bourgeoa opened this issue Jan 17, 2021 · 12 comments · Fixed by #346
Closed

Is Read required on top of Write for PATCH delete and where ? #220

bourgeoa opened this issue Jan 17, 2021 · 12 comments · Fixed by #346
Assignees
Labels
doc: Protocol status: Nominated An issue that has been nominated for the next monthly milestone topic: authorization topic: querying
Milestone

Comments

@bourgeoa
Copy link
Member

On PATCH spec seems to require only Write for sparql DELETE. This is implemented in solid/test-suite.
But NSS is requiring Read and Write, with the following explanations in the code comments in NSS PATCH implementation.

// Read access is required for DELETE and WHERE.
// If we would allows users without read access,
// they could use DELETE or WHERE to trigger 200 or 409,
// and thereby guess the existence of certain triples.
// DELETE additionally requires write access.

What should be the spec ? What should test-suite implement if the response is not straight forward ?

@csarven
Copy link
Member

csarven commented Jan 18, 2021

I'm not sure which spec you're referring to but will take it as underspecified/unknown at the very least in the issues here and current Protocol spec. But right that the current web-access-control-spec alludes to acl:Write allowing PATCH.. and the wiki WebAccessControl doesn't require acl:Read.


When using WAC, I agree on requiring acl:Read and acl:Write in order to accept PATCH operations including DELETE DATA or WHERE - at least those with application/sparql-update data type if anything.

When agent doesn't have acl:Read and acl:Write on target, rejection should use the 403 status code.

When agent has acl:Read and acl:Write on target and there is no match on DELETE DATA, rejection should use the 409 status code.

I've documented some PATCH requests in a table based on INSERT DATA and DELETE DATA in #14 (comment) as those appear to be the basic level of SPARQL Update we seem to agree on to date. Related issue: #125 . So, WHERE support needs an explicit agreement.

See also for recent-ish description of the current state of things: CommunitySolidServer/CommunitySolidServer#15 (comment)

@acoburn
Copy link
Member

acoburn commented Jan 18, 2021

With ESS, a PATCH operation does not require READ access.

There is also no locking mechanism and hence no notion of a semaphore in ESS (distributed locks are a nightmare); instead, it relies on a conflict resolution mechanism (e.g. last-write-wins)

@acoburn
Copy link
Member

acoburn commented Jan 18, 2021

Worth noting that one could apply a similar line of reasoning about conditional PUT requests (If-Match, etc)

@michielbdejong
Copy link
Contributor

So now that the test suite was updated 20 days ago to require Read access, CSS is failing that test. Marking that test as disputed now.

We probably can't answer this question without answering #139 first.

@kjetilk
Copy link
Member

kjetilk commented Mar 11, 2021

OK, catching up here...

Indeed, the Read requirement stems from the semaphore mechanism, because that leaks information.

The deeper problem here is really how we design systems that allow for actual least privileges to be used. It is easy to say that it should follow the principle of least privilege, a different thing to design for it. To design for it, we need to make sure that all of SPARQL semantics maps well to access modes.

We could take the easy way out and have the semaphore mechanism and say that all SPARQL Update queries require Read+Write, but that wouldn't be a great design in light of the principle of least privilege. Then, we could also ensure ACID in the database but not allow these semaphores, but that puts the ACID requirement on all implementations, which is also a lot to ask. We could also spec a lot of special cases around it.

I gave this some thought a long time ago, and I think there is a reasonable middle ground, but it requires changes to SPARQL.

@michielbdejong
Copy link
Contributor

Conclusion: clients should just avoid ever trying to delete a triple that's not present, because as we've seen, different pod servers will react differently. Instead, a PATCH that deletes triples should always have an If-Match header that avoids race conditions.

@kjetilk
Copy link
Member

kjetilk commented Jul 2, 2021

This has been discussed a bit amongst the Solid Editors, and it has been decided that the semaphore mechanism is a requirement. It is considered that we should come up with a better design the future, but for now, that is the case. That decision makes it clear that the answer to the title of this issue is yes.

AFAIK, the mechanism currently applies to the WHERE clause, but I think it is quite clear that the use case would naturally extend to a request with INSERT DATA and DELETE DATA as well. I also think timescale is a poor judge. In the post to SPARQL 1.2 CG, I use an example that involves these two operations, like this:

Concretely, say that client 1 goes:

DELETE DATA { <foo> <baz> "Dahut" } ;
INSERT DATA { <foo> <baz> "Bar" }

independently, client 2 goes

DELETE DATA { <foo> <baz> "Dahut" } ;
INSERT DATA { <foo> <baz> "Foobar" }

before the first client as finished. The same semaphore would apply, as the server would respond with a 409 to the DELETE DATA operation of the second client.

I realize that it would be a simpler implementation if the semaphore mechanism applied only to a WHERE clause, but it seems rather odd to tie it to that, given the above.

Thus, it seems to me that for now, we should require Read+Write privs for any SPARQL query with DELETE.

I hope that we can get to a SPARQL standard where any information exposure is tied to a projection, so that Read would only apply if you actually use the mechanism, but for now, I think the above is the best way to do it.

@kjetilk
Copy link
Member

kjetilk commented Jul 27, 2021

Can we arrive at a consensus that the semaphore mechanism applies to both DELETE operations, so that Read are required for PATCH containing those?

@csarven
Copy link
Member

csarven commented Aug 3, 2021

+0.5.

Just want to highlight that the work in #14 (comment) is generally based on:

Resources can be observable or discoverable - "knowable" - by agents having Read access privilege either on the resource or its container (inherited).

While it is possible to distinguish between knowledge about the existence of a resource from the description of a resource, the general principle, i.e., requiring Read, seems to apply to both.

Edit: It may also apply to scenarios such as PUT with If-None-Match: * requiring both Write and Read.

@kjetilk
Copy link
Member

kjetilk commented Aug 3, 2021

The Solid Editors Meeting today, with (just) @csarven and @kjetilk present resolved:

The semaphore mechanism applies to all DELETE queries, and therefore read and write are required.

@kjetilk kjetilk removed this from the Current Month milestone Aug 3, 2021
@kjetilk
Copy link
Member

kjetilk commented Aug 3, 2021

Yes, that will be resolved when we close #125

@kjetilk kjetilk added the status: Nominated An issue that has been nominated for the next monthly milestone label Sep 14, 2021
@kjetilk kjetilk self-assigned this Sep 22, 2021
@kjetilk kjetilk added this to the October 2021 milestone Sep 22, 2021
@kjetilk
Copy link
Member

kjetilk commented Sep 22, 2021

This issue has been nominated for drafting phase for the next milestone.

@kjetilk kjetilk linked a pull request Dec 14, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc: Protocol status: Nominated An issue that has been nominated for the next monthly milestone topic: authorization topic: querying
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants