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

Allow specifying key derivation method on sub-wallet create #1719

Merged
merged 14 commits into from
Apr 15, 2022

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Apr 7, 2022

When working in managed multi-tenancy mode, we store the keys for the sub-wallets within a WalletRecord in ACA-Py. These keys are then recalled when needed and used to unlock the wallet for processing messages or admin API requests intended for that sub-wallet. In the process of opening the wallet, the key is pushed through a key derivation algorithm to transform it into a wallet encryption key. This key derivation algorithm is a costly operation.

This PR implements allowing the key derivation method to be specified when creating a sub-wallet. This means that a multi-tenant ACA-Py agent can elect to use the RAW derivation method, for instance, and significantly reduce the wallet open cost.

Since these keys are securely stored in ACA-Py, using a RAW key does not seem to impact security of the key. Additionally, I don't think the profile of potential attack changes from using the key derivation method when considering wallet tokens.

Credit to @burdettadam.

burdettadam and others added 11 commits March 15, 2022 16:05
Signed-off-by: Adam Burdett <burdettadam@gmail.com>
Signed-off-by: Adam Burdett <burdettadam@gmail.com>
Signed-off-by: Adam Burdett <burdettadam@gmail.com>
Signed-off-by: Adam Burdett <burdettadam@gmail.com>
Signed-off-by: Adam Burdett <burdettadam@gmail.com>
Signed-off-by: Adam Burdett <burdettadam@gmail.com>
Signed-off-by: Adam Burdett <burdettadam@gmail.com>
ianco
ianco previously approved these changes Apr 12, 2022
Copy link
Contributor

@ianco ianco left a comment

Choose a reason for hiding this comment

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

LGTM

@swcurran
Copy link
Contributor

@dbluhm -- can you please update your branch for merging?

@codecov-commenter
Copy link

Codecov Report

Merging #1719 (2c37713) into main (66735f3) will decrease coverage by 0.01%.
The diff coverage is 78.26%.

@@            Coverage Diff             @@
##             main    #1719      +/-   ##
==========================================
- Coverage   95.25%   95.23%   -0.02%     
==========================================
  Files         528      528              
  Lines       33120    33132      +12     
==========================================
+ Hits        31547    31554       +7     
- Misses       1573     1578       +5     

@swcurran
Copy link
Contributor

Back out of sync @dbluhm -- and the "Update" button is not available. Over to you. I'll merge this next once done :-)

dbluhm and others added 2 commits April 13, 2022 13:35
@burdettadam burdettadam dismissed stale reviews from andrewwhitehead and ianco via 45de356 April 14, 2022 21:13
@swcurran swcurran merged commit ef8e258 into openwallet-foundation:main Apr 15, 2022
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.

6 participants