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

Make any deviation of SPARQL UPDATE explicit #139

Closed
RubenVerborgh opened this issue Jan 15, 2020 · 19 comments
Closed

Make any deviation of SPARQL UPDATE explicit #139

RubenVerborgh opened this issue Jan 15, 2020 · 19 comments
Assignees
Labels
doc: Protocol status: Nominated An issue that has been nominated for the next monthly milestone topic: resource access

Comments

@RubenVerborgh
Copy link
Contributor

RubenVerborgh commented Jan 15, 2020

No description provided.

@kjetilk
Copy link
Member

kjetilk commented Jan 15, 2020

Yes, indeed, this is relevant in the context of #85 and #125 .

@acoburn
Copy link
Member

acoburn commented Jan 18, 2021

We have implemented only standard SPARQL 1.1

@michielbdejong
Copy link
Contributor

Solid OS uses this willful violation.
NSS and PSS support it.
Neither CSS nor ESS do.
@acoburn is right, we could achieve a very similar effect with If-Match.
It wouldn't be too much work to implement that in rdflib if the consensus is that it's better to align with upstream sparql-update on this.

@timbl what should we do?

@timbl
Copy link
Contributor

timbl commented Feb 17, 2021

@RubenVerborgh - I agree. "I think that, if we make such deviations, they should be made explicit".

What do you suggest is the best way? We could flag it in the content type.

Maybe the ideal is to use a recognizably different syntax in the file, like REMOVE (which fails) instead of DELETE (which never fails) and so extend sparql in a way the WG could potentially agree to be generally adopted.

That would mean for us the cost of changing our syntax,

We could then flag it like with a ; remove=true or something on the content type.So we would be extending SPARQL UPDATE and not changing it. There must be a lot of discussion of this already elsewhere.

@michielbdejong
Copy link
Contributor

OK, so to summarize, the proposed change would be:

  • instead of application/sparql-update all Solid apps will send content-type application/sparql-update; remove=true
  • instead of DELETE DATA { ... }; INSERT DATA { ... } all Solid apps will write REMOVE DATA { ... }; INSERT DATA { ... }
  • when clients do this, servers will only execute the 'INSERT' if the 'REMOVE' matched, and fail the entire request otherwise

Right?

@michielbdejong
Copy link
Contributor

Great!

@michielbdejong
Copy link
Contributor

Looks like after almost two years we all agree now, awesome!!

@michielbdejong
Copy link
Contributor

OK, sorry I wasn't aware. What are the steps?

@kjetilk
Copy link
Member

kjetilk commented Mar 11, 2021

Obviously, I've lost a year of context, but I have some opposition to the semaphore mechanism due to security concerns. It would require not only write permission, but also read permission, which would violate the principle of least privilege. I tried to sum these problems in solid-contrib/query-panel#2 and proposed a solution to the SPARQL 1.2 CG: https://lists.w3.org/Archives/Public/public-sparql-12/2020Jan/0000.html

I certainly see the value of a light-weight semaphore mechanism (even though there is some opposition to that idea in the SPARQL 1.2 CG), but I believe that as it stands, it is too dangerous. We need to carefully consider the possible impact. Given time, I would certainly want to work on this.

@justinwb
Copy link
Member

OK, sorry I wasn't aware. What are the steps?

The proposal would need to be documented in the form of a change submission to the solid protocol specification, and go through editorial review. This issue and any related ones would/should be referenced as part of that proposal.

@michielbdejong
Copy link
Contributor

FWIW, NSS now accepts both application/sparql-update and application/sparql-update-single-match as the content-type. In both cases, the behaviour is "Solid-flavoured" sparql-update (as it has been for years in the case of application/sparql-update).

Solid OS (rdflib) still sends application/sparql-update and expects the pod server to behave the same way as NSS. If and when other servers support NSS-compatible behaviour with application/sparql-update-single-match, we'll update Solid OS (rdflib) to start sending the new content-type header. HTH.

@acoburn
Copy link
Member

acoburn commented Mar 11, 2021

Before implementing support for application/sparql-update-single-match (or any new content type), please first show me a formal specification (even in draft form) that formalizes the semantics. A github discussion is a fine way to find initial consensus, but there is no way to build a reasonable implementation based on a discussion thread such as this.

@kjetilk
Copy link
Member

kjetilk commented Mar 11, 2021

Here be dragons! And not the small and cute ones you see on dragon-taming shows, the big and fire-breathing ones. :-)

Let me reiterate: By doing it this way, we have to be really sure that the spec details security considerations, and that the use of a content type is a special case that requires read access. We also need to have tests that verify compliance. A semaphore is exposing information, it MUST be a read operation, even though the operations to the resource are only write operations.

I'm very skeptical about having such special cases in general, it can be misunderstood and bugs are likely. Thus, the big, dangerous dragons.

Instead, it should be understood in terms of a projection (in SPARQL terms, and more generally in database terms). With that, it is consistently understood in implementations that all projections are exposing information and require read privs. For something as security critical as this, that, I think, is the level of rigor we should have.

@michielbdejong
Copy link
Contributor

OK, too bad, I thought we had finally found a solution but sounds like this will take a few more rounds. I removed the -single-match from the content type again in the test suite, but the behaviour that is tested for is the Solid-flavoured one.

@kjetilk
Copy link
Member

kjetilk commented Mar 11, 2021

This was one of the things that made me start the query panel back in the day... I believe that format is the right one to discuss issues like these (even though this might be the single issue we have for the short term)

@michielbdejong
Copy link
Contributor

I'll also mark the test for this as disputed again, since it seems the dispute will still go on.
For now, we need to tell clients to avoid sending DELETE/INSERT since the different server implementations will behave differently if the DELETE fails. And then the same goes for #220

@michielbdejong
Copy link
Contributor

OK sorry maybe the word 'dispute' is too strong. Maybe we should call it an 'undecided issue'? Let's use that word from now on.

What I try to cover with it is the situation where different implementations behave in different ways. And that's the current reality of Solid. And it's not OK.

We are in the early stages of making a decision

I sincerely hope not! I brought this undecided issue to the attention of the spec editors in June 2019. So if this is early stages, then I dare not estimate how many more years Solid will stay in limbo on this issue.

I tried one last time to lead a decision by asking Tim to take a stance, which he did in #139 (comment) and we got pretty close last week. I proposed a resolution, @RubenVerborgh adapted it, @acoburn agreed. That's as close as we've ever been.

After that, @RubenVerborgh retracted his position saying it was a 5-second proposal. Fair enough, so then spend more than 5 seconds and tell us why your proposal is not good enough, so we can improve it. Is the specific string -single-match wrong? Given this issue is about "making our deviation explicit", doesn't your proposal solve that nicely?

@kjetilk welcome back! we missed you! <3 I see your points about whether or not we should use a semaphore at all. That's a good question, and it looks like it's unanswered. Tim seems to says yes, Ruben and Aaron seem to say ok, but only if we signal it properly, and you seem to say no. I don't care either way, I just want it to either be yes (signaled in whichever way we decide on) for all servers or no for all servers.

Emphasis on for all servers! :)

@kjetilk
Copy link
Member

kjetilk commented Mar 12, 2021

Hi @michielbdejong ! Thanks for the welcome back!

Actually, I'm very much in favour of having a semaphore mechanism, just not this one :-)

I just think we need to be very careful not to introduce a mechanism that will either limit our ability to design a good least privilege system, or that there will be confusion around that can introduce obscure security issues. Also, I don't want to break SPARQL 1.1.

I think it is useful to think in short term and long term. In the long term, it is very feasible to introduce generic functionality into SPARQL 1.2, which is being worked on, albeit with low intensity, that can support the semaphore mechanism. I'd love to prototype that, and champion it towards the SPARQL 1.2 effort.

In the short term, there are a few options:

  1. Don't have the semaphore mechanism, instead require backends to isolate different HTTP requests from each other (as I understand ESS is doing)
  2. Don't have a semaphore mechanism and say nothing about it, just wait for a long term solution.
  3. Have a short term solution based on e.g. a distinguishing content-type

I suppose all these could be orthogonal to a long term solution. I certainly see the value of having the semaphore, as I see the ACID requirement that could easily be the alternative as a pretty hefty one. But I am also very uneasy about the content-type proposed here in a broad security context, so I just don't see a good short-term solution to this problem.

BTW, if we go with a content-type, I think we should but it in the vnd tree.

@kjetilk
Copy link
Member

kjetilk commented Mar 12, 2021

BTW, I just rediscovered the Github issue I made with the SPARQL 1.2 CG on this topic: w3c/sparql-dev#60
It is useful to review for context and for what might be considered appropriate for SPARQL.

@kjetilk kjetilk added the status: Nominated An issue that has been nominated for the next monthly milestone label Oct 6, 2021
@kjetilk kjetilk added this to the October 2021 milestone Oct 6, 2021
@kjetilk kjetilk self-assigned this Oct 6, 2021
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: resource access
Projects
None yet
Development

No branches or pull requests

6 participants