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

Requirements around the @context proof property #104

Open
PatStLouis opened this issue Dec 10, 2024 · 1 comment
Open

Requirements around the @context proof property #104

PatStLouis opened this issue Dec 10, 2024 · 1 comment
Assignees
Labels
CR2 This item was processed during the second Candidate Recommendation Phase editorial This item is editorial in nature. ready for pr

Comments

@PatStLouis
Copy link

It was highlighted in #102 that the test vectors did not match the specification's algorithms.

Following the thread, I would like to raise an issue around the specific step which instructs issuers to include the @context property in their proof for the eddsa-jcs-2022 cryptosuite.

The test step in question reads
If unsecuredDocument.@context is present, set proof.@context to unsecuredDocument.@context.

I would like to discuss why was this step included as I understand there is a significant reason in relation with the ActivityPub work.

I understand it's a feature required when an issuer wants to add a proof set to an already secured document and wants to alter the @context value. I'm questioning why should this be a mandatory step for all issuers.

Following my comment:

I'm a bit torn on this requirement from the spec. Its a feature to support a very specific use case where an issuer might want to add it's own context to the proof in the case of a proof set. Its not normative and a vc without this step can verify perfectly well with the verification algorithm. I've implemented this cryptosuite without this step and I should be able to, as it meets all normative statements of the specification. A verifier shouldn't fail the verification because the base document has a context but the proof doesn't.

@titusz has replied:

I am confused. How is it not normative. I understand that section 3.3.1 clause 2 doesn't use RFC 2119 keywords (MUST, SHOULD, MAY, etc.). But it's part of a numbered algorithm in Section 3 which is explicitly defined as normative in Section 1.2 Conformance. If clause 2 is not normative then that would mean I could skip any of the algorithm steps an still conform?

Don´t get me wrong. I am agnostic on whether this step is meant to be normative or not but I think the spec should be clear about whether it is normative or not.

The conformance section reads:

Specifically, all relevant normative statements in Sections 2. Data Model and 3. Algorithms of this document MUST be enforced.

Some algorithms have normative statements, treating the whole algorithm as normative would defy the purpose of those normative statements. The create proof algorithm reads:

The following algorithm specifies how to create a data integrity proof given an unsecured data document. Required inputs are an unsecured data document (map unsecuredDocument), and a set of proof options (map options).

By omitting this step, a valid data integrity proof is still produced which can be verified using the Verify Proof algorithm.
It also doesn't prevent other issuers from leveraging this feature in subsequent proofs from my understanding.

I've not heard a compelling reason why this is a requirement, as including this step or omitting it both results in a valid data integrity proof. I would rather clarify this statement with something along the lines of:
If unsecuredDocument.@context is present, implementations MAY set proof.@context to unsecuredDocument.@context.

The inclusion or not of this step won't affect the verification algorithm, and is based on a specific requirement. If any of my understanding of the purpose of these algorithms is wrong, I would like for someone to point it out to me so I can correct my software. There is no verification step that warrants verifying that if the securedDocument has an @context, the proof must also have an @context.

@msporny msporny added normative This item is a substantive or normative change. discuss CR2 This item was processed during the second Candidate Recommendation Phase labels Jan 2, 2025
@msporny
Copy link
Member

msporny commented Jan 19, 2025

A couple of thoughts (and I'll raise a PR along these lines for review by those that have an opinion on this item):

  • It is true that all steps in an algorithm are expected to be implemented to achieve the return value for the algorithm, so the guidance is normative and we do expect implementations to follow it (@titusz is correct, it is currently a normative MUST).
  • It is also true that this particular step only matters if you are dealing with proof sets or proof chains that might use other context values. If you don't plan to include the generated proof in a proof set or a proof chain, you don't need to do this step. We didn't feel like highlighting the nuance was going to increase interop and we didn't think implementers would push back against this feature. Making it optional will almost certainly harm interop.
  • It is also true that implementations sometimes don't implement the algorithms exactly as written and this is a case where an implementer could choose to skip the step because they're not producing a proof that would ever be used in a proof set. Again, I personally think it's a bad idea to make this optional because a proof set might be added in a separate application unknown to the initial application that created the initial proof (which is where the interop challenge would come in).

I'll raise a PR to say that some implementations might choose to not do that step if the proof won't end up in a proof set, but doing so raises the likelihood of interop failures. I would much rather NOT add that language because we're inviting trouble. I do need to close this issue out so that's the path forward unless there are any objections.

@msporny msporny self-assigned this Jan 19, 2025
@msporny msporny added ready for pr editorial This item is editorial in nature. and removed discuss normative This item is a substantive or normative change. labels Jan 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR2 This item was processed during the second Candidate Recommendation Phase editorial This item is editorial in nature. ready for pr
Projects
None yet
Development

No branches or pull requests

2 participants