search

w3c / vc-di-eddsa

EdDSA Data Integrity Cryptosuites Specification
https://w3c.github.io/vc-di-eddsa/
Other
12 stars 10 forks source link

Proof configuration and previousProof (maybe editorial) #93

Closed iherman closed 2 months ago

iherman commented 3 months ago

Just checking.

§3.2.5 Proof Configuration does not mention the previousProof property, if applicable. I.e., when calculating the value of canonicalProofConfig, that value is not taken into consideration.

I do not know whether this is intentional or an omission.

If it is intentional, it might be worth emphasizing. The formulation in §3.2.2 Verify Proof, point (2) only mentions proofValue as the property to be removed, which gives the false impression that previousProof is fine. (I know that in §3.2.5 the properties to be used are listed explicitly, but then why bother with point (2) in §3.2.2 in the first place?)

(The ecdsa case is identical.)

dlongley commented 3 months ago

It's a bug, it should read that the proof config is initialized as a clone of the proof object like it is here:

https://www.w3.org/TR/vc-di-eddsa/#proof-configuration-eddsa-jcs-2022

We fixed this with proof generation, but it looks like it didn't get into the proof configuration algorithms:

Let proof be a clone of the proof options, options.

https://www.w3.org/TR/vc-di-eddsa/#create-proof-eddsa-rdfc-2022

@Wind4Greg, each of the proof configuration options should be initialized as a clone instead of an empty object, just like we did with the proof itself. (This needs to be done in all the proof config algorithms in all the cryptosuite specs if it isn't already done elsewhere).

Once that's done, this will automatically include the previousProof property (as it should).

Wind4Greg commented 3 months ago

@dlongley and @iherman I thought I covered this in PR https://github.com/w3c/vc-di-eddsa/pull/88. I looks like I did here: https://pr-preview.s3.amazonaws.com/Wind4Greg/vc-di-eddsa/pull/88.html#proof-configuration-eddsa-rdfc-2022. Or did I miss something?

dlongley commented 3 months ago

@Wind4Greg, ah, sorry forgot that PR was still in flight. That probably covers it -- can't look closely right now, but if you have a moment, @iherman, that other PR is meant to cover this.

iherman commented 3 months ago

Yep, @Wind4Greg and @dlongley, it seems that #88 indeed solves it. I did not follow the various cryptosuite PR-s closely, so I missed it.

This issue can be closed if and when #88 is merged.

msporny commented 2 months ago

PR #88 has been merged, closing.