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.

Sign up for GitHub

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

Jump to bottom

Support P256 keys & did:keys #3442

Merged
merged 16 commits into from
Jan 15, 2025
Merged

Support P256 keys & did:keys #3442

merged 16 commits into from
Jan 15, 2025

Conversation

gmulhearn
Copy link
Contributor

@gmulhearn gmulhearn commented Jan 14, 2025 •
edited
Loading

  • adds p256 did:key resolution (using Multikey VM type)
  • update askarwallet to support sign & verify with p256
  • update JWT signing to get the DID key's JWS Alg rather than hardcode to EdDSA
  • update parts of acapy_agent/wallet/keys/manager.py, removing some hardcoded ed25519 key types

@gmulhearn gmulhearn mentioned this pull request Jan 14, 2025
Merged
@gmulhearn-anonyome
fmt
c6ea8e4 
Signed-off-by: George Mulhearn <[email protected]>
@gmulhearn-anonyome
small test fix
5ba4f82 
Signed-off-by: George Mulhearn <[email protected]>
@gmulhearn-anonyome
testing fixes
e151706 
Signed-off-by: George Mulhearn <[email protected]>
@gmulhearn-anonyome
lint
a2e17fc 
Signed-off-by: George Mulhearn <[email protected]>
@gmulhearn-anonyome
add did key p256 tests
24df30b 
Signed-off-by: George Mulhearn <[email protected]>
@gmulhearn-anonyome
revert supported vm types change
b3399cf 
Signed-off-by: George Mulhearn <[email protected]>
@gmulhearn-anonyome
testing jwt and key manage for p256
c0f5152 
Signed-off-by: George Mulhearn <[email protected]>
@gmulhearn gmulhearn marked this pull request as ready for review January 14, 2025 10:18
jamshale
jamshale reviewed Jan 14, 2025
View reviewed changes
Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

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

Looks well done to me 👍 Will let others get a chance to review.

ff137
ff137 requested changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@ff137 ff137 left a comment

Choose a reason for hiding this comment

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

Overall pretty neat 👍
Just some recommendations for maintainability

acapy_agent/wallet/askar.py Outdated Show resolved Hide resolved
acapy_agent/wallet/askar.py Show resolved Hide resolved
acapy_agent/wallet/jwt.py Show resolved Hide resolved
acapy_agent/wallet/jwt.py Show resolved Hide resolved
acapy_agent/wallet/jwt.py Outdated Show resolved Hide resolved
acapy_agent/wallet/jwt.py Outdated Show resolved Hide resolved
@gmulhearn-anonyome
re review comments
f7dbe65 
Signed-off-by: George Mulhearn <[email protected]>
@gmulhearn-anonyome
typo
46265e0 
Signed-off-by: George Mulhearn <[email protected]>
@gmulhearn gmulhearn requested a review from ff137 January 15, 2025 12:15
ff137
ff137 previously approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@ff137 ff137 left a comment

Choose a reason for hiding this comment

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

Nice, looks good to me 👍

acapy_agent/wallet/jwt.py Show resolved Hide resolved
acapy_agent/wallet/jwt.py Outdated Show resolved Hide resolved
@gmulhearn-anonyome
re review
e742e52 
Signed-off-by: George Mulhearn <[email protected]>
@gmulhearn gmulhearn requested a review from ff137 January 15, 2025 12:34
ff137
ff137 previously approved these changes Jan 15, 2025
View reviewed changes
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
86.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@dbluhm dbluhm merged commit cd3a623 into openwallet-foundation:main Jan 15, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamshale jamshale jamshale left review comments

@ff137 ff137 ff137 approved these changes

@dbluhm dbluhm dbluhm approved these changes

@PatStLouis PatStLouis Awaiting requested review from PatStLouis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gmulhearn @dbluhm @ff137 @jamshale @gmulhearn-anonyome