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 ECDSA secpr1 cryptosuite to VCWG input documents. #63

Closed
wants to merge 3 commits into from

Conversation

msporny
Copy link
Member

@msporny msporny commented Jan 29, 2022

This PR adds the ECDSA secpr1 cryptosuite to the set of VCWG input documents. The work item is currently being processed for adoption by the CCG:

w3c-ccg/community#228

The PR is meant to be a placeholder until its adoption in CCG (and then the URLs to the spec will be updated).


Preview | Diff

Copy link
Contributor

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

Yes this is a useful addition.

@@ -219,8 +219,10 @@ <h3>
signatures or proofs of existence, for bounded documents, such as verifiable
credentials. Concrete serializations will be provided based on <a
href="https://w3c.github.io/vc-data-model/#json-web-token">VC-JWT</a>, the <a
href="https://w3c-ccg.github.io/lds-ed25519-2020/">Ed25519 Cryptosuite</a>, and
the <a href="https://w3c-ccg.github.io/lds-ecdsa-secp256k1-2019/">Secp256k1
href="https://w3c-ccg.github.io/lds-ed25519-2020/">Ed25519 Cryptosuite</a>,
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK the charter can in fact refer to work items anywhere on the internet... not just in the W3C CCG.

Copy link
Member Author

@msporny msporny Jan 31, 2022

Choose a reason for hiding this comment

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

I expect W3C AC Members to raise IPR concerns if they see that a draft is being pulled in (for a normative work item) from a location that they are not familiar with. Specifically, if the IPR for any work item isn't clear (using language that has been agreed to by W3C Members), there is a strong chance that one or more AC Representatives will take issue with it (e.g., as a stall tactic), even to the point of raising a formal objection. In the very best case, they're going to ask the document author to track down everyone that contributed to that document and sign an IPR release for the draft before it can be pulled into the WG. This IPR uncertainty when W3C AC Members review our charter is all stuff that we should avoid as much as possible... it'll unnecessarily slow the work down.

W3C AC Reps understand W3C Community Group specification -> W3C Working Group specification. They raise their eyebrows at most anything else. We should avoid giving them targets to shoot at.

Copy link
Contributor

Choose a reason for hiding this comment

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

Being allowed to do something, and having it be a good idea are not the same thing.

I don't think the W3C CCG is somehow "better" than other places when it comes to links related to "Verifiable Credentials"... and I am not sure the AC Members would agree with your assertions, but your clarity does help.

@OR13
Copy link
Contributor

OR13 commented Feb 4, 2022

I suggest we try our best to merge all these link PRs so the section can be reorganized... I am happy to do a PR to clean them up, but not with so many open PRs trying to add links.

@brentzundel
Copy link
Member

@msporny Is this still a WIP? It has sufficient approvals that I would like to merge it.

@msporny
Copy link
Member Author

msporny commented Feb 4, 2022

@msporny Is this still a WIP? It has sufficient approvals that I would like to merge it.

We are still waiting on a co-sponsor in CCG for the work item. If the Chairs are ok with listing a work item that has not yet been accepted as a CCG work item (and under the W3C CCG IPR policy), then please merge away.

I expect this item to be either 1) accepted by the CCG, or 2) removed from the VCWG charter.

@OR13
Copy link
Contributor

OR13 commented Feb 14, 2022

IMO, we should be accepting #51 and not this PR.

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

IMO, we should be handling these new key and signature representations consistently (for P-256, P-384, P-521) or not at all.

We should not be doing 1 offs for families like NIST curves.

@msporny
Copy link
Member Author

msporny commented Feb 14, 2022

IMO, we should be handling these new key and signature representations consistently (for P-256, P-384, P-521) or not at all.

That is why Data Integrity now covers doing exactly that for key formats (if the WG decides that they want to head in that direction): https://w3c-ccg.github.io/data-integrity-spec/#multikey

We should not be doing 1 offs for families like NIST curves.

If the WG wants to define a cryptosuite for NIST curve families, it can do that. Again, blocking an input document that would go into that conversation is probably not a way to ensure that all options are on the table.

@OR13
Copy link
Contributor

OR13 commented Feb 14, 2022

@msporny seems like this PR title has been overtaken by events, are we already linking to https://w3c-ccg.github.io/data-integrity-spec/#multikey ?

Why do we need this PR if we can just link directly to that section?

@msporny
Copy link
Member Author

msporny commented Feb 14, 2022

Why do we need this PR if we can just link directly to that section?

Because there is no focused cryptosuite defined for ECDSA using secpr1. Yes, JsonWebSignature2020 exists, but it pulls in many other cryptosuites that could be viewed as not relevant to systems that only want to support ECDSA using secpr1.

The ECDSA secpr1 cryptosuite might support publicKeyJwk, publicKeyMultibase, AND publicKeyPem... or only publicKeyMultibase... or something else -- that's up to the WG to decide (if we decide to separate key formats from cryptosuites -- which would be a good thing, imho). At the same time, if we separate key formats from cryptosuites, where will publicKeyPgp live? Where do we define it?

In any case, this PR continues to exist for the reasons stated in the first paragraph above.

@OR13
Copy link
Contributor

OR13 commented Feb 14, 2022

It feels like we are trying to do 2 things under this charter item:

  1. define LD Proofs that rely on publicKeyMultibase and raw digital signatures
  2. define LD Proofs that rely on publicKeyJwk and detached jws.

I support both of these things.

If the best we can do is point to a few different specs to speak to the desire to do multikey, that is the best we can do I suppose.

It feels like something like MultiCodecSignature2020 that supported Ed25519 and secp384r1 and others would be better than listing them one at a time.

@kdenhartog
Copy link
Member

kdenhartog commented Feb 15, 2022

I don't see the relevance of a key format to the signature format. The VCWG should only be concerned with the way that the data model is used to attach a signature to the credential. It's up to the DID Core or other relevant key representation specs to define the key formats. It's easy enough to convert a public key and associated metadata without needing to couple it to the signature format. This coupling brings in unnecessary couplings in my opinion.

@OR13 OR13 mentioned this pull request Feb 15, 2022
index.html Outdated Show resolved Hide resolved
Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

I have requested the link in this PR be updated to reflect the new home for the document.

@msporny msporny changed the title [DO NOT MERGE YET] Add ECDSA secpr1 cryptosuite to VCWG input documents. Add ECDSA secpr1 cryptosuite to VCWG input documents. Feb 15, 2022
index.html Outdated Show resolved Hide resolved
@msporny
Copy link
Member Author

msporny commented Feb 15, 2022

I have requested the link in this PR be updated to reflect the new home for the document.

Done.

@brentzundel
Copy link
Member

@msporny since this PR has broad approval and no requests for changes, we should merge it.
I leave it up to you to decide whether is makes sense to resolve the existing merge conflicts so that it can be merged or to simply close it in favor of PR #77

Copy link
Member

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

Approving because this won't affect charter text in any actionable way. However since the topic has come up during charter discussion I'd argue that the WG should consider splitting out key formats from the suites when standardizing this work.

@OR13
Copy link
Contributor

OR13 commented Feb 24, 2022

I replied to a similar comment here: #51 (comment)

@msporny
Copy link
Member Author

msporny commented Feb 24, 2022

@kdenhartog wrote:

Approving because this won't affect charter text in any actionable way. However since the topic has come up during charter discussion I'd argue that the WG should consider splitting out key formats from the suites when standardizing this work.

@OR13 wrote:

I replied to a similar comment here: #51 (comment)

I'm starting to feel like a broken record at this point, but just repeat myself again:

This is already contemplated in the Data Integrity spec: https://w3c-ccg.github.io/data-integrity-spec/#multikey

... and in the spec pointed to by this PR:

https://w3c-ccg.github.io/di-ecdsa-secpr1-2019/#example-a-multibase-encoded-multicodec-curve-p-384-public-key

https://w3c-ccg.github.io/di-ecdsa-secpr1-2019/#example-a-dataintegritysignature-example-using-a-nistecdsa2019-cryptosuite

The text clearly still needs work, but there is an expectation written in these input documents that we do plan to separate key format from cryptosuite in the general case, but still provide the option to specify key format in cryptosuites to support things like EthereumEip712Signature2020 (which doesn't have a JWK/JWS mapping, nor a Multikey mapping, and probably won't because it's more proof than signature). PGP falls into the same category of not having mappings in either JWK/JWS or Multikey. PEM, while having a JWK mapping (kinda sorta, but not really because many implementations do totally different things there) also falls into this category.

@msporny
Copy link
Member Author

msporny commented Feb 25, 2022

I leave it up to you to decide whether is makes sense to resolve the existing merge conflicts so that it can be merged or to simply close it in favor of PR #77

I'll close this PR after we merge #77.

@msporny
Copy link
Member Author

msporny commented Mar 2, 2022

PR #77 has been merged, which includes this change, closing.

@msporny msporny closed this Mar 2, 2022
@msporny msporny deleted the msporny-secpr1 branch March 2, 2022 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants