[SECURITY] - Multitenancy `requires_external_key` insecure token

[PatStLouis]

There is a conditional which will include both the wallet_id and the wallet_key in the jwt payload. I'm unsure which circumstances triggers this, but including both of these values in the token is unsecure.

acapy/acapy_agent/multitenant/base.py

Line 309 in 6173d9c

if wallet_record.requires_external_key:

[PatStLouis]

Here's the git blame history:
https://github.com/openwallet-foundation/acapy/blame/6173d9c9ea0b36e686504547c587080894186b42/acapy_agent/multitenant/base.py#L309

It seems to be related to managed / unmanaged wallets. I would propose to review this feature and perhaps consider removing it.

[PatStLouis]

There is mention of this in the multitenancy documentation:
http://github.com/openwallet-foundation/acapy/blob/main/docs/features/Multitenancy.md#method-2-get-tenant-token

It also says that this was temporary, 4 years ago. It might be time to review the authentication for multitenancy given the recent work on wallet types.

In unmanaged mode, the get token endpoint also requires the wallet_key parameter to be included in the request body. The wallet key will be included in the JWT so the wallet can be unlocked when making requests to the admin API.

In unmanaged mode, sending the wallet_key to unlock the wallet in every request is not “secure” but keeps it simple at the moment. Eventually, the authentication method should be pluggable, and unmanaged mode would just mean that the key to unlock the wallet is not managed by ACA-Py.

It also mentions that unmanaged mode isn't useable.

⚠️ Although support for unmanaged mode is mostly in place, the receiving of messages from other agents in unmanaged mode is not supported yet. This means unmanaged mode can not be used yet.

[ff137]

Btw, tracing the blame shows the logic was copied from the very first, original implementation here: https://github.com/openwallet-foundation/acapy/blame/2fda2a418a5b5b406ff6c7477dc1f4b3c65f8fb4/aries_cloudagent/multitenant/admin/routes.py#L183

So it's just been untouched since that very first implementation 5 years ago. I'd say it's important for someone to take ownership and make a call on how it should be -- since it's orphaned code (original creator isn't maintaining anymore)

[PatStLouis]

Thanks for having another pair of eyes on this. It seems like it boils down to the unmanaged wallet mode, which is partially implemented and not useable according to the readme (at least in multitenant mode).

Another issue that I see is that the wallet key is used both to unlock the wallet and request a token. This is a broader issue that might be more difficult to address. The multitenancy provider adds a tenant auth system on top of this which addresses this issue.

I wonder what the impact would be to deprecate the token request endpoint for the wallet and instead require the multitenancy provider plugin for tenant authentication.

[swcurran]

Assess as we do other core updates. Currently, the plan would be to remove unmanaged mode entirely, since it has not been completed in the history of the project. Leave open, and we'll consider it later.