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

Verifiable Credential Data Integrity (and vc-di-eddsa and vc-di-ecdsa) 2023-06-15 -> 2023-07-31 #120

Closed
msporny opened this issue Jun 15, 2023 · 4 comments
Assignees
Labels
LC Working Draft approaching Candidate Recommendation pending This issue needs to get a reviewer assigned to it REVIEW REQUESTED

Comments

@msporny
Copy link

msporny commented Jun 15, 2023

Other comments:

The three specifications listed above are cryptographic message securing mechanisms and are intended to be reviewed together. The first specification, Verifiable Credential Data Integrity, is the base specification that defines the base concepts and algorithms. The "EdDSA Cryptosuite" and "ECDSA Cryptosuite" specifications are concrete implementations of the base specification and each define specific cryptographic algorithms and processes to be used when providing data integrity protection for Verifiable Credentials.

When reviewing the Security and Privacy considerations, it is important to first be aware of the Security and Privacy Considerations for Verifiable Credentials:

and then consider the Security and Privacy considerations provided in the Verifiable Credential Data Integrity specification:

and then finally consider the Security and Privacy considerations for each cryptography suite.

@msporny msporny added LC Working Draft approaching Candidate Recommendation pending This issue needs to get a reviewer assigned to it REVIEW REQUESTED labels Jun 15, 2023
@kdenhartog
Copy link

kdenhartog commented Jul 17, 2023

VC Data Integrity specs

Notes for PING Reviewers

Noting that the terms First Party is not shared between the different VC data model specs. This isn't really an issue, but in general I think of the First party as the issuer, the holder as a UA, and the verifier as a third party so will refer to them within this context. This is the model that was used in the VC Data Model self review as well, but doesn't necessarily align with the specific definitions that are normally used within the browser and PING.

This spec is one of two classes of proof suites (the other is VC-JWT - no PING horizontal review request yet) that is used to produce a signature of the claims. The proofs can be used either to prove the assertions came from the issuer, or to assert the presentation by a holder to the verifier. Specifically with Data Integrity, this spec is standardizing the method of JSON-LD signatures which relies upon

Summary of review

  1. It may be worth adding a pointer back to the VC Data model's privacy considerations section here and throughout the various Cryptographic suites since the privacy considerations are often relevant to the various specs.

  2. When using a proof chain, secondary signers get to know who previous signers are. Which would reveal the same issuer based correlation as highlighted in issue 2 of the VC data model review in this comment. This is not an issue with proof sets because each proof can be independently generated and combined during the verifiable presentation. For this reason, a privacy considerations section should be added to highlight this trust of data being shared between issuers when proof chains are used.

  3. Security section should make mention of key reuse considerations and the impact of reusing a key for multiple algorithms. While some combinations of algorithms have proofs that it's sufficiently secure to reuse keys for various algorithms many do not. This spec should discourage the reuse of keys whenever possible to prevent cryptography attacks that could reveal the private key.

  4. Cryptography suites should be required to define a well known context so that the resolution of the context property can be cached. If this is not done, then the context property can be defined in such a way that it behaves like a unique identifier controlled by the issuer in order to correlate the subject when the verifier resolves the @context JSON-LD document.

ECDSA-2019 spec

  1. Is there value in allowing non-deterministic signatures or should this spec just require the usage of RFC6979 as noted in section 4.2 of the security considerations section, but this seems like an opportunity for the spec to eliminate behavior that has been implemented incorrectly quite a few times and led to private key reveal issues.

  2. This spec doesn't actually define a privacy section, but still has issues that are being used to track topics that still need to be authored.

  3. The spec should highlight the security tradeoffs that occur between section 3.1 and section 3.2 or select one to avoid encountering issue 1 highlighted in the VC Data Model PING review

EdDSA v2022

  1. This spec doesn't actually define a privacy section, but still has issues that are being used to track topics that still need to be authored.
  2. The spec should highlight the security tradeoffs that occur between section 3.1 and section 3.2 or select one to avoid encountering issue 1 highlighted in the [VC Data Model PING review](VC Data Model PING review).

@msporny
Copy link
Author

msporny commented Aug 17, 2023

@kdenhartog and PING, thank you for your thoughtful review of the W3C Data Integrity and cryptosuite specifications. I see no reason why we cannot address each of the items you raise above in the specification text using the suggestions that you provide above.

The next step is to create an issue per item that you raise above in the W3C Data Integrity and cryptosuite repositories and process each one separately as a PR. We will cc you, @kdenhartog, on each issue and PR to ensure that you are given a chance to review and provide feedback on the text that ends up going in the final specification.

@kdenhartog
Copy link

We reviewed these points today during the PING call and there appeared to be consensus agreement to address these points with the exception that the non-deterministic signatures can be left as SHOULD. To do this the WG should add these points as privacy considerations sections to the specification and that would be the only aspects necessary to address this review. Once you get those issues tag me in them and I'll work with the WG to discuss review the text to get them added.

@msporny
Copy link
Author

msporny commented Aug 18, 2023

@kdenhartog Issues have been raised for each of the items mentioned in the PING review and discussed during the PING call yesterday (see above). You have been tagged in each issue and will see progress as we make progress. A fair number of the issues have been tagged as "during CR" (which means that we will add the guidance to the Security and Privacy Considerations sections after we enter the Candidate Recommendation phase, which is expected to happen in a few weeks). Issues that might require normative guidance (or for us to check to make sure there is normative guidance) have been tagged as "before CR" and we will raise PRs as soon as possible to address those issues before we go into CR.

Thank you again for the thorough review and the extra time given on the PING call yesterday to review the concerns and proposed path forward. We really appreciate it! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LC Working Draft approaching Candidate Recommendation pending This issue needs to get a reviewer assigned to it REVIEW REQUESTED
Projects
None yet
Development

No branches or pull requests

3 participants