w3c / vc-data-integrity

W3C Data Integrity Specification
https://w3c.github.io/vc-data-integrity/
Other
41 stars 18 forks source link

Error in the definition of the "unsecured document" #287

Closed iherman closed 1 month ago

iherman commented 2 months ago

(This issue comes from w3c/vc-data-model#1519, more exactly https://github.com/w3c/vc-data-model/pull/1519#discussion_r1667664777).

The term unsecuredDocument is defined in §4 Algorithms of the DI specification as follows:

An unsecured data document is a map with no proof key.

This definition goes wrong for the case when proofs are chained (and if the intention is to prove the previous proof entry of the chain). See again https://github.com/w3c/vc-data-model/pull/1519#discussion_r1667664777 for more details on the effects on cryptosuites.

Also, it is not immediately obvious what happens in the case of a Verifiable Presentation: in a VP, on the top level there is no proof key in the data, because all other instantiations of the proof key (or equivalent) appear only "behind" the verifiableCredentialkey, so to say. Which does mean that the unsecuredDocument does in fact include the proofs for the VCs in the Presentation, but it may be worth spelling this out.

I would also note that there is a duplication of definitions between this and the definitions in §5.12 Securing Mechanism Specifications (whether w3c/vc-data-model#1519 gets merged or not). This is a possible source of errors. I would propose we choose one of the sections for the formal definition, clean it up, and then refer to it from the other place.

cc @dlongley @msporny @TallTed

iherman commented 2 months ago

Note, on the choice of where we want to have the final definition:

This means that

  1. If we define the normative behavior v.a.v. chained proofs in §5.12, that will be valid for all cryptosuites.
  2. At the moment, cryptosuites may decide to ignore the concept of unsecureDocument in their specification, i.e., not to secure chained proofs.

I.e, if we think we want to keep it open for cryptosuites (i.e., keep (2)) then the formal definition should not be in §5.12 but, rather, in §4, and a remark should be added in §5.12 making it clear that cryptosuites may choose something else than what is in §4.

I must admit, I do not know whether we really want to maintain (2). Is there really a use case? I would think that having an identical behavior for all cryptosuites in this respect would be a plus. But that is just a personal feeling, to an opinion based on use cases.

iherman commented 2 months ago

(I just realized that this error is in fact a discrepancy between the DI and the VCDM documents, although it grew out of a discussion on the VCDM repo. We may want to move this to the other repo, but whatever we decide to do, it will probably affect both documents...)

iherman commented 2 months ago

I have to conclude that I was wrong. The term unsecuredDocument, the way it is used, seems to be correct, although it does make your mind work for Verifiable Presentations. The case of proof chains and set is taken care of (I believe) in the separate section which, I hope, is correct.

Referring back to the VCDM section is probably a no no, because the intention of the DI spec is to work for JSON-LD at large, and not only for VCDM...

Propose closing.

@msporny w.d.y.t.?

msporny commented 2 months ago

@iherman wrote:

Propose closing.

Let's keep it open until I'm able to review the language. When it was written, I was probably not thinking of documents that legitimately already contain proofs (VPs, or proof chains). A minor language fix/update might be useful.

iherman commented 2 months ago

In any case, I move this issue to the DI issue list. The real question is how the unsecure document is defined and used, which is not a VCDM term. My bad.

iherman commented 2 months ago

So here is where I still see a problem:

cc @dlongley @msporny

(B.t.w., a bit of editorial issue with §4.2: shouldn't the description say that this algorithm has to be run incrementally with all elements on the chain? The algorithm only defines how I add one extra proof entry to a credential, resulting in a credential with, say, two proofs; then the algorithm has to be run again for a third proof, if applicable, and so on...)

msporny commented 1 month ago

PR #292 has been raised to address this issue. This issue will be closed once PR #292 has been merged.

msporny commented 1 month ago

PR #292 has been merged, closing.