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

fix: P256 key parity corrections #1137

Merged
merged 5 commits into from
Feb 24, 2023
Merged

fix: P256 key parity corrections #1137

merged 5 commits into from
Feb 24, 2023

Conversation

mirceanis
Copy link
Member

@mirceanis mirceanis commented Feb 24, 2023

What issue is this PR fixing

Example:
closes #1135
fixes #1136

What is being changed

The way publicKeyHex was computed for Secp256r1 keys was not correct, as the parity prefix was getting removed.
This PR fixes that problem and proposes a method for updating existing keys:
https://github.com/uport-project/veramo/blob/663f4b206ec2470126d5b423340045788dbcb3da/__tests__/data.migration.test.ts#L48

The method still has to be invoked manually, but it should be fairly easy to add just before a createAgent call, since all the other classes(KeyManagementSystem, KeyStore, PrivateKeyStore, encryption keys) and data should be available.

This also updates the JWK utils code that was inventing a parity prefix to the public keys, but would have worked only 50% of the time.

Quality

Check all that apply:

  • I want these changes to be integrated
  • I successfully ran pnpm i, pnpm build, pnpm test, pnpm test:browser locally.
  • I allow my PR to be updated by the reviewers (to speed up the review process).
  • I added unit tests.
  • I added integration tests.

@mirceanis mirceanis force-pushed the 1135-p256-key-parity branch 2 times, most recently from 5808f81 to 7be8f0b Compare February 24, 2023 21:40
@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Base: 85.17% // Head: 85.27% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (9279d02) compared to base (70ff72e).
Patch coverage: 89.79% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1137      +/-   ##
==========================================
+ Coverage   85.17%   85.27%   +0.09%     
==========================================
  Files         149      149              
  Lines       15261    15283      +22     
  Branches     1612     1615       +3     
==========================================
+ Hits        12999    13032      +33     
+ Misses       2262     2251      -11     
Impacted Files Coverage Δ
packages/did-provider-jwk/src/jwkDidUtils.ts 74.13% <78.26%> (+2.17%) ⬆️
...ata-store-json/src/identifier/private-key-store.ts 94.11% <100.00%> (+4.75%) ⬆️
...ges/data-store/src/identifier/private-key-store.ts 92.00% <100.00%> (+4.85%) ⬆️
packages/did-provider-jwk/src/jwk-did-provider.ts 66.41% <100.00%> (ø)
packages/kms-local/src/key-management-system.ts 90.08% <100.00%> (+1.10%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mirceanis mirceanis force-pushed the 1135-p256-key-parity branch from 7be8f0b to 9279d02 Compare February 24, 2023 22:12
@mirceanis mirceanis merged commit d0eca2b into next Feb 24, 2023
@mirceanis mirceanis deleted the 1135-p256-key-parity branch February 24, 2023 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants