-
Notifications
You must be signed in to change notification settings - Fork 32
Fix KeyStoreException crash on Nexus 5 devices (MOB-11856) #934
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Wrap keyStore.setEntry() in try-catch to handle SecretKeyEntry not supported - Add encryptor initialization error handling in keychain with graceful fallback - Resolves MOB-11856 with minimal changes
- Add encryptor initialization error handling in keychain with graceful fallback - KeyStoreException bubbles up naturally and gets handled properly - Resolves MOB-11856 with truly minimal changes
- Test documents expected behavior when IterableDataEncryptor initialization fails - Verifies graceful fallback to plaintext storage continues to work - Ensures app functionality is maintained after encryption failure
- Reset IterableDataEncryptor.kt to match master exactly - Only IterableKeychain.kt and test should have changes for minimal fix
encryptor = IterableDataEncryptor() | ||
IterableLogger.v(TAG, "SharedPreferences being used with encryption") | ||
try { | ||
encryptor = IterableDataEncryptor() | ||
IterableLogger.v(TAG, "SharedPreferences being used with encryption") | ||
} catch (e: Exception) { | ||
IterableLogger.e(TAG, "Failed to initialize encryption, falling back to plain text", e) | ||
handleDecryptionError(e) | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good. It actually prevents crashes which were reproducible
`when`(mockSharedPrefs.getString(eq("iterable-email"), isNull())).thenReturn(testEmail) | ||
`when`(mockSharedPrefs.getString(eq("iterable-user-id"), isNull())).thenReturn(testUserId) | ||
`when`(mockSharedPrefs.getBoolean(eq("iterable-email_plaintext"), eq(false))).thenReturn(true) | ||
`when`(mockSharedPrefs.getBoolean(eq("iterable-user-id_plaintext"), eq(false))).thenReturn(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is artificially forcing the behavior it wants to test:
It mocks getString() to return the test values
It mocks getBoolean() to return true for the _plaintext flags
Then it calls the methods and verifies they work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. However, test methods might be misleading.
`when`(mockSharedPrefs.getString(eq("iterable-email"), isNull())).thenReturn(testEmail) | ||
`when`(mockSharedPrefs.getString(eq("iterable-user-id"), isNull())).thenReturn(testUserId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For failure scenario, we should check if iterable-email
is actually stored null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ayyanchira I have added the failure scenario now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
- Address reviewer feedback: test now properly simulates null data after encryption failure - Verify that when encryption fails, existing data returns null (graceful degradation) - Test validates the actual failure behavior, not just successful plaintext storage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit pick:
One addition to test we can add is to verify iterable-email-plaintext
to have the stored value.
Problem
The Android SDK was crashing on Nexus 5 devices during initialization with:
This occurs because older Android KeyStore implementations (like on Nexus 5/API 23) don't support
SecretKeyEntry
for AES keys - they only supportPrivateKeyEntry
andTrustedCertificateEntry
.Related Ticket: MOB-11856
Solution
Minimal fix - added try-catch around
IterableDataEncryptor()
initialization in the keychain:What happens now on Nexus 5:
IterableDataEncryptor()
constructor fails withKeyStoreException
handleDecryptionError()
is called → encryption disabled permanentlyChanges:
IterableKeychain.kt
- wrap encryptor creation in try-catchTesting
testEncryptorInitializationFailureScenario()
validates fallback behaviorManual Test Plan
API 23 Emulator Test:
Functionality Test:
Resolves MOB-11856