digitalbazaar / data-integrity

Data Integrity Proof library for use with jsonld-signatures.
BSD 3-Clause "New" or "Revised" License
2 stars 1 forks source link

Support for jcs-based proofs according to specification #19

Open rflechtner opened 11 months ago

rflechtner commented 11 months ago

Note: I'm opening an issue here as there seems to be a plan to phase out https://github.com/digitalbazaar/jsonld-signatures/issues/178, but the problem we're facing concerns both libraries.

After having fixed an issue with jcs-based proofs not verifying properly in jsonld-signatures a few months back (https://github.com/digitalbazaar/jsonld-signatures/issues/176), I want to circle back to the issue of adding context definitions to the document and proof before signature generation. My concern is that the current praxis of altering @context properties during proof generation and verification may simply not be viable for proof mechanisms that directly sign the json representation of linked data. Specifically, I see two issues:

1) Adding a proof (suite) specific context to the document can invalidate pre-existing proofs. This praxis is adopted both in this library as well as in jsonld-signatures. Adding, for the purpose of supporting a wider range of verifiers, a Ed25519Signature2020 to a document that has already been signed with a DataIntegrityProof using eddsa-jcs-2022 will alter the document, after which the eddsa-jcs-2022 suite will no longer verify the existing proof.

2) Adding the document context prior to signing and verification, as done in this library and, inconsistently (only on verification), in jsonld-signatures seems to be at odds with the prescribed proof configuration of existing jcs based cryptosuite specifications authored by Digital Bazaar (https://www.w3.org/TR/vc-di-eddsa/#eddsa-jcs-2022 & https://www.w3.org/TR/vc-di-ecdsa/#proof-configuration-ecdsa-jcs-2019). Even though this behaviour can be patched for digitalbazaar/data-integrity by reimplementing createVerifyData in your cryptosuite, jsonld-signature's behaviour will break verification in this case, which leads into a dilemma where implementing the specs correctly actually harms interoperability between implementations.

I'm hoping to start a thread here to discuss improvements that can be made to the way this is handled. To kick this off, I can throw in two suggestions:

msporny commented 11 months ago

@rflechtner hmm, yes, you're right. I'm just commenting to confirm that we need to address this issue as it makes it difficult to use JCS-based cryptosuites with the RDFC-based cryptosuites (which was a design goal).

I expect that using DataIntegrityProof for everything would solve the issue for VCs... but that's a point solution that we should avoid... and it doesn't work if you want to mix in stuff like Ed25519Signature2020.

We could give advice like: If you're going to use JCS-based and RDFC-based cryptosuites on the same payload, do your signatures using the RDFC-based cryptosuites first (which would do any context injection necessary) and then the JCS-based cryptosuites (which would just sign over the injected contexts)... but that feels like brittle advice.

Or, perhaps the guidance should be: "Use DataIntegrityProof and don't inject contexts" -- perhaps injecting cryptosuite contexts should be an anti-pattern if you want to mix canonicalization mechanisms?

Embedding the context values in JCS proofs feels brittle as well... special processing for a cryptosuite used by people that tend to not want to deal w/ @context values.

We'll have to think about what the right path forward here is... it's not immediately obvious given the various other forms of cryptosuites that are in the works (like the one for Data Integrity-based CL Signatures).

rflechtner commented 11 months ago

@msporny I agree we should aim for a robust solution rather than patch the problem with brittle advice, even if that takes a bit more investment - if we spare that effort now it will most certainly come back to haunt us further down the line. Happy to contribute here in any way I can!

I do think we should give this option some thought: making context injection an internal feature of linked data proofs, such that documents are no longer edited in any way during signing. I get that some do not like the idea of having a separate @context property on a subsection of the document (i.e., the proof), but biting that bullet as far as I can see allows linked data proofs to continue to achieve the goals for which context injection has been introduced (making sure all proof properties are defined and cannot be redefined) while not interfering with proofs that do not need and cannot properly handle context injection.

What's your opinion on that direction, does this seem feasible?

rflechtner commented 10 months ago

Hi @msporny, any updates on this? Also wondering if this thread should move to a different forum (e.g., https://github.com/w3c/vc-data-integrity/issues) and if the topic warrants discussion in the relevant DIF WG?

msporny commented 10 months ago

Hi @msporny, any updates on this?

Yes, I discussed this with @dlongley briefly, and on a couple of internal calls. There was a solution proposed that seems like it would work, but it slips my mind on what it was.

if the topic warrants discussion in the relevant DIF WG?

Which DIF WG were you thinking of? I didn't think that DIF worked on Data Integrity-related items?

Also wondering if this thread should move to a different forum (e.g., https://github.com/w3c/vc-data-integrity/issues)

Yes, let's move the discussion to the vc-data-integrity repository... I want to get the others implementing the JCS version of Data Integrity involved. Whatever we do will need buy in from all of the implementers.

Apologies for the delay in response... we are heads-down right now in the W3C VCWG trying to get all the Data Integrity specifications and cryptosuites across the global standards line and so much of our focus is there. There are only around 6 months left and still lots of work to do to ensure there are enough implementations of the specification.

I will also note that the JCS implementations are dragging and we'll have to remove those features from the specifications if we don't have more demonstration of implementations (and test suites).

All that said, yes, please, raise an issue so we can officially bring it to the VCWG's attention.

rflechtner commented 10 months ago

Which DIF WG were you thinking of? I didn't think that DIF worked on Data Integrity-related items?

Yup, sorry, that was a brain-glitch, don't think they do. I was thinking of the W3C VCWG.

I will also note that the JCS implementations are dragging and we'll have to remove those features from the specifications if we don't have more demonstration of implementations (and test suites).

We've been working on an implementation of a eddsa-jcs-2022 suite in typescript, ready for use with @digitalbazaar/data-integrity, which we plan to release & open source in the next couple of weeks. Would it help to include this in the implementation report?

All that said, yes, please, raise an issue so we can officially bring it to the VCWG's attention.

Will do, and I'll ping Dave in the hopes that he remembers details about the solution you came up with. Thanks!

dlongley commented 10 months ago

I believe the solution was, for just the JCS cryptosuite proofs, to include the context from the document (and any JCS cryptosuite context, if necessary) in the JCS cryptosuite proof object itself (proof: [{"@context": "<here>", cryptosuite: "eddsa-jcs-2022", ...}, ...]) -- and update the algorithm steps for JCS cryptosuites to use it to override the document's context prior to signing or verifying.

This would avoid disturbing other cryptosuites that do not have this issue that rely on using the top-level @context and have done so for quite some time. Changing that around for every cryptosuite in an effort to be consistent / add additional separation starts messing with other requirements and usage patterns that we don't have time or resources to fully investigate.

msporny commented 10 months ago

We've been working on an implementation of a eddsa-jcs-2022 suite in typescript, ready for use with @digitalbazaar/data-integrity, which we plan to release & open source in the next couple of weeks. Would it help to include this in the implementation report?

Yes, it would help a bunch. Thank you! We plan on also doing an implementation, as it's fairly trivial given that we have the other cryptosuites already implemented and we're just talking about swapping out the canonicalization algorithm. We need a minimum of two independent implementations (yours and ours would meet that bar)... there are two other implementers that we know of that have promised implementations as well.

One caveat is that you'll have to set up a server (or Docker image) that exposes an API to "issue" and "verify" so we can run the W3C VCWG tests against your software:

https://github.com/w3c/vc-di-ecdsa-test-suite#implementation

Do you think you'd be able to do that in the next 3-5 months?

rflechtner commented 10 months ago

I believe the solution was, for just the JCS cryptosuite proofs, to include the context from the document (and any JCS cryptosuite context, if necessary) in the JCS cryptosuite proof object itself (proof: [{"@context": "<here>", cryptosuite: "eddsa-jcs-2022", ...}, ...]) -- and update the algorithm steps for JCS cryptosuites to use it to override the document's context prior to signing or verifying.

Thanks for clarifying! This seems like a reasonable compromise so far. I'll have to take some time to wrap my head around how this must be done to maintain integrity protection for the context values and hence for the semantics of the secured document, but I'll open an issue including your suggestion tomorrow. Feel free to correct me where I'm misrepresenting your ideas. I'll also make sure to mention the constraint that we can't break existing rdf based suites and implementations.

rflechtner commented 10 months ago

Do you think you'd be able to do that in the next 3-5 months?

I'm confident that we can do that, yes. Depends a little bit on what the scope is; our cryptosuite implementation also works as a drop-in replacement for other suites used with your library, which, as you mentioned should be fairly trivial to do.

If we should rather provide a solution that works standalone without relying on any of your code, we're pretty close to that too - we've got a working implementation of at least basic signing and verification.

msporny commented 10 months ago

Do you think you'd be able to do that in the next 3-5 months?

I'm confident that we can do that, yes.

Great!

our cryptosuite implementation also works as a drop-in replacement for other suites used with your library, which, as you mentioned should be fairly trivial to do.

I believe that as long as the cryptosuite is implemented by you, it should be fine... however,

If we should rather provide a solution that works standalone without relying on any of your code, we're pretty close to that too - we've got a working implementation of at least basic signing and verification.

This option would be better. I know it's a taller ask, so if you're able to do it, wonderful -- that's fully defensible in the WG as a completely independent implementation. If not, there might be some individuals that object because it's not a completely independent implementation (which is debatable, but they'd certainly debate the "it doesn't count unless is a full top-to-bottom implementation).

Any contribution along the lines above will help us get over the finish line, so thank you!

rflechtner commented 10 months ago

Any contribution along the lines above will help us get over the finish line, so thank you!

@msporny What we would have to align on though is how to handle the injection of the document context on the proof (I'm talking about this line: https://github.com/digitalbazaar/data-integrity/blob/674d5c142b95aa6825349b3bd4e3e31f86154858/lib/DataIntegrityProof.js#L371), which the eddsa-jcs-2022 specs says should not be done. Do you plan to change the library's approach to creating proof data so that it accommodates cryptosuites that do not do this or do this differently? Or should we reimplement createVerifyData on our jcs suite, which from what I can see replaces the default behaviour?

msporny commented 10 months ago

@msporny What we would have to align on though is how to handle the injection of the document context on the proof (I'm talking about this line:

https://github.com/digitalbazaar/data-integrity/blob/674d5c142b95aa6825349b3bd4e3e31f86154858/lib/DataIntegrityProof.js#L371

), which the eddsa-jcs-2022 specs says should not be done.

At this point, we should presume the eddsa-jcs-2022 spec is wrong and we should fix it.

Do you plan to change the library's approach to creating proof data so that it accommodates cryptosuites that do not do this or do this differently? Or should we reimplement createVerifyData on our jcs suite, which from what I can see replaces the default behaviour?

I expect it's the latter, but don't have time in the next week or two to do a deep dive. I'm requesting an opinion from @dlongley and @davidlehn in this thread, which we might want to document here as well:

https://github.com/w3c/vc-data-integrity/issues/225

I'm heads-down trying to get VC v2.0 to the W3C Candidate Recommendation (CR) phase (we're behind schedule by ~2-3 months there). After that is done, I have to circle back around to get BitStringStatusList to CR, and then I'll circle back to the Data Integrity suites to finish them up so we can go through a 2nd CR phase (we'll be forced to do this due to the issue above and because of requests for changes from Google).

That doesn't mean that implementers (such as you, @dlongley, @davidlehn, @HelgeKrueger, and @silverpill) shouldn't figure out the right way to do this and try it out in your implementations. Please do that so that when I get around to writing the text, there is consensus among the implementers on what should be done here.

silverpill commented 10 months ago

@msporny I don't quite understand the problem being discussed here. It seems to occur when proofs based on RDFC and JCS are used together in one document? In our case only JCS is used

At this point, we should presume the eddsa-jcs-2022 spec is wrong and we should fix it.

As far as I can tell, the proposed solution is injection of @context into document before canonicalization, but it is not backwards compatible. eddsa-jcs-2022 has been implemented by several projects and used in production in decentralized network, so it's too late for breaking changes. If this change is really needed, it should result in a change of cryptosuite name (e.g. eddsa-jcs-2023).

msporny commented 10 months ago

If this change is really needed, it should result in a change of cryptosuite name (e.g. eddsa-jcs-2023).

Yeah, we can do that to ensure we don't break production deployments. Could you please link to/list the projects that are using it in production -- it helps us make the case about the uptake of this technology outside of W3C Member companies.

silverpill commented 10 months ago

eddsa-jcs-2022 is a recommended cryptosuite in FEP-8b32, which (to my knowledge) is supported by Fediverse projects Mitra (Rust), Bovine (Python) and Vervis (Haskell). One more project is in the process of implementing it (Streams, PHP). Two or three others have expressed interest in this proposal.

I don't have data on usage in the wild. At least one implementation (Mitra) already produces and distributes signed objects containing eddsa-jcs-2022 proofs.

msporny commented 10 months ago

eddsa-jcs-2022 is a recommended cryptosuite in FEP-8b32, which (to my knowledge) is supported by Fediverse projects Mitra (Rust), Bovine (Python) and Vervis (Haskell). One more project is in the process of implementing it (Streams, PHP). Two or three others have expressed interest in this proposal.

Ok, thanks for the update. That's useful information that we can take back to the WG. There are clearly more than two independent implementations at this point, so all we need to do is prove that (via the test suite) and that'll ensure that the feature doesn't get cut during the W3C Candidate Recommendation phase.

We will absolutely need your help to prove that, though. If Mitra, Bovine, Vervis, Streams, and the others don't submit endpoints or Docker images proving that they pass the test suites, there are some in the group that will attempt to cut the feature.

I don't have data on usage in the wild. At least one implementation (Mitra) already produces and distributes signed objects containing eddsa-jcs-2022 proofs.

Looks like there is some data here? I don't know if that is counting the right thing, though:

https://mitra.fediverse.observer/list

One option here is to update Mitra to check signatures both ways... W3C counts "significant deployments" in the "tens of thousands to millions of people affected". I can see WG members noting that the deployment isn't significant yet and so breaking changes, especially because it's not a standard yet, is ok. That said, we don't want to cause you pain if we can avoid it. Can you see any other way of updating eddsa-jcs-2022 to fix this concern?

dlongley commented 9 months ago

Perhaps the existing implementations could be updated to allow both older proofs using eddsa-jcs-2022 that do not include @context in proof (which is what would become non-spec-compliant) but also support eddsa-jcs-2022 proofs that have @context in them (which is what we currently think the spec would change to require).

silverpill commented 9 months ago

We will absolutely need your help to prove that, though. If Mitra, Bovine, Vervis, Streams, and the others don't submit endpoints or Docker images proving that they pass the test suites, there are some in the group that will attempt to cut the feature.

@msporny If you're referring to EdDSA Cryptosuite test suite, I think it is unlikely that any of us will make an effort to pass it. The mentioned projects are social web servers, there's no reason for them to implement VC API.

If WG will try to remove eddsa-jcs-2022, we'll fork the vc-di-eddsa spec or consider using a different standard.

Can you see any other way of updating eddsa-jcs-2022 to fix this concern?

I have no control over existing deployments. A breaking change at this point would lead to a lot of coordination problems, so it should be avoided. Also, injecting @context could be too complicated for implementations that work with plain JSON (the majority of Fediverse software).

rflechtner commented 9 months ago

Perhaps the existing implementations could be updated to allow both older proofs using eddsa-jcs-2022 that do not include @context in proof (which is what would become non-spec-compliant) but also support eddsa-jcs-2022 proofs that have @context in them (which is what we currently think the spec would change to require).

I can see that adding a conditional rule to the spec could fix the issue while not breaking existing applications. E.g., if we say that when creating a proof, the document SHOULD be added to the proof. However, during issuance, we require updating the document context only if the proof has a @context property.

A remark on the concern by @silverpill:

Also, injecting @context could be too complicated for implementations that work with plain JSON (the majority of Fediverse software).

The data integrity specifications mandate context injection, so if they are implementing them to the letter, this should already be happening. If not, it can't be guaranteed anyway that other implementation would be able to verify the proofs created. It also sounds more complicated than it is; in the end, it is just operating on a set of strings. Context injection for example is adding a fixed string if it's not already in the set, and what we are proposing as a solution is comparing sets of strings and overriding the set if necessary.

msporny commented 9 months ago

@msporny If you're referring to EdDSA Cryptosuite test suite, I think it is unlikely that any of us will make an effort to pass it. The mentioned projects are social web servers, there's no reason for them to implement VC API.

That's really unfortunate, I wish you would have conveyed that before we put in the effort to get it spec'd and adopted by the group. If our primary group of implementers aren't going to make an effort to demonstrate that they're conformant with the specification the WG will almost certainly remove it from the specification.

At this point, we're going to have to depend on a different group of implementers to integrate w/ the test suite and demonstrate conformance (which is a requirement for all global standards produced by the W3C -- we have to demonstrate two independent interoperable implementations).

Integrating w/ the test suite isn't difficult, you just put up an endpoint that can digitally sign a payload and verify a payload. Are you saying that the fediverse community won't even put in that level of effort? I guess the same questions goes out to @HelgeKrueger and @rflechtner? At this point, I believe Digital Bazaar will submit an implementation for testing via the test suite... at a minimum, we need at least one more independent implementation.

silverpill commented 9 months ago

The data integrity specifications mandate context injection, so if they are implementing them to the letter, this should already be happening. If not, it can't be guaranteed anyway that other implementation would be able to verify the proofs created. It also sounds more complicated than it is; in the end, it is just operating on a set of strings. Context injection for example is adding a fixed string if it's not already in the set, and what we are proposing as a solution is comparing sets of strings and overriding the set if necessary.

@rflechtner Is it against the spec to sign JSON objects without the @context property? I didn't know about that.

In the following example, should object have @context in order to have a valid proof?

{
  "@context": [
    "https://www.w3.org/ns/activitystreams",
    "https://w3id.org/security/data-integrity/v2"
  ],
  "object": {
    "content": "test",
    "proof": {
      "type": "DataIntegrityProof",
      "cryptosuite": "eddsa-jcs-2022",
      ...
    }
  }
}

Integrating w/ the test suite isn't difficult, you just put up an endpoint that can digitally sign a payload and verify a payload. Are you saying that the fediverse community won't even put in that level of effort?

@msporny If it's really that simple I think somebody could do it. When do you expect the group to reach a decision on eddsa-jcs-2022 cryptosuite?

Also, why the test suite requires VC API? Data Integrity standard has many use cases beyond Verifiable Credentials, I'm surprised that such requirement has been introduced.

HelgeKrueger commented 9 months ago

@msporny Note: I haven't read through the details about what is being modified with eddsa-jcs-2022. I'm just responding about the test suite.

Integrating w/ the test suite isn't difficult, you just put up an endpoint that can digitally sign a payload and verify a payload. Are you saying that the fediverse community won't even put in that level of effort? I guess the same questions goes out to @HelgeKrueger and @rflechtner?

I'm planning to implement the test suite. However, when I first looked at https://github.com/w3c/vc-di-eddsa-test-suite/, I was confused, so it got low priority. The explanation is easier to understand now. Just to clarify what is needed.

{
  "name": "My Company",
  "implementation": "My implementation",
  "issuers": [{
    "id": "",
    "endpoint": "https://test.example/issue",
    "method": "POST",
    "tags": ["eddsa-jcs-2022"]
  }],
  "verifiers": [{
    "id": "",
    "endpoint": "https://test.example/verify",
    "method": "POST",
    "tags": ["eddsa-jcs-2022"]
  }]
}
rflechtner commented 9 months ago

Answering first to @silverpill:

@rflechtner Is it against the spec to sign JSON objects without the @context property? I didn't know about that.

Yes and no. Signer implementations are expected to add the @context property if it does not yet exist, so while you can sign a document that does not have it, the resulting secured document MUST have a @context. See https://www.w3.org/TR/vc-data-integrity/#context-injection

In the following example, should object have @context in order to have a valid proof?

That's a more subtle issue, because it depends on what the 'secured document' is here. Are you signing over the whole structure, or only over the contents of object?

The way I see it, if you are doing the former, this may be a valid implementation, although it would be first time I see that the proof property is added to a nested object. I can't tell if the proof being a top-level property is a hard requirement, what is your take on this @dlongley & @msporny? I can say though that many implementations will assume the proof to be present in the root of the document, so this would hurt interoperability.

The latter implementation however would be more problematic, because it would mean that the @context values are not secured and can be modified after signing, changing the semantics of the document. For that reason, and because the context is not part of what I would consider the secured document, this implementation would be deficient in my eyes.

rflechtner commented 9 months ago

I guess the same questions goes out to @HelgeKrueger and @rflechtner?

As discussed, we're happy to provide an integration with the test suite at the very least for our cryptosuite implementation, using @digitalbazaar/data-integrity as glue code. I can't say how likely it is that we get a completely independent implementation to work with the test suite at this point, I'd have to take a deeper look at the assumptions made there first. For example, controller documents in our system are DL-based DID documents; and while they can be resolved through http gateways like https://dev.uniresolver.io I suspect this could give us a bit of a headache.

msporny commented 9 months ago

@HelgeKrueger wrote:

The explanation is easier to understand now. Just to clarify what is needed.

Yes, that's more or less accurate. You don't need to do the controller document if you use a did:key, which I expect you will... a did:key is just a byte header with the raw public key bytes of a compressed ECDSA or EdDSA key appended to it and base58 encoded. We have code that can generate one for you if necessary, so that part should be simpler than what you wrote above.

@BigBlueHat, @JSAssassin, @aljones15 can help you through the details when the time comes to integrate... they're the ones that are putting together the test suite.

I'm planning to implement the test suite. However, when I first looked at https://github.com/w3c/vc-di-eddsa-test-suite/, I was confused, so it got low priority.

@BigBlueHat, @JSAssassin, @aljones15, please note the sub-par experience that @HelgeKrueger had wrt. integrating w/ the test suites. I agree that the documentation on how to do it is still quite difficult for implementers to understand, and it sounds like we've already lost one implementer because we've made it seem more difficult than it is to implement... and given how precious each implementer/implementation is for the standards ratification process, we need to improve things from where they are right now.

msporny commented 9 months ago

@HelgeKrueger wrote:

I'm planning to implement the test suite.

@rflechtner wrote:

As discussed, we're happy to provide an integration with the test suite ... for our cryptosuite implementation

Excellent, then we're back in business! That means there will most likely be 3 independent implementations, and all we need is two.

msporny commented 9 months ago

@rflechtner wrote:

I can't say how likely it is that we get a completely independent implementation to work with the test suite at this point

A completely independent implementation would be a "nice to have", not mandatory. The DID lookups you're doing now are not a requirement for the cryptosuite implementation; we have many other implementations that are implementing that portion of the specification. It's the JCS portion that we're lacking implementers on.

The way I see it, if you are doing the former, this may be a valid implementation, although it would be first time I see that the proof property is added to a nested object. I can't tell if the proof being a top-level property is a hard requirement, what is your take on this @dlongley & @msporny?

proof is not required to be a top-level property, but as you said, then only the sub-object is secured. That's a perfectly fine thing to do, but as a warning, that might lead developers to think that the entire document is secured when it is definitely not. Those kinds of misalignments lead to security failures, usually really bad ones, so perhaps the specification should say something about this.

We don't want to insist that proof is always at the top level because there are legitimate reasons for it not to be, such as an API result in JSON from a "System A" that can be trusted (because the API result came from "System A" over TLS), but the result contains information that another system, System B, asserted. The information from System B, which would be nested in System A's response, would need to be digitally signed to be trusted (to ensure that System A isn't lying about something System B said).

You could also argue that System A should digitally sign its payload as well, which would include a signature over System B's payload (so you could trust the outer payload AND the inner payload).

Talking about that sort of thing should probably be placed in the Security Considerations section, as talking about application-layer details a bit "out of scope" for the specification... but it's worth mentioning. Would one of you please raise an issue if you think this is worth noting in the specification?

I can say though that many implementations will assume the proof to be present in the root of the document, so this would hurt interoperability.

Yes, this is true. The algorithms are written such that if the proof isn't at the top-level of the object passed into verification, that verification will fail.

It is entirely possible that much of the above isn't easy to glean from the current specification, so if we need to add/update spec text, please raise an issue against the specification so we can fix this.

silverpill commented 9 months ago

That's a more subtle issue, because it depends on what the 'secured document' is here. Are you signing over the whole structure, or only over the contents of object?

@rflechtner In the example above I'm signing only the contents of object. Top-level structure remains unsecured.

The latter implementation however would be more problematic, because it would mean that the @context values are not secured and can be modified after signing, changing the semantics of the document. For that reason, and because the context is not part of what I would consider the secured document, this implementation would be deficient in my eyes.

I would accept this risk. The majority of consumers are expected to be JSON processors, not aware of JSON-LD.

In addition to that example, my software needs to support the following use cases:

https://www.w3.org/TR/vc-data-integrity/#context-injection says

If an @context property is not provided in a document that is being secured or verified, or the Data Integrity terms used in the document are not mapped by existing values in the @context property, implementations MUST inject or add an @context property with a value of https://w3id.org/security/data-integrity/v2.

but it is not entirely clear when and how to inject it. If context injection is a required part of the proof generation, it probably should be specified as an optional (?) step in https://www.w3.org/TR/vc-di-eddsa/#algorithms

HelgeKrueger commented 9 months ago

@BigBlueHat, @JSAssassin, @aljones15, @msporny

Yes, that's more or less accurate. You don't need to do the controller document if you use a did:key, which I expect you will... a did:key is just a byte header with the raw public key bytes of a compressed ECDSA or EdDSA key appended to it and base58 encoded. We have code that can generate one for you if necessary, so that part should be simpler than what you wrote above.

I think the use did:key is an excellent hint to simplify implementing the test suite.

Assuming did:key, one can both avoid the controller document and the parsing logic for controller documents. This means one is back to only testing the signing/verifying algorithms (as far as I can tell now).

My experience from the Fediverse is: Resolving controller documents is hard. In the Fediverse the controller is called an actor and the public key is in the now deprecated publicKey property. This tool, I build, helps ensure that people do it correctly.

msporny commented 9 months ago

@HelgeKrueger wrote:

I think the use did:key is an excellent hint to simplify implementing the test suite. Assuming did:key, one can both avoid the controller document and the parsing logic for controller documents.

Yes, exactly. That is why we only require did:key for the test suites.

This means one is back to only testing the signing/verifying algorithms (as far as I can tell now).

Yes, correct. We're trying to balance at least two things:

  1. Making it as easy as possible for implementers to integrate with the test suite.
  2. Ensuring that the test suite is run on a regular basis (e.g., at least weekly) against all the implementations that we know about.

My experience from the Fediverse is: Resolving controller documents is hard. In the Fediverse the controller is called an actor and the public key is in the now deprecated publicKey property. This tool, I build, helps ensure that people do it correctly.

Excellent! Thank you for writing that tool. We are attempting to write similar testing tools for the W3C Verifiable Credentials ecosystem, and our hope is that some of these tools w/ overlap w/ Fediverse needs.

As a related aside, this link is 404'ing, and I was hoping to see a list of conforming implementations there: https://funfedi.dev/support_tables/verify/ ... We define "Controller Documents" in https://www.w3.org/TR/vc-data-integrity/#controller-documents now, and we're thinking of moving the concept of "Actor/Controller Documents into their own standard at W3C: https://w3c.github.io/vc-controller-document/ (but may not be able to finish that work off during this round of the official Verifiable Credentials Working Group).

rflechtner commented 9 months ago

@rflechtner In the example above I'm signing only the contents of object. Top-level structure remains unsecured.

I think we all agree on one thing here: the 'secured document', that is, the structure that is secured by a proof, is the one that has the proof property. I also see that there are valid reasons to nest this secured document within a larger structure and have no qualms with that. The implication as I read it is that the @context property in the top level is irrelevant here, because a @context property MUST be added, explicitly or implicitly, before signing and verification. By that I mean: Even if it is not present on the secured document, the specification calls for you to add it during the signing process, so that what's actually being signed looks like this:

{
    "@context": [
      "https://w3id.org/security/data-integrity/v2"
    ],
    "content": "test",
    "proof": {
      "type": "DataIntegrityProof",
      "cryptosuite": "eddsa-jcs-2022",
      ...
     }
}

In turn, if you sent the document from your example to me, I would be adding the @context property to the secured document during verification, so that I could only successfully verify your proof if you had implicitly applied the transformation to the document above during signing.

I would accept this risk. The majority of consumers are expected to be JSON processors, not aware of JSON-LD.

The VC data integrity specifications have been designed with interoperability between jcs & rdfc based proof methods in mind; as @dlongley stated initially, this was an explicit design goal. We miss this goal if consumers of a signed piece of data cannot be sure that what they are presented with matches that which the issuer had originally signed. Be aware that changing the mapping provided by the context file can completely transform the meaning of this data in the eyes of a linked data processing application, so some mechanism needs to be in place to prevent this.

While this mechanism might not be particularly important for your use case, I think it is necessary to make this specification a success, as not having it would introduce nasty gotchas to this method of authenticating third party supplied data. And as outlined above, choosing not to implement it would mean that no other implementation would be able to verify the proofs you generated, which defeats the purpose of agreeing on a specification.

In addition to that example, my software needs to support the following use cases:

  • Both top-level structure and embedded object are signed.
  • Signing a JSON document without @context property.

The first should be no issue as far as I can see; simply sign the embedded object first, and the top-level structure second. Since we've established that a proof property concerns the object in which it is embedded, the mapping of which proof pertains to which secured document is unambiguous. Signing a JSON document without a @context property is possible as long the implementation adds a @context property containing [ "https://w3id.org/security/data-integrity/v2" ] in the process of signing. Ideally, this is done so that the modified document is returned by this implementation, as the property only existing for signing and verification algorithms, but never actually being visible to processing applications would be another confusing feature in the system.

If context injection is a required part of the proof generation, it probably should be specified as an optional (?) step in https://www.w3.org/TR/vc-di-eddsa/#algorithms

I agree that how to handle context injection is underdeveloped and needs to be made much more explicit in both the VC data integrity and the vc-di-eddsa specifications. That is why I opened issue https://github.com/w3c/vc-data-integrity/issues/225, but I can open another one that specifically asks to add language that clarifies when context injection should happen (e.g., do we really perform it on verification, or should we just error if the expected context isn't present?), and open a third on https://github.com/w3c/vc-di-eddsa/ asking to explicitly point out that this step needs to be performed.

silverpill commented 9 months ago

Thank you for this explanation @rflechtner ! I incorrectly assumed that injection is not needed if top-level structure already has @context.

Signing a JSON document without a @context property is possible as long the implementation adds a @context property containing [ "https://w3id.org/security/data-integrity/v2" ] in the process of signing.

Did you mean containing "https://w3id.org/security/data-integrity/v2", or it actually must be an array?

That is why I opened issue https://github.com/w3c/vc-data-integrity/issues/225, but I can open another one that specifically asks to add language that clarifies when context injection should happen (e.g., do we really perform it on verification, or should we just error if the expected context isn't present?), and open a third on https://github.com/w3c/vc-di-eddsa/ asking to explicitly point out that this step needs to be performed.

Yes, I'd appreciate if you open these additional issues

rflechtner commented 9 months ago

Did you mean containing "https://w3id.org/security/data-integrity/v2", or it actually must be an array?

That's one of the things that really should be properly defined in the specifications, especially if we treat adding the context on verification as allowable. Otherwise implementations may do this differently, resulting interoperability issues. I added this to the necessary clarifications in https://github.com/w3c/vc-data-integrity/issues/231

msporny commented 3 months ago

PRs w3c/vc-di-eddsa#79, w3c/vc-di-eddsa#80, w3c/vc-di-ecdsa#61, and w3c/vc-di-ecdsa#62 were raised and merged to address this issue (discussion happened in those issues).

This DB data-integrity library (really the cryptosuites) now need to implement this feature. I expect the libraries will fail the test suite until they properly implement the feature. I'm going to leave this open for now until we have a working set of cryptosuites that test successfully against the W3C test suite.

aljones15 commented 3 months ago

@HelgeKrueger sorry for the late response here, have you managed to get into the test suite yet? We are planning on updating the Data Integrity tests very soon. Are you part of the test suite office hours? We do have support for implementers. Anyways thanks for the feedback and I will try to further digest this thread.