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

Incorrect hashing used in tests for ecdsa-rdfc-2019 P-384 #86

Open
sbihel opened this issue Jul 4, 2024 · 6 comments
Open

Incorrect hashing used in tests for ecdsa-rdfc-2019 P-384 #86

sbihel opened this issue Jul 4, 2024 · 6 comments
Assignees

Comments

@sbihel
Copy link

sbihel commented Jul 4, 2024

Hiya, the following tests are failing for P-384 with our implementation:

  • ecdsa-rdfc-2019 (issuers) VC 1.1 The "proof" MUST verify with a conformant verifier.
  • ecdsa-rdfc-2019 (issuers) VC 2.0 The "proof" MUST verify with a conformant verifier.
  • ecdsa-rdfc-2019 (verifiers 1.1) MUST verify a valid VC with an ecdsa-rdfc-2019 proof.
  • ecdsa-rdfc-2019 (verifiers 2.0) MUST verify a valid VC with an ecdsa-rdfc-2019 proof.

After trying some random things, I noticed that if we used SHA-256 for the hash data instead of SHA-384 for P-384 keys, then all the tests would pass. So I'm assuming that the implementation used by the tests is incorrect, but maybe our interpretation of the specs is wrong?

@aljones15
Copy link
Collaborator

Your interpretation is probably correct:

This specification uses either the RDF Dataset Canonicalization Algorithm [RDF-CANON] or the JSON Canonicalization Scheme [RFC8785] to transform the input document into its canonical form. It uses one of two mechanisms to digest and sign: SHA-256 [RFC6234] as the message digest algorithm and ECDSA with Curve P-256 as the signature algorithm, or SHA-384 [RFC6234] as the message digest algorithm and ECDSA with Curve P-384 as the signature algorithm.

I'll look into this when I get a chance, but I think you're correct here and the problem is either that our verifier is incorrect or the implementation is incorrect. the implementation was use locally is DB's: https://github.com/digitalbazaar/ecdsa-rdfc-2019-cryptosuite/tree/main/lib

Thanks for the bug report.

@aljones15
Copy link
Collaborator

@sbihel if you can supply a VC signed with your implementation's p-384 key, I can run it through our stuff locally and debug a bit faster.

@sbihel
Copy link
Author

sbihel commented Jul 4, 2024

@sbihel if you can supply a VC signed with your implementation's p-384 key, I can run it through our stuff locally and debug a bit faster.

Oh yes of course, I forgot. Here is one:

{
  "@context": [
    "https://www.w3.org/ns/credentials/v2"
  ],
  "id": "urn:uuid:7a6cafb9-11c3-41a8-98d8-8b5a45c2548f",
  "type": [
    "VerifiableCredential"
  ],
  "credentialSubject": {
    "id": "did:key:z6MkhTNL7i2etLerDK8Acz5t528giE5KA4p75T6ka1E1D74r"
  },
  "issuer": "did:key:z82LkvutaARmY8poLhUnMCAhFbts88q4yDBmkqwRFYbxpFvmE1nbGUGLKf9fD66LGUbXDce",
  "proof": {
    "type": "DataIntegrityProof",
    "cryptosuite": "ecdsa-rdfc-2019",
    "created": "2024-07-04T13:52:02.458Z",
    "verificationMethod": "did:key:z82LkvutaARmY8poLhUnMCAhFbts88q4yDBmkqwRFYbxpFvmE1nbGUGLKf9fD66LGUbXDce#z82LkvutaARmY8poLhUnMCAhFbts88q4yDBmkqwRFYbxpFvmE1nbGUGLKf9fD66LGUbXDce",
    "proofPurpose": "assertionMethod",
    "proofValue": "z2PiWogU4CmVnUDLNKtUrT2cygxw8JWC4CFA7EzuSG1m5fa3xHsJtUNKQSDWXfdtwB2At3n3ew8RjXEgCNPt58F4jH5EveJ2YmM8sbq1ND8qX66Zcw4i7HdetygcSSi3mmmn2"
  }
}

@aljones15
Copy link
Collaborator

aljones15 commented Jul 5, 2024

Just so this known: here is the ecdsa-rdfc-2019 verifier code: https://github.com/digitalbazaar/ecdsa-multikey/blob/41c5a490a7acbf0fc11d37789768c27cdc1b73f3/lib/factory.js#L29-L50

And when it sees a P-384 it should be using the correct hash, so the issue might be that our implementation's publicKey algorithm is finding the wrong curve.

Interestingly the test project for ecdsa multikey does not actually have a test to ensure that createVerifier works with P-384 so I'll start with adding that: https://github.com/digitalbazaar/ecdsa-multikey/blob/41c5a490a7acbf0fc11d37789768c27cdc1b73f3/test/EcdsaMultikey.spec.js#L29

WRT to your key:

> bs58.decode('82LkvutaARmY8poLhUnMCAhFbts88q4yDBmkqwRFYbxpFvmE1nbGUGLKf9fD66LGUbXDce')
Uint8Array(51) [
  129,  36,   3,  27,  79,  78,  10, 193, 231, 161,
  233, 251,  33, 105, 192, 244,  68, 196,  42,  94,
  205, 152, 252,  26, 146, 145, 232, 244, 163, 135,
  229, 181,   1, 218, 114, 115, 202, 126,  89, 114,
  143, 170, 168,  23,  33, 169,  72, 181, 250, 230,
  179
]
// p384 public key header
> new Uint8Array([0x81, 0x24]);
Uint8Array(2) [ 129, 36 ]

so the verificaitonMethod you gave me does seem to match the P-384 header which should in turn fetch the correct hash.

@aljones15
Copy link
Collaborator

@sbihel just so you know a PR was merged to improve the test suite of ecdsa-multikey on our side. That pr does not show any regressions such as incorrect hashing, so that probably means the bug is in the test suite or your implementation. Taken that the prefix of your did key shows the correct header for P-384 I'm not sure on where the bug is coming from. the primary reason for this message is just to let you know that all of the current test developers are in a sprint that won't be over for a few weeks and no one has time to look at this bug right now so if you have a moment to track it down, please do.

You can check the test project here: https://github.com/digitalbazaar/ecdsa-multikey
Which in turn will depend on:
https://github.com/digitalbazaar/ecdsa-rdfc-2019-cryptosuite
which in turn is used by:
https://github.com/digitalbazaar/vc/tree/update-vc-2.0
which is then used by:
https://github.com/w3c/vc-di-ecdsa-test-suite/blob/main/tests/vc-verifier/index.js

@aljones15
Copy link
Collaborator

aljones15 commented Aug 26, 2024

@sbihel a patch release was made to digital bazaar's ecdsa-rdfc-2019 libraries WRT to the hash used for the P-384 keys: https://github.com/digitalbazaar/ecdsa-rdfc-2019-cryptosuite/blob/main/CHANGELOG.md

This might be relevant to your issue.

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

No branches or pull requests

2 participants