Allow generating wrapping keys without PURPOSE_WRAP_KEY with Flight, Fixes AB#3260422

changelog.txt

@@ -1,5 +1,6 @@
 vNext
 ----------
+- [PATCH] Allow generating wrapping key without PURPOSE_WRAP_KEY with Flight (#2633))
 - [MINOR] Adding a state parameter to prevent phish attacks, in “switch_browser” flow. (#2631)
 - [MINOR] Move camera logic to CameraPermissionRequestHandler and add SdmQrPinManager (#2630)
 - [MINOR] Pass Work Profile existence, OS Version, and Manufacturer to ESTS (#2627)

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

@@ -22,7 +22,6 @@
 // THE SOFTWARE.
 package com.microsoft.identity.common.crypto;
 
-import android.annotation.TargetApi;
 import android.content.Context;
 import android.os.Build;
 import android.security.KeyPairGeneratorSpec;
@@ -32,6 +31,7 @@
 import androidx.annotation.RequiresApi;
 
 import com.microsoft.identity.common.internal.util.AndroidKeyStoreUtil;
+import com.microsoft.identity.common.java.controllers.ExceptionAdapter;
 import com.microsoft.identity.common.java.crypto.key.AES256KeyLoader;
 import com.microsoft.identity.common.java.crypto.key.KeyUtil;
 import com.microsoft.identity.common.java.exception.ClientException;
@@ -43,14 +43,14 @@
 import com.microsoft.identity.common.java.opentelemetry.SpanName;
 import com.microsoft.identity.common.java.util.CachedData;
 import com.microsoft.identity.common.java.util.FileUtil;
+import com.microsoft.identity.common.java.util.StringUtil;
 import com.microsoft.identity.common.logging.Logger;
 
 import java.io.File;
 import java.math.BigInteger;
 import java.security.KeyPair;
 import java.security.KeyPairGenerator;
 import java.security.KeyStore;
-import java.security.ProviderException;
 import java.security.spec.AlgorithmParameterSpec;
 import java.util.Calendar;
 import java.util.Locale;
@@ -258,83 +258,153 @@ private void saveSecretKeyToStorage(@NonNull final SecretKey unencryptedKey) thr
         KeyPair keyPair = AndroidKeyStoreUtil.readKey(mAlias);
         if (keyPair == null) {
             Logger.info(methodTag, "No existing keypair. Generating a new one.");
-            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P && CommonFlightsManager.INSTANCE.getFlightsProvider().isFlightEnabled(CommonFlight.ENABLE_NEW_KEY_GEN_SPEC_FOR_WRAP)) {
-                final long keypairGenStartTime = System.currentTimeMillis();
-                final Span span = OTelUtility.createSpanFromParent(SpanName.KeyPairGeneration.name(), SpanExtension.current().getSpanContext());
-                try (final Scope scope = SpanExtension.makeCurrentSpan(span)) {
-                    keyPair = attemptKeyPairGeneration(mAlias, true, keypairGenStartTime);
-                    Logger.info(methodTag, "Successfully generated keypair with new KeyPairGeneratorSpec with wrap purpose.");
-                    span.setAttribute(AttributeName.key_pair_gen_successful_method.name(), "new_key_gen_spec_with_wrap");
-                    span.setStatus(StatusCode.OK);
-                } catch (final ProviderException e) {
-                    if ("SecureKeyImportUnavailableException".equals(e.getClass().getSimpleName())) {
-                        Logger.warn(methodTag, "Wrap purpose may not be supported. Retrying without wrap.");
-                        try {
-                            keyPair = attemptKeyPairGeneration(mAlias, false, keypairGenStartTime);
-                            Logger.info(methodTag, "Successfully generated keypair with new KeyPairGeneratorSpec without wrap purpose.");
-                            span.setAttribute(AttributeName.key_pair_gen_successful_method.name(), "new_key_gen_spec_without_wrap");
-                            span.setStatus(StatusCode.OK);
-                        } catch (final Exception ex) {
-                            // 2nd fallback to legacy keygen spec
-                            Logger.warn(methodTag, "Second attempt without wrap also failed. Falling back to legacy spec."+ ex);
-                            keyPair = generateKeyPairWithLegacySpec(mAlias, keypairGenStartTime);
-                            if (e.getMessage() != null) {
-                                span.setAttribute(AttributeName.keypair_gen_exception.name(), e.getMessage());
-                            }
-                            span.setAttribute(AttributeName.key_pair_gen_successful_method.name(), "legacy_key_gen_spec");
-                            span.setStatus(StatusCode.OK);
-                        }
-                    } else {
-                        Logger.warn(methodTag, "Some unknown exception occurred. Running legacy keygen spec logic."+ e);
-                        keyPair = generateKeyPairWithLegacySpec(mAlias, keypairGenStartTime);
-                        span.setAttribute(AttributeName.key_pair_gen_successful_method.name(), "legacy_key_gen_spec");
-                        span.setStatus(StatusCode.OK);
-                    }
-                } catch (final Throwable throwable) {
-                    Logger.warn(methodTag, "Unexpected error with new KeyPairGeneratorSpec. Falling back to legacy spec. "+ throwable);
-                    keyPair = generateKeyPairWithLegacySpec(mAlias, keypairGenStartTime);
-                    if (throwable.getMessage() != null) {
-                        span.setAttribute(AttributeName.keypair_gen_exception.name(), throwable.getMessage());
-                    }
-                    span.setAttribute(AttributeName.key_pair_gen_successful_method.name(), "legacy_key_gen_spec");
-                    span.setStatus(StatusCode.OK);
-                } finally {
-                    span.end();
-                }
-            }
-            else {
-                // If flight for using new keygen spec is not enabled, use the legacy spec.
-                Logger.info(methodTag, "Using legacy spec for keypair generation directly.");
-                keyPair = AndroidKeyStoreUtil.generateKeyPair(
-                        WRAP_KEY_ALGORITHM, getLegacySpecForKeyStoreKey(mContext, mAlias));
+            final Span span = OTelUtility.createSpanFromParent(SpanName.KeyPairGeneration.name(), SpanExtension.current().getSpanContext());
+            try (final Scope scope = SpanExtension.makeCurrentSpan(span)) {
+                keyPair = generateNewKeyPair();
+                span.setStatus(StatusCode.OK);
+            } catch (final ClientException e) {
+                span.setStatus(StatusCode.ERROR);
+                span.recordException(e);
+                throw e;
+            } finally {
+                span.end();
             }
         }
         final byte[] keyWrapped = AndroidKeyStoreUtil.wrap(unencryptedKey, keyPair, WRAP_ALGORITHM);
         FileUtil.writeDataToFile(keyWrapped, getKeyFile());
     }
 
-    @RequiresApi(api = Build.VERSION_CODES.P)
-    private KeyPair attemptKeyPairGeneration(@NonNull final String alias, boolean useWrapPurpose, long keypairGenStartTime) throws ClientException{
-        KeyPair keyPair = AndroidKeyStoreUtil.generateKeyPair(
-                WRAP_KEY_ALGORITHM, getSpecForKeyStoreKey(alias, useWrapPurpose));
-        recordKeyGenerationTime(keypairGenStartTime);
-        return keyPair;
+    /**
+     * Generate a new key pair wrapping key, based on API level uses different spec to generate
+     * the key pair.
+     * @return a key pair
+     */
+    @NonNull
+    private KeyPair generateNewKeyPair() throws ClientException {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) {
+            return generateNewKeyPairAPI28AndAbove();
+        } else if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
+            return generateNewKeyPairAPI23AndAbove();
+        } else {
+            return generateKeyPairWithLegacySpec();
+        }
     }
 
-    private KeyPair generateKeyPairWithLegacySpec(@NonNull final String alias, long keypairGenStartTime) throws ClientException{
+    /**
+     * Call this for API level >= 28. Starting level API 28 PURPOSE_WRAP_KEY is added. Based on flights
+     * this method may or may not use the PURPOSE_WRAP_KEY along with PURPOSE_ENCRYPT and PURPOSE_DECRYPT. The logic
+     * if (wrap key flight enabled) use all three purposes
+     * else if (new key gen flight enabled) use only encrypt and decrypt purposes
+     * else use legacy spec.
+     * @return key pair
+     */
+    @RequiresApi(Build.VERSION_CODES.P)
+    @NonNull
+    private KeyPair generateNewKeyPairAPI28AndAbove() throws ClientException {
+        if (CommonFlightsManager.INSTANCE.getFlightsProvider().isFlightEnabled(CommonFlight.ENABLE_NEW_KEY_GEN_SPEC_FOR_WRAP_WITH_PURPOSE_WRAP_KEY)) {
+            return generateWrappingKeyPair_WithPurposeWrapKey();
+        } else if (CommonFlightsManager.INSTANCE.getFlightsProvider().isFlightEnabled(CommonFlight.ENABLE_NEW_KEY_GEN_SPEC_FOR_WRAP_WITHOUT_PURPOSE_WRAP_KEY)) {
+            return generateWrappingKeyPair();
+        } else {
+            return generateKeyPairWithLegacySpec();
+        }
+    }
+
+    /**
+     * Call this for API level >= 23. Based on flight new key gen spec is used else legacy which
+     * is deprecated starting API 23.
+     * @return key pair
+     */
+    @RequiresApi(Build.VERSION_CODES.M)
+    @NonNull
+    private KeyPair generateNewKeyPairAPI23AndAbove() throws ClientException {
+        if (CommonFlightsManager.INSTANCE.getFlightsProvider().isFlightEnabled(CommonFlight.ENABLE_NEW_KEY_GEN_SPEC_FOR_WRAP_WITHOUT_PURPOSE_WRAP_KEY)) {
+            return generateWrappingKeyPair();
+        } else {
+            return generateKeyPairWithLegacySpec();
+        }
+    }
+
+    /**
+     * Generate a new key pair wrapping key based on legacy logic. Call this for API < 23 or as fallback
+     * until new key gen specs are stable.
+     * @return key pair generated with legacy spec
+     * @throws ClientException
+     */
+    @NonNull
+    private KeyPair generateKeyPairWithLegacySpec() throws ClientException{
+        final Span span = SpanExtension.current();
         try {
-            final KeyPair keyPair = AndroidKeyStoreUtil.generateKeyPair(
-                    WRAP_KEY_ALGORITHM, getLegacySpecForKeyStoreKey(mContext, alias));
-            recordKeyGenerationTime(keypairGenStartTime);
+            final AlgorithmParameterSpec keyPairGenSpec = getLegacySpecForKeyStoreKey();
+            final KeyPair keyPair = attemptKeyPairGeneration(keyPairGenSpec);
+            span.setAttribute(AttributeName.key_pair_gen_successful_method.name(), "legacy_key_gen_spec");
             return keyPair;
-        } catch (final ClientException e) {
-            SpanExtension.current().recordException(e);
-            SpanExtension.current().setStatus(StatusCode.ERROR);
+        } catch (final Throwable e) {
             Logger.error(TAG + ":generateKeyPairWithLegacySpec", "Error generating keypair with legacy spec.", e);
-            throw e;
+            throw ExceptionAdapter.clientExceptionFromException(e);
         }
     }
 
+    /**
+     * Generate a new key pair wrapping key, based on API level >= 28. This method uses new key gen spec
+     * with PURPOSE_WRAP_KEY. If this fails, it will fallback to generateWrappingKeyPair() which does not use
+     * PURPOSE_WRAP_KEY (still uses new key gen spec).
+     */
+    @RequiresApi(Build.VERSION_CODES.P)
+    private KeyPair generateWrappingKeyPair_WithPurposeWrapKey() throws ClientException {
+        final String methodTag = TAG + ":generateWrappingKeyPair_WithPurposeWrapKey";
+        final Span span = SpanExtension.current();
+        try {
+            Logger.info(methodTag, "Generating new keypair with new spec with purpose_wrap_key");
+            int purposes = KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT | KeyProperties.PURPOSE_WRAP_KEY;
+            final AlgorithmParameterSpec keyPairGenSpec = getSpecForWrappingKey(purposes);
+            final KeyPair keyPair = attemptKeyPairGeneration(keyPairGenSpec);
+            span.setAttribute(AttributeName.key_pair_gen_successful_method.name(), "new_key_gen_spec_with_wrap");
+            return keyPair;
+        } catch (final Throwable e) {
+            Logger.error(methodTag, "Error generating keypair with new spec with purpose_wrap_key." +
+                    "Attempting without purpose_wrap_key." , e);
+            if (!StringUtil.isNullOrEmpty(e.getMessage())) {
+                span.setAttribute(AttributeName.keypair_gen_exception.name(), e.getMessage());
+            }
+            return generateWrappingKeyPair();
+        }
+    }
+
+    /**
+     * Generate a new key pair wrapping key, based on API level >= 23. This method uses new key gen spec
+     * with purposes PURPOSE_ENCRYPT and PURPOSE_DECRYPT. If this fails, it will fallback to generateKeyPairWithLegacySpec()
+     * which uses olg key gen spec.
+     */
+    @RequiresApi(Build.VERSION_CODES.M)
+    private KeyPair generateWrappingKeyPair() throws ClientException {
+        final String methodTag = TAG + ":generateWrappingKeyPair";
+        final Span span = SpanExtension.current();
+        try {
+            Logger.info(methodTag, "Generating new keypair with new spec without wrap key");
+            int purposes = KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT;
+            final AlgorithmParameterSpec keyPairGenSpec = getSpecForWrappingKey(purposes);
+            final KeyPair keyPair = attemptKeyPairGeneration(keyPairGenSpec);
+            span.setAttribute(AttributeName.key_pair_gen_successful_method.name(), "new_key_gen_spec_without_wrap");
+            return keyPair;
+        } catch (final Throwable e) {
+            Logger.error(methodTag, "Error generating keypair with new spec." +
+                    "Attempting with legacy spec.", e);
+            if (!StringUtil.isNullOrEmpty(e.getMessage())) {
+                span.setAttribute(AttributeName.keypair_gen_exception.name(), e.getMessage());
+            }
+            return generateKeyPairWithLegacySpec();
+        }
+    }
+
+    private KeyPair attemptKeyPairGeneration(@NonNull final AlgorithmParameterSpec keyPairGenSpec) throws ClientException{
+        final long keypairGenStartTime = System.currentTimeMillis();
+        final KeyPair keyPair = AndroidKeyStoreUtil.generateKeyPair(
+                WRAP_KEY_ALGORITHM, keyPairGenSpec);
+        recordKeyGenerationTime(keypairGenStartTime);
+        return keyPair;
+    }
+
     private void recordKeyGenerationTime(long keypairGenStartTime) {
         long elapsedTime = System.currentTimeMillis() - keypairGenStartTime;
         SpanExtension.current().setAttribute(AttributeName.elapsed_time_keypair_generation.name(), elapsedTime);
@@ -353,47 +423,33 @@ public void deleteSecretKeyFromStorage() throws ClientException {
     /**
      * Generate a self-signed cert and derive an AlgorithmParameterSpec from that.
      * This is for the key to be generated in {@link KeyStore} via {@link KeyPairGenerator}
-     * Note : This is now only for API level < 28
-     *
-     * @param context an Android {@link Context} object.
+     * Note : This is now only for API level < 23 or as fallback.
+
      * @return a {@link AlgorithmParameterSpec} for the keystore key (that we'll use to wrap the secret key).
      */
-    private static AlgorithmParameterSpec getLegacySpecForKeyStoreKey(@NonNull final Context context,
-                                                                @NonNull final String alias) {
+    private AlgorithmParameterSpec getLegacySpecForKeyStoreKey() {
         // Generate a self-signed cert.
         final String certInfo = String.format(Locale.ROOT, "CN=%s, OU=%s",
-                alias,
-                context.getPackageName());
+                mAlias,
+                mContext.getPackageName());
 
         final Calendar start = Calendar.getInstance();
         final Calendar end = Calendar.getInstance();
         final int certValidYears = 100;
         end.add(Calendar.YEAR, certValidYears);
 
-        return new KeyPairGeneratorSpec.Builder(context)
-                .setAlias(alias)
+        return new KeyPairGeneratorSpec.Builder(mContext)
+                .setAlias(mAlias)
                 .setSubject(new X500Principal(certInfo))
                 .setSerialNumber(BigInteger.ONE)
                 .setStartDate(start.getTime())
                 .setEndDate(end.getTime())
                 .build();
     }
 
-    /**
-     * Generate a self-signed cert and derive an AlgorithmParameterSpec from that.
-     * This is for the key to be generated in {@link KeyStore} via {@link KeyPairGenerator}
-     *
-     * @param alias   the alias for the key.
-     * @param tryPurposeWrap whether to try to use the wrap purpose in the key generation spec.
-     * @return a {@link AlgorithmParameterSpec} for the keystore key (that we'll use to wrap the secret key).
-     */
-    @RequiresApi(api = Build.VERSION_CODES.P)
-    private static AlgorithmParameterSpec getSpecForKeyStoreKey(@NonNull final String alias, boolean tryPurposeWrap) {
-        int purposes = KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT;
-        if (tryPurposeWrap) {
-            purposes |= KeyProperties.PURPOSE_WRAP_KEY;
-        }
-        return new KeyGenParameterSpec.Builder(alias, purposes)
+    @RequiresApi(api = Build.VERSION_CODES.M)
+    private AlgorithmParameterSpec getSpecForWrappingKey(int purposes) {
+        return new KeyGenParameterSpec.Builder(mAlias, purposes)
                 .setKeySize(2048)
                 .setDigests(KeyProperties.DIGEST_SHA256, KeyProperties.DIGEST_SHA512)
                 .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_RSA_PKCS1)

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

@@ -91,9 +91,10 @@ public enum CommonFlight implements IFlightConfig {
     ENABLE_ATTACH_NEW_PRT_HEADER_WHEN_NONCE_EXPIRED("EnableAttachNewPrtHeaderWhenNonceExpired", true),
 
     /**
-     * Flight to enable the new key generation spec for wrap key. Default is true.
+     * Flight to enable the new key generation spec for wrap key using PURPOSE_WRAP_KEY in key gen spec. Default is true.
+     * This is applicable for API >= 28
      */
-    ENABLE_NEW_KEY_GEN_SPEC_FOR_WRAP("EnableNewKeyGenSpecForWrap", true),
+    ENABLE_NEW_KEY_GEN_SPEC_FOR_WRAP_WITH_PURPOSE_WRAP_KEY("EnableNewKeyGenSpecForWrapWithPurposeWrapKey", true),
 
     /**
      * Flight to enable the attachment of PRT header in cross cloud requests. Default is true.
@@ -103,7 +104,12 @@ public enum CommonFlight implements IFlightConfig {
     /**
      * Flight to make the state parameter required for the switch browser protocol.
      */
-    SWITCH_BROWSER_PROTOCOL_REQUIRES_STATE("SwitchBrowserProtocolRequiresState", false);
+    SWITCH_BROWSER_PROTOCOL_REQUIRES_STATE("SwitchBrowserProtocolRequiresState", false),
+
+    /** Flight to enable the new key generation without PURPOSE_WRAP_KEY. Default is true.
+    * This is applicable for API >= 23
+    */
+    ENABLE_NEW_KEY_GEN_SPEC_FOR_WRAP_WITHOUT_PURPOSE_WRAP_KEY("EnableNewKeyGenSpecForWrapWithoutPurposeWrapKey", true);
 
     private String key;
     private Object defaultValue;