Skip to content

Conversation

@mohitc1
Copy link
Contributor

@mohitc1 mohitc1 commented May 29, 2025

AndroidWrappedKeyLoader currently has code cache secret key in memory, but there are multiple instances of AndroidWrappedKeyLoader active (one per BrokerPlatformComponents). So, each object gets is own cache and there's no benefit of caching. Moreover, this makes keystore reads more frequent and concurrent.

Changes:
Update caching logic in AndroidWrappedKeyLoader by static ConcurrentHashMap so cache is shared across all ConcurrentHashMap. This fixes caching (effective) logic and reduces keystore unwrap and decrypt calls. A good side effect of this would be lesser number of failures due to Keystore unwrap calls that we have seen.

AndroidKeyStoreUtil changes:
Load keystore always instead of caching, as it is recommended to not cache it statically.

@github-actions
Copy link

❌ Work item link check failed. Description does not contain AB#{ID}.

Click here to Learn more.

@mohitc1 mohitc1 added the Skip-Consumers-Check Only include this if making a breaking change purposefully, and there is an MSAL/ADAL/Broker PR label Jul 9, 2025
@mohitc1 mohitc1 marked this pull request as ready for review July 9, 2025 17:52
Copilot AI review requested due to automatic review settings July 9, 2025 17:52
@mohitc1 mohitc1 requested review from a team as code owners July 9, 2025 17:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR centralizes secret key caching across all AndroidWrappedKeyProvider instances using a static ConcurrentHashMap, and ensures the Android KeyStore is reloaded on each access rather than statically cached.

  • Use a shared static ConcurrentHashMap<String, SecretKey> in AndroidWrappedKeyProvider for effective cross-instance caching
  • Remove per-instance CachedData and introduce getKeyFromCache/clearKeyFromCache helpers
  • Update AndroidKeyStoreUtil to always load a fresh KeyStore on demand
  • Adjust Android test suites and update changelog.txt accordingly
Comments suppressed due to low confidence (3)

common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyProvider.java:186

  • [nitpick] The log message "Key not in cache is empty, loading key from storage" is confusing. Consider renaming it to something clearer like "Cache miss: loading key from storage".
        Logger.info(methodTag, "Key not in cache is empty, loading key from storage");

common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyProvider.java:176

  • Add a unit test to verify that cache entries are evicted when AndroidKeyStoreUtil.canLoadKey(...) returns false or when the key file is missing, to ensure the invalidation logic is correct.
        if (!sSkipKeyInvalidationCheck &&

common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyProvider.java:132

  • [nitpick] Consider adding Javadoc to getKeyFromCache() (and clearKeyFromCache()) to clarify their intended use and thread-safety guarantees.
    /* package */ SecretKey getKeyFromCache() {

@mohitc1 mohitc1 merged commit 45eba57 into dev Jul 14, 2025
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Skip-Consumers-Check Only include this if making a breaking change purposefully, and there is an MSAL/ADAL/Broker PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants