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

Cache bleeding between wallets in single-wallet-askar #3471

Open
dbluhm opened this issue Jan 27, 2025 · 8 comments · Fixed by #3470
Open

Cache bleeding between wallets in single-wallet-askar #3471

dbluhm opened this issue Jan 27, 2025 · 8 comments · Fixed by #3470

Comments

@dbluhm
Copy link
Contributor

dbluhm commented Jan 27, 2025

I have evidence that some cached values are being used inappropriately across subwallets when in single-wallet-askar mode. The cache collision occurs when using profile.name as part of a cache key where that is the sole "unique" portion of the key. My superficial scans suggest that the only place this is occurring is in TAA acceptance records but there may be other locations.

To reproduce, I have the following minimal example: https://github.com/Indicio-tech/acapy-minimal-example/blob/test/mt-taa-cache-bleed/examples/mt_taa_test/example.py

The example script does the following:

  • Provisions a new Alice subwallet
  • As Alice, accepts the TAA of the connected ledger
  • Provisions a new Bob subwallet
  • As Bob, retrieves the TAA from ACA-Py, returning the property taa_accepted

The result of Bob's GET /ledger/taa request shows the TAA acceptance from Alice. To verify that this is definitely a cache issue, I did a very simple check: I introduced a sleep exceeding the default ttl of the cache (10 minutes). This delay can be seen commented out in the example script as committed. When this delay is introduced, the value of taa_accepted in the response to Bob's request correctly shows None.

Further investigation revealed that, for subwallets of a single-wallet-askar instance, the profile.name property is always multitenant_sub_wallet. This impacts the following lines:

cache_key = TAA_ACCEPTED_RECORD_TYPE + "::" + self.profile.name

https://github.com/openwallet-foundation/acapy/blob/main/acapy_agent/ledger/indy_vdr.py#L995

These lines represent where the TAA acceptance is recorded and retrieved. The profile.name value not being unique means the cache retrieval is not correctly scoped to the current profile.

@dbluhm
Copy link
Contributor Author

dbluhm commented Jan 27, 2025

The other location where profile.name seems to be used for anything interesting is in anoncreds wallet upgrade. @jamshale your input on how it's used and whether the uniqueness of profile.name is significant would be appreciated.

@dbluhm dbluhm linked a pull request Jan 27, 2025 that will close this issue
@dbluhm
Copy link
Contributor Author

dbluhm commented Jan 27, 2025

The TAA issue leads to subwallet controllers incorrectly thinking they are ready to create ledger artifacts (e.g. cred def) but the timing of the request may result in TAA not accepted errors.

@dbluhm
Copy link
Contributor Author

dbluhm commented Jan 27, 2025

This likely impacts LTS releases; a fix may be required for them as well.

@jamshale
Copy link
Contributor

jamshale commented Jan 27, 2025

Ugghh. This TAA issue was reported a long time ago...I never realized that the profile name wasn't unique for single wallet scenarios... Back when I looked at it I didn't understand the single-wallet vs multi-wallet differences very well.

The uniqueness for the the upgrade would be significant if multiple upgrades were occurring at the same time.

Edit: There also is a failing scenario test I'm looking at. So it might have more implication than I thought.

@jamshale
Copy link
Contributor

The Anoncreds profile has the same issue and also needs to be updated. I don't think any issues with an upgrade are likely but this changed should be added to lts releases. More likely the TAA and related issues will come up.

@jamshale
Copy link
Contributor

For the upgrade it does have a bit of a issue because profile.name was used to indicate which profiles were being upgraded. So it turned on and off the flag for just the sub_wallet profile and all of the profiles were using it, instead of the individual wallet.

@dbluhm
Copy link
Contributor Author

dbluhm commented Jan 27, 2025

Reopening to use this as a place to discuss what LTS releases are needed.

cc @swcurran

@dbluhm dbluhm reopened this Jan 27, 2025
@swcurran
Copy link
Contributor

0.12.x is the only LTS we have, so that would be what is needed. We EOL’d 0.11.x this month, so no need to include it.

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.

3 participants