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

rekey feature doesn't work with multitenancy #3307

Closed
jamshale opened this issue Oct 22, 2024 · 4 comments · Fixed by #3312
Closed

rekey feature doesn't work with multitenancy #3307

jamshale opened this issue Oct 22, 2024 · 4 comments · Fixed by #3312
Assignees

Comments

@jamshale
Copy link
Contributor

jamshale commented Oct 22, 2024

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
Copy link
Contributor

kukgini commented Oct 22, 2024

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
Copy link
Contributor

kukgini commented Oct 23, 2024

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
Copy link
Contributor

kukgini commented Oct 23, 2024

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:

async def get_wallet_profile(
self,
base_context: InjectionContext,
wallet_record: WalletRecord,
extra_settings: Optional[dict] = None,
*,
provision=False,
) -> Profile:
"""Get Askar profile for a wallet record.
An object of type AskarProfile is returned but this should not be
confused with the underlying profile mechanism provided by Askar that
enables multiple "profiles" to share a wallet. Usage of this mechanism
is what causes this implementation of BaseMultitenantManager.get_wallet_profile
to look different from others, especially since no explicit clean up is
required for profiles that are no longer in use.
Args:
base_context: Base context to extend from
wallet_record: Wallet record to get the context for
extra_settings: Any extra context settings
provision: Whether to provision the wallet
Returns:
Profile: Profile for the wallet record
"""
extra_settings = extra_settings or {}
if not self._multitenant_profile:
multitenant_wallet_name = base_context.settings.get(
"multitenant.wallet_name", self.DEFAULT_MULTITENANT_WALLET_NAME
)
context = base_context.copy()
sub_wallet_settings = {
"wallet.recreate": False,
"wallet.seed": None,
"wallet.rekey": None,
"wallet.id": None,
"wallet.name": multitenant_wallet_name,
"wallet.type": "askar",
"mediation.open": None,
"mediation.invite": None,
"mediation.default_id": None,
"mediation.clear": None,
"auto_provision": True,
}
context.settings = context.settings.extend(sub_wallet_settings)
profile, _ = await wallet_config(context, provision=False)
self._multitenant_profile = cast(AskarProfile, profile)

@kukgini
Copy link
Contributor

kukgini commented Oct 23, 2024

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.

@jamshale jamshale self-assigned this Oct 24, 2024
@jamshale jamshale linked a pull request Oct 29, 2024 that will close this issue
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 a pull request may close this issue.

2 participants