Skip to content

Commit 4e91526

Browse files
authored
Debug errors in Keystore layer , Fixes AB#3092835 (#2544)
Issue : We got a couple of IcMs (https://portal.microsofticm.com/imp/v5/incidents/details/561690347/summary) in which we found that the keystore operation "unwrap" is throwing InvalidKey Exception. This is only happening on Pixel 5 devices so far and we have not noticed any recent OS update or security patch released. One of the IcMs says that this has been happening for 6 months now. Although the impacted devices are very low, Auth app team has been receiving at-least one similar IcM every other week. Fix : - Synchronizing the unwrap method. I am not sure if this would work as I have not been able to reproduce the same error on the test devices i have. Hence based on some online resources where other type of keystore errors were observed due to multiple threads accessing the keystore at the same time, I have added synchronized keyword on "unwrap" and 2 other methods. - Also added a telemetry metric to get the exceptions (if it is still happening even after my fix) Fixes [AB#3092835](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3092835)
1 parent e827f2f commit 4e91526

File tree

4 files changed

+38
-5
lines changed

4 files changed

+38
-5
lines changed

changelog.txt

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
vNext
22
----------
33

4+
Version 18.2.2
5+
----------
6+
(common4j 15.2.2)
7+
- [PATCH] Debug errors in Keystore layer (#2544)
8+
49
Version 18.2.0
510
----------
611
(common4j 15.2.0)

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ protected SecretKey generateRandomKey() throws ClientException {
186186
* @return SecretKey. Null if there isn't any.
187187
*/
188188
@Nullable
189-
/* package */ SecretKey readSecretKeyFromStorage() throws ClientException {
189+
/* package */ synchronized SecretKey readSecretKeyFromStorage() throws ClientException {
190190
final String methodTag = TAG + ":readSecretKeyFromStorage";
191191
try {
192192
final KeyPair keyPair = AndroidKeyStoreUtil.readKey(mAlias);

common/src/main/java/com/microsoft/identity/common/internal/util/AndroidKeyStoreUtil.java

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@
2626

2727
import com.microsoft.identity.common.java.exception.ClientException;
2828
import com.microsoft.identity.common.java.logging.Logger;
29+
import com.microsoft.identity.common.java.opentelemetry.AttributeName;
30+
import com.microsoft.identity.common.java.opentelemetry.OTelUtility;
31+
import com.microsoft.identity.common.java.util.ThrowableUtil;
2932
import com.microsoft.identity.common.java.util.ported.DateUtilities;
3033

3134
import java.io.IOException;
@@ -51,6 +54,8 @@
5154
import javax.crypto.SecretKey;
5255

5356
import edu.umd.cs.findbugs.annotations.Nullable;
57+
import io.opentelemetry.api.common.Attributes;
58+
import io.opentelemetry.api.metrics.LongCounter;
5459
import lombok.NonNull;
5560

5661
import static com.microsoft.identity.common.java.exception.ClientException.UNKNOWN_CRYPTO_ERROR;
@@ -84,6 +89,11 @@ private AndroidKeyStoreUtil() {
8489

8590
private static KeyStore mKeyStore;
8691

92+
private static final LongCounter sFailedAndroidKeyStoreUnwrapOperationCount = OTelUtility.createLongCounter(
93+
"failed_keystore_key_unwrap_operation_count",
94+
"Number of failed Android KeyStore unwrap operations"
95+
);
96+
8797
private static synchronized KeyStore getKeyStore()
8898
throws KeyStoreException, CertificateException, NoSuchAlgorithmException, IOException {
8999
if (mKeyStore == null){
@@ -342,7 +352,7 @@ public static synchronized void deleteKey(
342352
* @param wrapAlgorithm the algorithm used to wrap the key.
343353
* @return the wrapped key data blob.
344354
*/
345-
public static byte[] wrap(@NonNull final SecretKey key,
355+
public static synchronized byte[] wrap(@NonNull final SecretKey key,
346356
@NonNull final KeyPair keyToWrap,
347357
@NonNull final String wrapAlgorithm)
348358
throws ClientException {
@@ -397,7 +407,7 @@ public static byte[] wrap(@NonNull final SecretKey key,
397407
* @param wrapAlgorithm the algorithm used to wrap the key.
398408
* @return the unwrapped key.
399409
*/
400-
public static SecretKey unwrap(@NonNull final byte[] wrappedKeyBlob,
410+
public static synchronized SecretKey unwrap(@NonNull final byte[] wrappedKeyBlob,
401411
@NonNull final String wrappedKeyAlgorithm,
402412
@NonNull final KeyPair keyPairForUnwrapping,
403413
@NonNull final String wrapAlgorithm) throws ClientException {
@@ -440,7 +450,15 @@ public static SecretKey unwrap(@NonNull final byte[] wrappedKeyBlob,
440450
exception.getMessage(),
441451
exception
442452
);
443-
453+
if (exception instanceof InvalidKeyException) {
454+
final Attributes attributes = Attributes.builder()
455+
.put(AttributeName.keystore_operation.name(), "unwrap")
456+
.put(AttributeName.error_code.name(), errCode)
457+
.put(AttributeName.error_type.name(), clientException.getClass().getSimpleName())
458+
.put(AttributeName.keystore_exception_stack_trace.name(), ThrowableUtil.getStackTraceAsString(clientException))
459+
.build();
460+
sFailedAndroidKeyStoreUnwrapOperationCount.add(1, attributes);
461+
}
444462
Logger.error(
445463
methodTag,
446464
errCode,

common4j/src/main/com/microsoft/identity/common/java/opentelemetry/AttributeName.java

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -292,5 +292,15 @@ public enum AttributeName {
292292
/**
293293
* Specify the result (or error stack trace) when determining if RT should be returned with AT response.
294294
*/
295-
stop_returning_rt_result
295+
stop_returning_rt_result,
296+
297+
/**
298+
* Indicates the operation name for Android KeyStore.
299+
*/
300+
keystore_operation,
301+
302+
/**
303+
* Indicates the stack trace from a Android KeyStore operation exception.
304+
*/
305+
keystore_exception_stack_trace,
296306
}

0 commit comments

Comments
 (0)