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

Update Ed25519 and ES256K Test Vectors and Ensure Low-S ECDSA Signatures #375

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

frankhinek
Copy link
Contributor

This PR will:

  • Migrate to using Web5 SDK Test Vectors for Ed25519 and Secp256k1 sign() and verify() methods.
  • Ensure ES256K signatures are low-S form in @web5/crypto and @web5/crypto-aws-kms.

Note

A future PR will switch to using git submodules for Web5 SDK Test Vectors.

@frankhinek frankhinek added dsa Cryptographic Digital Signature Algorithms key-mgmt Key Management package: crypto @web5/crypto package package: crypto-aws-kms @web5/crypto-aws-kms package labels Jan 13, 2024
@frankhinek frankhinek self-assigned this Jan 13, 2024
Copy link

codesandbox bot commented Jan 13, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link
Contributor

github-actions bot commented Jan 13, 2024

TBDocs Report

✅ No errors or warnings

@web5/api

  • Project entry file: packages/api/src/index.ts

TBDocs Report Updated at 2024-01-13T18:41:16Z 6994521

Copy link

codecov bot commented Jan 13, 2024

Codecov Report

Merging #375 (6994521) into main (44c38a1) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #375      +/-   ##
==========================================
+ Coverage   91.85%   91.90%   +0.05%     
==========================================
  Files          67       67              
  Lines       18163    18241      +78     
  Branches     1543     1550       +7     
==========================================
+ Hits        16683    16764      +81     
+ Misses       1457     1454       -3     
  Partials       23       23              
Components Coverage Δ
agent 88.71% <ø> (ø)
api 96.94% <ø> (+0.21%) ⬆️
common 98.57% <ø> (ø)
credentials ∅ <ø> (∅)
crypto 94.60% <100.00%> (+0.06%) ⬆️
dids 92.24% <ø> (ø)
identity-agent 56.81% <ø> (ø)
crypto-aws-kms 100.00% <100.00%> (ø)
proxy-agent 58.43% <ø> (ø)
user-agent 55.22% <ø> (ø)

@frankhinek frankhinek added the testing related to new or existing tests label Jan 13, 2024

const signatureHex = Convert.uint8Array(signature).toHex();
expect(signatureHex).to.deep.equal(vector.output);
describe('Web5TestVectorsCryptoEd25519', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

all the other examples are 1 level deep:

describe('PresentationExchange', () -> describe('Web5TestVectorsPresentationExchange', () 
describe('Verifiable Credential Tests', () -> describe('Web5TestVectorsCredentials', ()

yours are 2 levels deep

describe('Ed25519', () -> describe('verify()', () -> describe('Web5TestVectorsCryptoEd25519', ()

I -think- it should work and get to the checkmark screen, but if it doesn't then this will probably be the reason.

I think we should put all vector test in one file eventually for better ease of use, but we can discuss that offline

Copy link
Contributor

@finn-block finn-block Jan 16, 2024

Choose a reason for hiding this comment

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

junit xml doesnt have a way to do more than one class deep as far as i know, so the mocha junit reporter thing will just smush it down, which may or may not pass the regex. Probably best to just keep doing it the way we've established works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's give it a shot and see how it does.

I had the same thought but the reason I ended up nesting them is that, otherwise, it isn't clear by looking at the tests that those methods are being rigorously tested.

@frankhinek frankhinek merged commit 12f7af5 into main Jan 16, 2024
32 checks passed
@frankhinek frankhinek deleted the test-vectors-crypto branch January 16, 2024 18:22
finn-block pushed a commit that referenced this pull request Mar 19, 2024
…res (#375)

* Migrate to using Web5 Test Vectors for Ed25519 and Secp256k1 sign and verify
* Ensure ES256K signatures are low-S form in crypto and crypto-aws-kms
finn-block pushed a commit that referenced this pull request Mar 19, 2024
…res (#375)

* Migrate to using Web5 Test Vectors for Ed25519 and Secp256k1 sign and verify
* Ensure ES256K signatures are low-S form in crypto and crypto-aws-kms
finn-block pushed a commit that referenced this pull request Mar 19, 2024
…res (#375)

* Migrate to using Web5 Test Vectors for Ed25519 and Secp256k1 sign and verify
* Ensure ES256K signatures are low-S form in crypto and crypto-aws-kms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dsa Cryptographic Digital Signature Algorithms key-mgmt Key Management package: crypto @web5/crypto package package: crypto-aws-kms @web5/crypto-aws-kms package testing related to new or existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants