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

Clarify when and how to perform context injection #231

Closed
rflechtner opened this issue Dec 21, 2023 · 17 comments
Closed

Clarify when and how to perform context injection #231

rflechtner opened this issue Dec 21, 2023 · 17 comments
Assignees
Labels
CR1 This item was processed during the first Candidate Recommendation phase. normative This issue or PR will trigger the need for another Candidate Recommendation Snapshot pending close (7 days) This issue will be closed after 7 days.

Comments

@rflechtner
Copy link

The way the section on context injection is currently written leaves a lot of questions open regarding when and how exactly context injection is to be perfomed, which can easily lead to interoperability issues between different implementations (see digitalbazaar/data-integrity#19 (comment)). The following should be made explicit:

  • Should implementations perform context injection on verification, or simply error if the suite context is not present? I think the latter is advisable, but the language suggests simply adding the missing context before verification is perfectly legal.
  • Clarify that the context must be present at the root of the secured document, so that if this is nested in a larger structure the secured document still comprises the security context.
  • Define how exactly the context is represented; this is important especially if adding the missing context before verification is treated as allowable, as different implementations of jcs proof verification implementing this differently (e.g., one adding "@context": [ "..." ], while another adds "@context": "..." ,) leads to a loss of interoperability.
@msporny msporny added CR1 This item was processed during the first Candidate Recommendation phase. normative This issue or PR will trigger the need for another Candidate Recommendation Snapshot labels Feb 24, 2024
@msporny msporny added the pr exists A pull request exists to address this issue. label Jun 9, 2024
@msporny
Copy link
Member

msporny commented Jun 9, 2024

PRs w3c/vc-di-eddsa#79, w3c/vc-di-eddsa#80, w3c/vc-di-ecdsa#61, and w3c/vc-di-ecdsa#62 were raised to address this issue. This issue is now addressed since those PRs have been merged, closing.

@msporny msporny closed this as completed Jun 9, 2024
@silverpill
Copy link

@msporny These PRs addressed a different issue. Specifically, it looks like points 2 and 3 from the description of this issue has not been clarified yet.

@msporny msporny reopened this Jun 9, 2024
@msporny
Copy link
Member

msporny commented Jun 9, 2024

@msporny These PRs addressed a different issue. Specifically, it looks like points 2 and 3 from the description of this issue has not been clarified yet.

Apologies, @silverpill and @rflechtner, I missed that in my hurried review to close the issue. I've re-opened this issue for further discussion and additional PRs to clarify the 2nd and 3rd points raised by @rflechtner.

@msporny msporny removed the pr exists A pull request exists to address this issue. label Jun 9, 2024
@msporny msporny assigned msporny and unassigned Wind4Greg Jun 28, 2024
@msporny
Copy link
Member

msporny commented Jun 29, 2024

@rflechtner and @silverpill, the Editors of the spec discussed this issue this past Friday and came to the following conclusions:

Should implementations perform context injection on verification, or simply error if the suite context is not present? I think the latter is advisable, but the language suggests simply adding the missing context before verification is perfectly legal.

When to perform context injection is now defined here:

https://w3c.github.io/vc-data-integrity/#context-injection

Clarify that the context must be present at the root of the secured document, so that if this is nested in a larger structure the secured document still comprises the security context.

The add proof algorithms now define this sort of stuff for JCS:

https://w3c.github.io/vc-di-ecdsa/#create-proof-ecdsa-jcs-2019

Define how exactly the context is represented; this is important especially if adding the missing context before verification is treated as allowable, as different implementations of jcs proof verification implementing this differently (e.g., one adding "@context": [ "..." ], while another adds "@context": "..." ,) leads to a loss of interoperability.

It can be any valid expression of @context which includes the two examples you gave above. This is explained here:

https://w3c.github.io/vc-data-integrity/#data-model

With a normative reference to JSON-LD. For JCS, you just copy and use whatever is provided to you. At a minimum, JCS verification software should follow the rules described here:

https://w3c.github.io/vc-data-integrity/#data-model

Systems that do more complete JSON-LD processing can follow the rules in the JSON-LD specification.

At this point, we believe we've done everything that was requested in the issue and are closing it again. If you disagree, please re-open the issue (and provide exactly the sort of additional guidance you'd like to see in the specification to help us ensure that it gets into the spec).

@msporny msporny closed this as completed Jun 29, 2024
@silverpill
Copy link

It can be any valid expression of @context which includes the two examples you gave above. This is explained here:
https://w3c.github.io/vc-data-integrity/#data-model

According to section 2.9.1 Context Injection, if an @context property is not provided, "implementations MUST inject or add an @context property with a value of https://w3id.org/security/data-integrity/v2".

However, as @rflechtner noted there are at least two possible ways to add an @context property:

  • Add "@context": "https://w3id.org/security/data-integrity/v2"
  • Add "@context": ["https://w3id.org/security/data-integrity/v2"]

These two operations will lead to two different transformed documents, and ultimately to two different proof values. I think specification should say that:

  • The value of added @context property MUST be a single string.
  • The @context property MUST be added before performing 1st step of "Create Proof" algorithm (Set options.@context to unsecuredDocument.@context.)

(this is my interpretation of the current text, which I'm not 100% sure is correct)

@rflechtner
Copy link
Author

I don't seem to have sufficient permission to do so, but I'd indeed like to reopen this issue because I still think there is a need for clarification; the text in https://w3c.github.io/vc-data-integrity/#context-injection has not changed since this issue has been opened, and its instructions are too broad and vague to prevent a number of foreseeable issues, most of which (but not all) relate to JCS cryptosuites. I suggest that explicit instructions for context injection are added as a step to 4.3 & 4.4.

Context injection is quite difficult to get right, and the lack of emphasis in the specification does not do justice to its complexity. It's critical that implementers understand which steps need to be taken and when; the formulation "implementations MUST inject or add an @context property with a value of https://w3id.org/security/data-integrity/v2" is hardly up to the task.

JCS suites require that implementations modify the @context of an unsecured document prior to proof generation and return the result of that transformation alongside the proof so that any modifications made to the @context property are persisted the way they had been processed in the proof (or at least in the same order). But 4.3 & 4.4 suggest that the output of the algorithm to add a proof is exactly identical to the input, with exception of the proof property. As an implementer, these steps would lead me to believe that the injected context is not part of securedDataDocument, and that verifying applications would just re-inject the same context instead.
This however is a really, really, really bad idea and will lead to spurious problems with verification and to interoperability issues that will be super hard to figure out.

I therefore suggest to adjust the Add Proof algorithm so that it explicitly considers context injection, e.g.:

  1. Let securedDataDocument be a copy of unsecuredDocument.
  2. Add a reference to the Data Integrity JSON-LD context file to the @context property of securedDataDocument, as described in 2.9.1 Context Injection.
  3. Let proof be the result of calling the createProof algorithm specified in cryptosuite.createProof with securedDataDocument and options passed as a parameters. If the algorithm produces an error, the error MUST be propagated and SHOULD convey the error type.
  4. If one or more of the proof.type, proof.verificationMethod, and proof.proofPurpose values is not set, an error MUST be raised and SHOULD convey an error type of PROOF_GENERATION_ERROR.
  5. If options has a non-null domain item, it MUST be equal to proof.domain or an error MUST be raised and SHOULD convey an error type of PROOF_GENERATION_ERROR.
  6. If options has a non-null challenge item, it MUST be equal to proof.challenge or an error MUST be raised and SHOULD convey an error type of PROOF_GENERATION_ERROR.
  7. Set securedDataDocument.proof to the value of proof.
  8. Return securedDataDocument as the secured data document.

4.4 Add Proof Set/Chain should then be adjusted accordingly.

@silverpill These measures would alleviate the need to prescribe in which variation the context is added, because JCS suites during verification already take care of 'resetting' the document @context to the state in which the proof was added. This, as far as I can see, should lead to reproducible proof values. But I still see value in being explicit what injection means by describing (in https://w3c.github.io/vc-data-integrity/#context-injection) how exactly the Data Integrity context is to be added to the @context property.

In addition, what could be considered is to require implementations to fail proof verification if the document does not contain required contexts, i.e., disallow injection on verification. This is to push all implementations to respect the steps described in 4.3 & 4.4. @msporny was there an explicit decision by the Editors of the spec to stick to the status quo of the spec, i.e., allow implementations to perform context injection on verification, or was this overlooked?
I hope my suggestions are sufficiently clear and straightforward, let me know if there is additional info or justification you'd need.

@msporny msporny reopened this Jul 1, 2024
@msporny
Copy link
Member

msporny commented Jul 1, 2024

I've re-opened this issue based on your request.

@msporny was there an explicit decision by the Editors of the spec to stick to the status quo of the spec, i.e., allow implementations to perform context injection on verification, or was this overlooked?

No, there wasn't an explicit decision. I wouldn't say it was overlooked either -- we were hesitant to provide too much guidance as we didn't want to make certain use cases impossible. That is, context injection tends to be a highly application-dependent thing, and it's only the application developer (operating within their community) that can probably pick the right approach.

That said, I agree with you, we should have better language in the spec around this and you've given the Editors enough to try to construct the right sort of language and put it into the spec. I would prefer that you write the PR so we get everything in there that you want (the Editor's can align/clean it up), but barring that, we will revise the context injection section to be more prescriptive (ideally, without accidentally limiting a use case or two w/ JCS).

@rflechtner
Copy link
Author

I would prefer that you write the PR so we get everything in there that you want (the Editor's can align/clean it up), but barring that, we will revise the context injection section to be more prescriptive (ideally, without accidentally limiting a use case or two w/ JCS).

I'm happy to draft a PR for this. Would you suggest I edit the context injection section as well, or shall I leave that part to you and your team?

@msporny
Copy link
Member

msporny commented Jul 2, 2024

I'm happy to draft a PR for this. Would you suggest I edit the context injection section as well, or shall I leave that part to you and your team?

Great, thanks for being willing to raise a PR! :)

Go ahead and raise a PR for the context injection section as well. It'll be helpful to review all the text in one pass.

@msporny msporny assigned rflechtner and unassigned msporny Jul 3, 2024
@rflechtner
Copy link
Author

@msporny I'm working on it, I have a draft ready and will raise a PR shortly. I'm also trying to catch up with the discussion in #272 because what's happening there seems related to what we're trying to fix here. Some of the assumptions made in that thread are simply not true for JCS-based proofs, which is why context injection is such an issue – for example, that the exact context values are not and should not be committed to, or that it is only the data that's secured, not the shape.

If there is a move to make adjustments to the way @context values are being treated, please let me know. This is probably a good time to point out again that the complications around context injection that require this clarification, as well as the recent related work we've done on https://www.w3.org/TR/vc-di-eddsa/ can all be traced back to a single source: the assumption that @context values are not 'data' and thus can be altered without consequences. From the POV of a JCS based proof, the data to be secured is changed by adding a proof, which results in an unhealthy tight coupling between a document and proof. It also messes with the strategy offered to JSON-LD sceptics, which as far as I understood consists in hard-coding a set and order of expected contexts on a document - a solution that is much harder to implement if the set of contexts and their order depends on which proof types are added in which order.

For the record, if this specification was being designed in a 'vacuum', that is, without having to consider other existing solutions and implementations, I would strongly advise to abandon context injection altogether and enforce that a document is not altered in any way by adding a proof, with exception of it's proof property of course. From where I'm standing, context injection seems like the single most limiting and unfortunate feature of this specification. I'm bringing this up in case there are any larger changes in how @context values are approached, in which case it may be worth re-evaluating context injection.

@msporny
Copy link
Member

msporny commented Jul 5, 2024

I would strongly advise to abandon context injection altogether

This is still an option we can take. One of the strongest reasons for context injection was people that didn't like Linked Data asserting that they didn't want @context at all in their documents... and then complaining that for the cases where they did want @context, that they wanted to be able to inject the context. Those people are gone from the group now (for the most part), so let's go back to first principles and discuss whether context injection is a good idea at all, or if we want to just not say anything about it.

We have a few weeks to figure this out and lock it in (with multiple implementations). It's unfortunate that we're discussing this at this point in the process, but I agree, it's a feature that seems to be causing more problems than it is solving.

@msporny
Copy link
Member

msporny commented Jul 5, 2024

Some of the assumptions made in that thread are simply not true for JCS-based proofs

Yes, that's true. Most of the usage of DI tends to be the RDFC version of it, and not the JCS version of it... that said, the JCS version of it does change (or eliminate) some of the arguments in that thread. It's not lost on the Editors of the cryptosuites, not all of the people discussing in that thread have a full depth of knowledge to engage on both types of canonicalization that we have today and reason through the ramifications of each approach. I just wanted to note that we do see your point, and you're right. I'm not sure adding JCS to the discussion will help, though, as it makes the discussion even more nuanced (and doesn't really address the core of the issue, which is the debate around whether a verifier can process inputs that are unknown to them).

@rflechtner
Copy link
Author

I'm not sure adding JCS to the discussion will help

Yeah I was hesitant to raise this there given that the discussion is already quite heated, but wanted to bring it up nevertheless because I having growing concerns w/r/ to context injection and worry that if we do not tackle this we may be reinforcing the commitment to a feature (or quirk?) that has the potential to do real harm to the prospects of DI and ultimately the VC space.

let's go back to first principles and discuss whether context injection is a good idea at all

Very happy to do that and happy to provide arguments against it - do you reckon we should open up a new issue for that? Given that time is of the essence, let me know how you think we can most efficiently move this forward, I'm happy to support in any way I can.

One of the strongest reasons for context injection was people that didn't like Linked Data asserting that they didn't want @context at all in their document

I would even argue that context injection is doing a disservice to those people. Abandoning context injection would give us a solution where Linked Data/JSON-LD truly becomes an add-on feature where you can have

  • A pure JSON document secured by a JCS proof, with no JSON-LD processing happening whatsoever
  • A document with a @context that partially or fully maps its terms to Linked Data secured by a JCS proof
  • A fully mapped JSON-LD document secured by an RDF-canonicalization based proof

This greatly reduces the barrier for entry and would most likely help adoption of this specification. I am also convinced that allowing the incremental addition of Linked Data semantics and features is a better path to warming people up to Linked Data than a 'learn JSON-LD or find something else' attitude that I've seen some express in the discussion. JSON-LD has some complex interactions, and as the specification is set up right now, it's so tightly coupled to Linked Data that I would not recommend those who don't have a good understanding of JSON-LD to try to implement it. In fact, even though I'd say I've had quite a bit of exposure to JSON-LD I'm still not sure how I'd go about making sure that a JSON-LD document secured by a JCS proof can be safely consumed by applications using an LD processor.

We have a few weeks to figure this out and lock it in (with multiple implementations). It's unfortunate that we're discussing this at this point in the process, but I agree, it's a feature that seems to be causing more problems than it is solving.

It is certainly unfortunate that we've put so much effort in workarounds, but it was also a process of uncovering the extent of the implications that context injection really has. And I expect that dropping it would both lower cognitive as well as implementation complexity, so I'm not too worried about producing conformant implementations in time. In fact, there's going to be some work to do on the implementation front no matter if we abandon context injection or not. I've for example discovered, while trying to implement the workaround we've come up with for JCS signature suites, that both digitalbazaar/data-integrity and digitalbazaar/jsonld-signatures would require changes to support it.

@msporny
Copy link
Member

msporny commented Jul 5, 2024

Very happy to do that and happy to provide arguments against it - do you reckon we should open up a new issue for that?

I have raised #281 to track the concern.

Please copy what you feel is appropriate over to that new issue and we can continue the discussion there.

@msporny
Copy link
Member

msporny commented Jul 22, 2024

@silverpill and @rflechtner would either of you object to closing this issue and continuing the discussion in #281 -- as an Editor, I'm being asked to clean up any duplicated issues. I know we need to deal w/ the context injection questions, but that seems to be better tracked in #281 than here. I'm marking this as "pending close in 7 days" for now.

@msporny msporny added the pending close (7 days) This issue will be closed after 7 days. label Jul 22, 2024
@silverpill
Copy link

No objections from me.
I think not having to inject @context would be great.

@rflechtner
Copy link
Author

@silverpill and @rflechtner would either of you object to closing this issue and continuing the discussion in #281 -- as an Editor, I'm being asked to clean up any duplicated issues. I know we need to deal w/ the context injection questions, but that seems to be better tracked in #281 than here. I'm marking this as "pending close in 7 days" for now.

Yes, happy to close this, as we seem to lean towards completely rethinking context injection - for which #281 is more appropriately framed.

@rflechtner rflechtner closed this as not planned Won't fix, can't repro, duplicate, stale Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR1 This item was processed during the first Candidate Recommendation phase. normative This issue or PR will trigger the need for another Candidate Recommendation Snapshot pending close (7 days) This issue will be closed after 7 days.
Projects
None yet
Development

No branches or pull requests

4 participants