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

Add cryptographic suite instantiation algorithm. #57

Merged
merged 10 commits into from
Feb 25, 2024

Conversation

msporny
Copy link
Member

@msporny msporny commented Feb 11, 2024

This PR adds the "Cryptographic Suite Selection Algorithm" to the ECDSA Cryptosuite that was defined by @jyasskin in the Data Integrity specification as an interface that MUST be defined by all DI cryptographic suite specifications.

Additional alignments are needed in the Data Integrity spec to line all of this up (I'll link to that PR once that's done). There is some "misalignment" w/ the interface and the algorithms for deriving selective disclosure proofs that we might need to handle in a future PR.


Preview | Diff

Copy link
Member

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

index.html Outdated
Comment on lines 528 to 534
([=map=] |options|) as input and returns a cryptosuite
([=struct=] |cryptosuite|).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should ideally link to the particular struct type, VC cryptographic suite.

Suggested change
([=map=] |options|) as input and returns a cryptosuite
([=struct=] |cryptosuite|).
([=map=] |options|) as input and returns a [=VC cryptographic suite=] |cryptosuite|.

should work, except https://respec.org/xref/?term=vc+cryptographic+suite doesn't find it, so ... w3c/browser-specs#1205.

Copy link
Member Author

@msporny msporny Feb 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 94fd162 .

Unfortunately, there are now ref errors, and I spent an hour trying to track down what's going wrong. The ref to
data integrity cryptographic suite instance (and other ref errors) exists here:

https://github.com/w3c/webref/blob/curated/ed/dfns/vc-data-integrity.json#L1007

... but it's not showing up in the ReSpec xref database:

https://respec.org/xref/

image

I don't know what intermediate file respec is using, but some of the cross-references to vc-data-integrity aren't working now that we've updated to the new way of doing things. /cc @dontcallmedom

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, it's unexpected that they would not show up in xref once they've hit webref - @marcoscaceres @sidvishnoi is that something you can help debug?

Copy link
Member

@sidvishnoi sidvishnoi Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debugging... For some reason nighty doesn't exist on some entry..

file:///.../respec.org/build/routes/xref/lib/scraper.js:159
        specUrls.add(entry.nightly.url);
                                   ^
TypeError: Cannot read properties of undefined (reading 'url')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following spec entries were causing breakage:

  iso18181-2
  iso14496-22

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @tidoust another example of downstream breakage coming from w3c/browser-specs#1192

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a check to ignore this breakage in server directly, and updated data. New data is live.

index.html Outdated
Comment on lines 533 to 548
<li>
Initialize |cryptosuite| to an empty [=struct=].
</li>
<li>
If |options|.|type| is `DataIntegrityProof` and |options|.|cryptosuite| is
`ecdsa-rdfc-2019` then:
<ol class="algorithm">
<li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some markup in https://w3c.github.io/tr-design/src/README#switch that should make this look a little better. I'd also take the DataIntegrityProof bit out of the main switch to simplify the prose.

Suggested change
<li>
Initialize |cryptosuite| to an empty [=struct=].
</li>
<li>
If |options|.|type| is `DataIntegrityProof` and |options|.|cryptosuite| is
`ecdsa-rdfc-2019` then:
<ol class="algorithm">
<li>
<li>
If |options|["type"] is not `DataIntegrityProof`, return failure.
</li>
<li>
If |options|["cryptosuite"] is:
<dl class="switch">
<dt>`ecdsa-rdfc-2019`</dt>
<dd>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied the markup and the CSS conflicted with some stuff that's going on in the local spec wrt. appropriate numbering for algorithms (the indention and the numbering got messed up). We've found it useful for implementers to be able to speak to step an substep numbers using the numbers we provide in the spec.

I'll try to do this in a future edit, after I get the CSS issues sorted (I have a deep work queue today, so have to move on to other things and need to get the normative text in this PR merged before getting to other PRs).

There is also an issue w/ variable highlighting... I'd like to be able to define the variables in the paragraph above and have the highlighting continue in the algorithm below, but it looks like ReSpec has change the way the highlighting works and the variable highlighting is now broken. I'll need to look into the ReSpec code to see if what I'm trying to do is possible (be able to specify an entire section including paragraph tags and algorithm classes as all in the same scope. If not, I'll have to figure out how to modify the numbering CSS we're using in order to not conflict w/ the algorithm numbering.

All that to say, skipping this (good) suggestion for now because I've spent too much time fighting w/ the tooling today (broken refs in reffy, broken CSS for "switch" and "algorithm").

index.html Outdated
Comment on lines 541 to 556
Set |cryptosuite|.|transform| to the algorithm in Section
<a href="#transformation-ecdsa-rdfc-2019"></a>.
</li>
<li>
Set |cryptosuite|.|hash| to the algorithm in Section
<a href="#hashing-ecdsa-rdfc-2019"></a>.
</li>
<li>
Set |cryptosuite|.|createProofConfig| to the algorithm in Section
<a href="#proof-configuration-ecdsa-rdfc-2019"></a>.
</li>
<li>
Set |cryptosuite|.|serializeProof| to the algorithm in Section
<a href="#proof-serialization-ecdsa-rdfc-2019"></a>.
</li>
<li>
Set |cryptosuite|.|verifyProof| to the algorithm in Section
<a href="#proof-verification-ecdsa-rdfc-2019"></a>.
</li>
</ol>
Copy link
Member

@jyasskin jyasskin Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also shorten this:

Suggested change
Set |cryptosuite|.|transform| to the algorithm in Section
<a href="#transformation-ecdsa-rdfc-2019"></a>.
</li>
<li>
Set |cryptosuite|.|hash| to the algorithm in Section
<a href="#hashing-ecdsa-rdfc-2019"></a>.
</li>
<li>
Set |cryptosuite|.|createProofConfig| to the algorithm in Section
<a href="#proof-configuration-ecdsa-rdfc-2019"></a>.
</li>
<li>
Set |cryptosuite|.|serializeProof| to the algorithm in Section
<a href="#proof-serialization-ecdsa-rdfc-2019"></a>.
</li>
<li>
Set |cryptosuite|.|verifyProof| to the algorithm in Section
<a href="#proof-verification-ecdsa-rdfc-2019"></a>.
</li>
</ol>
Return a new [=VC cryptographic suite=] whose items are:
<dl>
<dt>[=cryptographic suite/transform=]</dt>
<dd>
the algorithm in Section <a href="#transformation-ecdsa-rdfc-2019"></a>
</dd>
<dt>[=cryptographic suite/hash=]</dt>
<dd>
the algorithm in Section <a href="#hashing-ecdsa-rdfc-2019"></a>
</dd>
<dt>[=cryptographic suite/createProofConfig=]</dt>
<dd>
the algorithm in Section <a href="#proof-configuration-ecdsa-rdfc-2019"></a>
</dd>
<dt>[=cryptographic suite/serializeProof=]</dt>
<dd>
the algorithm in Section <a href="#proof-serialization-ecdsa-rdfc-2019"></a>
</dd>
<dt>[=cryptographic suite/verifyProof=]</dt>
<dd>
the algorithm in Section <a href="#proof-verification-ecdsa-rdfc-2019"></a>
</dd>
</dl>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up fighting the tooling trying to apply this: #57 (comment)

I'll try later, after we have all the normative stuff lined up.

</ol>
</li>
<li>
Return |cryptosuite|.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that https://w3c.github.io/vc-data-integrity/#dfn-suite-selection-algorithm expects this to return "failure" for values it can't handle, rather than an empty suite.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithm was updated to never return a failure. It either returns an empty cryptosuite instance (which doesn't have the functions set) or one that does. The error handling bits were removed to be out of scope in VC-DATA-INTEGRITY as well. This was largely done to simplify the algorithms. The algorithms in VC-DATA-INTEGRITY presume that they're handed a valid cryptosuite instance... if they're not, errors will be thrown. It's up to the implementations to decide how they handle empty cryptosuite instances.

IOW, this feels like implementation-specific stuff that we shouldn't have to spell out to implementers. Unless I'm missing something, I don't think we lose much if we don't return an error here. If you feel strongly about this, we can rework it so it generates an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you'll cause implementation divergence with this or make it significantly more difficult to read the spec, so it doesn't matter a lot, but pedantically, https://w3c.github.io/vc-data-integrity/#dfn-data-integrity-cryptographic-suite-instance doesn't admit the possibility that it could be empty. Both items are just "an algorithm that ..." rather than "null or an algorithm that ...". I think that's the right choice there, so to make the types work here, it'd be better to have this algorithm return a sentinel value instead of an "empty" cryptosuite. I used "failure", matching places like https://url.spec.whatwg.org/#concept-url-parser, but "null" would also work perfectly well. But again, this isn't super important.

@iherman
Copy link
Member

iherman commented Feb 14, 2024

The issue was discussed in a meeting on 2024-02-14

  • no resolutions were taken
View the transcript

1.1. Fix algorithm misalignments using new cryptosuite interface. (pr vc-data-integrity#244)

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

Manu Sporny: processed a number of PRs across VCDM, DI, cryptosuites. need to talk about Jeffery Yaskin's PR (#244) to create an interface for all DI specs.

See github pull request vc-di-ecdsa#57.

Manu Sporny: that broke interfaces b/w DI specs. trying to get them re-aligned. 2 PRs - 1 for DI, 1 for ECDSA-SD. heads up to the group we're trying to align these interfaces.
… some misalignment on how they would work. have a plan forward to address this. plan is for an interface in all DI specs that all have 'functions' each cryptosuite executes to create/verify proofs. a standard interface.
… the functions to expose was under debate. based on discussion we will only define 2 functions on the interface: create proof and verify proof.
… will require changes to algorithms across these specs. pushing more details into the cryptosuite specs. less in DI the spec. should not impact implementations. we know we will go through a 2nd CR. the interfaces are changing, not the algorithms.
… are there any concerns/guidance before I start making those changes?

Dave Longley: +1 to those changes.

Ivan Herman: presume that ECDSA then EDDSA and then BBS?

Manu Sporny: correct.

Michael Jones: think it takes us down a bad path to build interfaces that no one will build. we should not be creating APIs, that is out of scope.

Manu Sporny: agree that APIs are out of scope, but that's not what we're creating here.

Michael Jones: that is what you described.

Manu Sporny: have discussed this before. we're discussing interfaces, which is what the w3c does, not in web IDL which would define an API. implementations are implementing in this way. they are abstract, not concrete web IDL interfaces.

Michael Jones: I am missing context. what else are you planning to do?

Manu Sporny: changing the interfaces that we had months ago, which Jefferey asked for. that PR had weeks of review and already went in.

Brent Zundel: any other comments?

Manu Sporny: no - that's the major PR I need feedback on.

@msporny msporny changed the title Add suite selection algorithm. Add cryptographic suite instantiation algorithm. Feb 17, 2024
@msporny msporny added the normative This item is a normative change. label Feb 21, 2024
@msporny msporny force-pushed the msporny-add-suite-selection branch from 18d9f99 to d65a7e8 Compare February 25, 2024 17:00
@msporny msporny force-pushed the msporny-add-suite-selection branch from d65a7e8 to 69b6b45 Compare February 25, 2024 17:58
Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few adjustments need to be made to what's passed when creating the proof, I think.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@msporny
Copy link
Member Author

msporny commented Feb 25, 2024

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

@msporny msporny merged commit 7e7bab9 into main Feb 25, 2024
1 check failed
@msporny msporny deleted the msporny-add-suite-selection branch February 25, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative This item is a normative change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants