Fix algorithm misalignments using new cryptosuite interface.

[msporny]

This PR updates the "Cryptographic Suite Selection Algorithm" interface that was defined by @jyasskin that MUST be defined by all DI cryptographic suite specifications. PR w3c/vc-di-ecdsa#57 implements the interface on the ECDSA cryptosuite and this PR attempts to align that PR with the DI specification. This PR also fixes some misalignments between the base algorithms and the usage of the suite.

This PR doesn't attempt to align the selective disclosure algorithms, that might happen in a separate PR.

---

Preview | Diff

[Wind4Greg]

Looks good with only one word usage issue: "signal" should be "single"? Right?

[dlongley]

I need to look more closely at this, but it seems like it might be slightly misaligned or overly prescriptive for use with selective disclosure cryptosuites that return other cryptosuite-specific outputs (e.g., https://w3c.github.io/vc-di-ecdsa/#base-proof-hashing-ecdsa-sd-2023 returns an object with multiple values, some hashes and others not, as key-value pairs).

I'm concerned we're trying too hard to pin down too much of the interface here rather than providing core functions and a base, loose, architecture for extending cryptosuites to fit into. There is generally a transformation step, which always takes the unsecured data document as an input -- but the output can be specialized to the cryptosuite -- and, generally, a hashing step follows that receives this cryptosuite-specific output and generates its own cryptosuite-specific output, which is then followed by a proof generation/serialization step that finally outputs the data integrity proof object.

In short, there's a pipeline that is cryptosuite specific that always starts with an unsecured document and ends up with a data integrity proof at the end of it, but cryptosuites should be free to define their own intermediates. The fact that the data integrity core spec provides some useful primitives to use there does not mean that every cryptosuite needs to use them exactly as they are for "simple digital signatures". What is required, however, is unsecured document in, and data integrity proof out (for the entire pipeline).

I also think it's useful to keep the "proof configuration options" typing information we've added here, but we might need to relax some of the "pipeline intermediates" to prevent unnecessarily restricting innovation in cryptosuites in that space. It's also just not even aligned with current selective disclosure suites (slightly as mentioned).

[jyasskin]

+1 to the idea of merging the "transform", "hash", and "serializeProof" algorithms for the purposes of this interface. #226 (comment) listed a couple options for improving the flexibility here, but since the only use of this interface just calls the operations as a pipeline, merging them seems best.

[msporny]

Ok, we had a conversation about this during the call yesterday and are taking @jyasskin and @dlongley's guidance and fixing up the interface to be simpler / higher level.

[msporny]

Ok, reworked the entire PR, but only focused on the interface definition and the usage of the interface in Add Proof and Verify Proof. It was more painful than I was hoping it would be... but it might be closer to what folks want now. I still need to work on the other side of the implementation in the matching ECDSA PR.

We would still need to do alignments w/ the Proof Set/Chain stuff in a later PR.

[jyasskin]

You really should keep the exported definition, so that other specs can refer to the whole struct at once.

[msporny]

Removing the export was a mistake, I'll fix it in the next revision.

The one concern was calling it a "VC cryptographic suite" -- the only reason we're doing the Data Integrity work in the VCWG is due to politics. The Data Integrity technology is NOT supposed to be bound at all to the Verifiable Credential work (except that VCs are just ONE use of Data Integrity)... my concern in doing this (many years ago when the work was chartered) was that people would think that DI is tightly coupled to VCs, which is what certain actors wanted during the chartering process, but was never the intention of the work. All that to say "VC cryptographic suite" is wrong, "DI cryptographic suite" might be an ok compromise, but exporting it as "cryptographic suite" doesn't feel wrong either (since the xref would have to refer to the spec to import the language).

Just noting this here to see if you have any thoughts. I'll export the definition and adjust further if you have strong feelings one way or the other.

[jyasskin]

Defining "cryptographic suite" is fine with me. I only picked the more specific term because I wasn't sure it's what the WG intended.

[iherman]

I would go for "DI Cryptographic Suite".

[jyasskin]

FWIW, Bikeshed has xrefs that pull in everything, without needing the spec to be listed, so we'd want to resolve a collision on "cryptographic suite" if one happens, but resolving those isn't the end of the world. If the WG thinks the interface here will work for cryptographic suites outside of this ecosystem (like in https://w3c.github.io/webcrypto/), it'd be fine to claim the term, IMO. If the WG doesn't want to be that general, then picking the more specific term is better.

[msporny]

FWIW, Bikeshed has xrefs that pull in everything, without needing the spec to be listed

Ah, well, yes, that would be an issue. I'll go with the longer name for now. We could probably utilize the same concept for webcrypto, but that is far more work than this WG has signed up to do.

[msporny]

I settled on cryptographic suite instance for now, and exported the definition. I tried data integrity cryptographic suite instance and repeatedly using that in the refs in the spec was a pain.

[iherman]

I understand it is a pain but, thinking a bit further, I do believe it is worth having a reference to DI in the name. If I look at the spec from a purely Linked Data perspective using this spec for any RDF Dataset, then there are some problems that these specs do not really solve (we would get back to the discussions we had with Henry Story, which could be settled properly for VC but not in general).

Solving those issues is not in the charter of this WG, so that is o.k., but that is why I believe a reference to DI would make sense.

[msporny]

Done in 63631f5.

[jyasskin]

"struct" shouldn't really be used as the type of an input parameter, any more than you use "struct" as the type of a C variable. Instead, you create concrete types as structs of a particular set of "items", and then input parameters are typed by those concrete types. If you want an unbounded set of keys, use a map.

[msporny]

Fixed to use a map.

[iherman]

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]

Alright @jyasskin, @dlongley, @iherman, @TallTed, I've made another pass at this and attempted to apply each of your concerns... please re-review and let me know if further adjustments are needed.

[jyasskin]

I haven't done a thorough review yet, but please don't block on me.

[jyasskin]

Note that data-lt will capture links to [=cryptosuite instance=] from any spec. If you just want a shorthand for this spec to use, use data-local-lt.

[msporny]

Thanks, that's better, fixed in c46e47d.

[jyasskin]

verifyProof is defined to be able to return an error. This algorithm needs to handle that error.

[msporny]

Fixed in 2296634.

[jyasskin]

This sets up 3 kinds of return values:

1. error
2. non-error with verified==false
3. non-error with verified==true

If the first two are equivalent, you should collapse them, probably by removing the verified field. If they're different, you should explain what the distinction is.

If the verified field sticks around, you should probably require the verifiedDocument to only be present if it's true?

[dlongley]

I still haven't done a thorough review here yet, but wanted to note that the first two here aren't equivalent -- so we should explain this a bit more (as @jyasskin says). An error means the verification check couldn't even be completed (because some error prevented it), whereas a non-error with verified=false means the process was completed and verification was false. This might be meaningful in cases where a network error occurred, but verification could actually become true if, for example, network access were restored.

[msporny]

Fixed in 1f7ee96.

[jyasskin]

I'd usually use "returns" for an algorithm instead of "produces" or "outputs" (see https://infra.spec.whatwg.org/#algorithm-declaration), but I also don't insist on it.

Suggested change

	|securedDocument|) as input, and produces a verification result
	|securedDocument|) as input, and returns a verification result

[msporny]

I was trying to avoid "returns" as we haven't defined an error structure to return; we depend on functions throwing when there are errors/exceptions. It's debatable whether or not that is a "return" value or an exception value since programming environments typically have different syntax for handling each path.

I admit to having struggled with this particular part of documenting lower-level functions... I don't want to insist that developers use return structures for errors (because languages that prefer throwing exceptions exist). "Produces" feels like it gives the implementer more latitude to make the choice, where "returns" feels like we're taking exception handling off of the table.

I know that's a bit esoteric, but that's the current approach that many of the algorithms have taken. Changing it to "return" would require a rewrite to most/all of the algorithms. Do we have a best practices wrt. this particular concern at W3C?

[jyasskin]

I think the Infra spec strives to document whatever best practices we have, but it's fine for WGs to override that in particular cases. Note that whatever you write, you're not insisting that developers use any particular structures for errors, due to https://infra.spec.whatwg.org/#algorithm-conformance:~:text=Conformance%20requirements%20phrased%20as%20algorithms%20or%20specific%20steps%20may%20be%20implemented%20in%20any%20manner%2C%20so%20long%20as%20the%20end%20result%20is%20equivalent. The most egregious example I know of is https://html.spec.whatwg.org/multipage/infrastructure.html#starting-a-new-parallel-queue, which is specified as a busy wait. But again, I'm not insisting on changing this.

[jyasskin]

I'd make "verifiedDocument" into a <dfn>. If there's more than one defined key, I'd then use a <dl> to organize them, but you may only have the 1 after handling my previous comment.

Suggested change

	verification status ([=boolean=] |verified|) value and an [=unsecured data
	document=] ([=map=] |verifiedDocument|), and MAY contain information
	<dfn data-dfn-for="cryptosuite instance/verifyProof">verifiedDocument</dfn>
	[=map/key=] whose [=map/value=] is an [=unsecured data
	document=]. Other keys MAY contain information

[msporny]

Fixed in 1f7ee96.

[msporny]

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