diff --git a/changelog.txt b/changelog.txt index 8accfd8231..7c5a2d4098 100644 --- a/changelog.txt +++ b/changelog.txt @@ -1,5 +1,6 @@ vNext ---------- +- [PATCH] Fix caching of secret key and add retries for InvalidKeyException during unwrap (#2659) - [MINOR] Replace AbstractSecretKeyLoader with ISecretKeyProvider (#2666) - [MINOR] Update IP phone app teams signature constants to use SHA-512 format (#2700) - [MINOR] Updating handling of ssl error received in Android WebView's onReceivedSslError callback (#2691) diff --git a/common/src/androidTest/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyProviderTest.java b/common/src/androidTest/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyProviderTest.java index e23cd4adb2..c3bfa44ea5 100644 --- a/common/src/androidTest/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyProviderTest.java +++ b/common/src/androidTest/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyProviderTest.java @@ -22,6 +22,8 @@ // THE SOFTWARE. package com.microsoft.identity.common.crypto; +import static com.microsoft.identity.common.java.exception.ClientException.INVALID_KEY; + import android.content.Context; import android.security.KeyPairGeneratorSpec; @@ -47,8 +49,6 @@ import javax.crypto.SecretKey; import javax.security.auth.x500.X500Principal; -import static com.microsoft.identity.common.java.exception.ClientException.INVALID_KEY; - public class AndroidWrappedKeyProviderTest { final Context context = ApplicationProvider.getApplicationContext(); @@ -144,7 +144,7 @@ public void testLoadKey() throws ClientException { final AndroidWrappedKeyProvider keyProvider = new AndroidWrappedKeyProvider(MOCK_KEY_ALIAS, MOCK_KEY_FILE_PATH, context); final SecretKey secretKey = keyProvider.getKey(); - final SecretKey key = keyProvider.getKeyCache().getData(); + final SecretKey key = keyProvider.getKeyFromCache(); Assert.assertNotNull(key); Assert.assertEquals(AES_ALGORITHM, secretKey.getAlgorithm()); Assert.assertArrayEquals(secretKey.getEncoded(), key.getEncoded()); @@ -228,7 +228,7 @@ public void testPerf_NoCachedKey() throws ClientException { long timeStartLoopNotCached = System.nanoTime(); for (int i = 0; i < 100; i++) { - keyProvider.getKeyCache().clear(); + keyProvider.clearKeyFromCache(); keyProvider.getKey(); } long timeFinishLoopNotCached = System.nanoTime(); @@ -246,7 +246,7 @@ public void testLoadDeletedKeyStoreKey() throws ClientException { AndroidKeyStoreUtil.deleteKey(MOCK_KEY_ALIAS); // Cached key also be wiped. - final SecretKey key = keyProvider.getKeyCache().getData(); + final SecretKey key = keyProvider.getKeyFromCache(); Assert.assertNull(key); } @@ -257,7 +257,7 @@ public void testLoadDeletedKeyFile() throws ClientException { FileUtil.deleteFile(getKeyFile()); // Cached key also be wiped. - final SecretKey key = keyProvider.getKeyCache().getData(); + final SecretKey key = keyProvider.getKeyFromCache(); Assert.assertNull(key); } @@ -265,7 +265,7 @@ private AndroidWrappedKeyProvider initkeyProviderWithKeyEntry() throws ClientExc final AndroidWrappedKeyProvider keyProvider = new AndroidWrappedKeyProvider(MOCK_KEY_ALIAS, MOCK_KEY_FILE_PATH, context); final SecretKey key = keyProvider.getKey(); Assert.assertNotNull(key); - Assert.assertNotNull(keyProvider.getKeyCache().getData()); + Assert.assertNotNull(keyProvider.getKeyFromCache()); return keyProvider; } } diff --git a/common/src/androidTest/java/com/microsoft/identity/common/internal/util/AndroidKeyStoreUtilTest.java b/common/src/androidTest/java/com/microsoft/identity/common/internal/util/AndroidKeyStoreUtilTest.java new file mode 100644 index 0000000000..18c0e125a3 --- /dev/null +++ b/common/src/androidTest/java/com/microsoft/identity/common/internal/util/AndroidKeyStoreUtilTest.java @@ -0,0 +1,123 @@ +// Copyright (c) Microsoft Corporation. +// All rights reserved. +// +// This code is licensed under the MIT License. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files(the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions : +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package com.microsoft.identity.common.internal.util; + +import static com.microsoft.identity.common.java.exception.ClientException.INVALID_KEY; +import static com.microsoft.identity.common.java.exception.ClientException.NO_SUCH_ALGORITHM; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.fail; + +import androidx.test.ext.junit.runners.AndroidJUnit4; + +import com.microsoft.identity.common.java.exception.ClientException; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.security.KeyPair; +import java.security.KeyPairGenerator; + +import javax.crypto.SecretKey; +import javax.crypto.spec.SecretKeySpec; + +@RunWith(AndroidJUnit4.class) +public class AndroidKeyStoreUtilTest { + private static final String TEST_WRAP_ALGORITHM = "RSA/ECB/PKCS1Padding"; + private static final String TEST_KEY_ALGORITHM = "AES"; + private static final byte[] TEST_KEY_BYTES = new byte[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}; + private static final byte[] TEST_WRAPPED_KEY_BYTES = new byte[]{16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1}; + + private KeyPair mKeyPair; + private SecretKey mSecretKey; + + @Before + public void setUp() throws Exception { + // Create a real KeyPair for testing + KeyPairGenerator keyGen = KeyPairGenerator.getInstance("RSA"); + keyGen.initialize(2048); + mKeyPair = keyGen.generateKeyPair(); + + // Create a test SecretKey + mSecretKey = new SecretKeySpec(TEST_KEY_BYTES, TEST_KEY_ALGORITHM); + } + + @Test + public void testUnwrap_Success() throws ClientException { + // Test successful wrapping + final byte[] wrapped = AndroidKeyStoreUtil.wrap( + mSecretKey, + mKeyPair, + TEST_WRAP_ALGORITHM + ); + + // Test successful unwrap + final SecretKey unwrapped = AndroidKeyStoreUtil.unwrap( + wrapped, + TEST_KEY_ALGORITHM, + mKeyPair, + TEST_WRAP_ALGORITHM + ); + + assertNotNull(unwrapped); + assertEquals(mSecretKey, unwrapped); + } + + @Test + public void testUnwrap_NoSuchAlgorithm() { + try { + final byte[] wrapped = AndroidKeyStoreUtil.wrap( + mSecretKey, + mKeyPair, + TEST_WRAP_ALGORITHM + ); + + final SecretKey secretKey = AndroidKeyStoreUtil.unwrap( + wrapped, + TEST_KEY_ALGORITHM, + mKeyPair, + "NoAlg" + ); + fail("Should have thrown ClientException"); + } catch (final ClientException e) { + assertEquals(NO_SUCH_ALGORITHM, e.getErrorCode()); + } + } + + @Test + public void testUnwrap_InvalidKey() { + try { + AndroidKeyStoreUtil.unwrap( + TEST_WRAPPED_KEY_BYTES, + TEST_KEY_ALGORITHM, + mKeyPair, + TEST_WRAP_ALGORITHM + ); + fail("Should have thrown ClientException"); + } catch (final ClientException e) { + assertEquals(INVALID_KEY, e.getErrorCode()); + } + } +} diff --git a/common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyProvider.java b/common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyProvider.java index 9db8a20e39..088f728fac 100644 --- a/common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyProvider.java +++ b/common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyProvider.java @@ -31,6 +31,7 @@ import android.security.keystore.KeyProperties; import androidx.annotation.RequiresApi; +import androidx.annotation.VisibleForTesting; import com.microsoft.identity.common.internal.util.AndroidKeyStoreUtil; import com.microsoft.identity.common.java.controllers.ExceptionAdapter; @@ -44,7 +45,6 @@ import com.microsoft.identity.common.java.opentelemetry.OTelUtility; import com.microsoft.identity.common.java.opentelemetry.SpanExtension; 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; @@ -57,6 +57,8 @@ import java.security.spec.AlgorithmParameterSpec; import java.util.Calendar; import java.util.Locale; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import javax.crypto.SecretKey; import javax.security.auth.x500.X500Principal; @@ -109,6 +111,10 @@ public class AndroidWrappedKeyProvider implements ISecretKeyProvider { // Exposed for testing only. /* package */ static final int KEY_FILE_SIZE = 1024; + /** + * SecretKey cache. Maps wrapped secret key file path to the SecretKey. + */ + private static final ConcurrentMap sKeyCacheMap = new ConcurrentHashMap<>(); private final Context mContext; /** @@ -122,21 +128,23 @@ public class AndroidWrappedKeyProvider implements ISecretKeyProvider { */ private final String mFilePath; - private final CachedData mKeyCache = new CachedData() { - @Override - public SecretKey getData() { - if (!sSkipKeyInvalidationCheck && - (!AndroidKeyStoreUtil.canLoadKey(mAlias) || !getKeyFile().exists())) { - this.clear(); - } - return super.getData(); + // Exposed for testing only. + @Nullable + @VisibleForTesting + /* package */ SecretKey getKeyFromCache() { + final String methodTag = TAG + ":getKeyFromCache"; + if (!sSkipKeyInvalidationCheck && + (!AndroidKeyStoreUtil.canLoadKey(mAlias) || !this.getKeyFile().exists())) { + Logger.warn(methodTag, "Key is invalid, removing from cache"); + clearKeyFromCache(); } - }; + return sKeyCacheMap.get(mFilePath); + } // Exposed for testing only. - @NonNull - /* package */ CachedData getKeyCache() { - return mKeyCache; + @VisibleForTesting + /* package */ void clearKeyFromCache() { + sKeyCacheMap.remove(mFilePath); } /** @@ -173,18 +181,23 @@ public String getKeyTypeIdentifier() { @Override @NonNull public synchronized SecretKey getKey() throws ClientException { - SecretKey key = mKeyCache.getData(); + final String methodTag = TAG + ":getKey"; - if (key == null) { - key = readSecretKeyFromStorage(); + SecretKey key = this.getKeyFromCache(); + if (key != null) { + return key; } + Logger.info(methodTag, "Key not in cache or cache is empty, loading key from storage"); + key = readSecretKeyFromStorage(); + // If key doesn't exist, generate a new one. if (key == null) { + Logger.info(methodTag, "Key does not exist in storage, generating a new key"); key = generateRandomKey(); } - mKeyCache.setData(key); + sKeyCacheMap.put(mFilePath, key); return key; } @@ -223,7 +236,7 @@ protected SecretKey generateRandomKey() throws ClientException { // Do not delete the KeyStoreKeyPair even if the key file is empty. This caused credential cache // to be deleted in Office because of sharedUserId allowing keystore to be shared amongst apps. FileUtil.deleteFile(getKeyFile()); - mKeyCache.clear(); + sKeyCacheMap.remove(mFilePath); return null; } @@ -427,7 +440,7 @@ private void recordKeyGenerationTime(long keypairGenStartTime) { public void deleteSecretKeyFromStorage() throws ClientException { AndroidKeyStoreUtil.deleteKey(mAlias); FileUtil.deleteFile(getKeyFile()); - mKeyCache.clear(); + sKeyCacheMap.remove(mFilePath); } /** diff --git a/common/src/main/java/com/microsoft/identity/common/internal/util/AndroidKeyStoreUtil.java b/common/src/main/java/com/microsoft/identity/common/internal/util/AndroidKeyStoreUtil.java index 70ecdcc7fa..787f03ac8e 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/util/AndroidKeyStoreUtil.java +++ b/common/src/main/java/com/microsoft/identity/common/internal/util/AndroidKeyStoreUtil.java @@ -87,8 +87,6 @@ public class AndroidKeyStoreUtil { private AndroidKeyStoreUtil() { } - private static KeyStore mKeyStore; - private static final LongCounter sFailedAndroidKeyStoreUnwrapOperationCount = OTelUtility.createLongCounter( "failed_keystore_key_unwrap_operation_count", "Number of failed Android KeyStore unwrap operations" @@ -96,11 +94,9 @@ private AndroidKeyStoreUtil() { private static synchronized KeyStore getKeyStore() throws KeyStoreException, CertificateException, NoSuchAlgorithmException, IOException { - if (mKeyStore == null){ - mKeyStore = KeyStore.getInstance(ANDROID_KEY_STORE_TYPE); - mKeyStore.load(null); - } - return mKeyStore; + final KeyStore keyStore = KeyStore.getInstance(ANDROID_KEY_STORE_TYPE); + keyStore.load(null); + return keyStore; } /** @@ -307,8 +303,7 @@ public static synchronized void deleteKey( final Throwable exception; final String errCode; try { - final KeyStore keyStore = KeyStore.getInstance(ANDROID_KEY_STORE_TYPE); - keyStore.load(null); + final KeyStore keyStore = getKeyStore(); keyStore.deleteEntry(aliasOfKeyToDelete); return; } catch (final KeyStoreException e) {