Replace AbstractSecretKeyLoader with ISecretKeyLoader, Fixes AB#3297746 #2666

p3dr0rv · 2025-06-12T00:04:22Z

In the next PR:

We are replacing AbstractSecretKeyLoader with ISecretKeyLoader.
We are introducing AndroidWrappedKeyLoaderFactory, which will return either the current implementation or the new one. For now, regardless of whether the feature flag is enabled or disabled, it will return the same object. In the following PR, this will be updated to return the new implementation based on the flag.
We are also delegating the secret key generation to a new class to separate concerns and improve code readability.

All these changes lay the groundwork for the next PR, where the new implementation will be introduced and the feature flag will be used to switch between the two implementations.

AB#3297746

github-actions · 2025-06-12T00:04:37Z

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

Click here to Learn more.

github-actions · 2025-06-12T00:25:34Z

✅ Work item link check complete. Description contains link AB#3297746 to an Azure Boards work item.

Copilot

Pull Request Overview

This PR replaces the deprecated AbstractSecretKeyLoader with the new ISecretKeyLoader interface, introduces the AndroidWrappedKeyLoaderFactory for future runtime switching of loader implementations, and delegates secret key generation to a dedicated generator class. Key changes include:

Refactoring all key loader references from AbstractSecretKeyLoader to ISecretKeyLoader.
Adding the getCipherTransformation method and updating encryption/decryption flows.
Introducing a feature flag (ENABLE_NEW_ANDROID_WRAPPED_KEY_LOADER) and a new factory to support future loader changes.

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
common4j/src/test/com/microsoft/identity/common/java/crypto/key/AES256SecretKeyGeneratorTest.kt	Added tests for AES256SecretKeyGenerator functionality.
common4j/src/test/com/microsoft/identity/common/java/crypto/StorageEncryptionManagerTest.java	Updated tests to reference ISecretKeyLoader instead of AbstractSecretKeyLoader.
common4j/src/test/com/microsoft/identity/common/java/crypto/MockStorageEncryptionManager.java	Refactored key loader types to use ISecretKeyLoader.
common4j/src/test/com/microsoft/identity/common/java/crypto/MockAES256KeyLoaderWithGetKeyError.java	Added getCipherTransformation implementation.
common4j/src/test/com/microsoft/identity/common/java/crypto/MockAES256KeyLoader.java	Updated secret key generation calls to use the new method.
common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java	Added the new feature flag for the Android wrapped key loader.
common4j/src/main/com/microsoft/identity/common/java/crypto/key/PredefinedKeyLoader.java	Updated key loading and method signatures to work with ISecretKeyLoader.
common4j/src/main/com/microsoft/identity/common/java/crypto/key/AES256KeyLoader.java	Modified to implement ISecretKeyLoader and updated key generator retrieval.
common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyLoaderFactory.kt	Introduced a new factory to choose between loader implementations based on feature flags.
common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyLoader.java	Refactored to use getCipherTransformation and updated key unwrapping logic.
common/src/main/java/com/microsoft/identity/common/crypto/AndroidAuthSdkStorageEncryptionManager.java	Updated to integrate ISecretKeyLoader and the new wrapped key loader factory.
common/src/androidTest/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyLoaderTest.java	Updated tests to reflect changes in key algorithm referencing.

Comments suppressed due to low confidence (1)

common4j/src/main/com/microsoft/identity/common/java/crypto/key/PredefinedKeyLoader.java:53

[nitpick] If rawBytes is expected to be non-null, consider adding a @nonnull annotation for clarity and to prevent potential null-related issues.

public PredefinedKeyLoader(@NonNull final String alias, final byte[] rawBytes) {

common4j/src/main/com/microsoft/identity/common/java/crypto/key/AES256KeyLoader.java

...n/src/androidTest/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyLoaderTest.java

rpdome · 2025-06-19T18:51:06Z

common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyLoaderFactory.kt

+ * specific loader implementation should be used, allowing for runtime switching between
+ * different implementations without affecting client code.
+ */
+object AndroidWrappedKeyLoaderFactory {


Rather than a keyloader factory, it looks like you need a factory for storage layer instead?

two ways to tackle this

the new storage layer can decrypt (read) with both old and new keyloader. it would only write with the new keyloader

the storage re-encrypts its data with the new keyloader, and use the new keyloader to read/write.

If we simply use the flight to switch here - it would lead to user's data loss (since they can't decrypt existing data)

Yeah, I’m planning to address this in the next PR, where I’ll be using strategy 1, see ->

microsoft-authentication-library-common-for-android/common/src/main/java/com/microsoft/identity/common/crypto/AndroidKeyStoreRsaKekManager.kt

Line 111 in db8781a

override fun unwrapKey(wrappedSecretKey: ByteArray, secretKeyAlgorithm: String): SecretKey {

Instead of handling it at the storage layer as you mentioned, I’m just plugging in a new keyloader. This new keyloader will be easier to update if we need to introduce a new algorithm, whether for the key itself or the key-encryption-key.

…ance

mohitc1 · 2025-06-25T04:52:26Z

common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyLoaderFactory.kt

+    fun createWrappedKeyLoader(
+        keyIdentifier: String,
+        fileName: String,
+        context: android.content.Context


android.content.Context

nit: Context

mohitc1 · 2025-06-25T05:00:37Z

common4j/src/main/com/microsoft/identity/common/java/crypto/key/ISecretKeyGenerator.kt

+     * This should be compatible with cryptographic providers, such as
+     * those used with KeyGenerator.getInstance(algorithm).
+     */
+    val keyAlgorithm: String


do we need these?

mohitc1 · 2025-06-25T05:04:14Z

common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyLoader.java

@@ -177,12 +185,11 @@ public synchronized SecretKey getKey() throws ClientException {
        return key;
    }

-    @Override
    @NonNull
    protected SecretKey generateRandomKey() throws ClientException {


protected

private?

mohitc1 · 2025-06-25T05:05:55Z

common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyLoaderFactory.kt

+ * different implementations without affecting client code.
+ */
+object AndroidWrappedKeyLoaderFactory {
+    const val WRAPPED_KEY_KEY_IDENTIFIER: String = "A001"


WRAPPED_KEY_KEY_IDENTIFIER

do you need this here? (maybe for next pr)

mohitc1 · 2025-06-25T05:12:20Z

common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java

+    /**
+     * Flight to enable the new Android wrapped key loader.
+     */
+    ENABLE_NEW_ANDROID_WRAPPED_KEY_LOADER("EnableNewAndroidWrappedKeyLoader", true);


Do you need this in this PR?

You could add this in next PR and factory class

mohitc1 · 2025-06-25T05:17:47Z

common4j/src/main/com/microsoft/identity/common/java/crypto/key/ISecretKeyLoader.kt

+ *
+ * [ISecretKeyLoader] provides a consistent abstraction layer for cryptographic key operations
+ */
+interface ISecretKeyLoader {


ISecretKeyLoader

Given it's functionality, I think, a better name would be ISecretKeyProvider.

your call.

mohitc1

Left some suggestions

add interface ISecretKeyLoader

df9555d

p3dr0rv marked this pull request as ready for review June 12, 2025 00:24

Copilot AI review requested due to automatic review settings June 12, 2025 00:24

p3dr0rv requested a review from a team as a code owner June 12, 2025 00:24

This comment was marked as outdated.

Sign in to view

github-actions bot changed the title ~~Replace AbstractSecretKeyLoader with ISecretKeyLoader~~ Replace AbstractSecretKeyLoader with ISecretKeyLoader, Fixes AB#3297746 Jun 12, 2025

nits

0b256af

p3dr0rv requested a review from Copilot June 12, 2025 03:48

Copilot AI reviewed Jun 12, 2025

View reviewed changes

common4j/src/main/com/microsoft/identity/common/java/crypto/key/AES256KeyLoader.java Outdated Show resolved Hide resolved

p3dr0rv and others added 7 commits June 11, 2025 22:00

nits

1637d5c

nits

baf65c7

fix interface error

e8a1f8f

fix error

f644d7a

fix tests

3e203d5

Merge branch 'dev' into pedroro/interface-for-secretkeyloader

53377dc

Update AndroidWrappedKeyLoader.java

6a96645

rpdome reviewed Jun 19, 2025

View reviewed changes

p3dr0rv added 2 commits June 19, 2025 16:54

Refactor key generation to use a shared AES256SecretKeyGenerator inst…

56aeb2a

…ance

Merge branch 'dev' into pedroro/interface-for-secretkeyloader

a4a3ed4

p3dr0rv added the Skip-Consumers-Check Only include this if making a breaking change purposefully, and there is an MSAL/ADAL/Broker PR label Jun 20, 2025

p3dr0rv added 2 commits June 23, 2025 09:35

Merge branch 'dev' into pedroro/interface-for-secretkeyloader

32a1bba

Merge branch 'dev' into pedroro/interface-for-secretkeyloader

e689c61

mohitc1 reviewed Jun 25, 2025

View reviewed changes

mohitc1 approved these changes Jun 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace AbstractSecretKeyLoader with ISecretKeyLoader, Fixes AB#3297746 #2666

Replace AbstractSecretKeyLoader with ISecretKeyLoader, Fixes AB#3297746 #2666

Uh oh!

p3dr0rv commented Jun 12, 2025 •

edited by azure-boards bot

Loading

Uh oh!

github-actions bot commented Jun 12, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

github-actions bot commented Jun 12, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

rpdome Jun 19, 2025

Uh oh!

rpdome Jun 19, 2025

Uh oh!

p3dr0rv Jun 19, 2025

Uh oh!

mohitc1 Jun 25, 2025

Uh oh!

mohitc1 Jun 25, 2025

Uh oh!

mohitc1 Jun 25, 2025

Uh oh!

mohitc1 Jun 25, 2025

Uh oh!

mohitc1 Jun 25, 2025

Uh oh!

mohitc1 Jun 25, 2025

Uh oh!

mohitc1 left a comment

Uh oh!

Uh oh!

Replace AbstractSecretKeyLoader with ISecretKeyLoader, Fixes AB#3297746 #2666

Are you sure you want to change the base?

Replace AbstractSecretKeyLoader with ISecretKeyLoader, Fixes AB#3297746 #2666

Uh oh!

Conversation

p3dr0rv commented Jun 12, 2025 • edited by azure-boards bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 12, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

github-actions bot commented Jun 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohitc1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

p3dr0rv commented Jun 12, 2025 •

edited by azure-boards bot

Loading