Closed Wind4Greg closed 1 year ago
Review
Thanks for the thorough review, @Wind4Greg! Commentary below:
- Reference to Multiformats is out of date. New reference: https://datatracker.ietf.org/doc/html/draft-multiformats-multibase-06
Yes, agreed that this needs to be updated.
- On example 1 "An Ed25519 public key encoded as a Multikey", Should this example use the context field as shown in section 2.3.1.2 of Data Integrity specification? i.e., add
@context": ["https://w3id.org/security/multikey/v1"],
?
We should probably use https://w3id.org/security/data-integrity/v1
instead. https://w3id.org/security/multikey/v1
is meant to be used when the only thing you need to do is define a multikey in a document. The example you are referring to is a controller document, so we probably want the examples to use the broader https://w3id.org/security/data-integrity/v1
value (after we define Multikey in that file), IIRC.
@dlongley thoughts?
- In example 2 "An Ed25519 public key encoded as a Multikey in a controller document" do we also need to add the additional item "https://w3id.org/security/multikey/v1" to the
@context
array?
No, if we do the thing referenced in the previous question, I think all we have to use is https://w3id.org/security/data-integrity/v1
.
- Section 2.2.1 DataIntegrityProof. It says: "The proofValue property of the proof MUST be a detached EdDSA produced according to [RFC8032], encoded according to [MULTIBASE] using the base58-btc base encoding." Doesn't this need to be produced according to the "Algorithms" section (which includes RFC8032 as part of the procedure.) editorial would revise to: The proofValue property of the proof MUST be a detached EdDSA produced according to section 3 Algorithms"
Yes, good catch, correct.
- Question for clarification. Are DataIntegrityProof and Ed25519Signature2020 two containers for the same information the former being more general? Is one to be preferred? Should something be said about this in the text?
Yes, they are two containers for the same information. DataIntegrityProof
is preferred. Yes, we should say something about this. Ed25519Signature2020
escaped into production deployments and so we're defining it here just to make sure people can interop on it... but we should probably immediately deprecate it and strongly urge people to switch to the more generalized DataIntegrityProof
format.
- Section 3.2 Hashing. Step 3 "Let hashData be the result of concatenating transformedDocumentHash and proofConfigHash." Using VC generated by the CHAPI Playground or Verifiable Credentials JS Library I verified sample credentials Example: Verifying Verifiable Credentials with the order of these to hashes reverse, i.e., in Python
test_combined_hash = proof_hash + doc_hash
. Am I misinterpreting the text or is this an error.
I would expect there to be an error in the specification text. The implementations are the real source of truth, especially https://www.npmjs.com/package/@digitalbazaar/vc. Ping'ing @dlongley to confirm.
- The current test vectors don't seem to verify and could use more elaboration. It would be nice to illustrate all the steps: private and public keys, raw document, options, canonized versions of both, hashes of both, raw signature, encoded signature.
Yes, agreed, and saw that you did this in your review -- THANK YOU! We should replace all the test vectors with your ones.
- I've furnished an example set of test vectors below in with proof type "Ed25519Signature2020".
Great, thank you!
- Test vectors: do we want two different example chains? One corresponding to DataIntegrityProof and one to Ed25519Signature2020, i.e., to produce something like examples 5 and 6 as output.
Yes, that would be helpful.
Test Vector Suggestion
The updated test vectors look great, thank you! Please raise a PR to replace the current test vectors w/ the new ones you generated. I'll have some minor suggestions in the PR, but overall, huge improvement.
Review
@context": ["https://w3id.org/security/multikey/v1"],
?@context
array?test_combined_hash = proof_hash + doc_hash
. Am I misinterpreting the text or is this an error.Test Vector Suggestion
Key Information
Console logged the keypair info from the VC ReSpec plugin...
Notes: The private key above is actually two keys. We only want the first 32 bytes. Can show these in Multibase or hex format.
Credential with Proof
Credential with Proof Removed
Canonized Document without Proof
Hash of Canonized VC w/o Proof
As a hexadecimal string:
Proof Options Document
Canonized Proof Options
Hash of Canonized Proof Options
As a hexadecimal string:
Concatenate and Sign with Private Key.
Concatenation of proof_hash then raw doc_hash (in this order):
Signature in hexadecimal:
Signature Base58btc encoded: