Update algorithm for Verify Proof #64

msporny · 2022-10-17T01:55:17Z

This PR updates the algorithm used to verify proofs to align with the language used to generate proofs (now merged into main).

At present, both the proof generation and proof verification algorithms presume only a single proof. The algorithms will need to be updated to support proof sets and proof chains in a future PR.

Preview | Diff

iherman · 2022-10-17T07:26:13Z

A minor comment, though not strictly to §4.2: do we have to say somewhere what "raise XYZ ERROR" means (e.g., in 2.1), or do we intend to leave it at that and leave it to the implementations?

iherman

There is an inconsistency in the value references. $4.2/(2) uses "dotted" term references like proof.type, proof.verificationMethod, etc. However, §4.1/(5) uses references to the same terms without the "dotted" formalism. (I did not check all items, there may be such discrepancies elsewhere, too.) Even §4.2 is not fully consistent; shouldn't (5) and (6) say proof.cryptosuite rather than "the cryptosuite specified in proof"?

Technically, it is not clear to me where the parameters like options come from. Or is this the ISSUE right after the algorithm (that only refers to verificationMethod)?

TallTed · 2022-10-17T15:58:23Z

I would reword your original comment slightly, from algorithms only presume a single proof to algorithms presume only a single proof.

TallTed

Some minor rephrasing

index.html

dlongley

Thanks! Approving to keep things moving, but see suggestions (and +1 to @TallTed's suggestions).

dlongley · 2022-10-17T21:13:26Z

index.html

+`MALFORMED_PROOF_ERROR` MUST be raised.
+          </li>
+          <li>
+If the cryptographic suite does not support unlinkability and the


Maybe we just want to say "if the cryptographic suite requires proof.created" rather than make it hinge on the unlinkability property.

dlongley · 2022-10-17T21:14:11Z

index.html

+<var>proof</var>.<var>created</var> value is not set, a `MALFORMED_PROOF_ERROR`
+MUST be raised.


This error description is fine for now -- but we will want to see if we can improve / draw from other specs for better error patterns and error type / code reuse.

dlongley · 2022-10-17T21:18:32Z

index.html

+<a href="#dfn-transformation">transforming</a> <var>unsecuredDocument</var>
+according to a <a>transformation algorithm</a> associated with the
+<var>cryptosuite</var> specified in <var>proof</var> and the <var>options</var>
+parameters provided as inputs to the algorithm.


Can we say how the cryptosuite is specified, i.e., via type (and perhaps other cryptosuite type-specific properties)?

Suggested change

parameters provided as inputs to the algorithm.

parameters provided as inputs to the algorithm. Note: The <var>cryptosuite</var>

is specified by <var>proof.type</var> and MAY be further described by

cryptosuite-specific properties.

Seeing as the <var>cryptosuite</var> specified in <var>proof</var> is mentioned in other steps as well -- we might want the above language more generally expressed in an early step.

This presumes the cryptosuite property is in the proof, which it isn't for anything other than DataIntegrityProof... If it is, that'll have to be fixed. I'm also hesitating to clarify this in the body of the algorithm. I'll try to think about the best way to integrate this into this section and agree that we want to say something about cryptosuites being able to define other properties in a proof.

I misunderstood what you were going for the first time I read your comments. I've applied your note and clarified the thing that threw me off the first time in f0732e5.

index.html

dlongley · 2022-10-17T21:24:02Z

index.html

+          <li>
+If the <var>proof</var>.<var>proofPurpose</var> value does not match
+<var>options</var>.<var>proofPurpose</var>, a `MISMATCHED_PROOF_PURPOSE_ERROR`
+MUST be raised.


This step should be run before any effort is made to verify the proof, i.e., the expected proof purpose should be checked first to find a matching proof. We might want the options name to be "expectedProofPurpose" as well, for clarity.

Ok, will look into rewriting the algorithm in this way.

Fixed in f3a3d2c.

index.html

iherman · 2022-10-19T16:10:56Z

The issue was discussed in a meeting on 2022-10-19

no resolutions were taken

View the transcript

2.3. Update algorithm for Verify Proof (pr vc-data-integrity#64)

See github pull request vc-data-integrity#64.

Manu Sporny: this is just updating the language.

msporny · 2022-10-21T23:09:20Z

A minor comment, though not strictly to §4.2: do we have to say somewhere what "raise XYZ ERROR" means (e.g., in 2.1), or do we intend to leave it at that and leave it to the implementations?

Ultimately, it's up to the implementations to expose those errors. Exactly how each one does it is implementation specific at the moment. We might want to do something like what the JSON-LD API did, though some have complained about the use of text strings instead of error codes.

We do plan to expose those error types in protocols such as the VC API via a code field, but that's out of scope for this spec (probably). In short, we're still working things out. I'm trying to be consistent with the error codes so that when it comes time to expose them further up the software stack, people aren't caught off guard.

There is an inconsistency in the value references. $4.2/(2) uses "dotted" term references like proof.type, proof.verificationMethod, etc. However, §4.1/(5) uses references to the same terms without the "dotted" formalism. (I did not check all items, there may be such discrepancies elsewhere, too.) Even §4.2 is not fully consistent; shouldn't (5) and (6) say proof.cryptosuite rather than "the cryptosuite specified in proof"?

Yes, thanks for spotting that, it is an oversight and I'll fix that in an upcoming commit. I was going back and forth on the easiest way to mark up these variables.

Technically, it is not clear to me where the parameters like options come from. Or is this the ISSUE right after the algorithm (that only refers to verificationMethod)?

options is specified as an input to the algorithm in the opening paragraph. I'm still trying to figure out if the ReSpec var parameter approach is more helpful than harmful... I think it's helping, but the github preview doesn't show the markup, so it's difficult to see it without checking out the source and rendering the document locally. The var parameter highlights everywhere the variable is used, so in theory, you should be able to click on the word options and it will highlight everywhere the variable is used in the algorithm. In short, if you had a properly rendered document, this might have been easier to find for you (or I screwed up the markup and I'll fix it :P ).

index.html

Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>

Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>

msporny · 2022-10-23T20:51:09Z

@iherman wrote:

There is an inconsistency in the value references. $4.2/(2) uses "dotted" term references like proof.type, proof.verificationMethod, etc. However, §4.1/(5) uses references to the same terms without the "dotted" formalism. (I did not check all items, there may be such discrepancies elsewhere, too.) Even §4.2 is not fully consistent; shouldn't (5) and (6) say proof.cryptosuite rather than "the cryptosuite specified in proof"?

Yes, thanks for spotting that, it is an oversight and I'll fix that in an upcoming commit. I was going back and forth on the easiest way to mark up these variables.

Fixed in 520a357.

msporny · 2022-10-23T20:51:48Z

Normative, multiple reviews, changes requested and made, no objections.

msporny requested a review from mprorock as a code owner October 17, 2022 01:55

msporny requested review from TallTed, dlongley, OR13, peacekeeper and iherman October 17, 2022 01:55

iherman reviewed Oct 17, 2022

View reviewed changes

TallTed suggested changes Oct 17, 2022

View reviewed changes

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

OR13 approved these changes Oct 17, 2022

View reviewed changes

dlongley approved these changes Oct 17, 2022

View reviewed changes

mprorock approved these changes Oct 19, 2022

View reviewed changes

OR13 approved these changes Oct 19, 2022

View reviewed changes

sbutterfield approved these changes Oct 19, 2022

View reviewed changes

msporny commented Oct 21, 2022

View reviewed changes

index.html Outdated Show resolved Hide resolved

msporny mentioned this pull request Oct 21, 2022

Cleanly separate cryptosuite verification and purpose verification algorithms #66

Closed

msporny force-pushed the msporny-remove-cvh branch from b150567 to 8aeba01 Compare October 23, 2022 20:11

Base automatically changed from msporny-remove-cvh to main October 23, 2022 20:16

msporny force-pushed the msporny-fix-verify-proof branch from 7c81a0b to 795a4d9 Compare October 23, 2022 20:16

msporny and others added 5 commits October 23, 2022 16:19

Rewrite algorithm for Verify Proof.

1151f83

Apply grammar fixes from @TallTed to Verify Proof.

6cabc3f

Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>

Ensure created requirement isn't tied to unlinkability.

1a7b2ec

Rename "Valid" to "Verified".

da58a31

Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>

Clarify that JWK key_ops/KeyUsage is different from proofPurpose.

5d540b0

msporny force-pushed the msporny-fix-verify-proof branch from 795a4d9 to 5d540b0 Compare October 23, 2022 20:19

msporny added 2 commits October 23, 2022 16:29

Clarify how cryptographic suite type is expressed in Verify Proof.

f0732e5

Check expectedProofPurpose before checking signature.

f3a3d2c

Align variable markup with Verify Proof algorithm.

520a357

msporny merged commit a7a04a2 into main Oct 23, 2022

msporny deleted the msporny-fix-verify-proof branch October 23, 2022 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update algorithm for Verify Proof #64

Update algorithm for Verify Proof #64

msporny commented Oct 17, 2022 •

edited by pr-preview bot

iherman commented Oct 17, 2022

iherman left a comment •

edited

TallTed commented Oct 17, 2022

TallTed left a comment

dlongley left a comment

dlongley Oct 17, 2022

dlongley Oct 17, 2022

dlongley Oct 17, 2022 •

edited by msporny

msporny Oct 21, 2022

msporny Oct 23, 2022

dlongley Oct 17, 2022

msporny Oct 21, 2022

msporny Oct 23, 2022

iherman commented Oct 19, 2022

2.3. Update algorithm for Verify Proof (pr vc-data-integrity#64)

msporny commented Oct 21, 2022 •

edited

msporny commented Oct 23, 2022

msporny commented Oct 23, 2022

		<var>proof</var>.<var>created</var> value is not set, a `MALFORMED_PROOF_ERROR`
		MUST be raised.

-parameters provided as inputs to the algorithm.
+parameters provided as inputs to the algorithm. Note: The <var>cryptosuite</var>
+is specified by <var>proof.type</var> and MAY be further described by
+cryptosuite-specific properties.

Update algorithm for Verify Proof #64

Update algorithm for Verify Proof #64

Conversation

msporny commented Oct 17, 2022 • edited by pr-preview bot

iherman commented Oct 17, 2022

iherman left a comment • edited

Choose a reason for hiding this comment

TallTed commented Oct 17, 2022

TallTed left a comment

Choose a reason for hiding this comment

dlongley left a comment

Choose a reason for hiding this comment

dlongley Oct 17, 2022

Choose a reason for hiding this comment

dlongley Oct 17, 2022

Choose a reason for hiding this comment

dlongley Oct 17, 2022 • edited by msporny

Choose a reason for hiding this comment

msporny Oct 21, 2022

Choose a reason for hiding this comment

msporny Oct 23, 2022

Choose a reason for hiding this comment

dlongley Oct 17, 2022

Choose a reason for hiding this comment

msporny Oct 21, 2022

Choose a reason for hiding this comment

msporny Oct 23, 2022

Choose a reason for hiding this comment

iherman commented Oct 19, 2022

2.3. Update algorithm for Verify Proof (pr vc-data-integrity#64)

msporny commented Oct 21, 2022 • edited

msporny commented Oct 23, 2022

msporny commented Oct 23, 2022

msporny commented Oct 17, 2022 •

edited by pr-preview bot

iherman left a comment •

edited

dlongley Oct 17, 2022 •

edited by msporny

msporny commented Oct 21, 2022 •

edited