openwallet-foundation / acapy

ACA-Py is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://aca-py.org
Apache License 2.0
419 stars 512 forks source link

rekey feature doesn't work with multitenancy #3307

Closed jamshale closed 1 week ago

jamshale commented 1 month ago

A rekey feature was added a few months ago and works for standalone agents. However, in multitenancy mode rekey-ing the base wallet causes the subwallets to get a AEAD decryption error error. I'm not sure of the relation between the base and subwallet keys that would be causing this.

I think supporting rekey for multitenancy should be supported and this problem fixed. It can essentially work as a key-rotate feature which is a security feature.

Also, it will help any old multitenant deployments (<0.12.0) that created their base wallets with a blank key to upgrade.

kukgini commented 1 month ago

This is a full stack trace for the error. acapy-error-log.txt

What's unusual is that the wallet.key attribute is None in the subwallet profile because I didn't include a wallet key when I created the subwallet with managed type. I don't know yet if this peculiarity is related to the error.

kukgini commented 1 month ago

Here's more information about this issue:

This error occurs when single_wallet_askar_manager opens a subwallet using it's profile config.

As I already described, I didn't set the wallet.key parameter when creating a subwallet, so the key of the subwallet happens to be an empty string.

This is due to a misunderstanding I had about managed mode: I thought that creating a tenant in managed mode meant that the key would be generated and managed by acapy, but this is irrelevant to the error I am faced.

In acapy 1.0.1, when I use the rekey feature to change the base wallet's wallet key from DEFAULT_KEY, which is an empty string, to <key123>, and then call a subwallet/tenant API (e.g. GET /schemas), the profile config passes the key parameter to <key123> instead of the empty string. (This is the key of the base wallet, not the key of the subwallet)

This is not the key that was used to create the subwallet and as a result askar throws an AEAD decryption error.

kukgini commented 4 weeks ago

And the reason why the subwallet profile have base wallet key is that, in the get_wallet_profile() function, single_wallet_askar_manager extands context.settings by setting sub_wallet_settings without wallet.key of the subwallet. see below:

https://github.com/openwallet-foundation/acapy/blob/06d1cf8f9d36fec2d2054ce6d81eba0f962867eb/aries_cloudagent/multitenant/single_wallet_askar_manager.py#L40-L90

kukgini commented 4 weeks ago

I think this issue should be split into a couple of different issues.

Firstly, the main issue could be solved if single_wallet_askar_manager.get_wallet_profile() should set the wallet.key of the subwallet if it is managed. But i'm not sure this is right way to solve it.

There are two things I'd like to see done separately from the main issue.

Second, either the PUT /multitenancy/wallet/{wallet_id}/rekey API should be added to allow rotating the key of a subwallet that is already set incorrectly, or the PUT /multitenancy/wallet/{wallet_id} API should be modified to allow replacing the wallet.key.

Thirdly, POST /multitenancy/wallet should either generate a random wallet key from acapy if the subwallet is created in managed mode, or raise an error indicating that wallet.key is missing, as in unmanaged mode.