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 3 ecdsa-sd-2023 proof value tests (D -> C) #101

Open
wants to merge 7 commits into
base: add-cbor-tests-sd
Choose a base branch
from

Conversation

aljones15
Copy link
Contributor

@aljones15 aljones15 commented Nov 17, 2024

Adds 3 proofValue related tests:

  1. "If the proofValue string does not start with u, indicating that it is a multibase-base64url-no-pad-encoded value, an error MUST be raised and SHOULD convey an error type of PROOF_VERIFICATION_ERROR"
  "proof": {
    "type": "DataIntegrityProof",
    "created": "2024-11-17T15:51:32Z",
    "verificationMethod": "did:key:zDnaepBuvsQ8cpsWrVKw8fbpGpvPeNSjVPTWoq6cRqaYzBKVP#zDnaepBuvsQ8cpsWrVKw8fbpGpvPeNSjVPTWoq6cRqaYzBKVP",
    "cryptosuite": "ecdsa-sd-2023",
    "proofPurpose": "assertionMethod",
    "proofValue": "2V0BhVhANk3uoC2QPhe9knhEUtqpGS9cdbp1oRKf3SZ9v1iUO-UKS-hFqgacRYOrCSh8duVDK_zY5N5Hndkq8l7VRVAMNFgjgCQDR-DKGWfNv3Kq9TKO1wKXsgvmVlsjqhTWw4U8N-zL23yHWEAjupTpyzvis37wkzIwiXKuizfcF9VvpZfMx7uF2tVW3Al-BbTXQy_HO-EOM3WdH_sZO7K0CP_8fgGMk3UwsmrzWEDBizvJxii_nXROPhqvulJp1e27ZmixVmPUm9ZMT-zl-WAoocGzZnLfwdP_ujRfZVSs_CSJRnz87cqKtgRxG0HqWEBTp4qhxcXJbNVup9iCZAlHLt5oKEYNDEn2KWFMeUqBtwFAWjbVDOQQbPiiZ8XN6Ko8sD0MsSmQ28YkyQD9sir5WEA00uDbdz-gaeeQwPMm4gO-eLWSoIx9LRGLK8F5QEJGa4amwfn8eJ178DUCpr1BRZoGiJAkVR8AzE1Xk8vuXF-JWEAgknRMffrzujB2IW79_1O0SnQf9NyOzPpsws5dbhD8l5PsrzQI6HFaZrhSNayha0ppCw50UdtMo2zSRXWYoVMRWECaWckwEmY6BgddbWySxeYgUwHB3p72kxQ7Eg-9Idd5Z9fEcj8tFwCFDjZJJdXKhBSM71J8dAf9HVZo12Wm51_CWECmmVvtaKWgUlvRoPN6G6wfV25e4fPeo4d8l1ApSllxZ19Ls7-uCRl2jE28NbrCvzmPfg3mLbQBRhM3fabvdcbhoQBYIKzZT5KJrjiCO1Dhv0Ww6rqGS8K_yY56UpQc9j12-oHohAECBAU"
  }
  1. The base proof assertion: "If the decodedProofValue does not start with the ECDSA-SD base proof header bytes 0xd9, 0x5d, and 0x00, an error MUST be raised and SHOULD convey an error type of PROOF_VERIFICATION_ERROR." Is tested by examing the base proof from the issuer. As base proofs are not designed to pass verification the statement from the spec might be incorrect.

  2. "If the decodedProofValue does not start with the ECDSA-SD disclosure proof header bytes 0xd9, 0x5d, and 0x01, an error MUST be raised and SHOULD convey an error type of PROOF_VERIFICATION_ERROR."

The invalid disclosure proof header bytes are [0xA1, 0x44, 0x01] and replace the original proof value header bytes. The actual proofValue is left as is and the invalid header bytes are encoded as a base64 url with the prefix u.

Additionally:

  • DRYs up an assertion on base proof proofValues into a single assertion
  • Changes several async function related to decoding bs58 and bs64 strings into non-async functions
  • Changes relevant helpers and assertions into non-async functions if the function only depended on a bs decode function previously.

@aljones15 aljones15 self-assigned this Nov 17, 2024
@aljones15 aljones15 changed the base branch from main to add-cbor-tests-sd November 17, 2024 18:01
@aljones15 aljones15 changed the title Add 3 sd proof value tests Add 3 ecdsa-sd-2023 proof value tests Nov 17, 2024
@aljones15 aljones15 marked this pull request as ready for review November 17, 2024 18:04
@aljones15 aljones15 changed the title Add 3 ecdsa-sd-2023 proof value tests Add 3 ecdsa-sd-2023 proof value tests (D -> C) Nov 17, 2024
Comment on lines +190 to +200
`Expected VC from issuer ${name} to have an ' +
'"ecdsa-sd-2023" proof`).to.exist;
expect(
proof.proofValue,
`Expected VC from issuer ${name} to have a ' +
'"proof.proofValue"`
).to.exist;
expect(
proof.proofValue,
`Expected VC "proof.proofValue" from issuer ${name} to be ` +
'a string.'
Copy link
Member

@TallTed TallTed Nov 18, 2024

Choose a reason for hiding this comment

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

Either all Expect ... clauses should end with a fullstop ., or none should. I prefer the former.

I also note mixed up use of ' and `. I strongly advise that these be used consistently, especially but not only in any given statement.

I'm not going to make all the suggestions the above would call for, unless you request that I do so. (I'm betting that changes will be needed throughout the documents touched by this PR, and possibly across other documents.)

Suggested change
`Expected VC from issuer ${name} to have an ' +
'"ecdsa-sd-2023" proof`).to.exist;
expect(
proof.proofValue,
`Expected VC from issuer ${name} to have a ' +
'"proof.proofValue"`
).to.exist;
expect(
proof.proofValue,
`Expected VC "proof.proofValue" from issuer ${name} to be ` +
'a string.'
'Expected VC from issuer ${name} to have an ' +
'"ecdsa-sd-2023" proof.').to.exist;
expect(
proof.proofValue,
'Expected VC from issuer ${name} to have a ' +
'"proof.proofValue".'
).to.exist;
expect(
proof.proofValue,
'Expected VC "proof.proofValue" from issuer ${name} to be ' +
'a string.'

Comment on lines +207 to +208
`Expected "proof.proofValue" to start with u received ` +
`${proof.proofValue[0]}`).to.be.true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`Expected "proof.proofValue" to start with u received ` +
`${proof.proofValue[0]}`).to.be.true;
'Expected "proof.proofValue" to start with u received ' +
'${proof.proofValue[0]}.').to.be.true;

Comment on lines +209 to +212
// now test the encoding which is bs64 url no pad for this suite
expect(
shouldBeBs64UrlNoPad(proof.proofValue),
'Expected "proof.proofValue" to be bs64 url no pad encoded.'
Copy link
Member

Choose a reason for hiding this comment

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

When aimed at humans, best to use the full string for "bs64 url no pad encoded".

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.

2 participants