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 links from stalled prs #73

Closed
wants to merge 2 commits into from
Closed

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Feb 15, 2022


Preview | Diff

Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

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

I only object to one line in the PR (that sends a mixed message). Remove it and I will approve.

index.html Outdated
<li>Other VC Data Integrity proof types:
<ul>
<li>VC Data Integrity - <a href="https://or13.github.io/lds-pgp2021"></a>PGP</li>
<li>VC Data Integrity - <a href="https://ns.did.ai/suites/multikey-2021"></a>Multbase / MultiCodec</li>
Copy link
Member

Choose a reason for hiding this comment

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

This is the only line I would object to. We put publicKeyMultibase and Multikey in DID Core with the explicit goal to finish standardizing it in the VC 2.0 WG. This is a normative deliverable, to be defined in Data Integrity, and must remain as such.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a normative deliverable, to be defined in Data Integrity, and must remain as such.

I didn't touch that part of the charter, which already covers Ed25519 2020 and with your PR (#63) will add Secp384r1 2022.

Until the normatively deliverables cover other popular key formats (like P-256 / ES256) which is mandatory here: https://www.iana.org/assignments/jose/jose.xhtml

I think its smart for us to keep the door open to working on them non normatively.

@OR13
Copy link
Contributor Author

OR13 commented Feb 15, 2022

@msporny either update your PR (#63) to cover P-256 (and others), or accept this PR to allow us to define them non normatively... we can't skip over mandatory to implement key and signature formats and expect to have the charter approved.

Another alternative would be to move the LD specific key and signature representations that are not based on JWS to the section in this PR.

I don't consider the current attempts at defining and omitting specific keys and signatures in this WG to be a path to success.

It's going to lead to formal objections for not addressing the topic consistently.

Either publicKeyMultibase is a viable alternative to publicKeyJwk, or our charter should close the door on it, and allow us all to focus on other components of this specification.

@msporny
Copy link
Member

msporny commented Feb 15, 2022

Either publicKeyMultibase is a viable alternative to publicKeyJwk, or our charter should close the door on it, and allow us all to focus on other components of this specification.

Can you please explain how what is proposed in this spec (normative deliverable of WG) doesn't make publicKeyMultibase a viable alternative to publicKeyJwk?

The goal there is to define all possible key types in the multicodec table, which include (IIRC) all key types that are possible to be expressed in publicKeyJwk. You asked me to write spec text to enable that, I did that, and now you are still suggesting that that didn't happen. Have you read the spec text? What am I missing here?

Please state, concretely, why you believe the Multikey that you want is not supported via a normative work item that is currently listed in the charter?

@OR13
Copy link
Contributor Author

OR13 commented Feb 15, 2022

@msporny your PR #63

is titled:

[DO NOT MERGE YET] Add ECDSA secpr1 cryptosuite to VCWG input documents.

And it points to:

https://digitalbazaar.github.io/di-ecdsa-secpr1-2019/

Which in turn looks like this:

Screen Shot 2022-02-15 at 12 05 59 PM

https://digitalbazaar.github.io/di-ecdsa-secpr1-2019/#ecdsasecp256r1signature2019

I suppose we might consider this "close enough"... but its not defining:

  • Ed25519
  • Secp256k1
  • Bls12381
  • RSA

I guess if we add up all the CCG specs for key types and signatures, we do indeed have a working solution (except for RSA and P-521).

My point was that its pretty painful to have to review:

If you are saying that this is just a function of where the input documents are today / how they were created, and that indeed our intention truly is to solve this key type and signature type for all entries in:

https://github.com/multiformats/multicodec/blob/master/table.csv#L90

We are aligned, but this feels significantly less concrete than a single document stating this clearly.

We have such a document, here: https://w3c-ccg.github.io/data-integrity-spec/#multikey

Perhaps we don't need all these suites in the "normatively defined" section if we have the link above? (which is already in the normative section?)

This PR (#73) makes it clear that we do intend to address these concerns at least non normatively for existing crypto that we are using today.

I will be happy to remove multikey from the non normative section, or approve your PR if it removes it from this section in the future.

If you require me to remove a non normative reference to the key format we use to address these issues currently, which is already conformant with https://w3c-ccg.github.io/data-integrity-spec/#multikey modulo choices for names, I can do that.

index.html Outdated Show resolved Hide resolved
@OR13
Copy link
Contributor Author

OR13 commented Feb 15, 2022

@msporny I have accepted your change suggestion, we can tackle the rest of the commentary on this thread via this issue:

#54

Copy link

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

Looking at the diffs, it doesn't appear that this adds a reference to the test suite. I'd prefer that we did.

@brentzundel
Copy link
Member

@OR13 what is the status of this PR? Does it still reflect changes you feel are needed, or would it be best to see what happens with PR #77 before merging?

Also, I share Mike's question about a reference to the test suite and agree that is should be included.

<li>Test suites for verifiable credential proof types:
<ul>
<li>VC Data Integrity - Ed25519</li>
<li>VC Data Integrity - BBS+</li>
<li>VC Data Integrity - Json Web Signatures</li>
Copy link
Member

@msporny msporny Feb 25, 2022

Choose a reason for hiding this comment

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

We should collapse all items above to "Test Suites for WG-defined crypographic container formats and suites."

@OR13
Copy link
Contributor Author

OR13 commented Feb 25, 2022

closing due to instability in the section, I will take another stab when it settles.

@OR13 OR13 closed this Feb 25, 2022
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.

None yet

4 participants