okta / okta-oidc-android

OIDC SDK for Android
https://github.com/okta/okta-oidc-android
Other
60 stars 45 forks source link

Fatal Exception: android.security.keystore.KeyStoreConnectException thrown by Huawei Android 10 devices #310

Closed kaichunlin closed 2 years ago

kaichunlin commented 2 years ago

Describe the bug?

We are experiencing crashes from calling SessionClient.isAuthenticated() on some Huawei phones (Mate 20 Pro, P30 Pro), all of them Android 10:

Fatal Exception: android.security.keystore.KeyStoreConnectException: Failed to communicate with keystore service
       at android.security.keystore.AndroidKeyStoreCipherSpiBase.ensureKeystoreOperationInitialized(AndroidKeyStoreCipherSpiBase.java:256)
       at android.security.keystore.AndroidKeyStoreCipherSpiBase.engineDoFinal(AndroidKeyStoreCipherSpiBase.java:495)
       at javax.crypto.Cipher.doFinal(Cipher.java:2055)
       at com.okta.oidc.storage.security.BaseEncryptionManager.decrypt(BaseEncryptionManager.java:299)
       at com.okta.oidc.storage.security.DefaultEncryptionManager.decrypt(DefaultEncryptionManager.java:51)
       at com.okta.oidc.storage.OktaRepository.getDecrypted(OktaRepository.java:208)
       at com.okta.oidc.storage.OktaRepository.get(OktaRepository.java:132)
       at com.okta.oidc.OktaState.getTokenResponse(OktaState.java:46)
       at com.okta.oidc.clients.sessions.SyncSessionClientImpl.isAuthenticated(SyncSessionClientImpl.java:218)
       at com.okta.oidc.clients.sessions.SessionClientImpl.isAuthenticated(SessionClientImpl.java:145)
       <client app code>

What is expected to happen?

No exception is thrown by BaseSessionClient.isAuthenticated()

What is the actual behavior?

An unexpected android.security.keystore.KeyStoreConnectException is thrown, leading to crash

Reproduction Steps?

We don't have the affected Huawei devices to try and reproduce it.

Additional Information?

No response

SDK Version

com.okta.android:okta-oidc-android:1.0.18 com.okta.authn.sdk:okta-authn-sdk-api:2.0.2 com.okta.authn.sdk:okta-authn-sdk-impl:2.0.2 com.okta.sdk:okta-sdk-okhttp:4.1.0

Build Information

No response

aarongranick-okta commented 2 years ago

@kaichunlin Thank you for the report. We will investigate and see if we are able to find a reproduction.

JayNewstrom commented 2 years ago

Looks like this is a known issue in Android 10: https://issuetracker.google.com/issues/147384380?pli=1

It sounds like there might be a workaround for it. Could you give us some more information about how you're configuring the SDK? Specifically if you're customizing the storage/encryption interfaces.

kaichunlin commented 2 years ago

@JayNewstrom That's my initial thought as well, but our crashes do not have java.lang.InterruptedException as its source and so far all the crashes happened on Huawei devices that are not popular among our users, so now I'm not entirely certain it's the cause.

For SDK customizations:

JayNewstrom commented 2 years ago

I think the InterruptedException was just due to how the developer as calling the API (via futures).

In V2 of the SDK, we're using EncryptedSharedPreferences as the default implementation for encryption/storage. It looks like they're doing a sleep and trying the crypto operation again. https://github.com/google/tink/blob/cb814f1e1b69caf6211046bee083a730625a3cf9/java_src/src/main/java/com/google/crypto/tink/integration/android/AndroidKeystoreAesGcm.java#L71

We can/should do the same thing for the implementation we have in this repo.

I created an internal issue for us to implement this. OKTA-488271

JayNewstrom commented 2 years ago

If you need an immediate workaround (and don't mind your users sessions not being converted), you can use a NoEncryption implementation and a EncryptedSharedPreferenceStorage implementation.

This will prevent the default SDK implementation (which is causing the issue you reported), and use the fixed version from Androidx Crypto/Google Tink.

kaichunlin commented 2 years ago

First thanks for the quick replies!

Given it's only happening in a small segment of our user base so far, the workaround may be a bit drastic as it'll also require some data migration as well. We may simply catch the exception and fail the associated operation instead, and sends analytics to understand if the app can recover from this on subsequent retries.

JayNewstrom commented 2 years ago

Released as 1.2.4