Android key store encryption refactor [No Op] #3144
Conversation
📝 WalkthroughWalkthroughThis set of changes introduces a modular and extensible key management system for encryption in the application. The previous logic for handling encryption keys, which was embedded in the Sequence Diagram(s)sequenceDiagram
participant Application
participant EncryptionKeyProvider
participant KeystoreHandler
participant RsaKeystoreHandler
participant AesKeystoreHandler
participant AndroidKeyStore
Application->>EncryptionKeyProvider: getKeyForEncryption()/getKeyForDecryption()
EncryptionKeyProvider->>EncryptionKeyProvider: getHandler(isEncrypt)
alt If RSA key exists
EncryptionKeyProvider->>RsaKeystoreHandler: getKeyOrGenerate()
RsaKeystoreHandler->>AndroidKeyStore: doesKeyExists(alias)
alt Key exists
RsaKeystoreHandler->>AndroidKeyStore: retrieve RSA Key
else Key does not exist
RsaKeystoreHandler->>AndroidKeyStore: generate RSA Key
end
RsaKeystoreHandler-->>EncryptionKeyProvider: EncryptionKeyAndTransform
else If Android M+ and no RSA key
EncryptionKeyProvider->>AesKeystoreHandler: getKeyOrGenerate()
AesKeystoreHandler->>AndroidKeyStore: doesKeyExists(alias)
alt Key exists
AesKeystoreHandler->>AndroidKeyStore: retrieve AES Key
else Key does not exist
AesKeystoreHandler->>AndroidKeyStore: generate AES Key
end
AesKeystoreHandler-->>EncryptionKeyProvider: EncryptionKeyAndTransform
end
EncryptionKeyProvider-->>Application: EncryptionKeyAndTransform
Suggested labels
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
app/unit-tests/src/org/commcare/utils/EncryptionUtilsTest.java (1)
38-38: Consider using getKeyForEncryption() for encryption operations.In the
testDecryption()method, you're usinggetKeyForDecryption()to obtain a key for the encryption step. For consistency and clarity, consider usinggetKeyForEncryption()for encryption operations, even in decryption tests.- EncryptionKeyAndTransform kat = provider.getKeyForDecryption(); + EncryptionKeyAndTransform kat = provider.getKeyForEncryption();app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (2)
259-259: Method call added but implementation is missing.The call to
storeBiometricInvalidationKey()is added at the appropriate place when biometric or PIN configuration is confirmed, but the method implementation is empty.This appears to be preparation for biometric key invalidation functionality. Would you like me to help implement this method or open an issue to track this TODO?
273-275: Empty method requires implementation.The
storeBiometricInvalidationKey()method is declared but contains no implementation, making it a no-op currently.Based on the method name and context within the encryption key management refactor, this method should likely handle storing or managing biometric invalidation keys. Would you like me to generate an implementation or create a TODO issue for this?
app/src/org/commcare/android/security/AndroidKeyStore.kt (1)
8-19: LGTM: Well-designed KeyStore singleton with minor style nitpick.The singleton design with lazy initialization is appropriate for centralizing Android KeyStore access. The API is clean and the KeyStore is properly initialized.
Minor style improvement:
fun doesKeyExists(alias: String): Boolean { - return instance.containsAlias(alias); + return instance.containsAlias(alias) }The semicolon is not needed in Kotlin.
app/src/org/commcare/android/security/AesKeystoreHandler.kt (1)
21-33: LGTM: Solid AES key management implementation with minor suggestion.The implementation correctly handles key retrieval and generation with appropriate security configurations. The use of
EncryptionKeyAndTransformwith the standard AES/CBC/PKCS7Padding transformation is appropriate.Consider adding explicit type checking for robustness:
val key = if (entry is KeyStore.SecretKeyEntry) { entry.secretKey + } else if (entry != null) { + // Log unexpected key type and regenerate + generateAesKey(alias, needsUserAuth) } else { generateAesKey(alias, needsUserAuth) }This would handle cases where a key exists but is not the expected SecretKey type.
app/src/org/commcare/android/security/RsaKeystoreHandler.kt (1)
15-15: Fix typo in documentation.-* User for Pre Android M devices to do RSA encryption with Android Key Store +* Used for Pre Android M devices to do RSA encryption with Android Key Storeapp/src/org/commcare/utils/EncryptionKeyProvider.java (1)
45-45: Consider making user authentication configurable.The user authentication requirement is hardcoded to
false. Consider making this configurable for future flexibility.For future extensibility, consider making the user authentication requirement configurable through a constructor parameter or system property, especially since the PR objectives mention making the code easier to extend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/src/org/commcare/CommCareApplication.java(1 hunks)app/src/org/commcare/android/security/AesKeystoreHandler.kt(1 hunks)app/src/org/commcare/android/security/AndroidKeyStore.kt(1 hunks)app/src/org/commcare/android/security/KeystoreHandler.kt(1 hunks)app/src/org/commcare/android/security/RsaKeystoreHandler.kt(1 hunks)app/src/org/commcare/connect/database/ConnectDatabaseUtils.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java(2 hunks)app/src/org/commcare/utils/EncryptionKeyProvider.java(1 hunks)app/unit-tests/src/org/commcare/CommCareTestApplication.java(1 hunks)app/unit-tests/src/org/commcare/utils/EncryptionUtilsTest.java(4 hunks)app/unit-tests/src/org/commcare/utils/MockEncryptionKeyProvider.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
app/unit-tests/src/org/commcare/utils/EncryptionUtilsTest.java (1)
app/unit-tests/src/org/commcare/CommCareTestApplication.java (1)
CommCareTestApplication(65-327)
app/src/org/commcare/connect/database/ConnectDatabaseUtils.java (1)
app/src/org/commcare/CommCareApplication.java (1)
CommCareApplication(154-1260)
app/src/org/commcare/android/security/AesKeystoreHandler.kt (1)
app/src/org/commcare/utils/EncryptionKeyAndTransform.java (1)
EncryptionKeyAndTransform(10-39)
app/src/org/commcare/android/security/RsaKeystoreHandler.kt (2)
app/src/org/commcare/android/security/AndroidKeyStore.kt (1)
doesKeyExists(16-18)app/src/org/commcare/utils/EncryptionKeyAndTransform.java (1)
EncryptionKeyAndTransform(10-39)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (11)
app/unit-tests/src/org/commcare/CommCareTestApplication.java (1)
81-81: LGTM! Context parameter correctly added.The change appropriately updates the
MockEncryptionKeyProviderinstantiation to pass the application context, aligning with the refactored constructor that now requires aContextparameter.app/src/org/commcare/android/security/KeystoreHandler.kt (1)
5-7: Excellent interface design!The
KeystoreHandlerinterface provides a clean abstraction for key management with a single, well-named method that clearly indicates its dual purpose of retrieving existing keys or generating new ones when needed.app/unit-tests/src/org/commcare/utils/EncryptionUtilsTest.java (3)
20-20: LGTM! Provider instantiation correctly updated.The change properly updates the
MockEncryptionKeyProviderinstantiation to include the required context parameter fromCommCareTestApplication.instance().
26-26: LGTM! API methods correctly updated.The migration from the generic
getKey(null, boolean)method to the more explicitgetKeyForEncryption()method improves code clarity and aligns with the refactored encryption key provider API.Also applies to: 51-51, 57-57, 63-63
42-42: LGTM! Correct use of getKeyForDecryption() for decryption operation.Appropriately uses
getKeyForDecryption()for the actual decryption step in the test.app/src/org/commcare/CommCareApplication.java (1)
266-266: LGTM! Context parameter correctly added to EncryptionKeyProvider.The change appropriately updates the
EncryptionKeyProviderinstantiation to pass the application context (this), which is required by the refactored constructor and enables the new modular key management system.app/src/org/commcare/connect/database/ConnectDatabaseUtils.java (2)
23-24: LGTM: Clean API change for encryption key retrieval.The change from
getKey(context, forEncryption)togetKeyForEncryption()provides a cleaner, more explicit API. The context is no longer needed as a parameter since theEncryptionKeyProvidernow holds it internally.
80-81: LGTM: Consistent API usage for decryption key retrieval.The change to
getKeyForDecryption()is consistent with the encryption case and provides clear separation of concerns between encryption and decryption key operations.app/src/org/commcare/android/security/AesKeystoreHandler.kt (1)
35-53: LGTM: Proper AES key generation with security best practices.The key generation follows Android security best practices:
- Correct use of KeyGenParameterSpec with appropriate purposes
- Proper block mode (CBC) and padding (PKCS7)
- Version-appropriate user authentication settings
- Biometric enrollment invalidation for enhanced security
app/unit-tests/src/org/commcare/utils/MockEncryptionKeyProvider.java (1)
20-40: LGTM! Clean refactoring for test mock implementation.The separation of encryption and decryption key retrieval methods improves API clarity. The exception handling approach is appropriate for test code.
app/src/org/commcare/utils/EncryptionKeyProvider.java (1)
29-35: Clean delegation to handler pattern.The refactoring to use specialized handlers improves modularity and aligns with the Single Responsibility Principle.
| if (doesKeyExists()) { | ||
| val entry = AndroidKeyStore.instance.getEntry(alias, null) as KeyStore.PrivateKeyEntry | ||
| val key = entry.privateKey | ||
| return EncryptionKeyAndTransform(key, "RSA/ECB/PKCS1Padding") | ||
| } else { | ||
| return generateRsaKey() | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Return appropriate key based on encryption mode.
The method always returns the private key when a key exists, but for encryption operations, it should return the public key.
override fun getKeyOrGenerate(): EncryptionKeyAndTransform {
if (doesKeyExists()) {
val entry = AndroidKeyStore.instance.getEntry(alias, null) as KeyStore.PrivateKeyEntry
- val key = entry.privateKey
+ val key = if (isEncryptMode) entry.certificate.publicKey else entry.privateKey
return EncryptionKeyAndTransform(key, "RSA/ECB/PKCS1Padding")
} else {
return generateRsaKey()
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/android/security/RsaKeystoreHandler.kt around lines 24
to 31, the code always returns the private key when a key exists, but for
encryption, it should return the public key instead. Modify the code to check
the encryption mode and return the public key from the KeyStore.PrivateKeyEntry
when performing encryption operations, ensuring the correct key type is used.
| fun doesKeyExists(): Boolean { | ||
| val keystore = AndroidKeyStore.instance | ||
| if (keystore.containsAlias(alias)) { | ||
| val entry = keystore.getEntry(alias, null) | ||
| if (entry is KeyStore.PrivateKeyEntry) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify by using existing AndroidKeyStore utility.
The doesKeyExists() method duplicates functionality already available in AndroidKeyStore.
Based on the relevant code snippet, AndroidKeyStore already provides a doesKeyExists(alias: String) method. Consider using it directly:
-fun doesKeyExists(): Boolean {
- val keystore = AndroidKeyStore.instance
- if (keystore.containsAlias(alias)) {
- val entry = keystore.getEntry(alias, null)
- if (entry is KeyStore.PrivateKeyEntry) {
- return true;
- }
- }
- return false;
-}
+fun doesKeyExists(): Boolean {
+ return AndroidKeyStore.doesKeyExists(alias) &&
+ AndroidKeyStore.instance.getEntry(alias, null) is KeyStore.PrivateKeyEntry
+}Also remove the unnecessary semicolon on line 38:
- return true;
+ return trueCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/android/security/RsaKeystoreHandler.kt lines 33 to 42,
simplify the doesKeyExists() method by replacing the manual key existence check
with a direct call to AndroidKeyStore's existing doesKeyExists(alias: String)
method. Also, remove the unnecessary semicolon on line 38 to clean up the code.
| public EncryptionKeyAndTransform getKeyForEncryption(Context context) { | ||
| return getHandler(true).getKeyOrGenerate(); | ||
| } | ||
|
|
||
| private static EncryptionKeyAndTransform generateKeyInKeystore(Context context, boolean trueForEncrypt) | ||
| throws NoSuchAlgorithmException, NoSuchProviderException, InvalidAlgorithmParameterException { | ||
| if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.M) { | ||
| KeyGenerator keyGenerator = KeyGenerator.getInstance( | ||
| KeyProperties.KEY_ALGORITHM_AES, KEYSTORE_NAME); | ||
| KeyGenParameterSpec keySpec = new KeyGenParameterSpec.Builder(SECRET_NAME, | ||
| KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT) | ||
| .setBlockModes(KeyProperties.BLOCK_MODE_CBC) | ||
| .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_PKCS7) | ||
| .build(); | ||
|
|
||
| keyGenerator.init(keySpec); | ||
| return new EncryptionKeyAndTransform(keyGenerator.generateKey(), String.format("%s/%s/%s", ALGORITHM, BLOCK_MODE, PADDING)); | ||
| } else { | ||
| KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA", KEYSTORE_NAME); | ||
|
|
||
| GregorianCalendar start = new GregorianCalendar(); | ||
| GregorianCalendar end = new GregorianCalendar(); | ||
| end.add(Calendar.YEAR, 100); | ||
| KeyPairGeneratorSpec keySpec = new KeyPairGeneratorSpec.Builder(context) | ||
| // You'll use the alias later to retrieve the key. It's a key for the key! | ||
| .setAlias(SECRET_NAME) | ||
| // The subject used for the self-signed certificate of the generated pair | ||
| .setSubject(new X500Principal(String.format("CN=%s", SECRET_NAME))) | ||
| // The serial number used for the self-signed certificate of the | ||
| // generated pair. | ||
| .setSerialNumber(BigInteger.valueOf(1337)) | ||
| // Date range of validity for the generated pair. | ||
| .setStartDate(start.getTime()) | ||
| .setEndDate(end.getTime()) | ||
| .build(); | ||
|
|
||
| generator.initialize(keySpec); | ||
| KeyPair pair = generator.generateKeyPair(); | ||
|
|
||
| Key key = trueForEncrypt ? pair.getPublic() : pair.getPrivate(); | ||
| return new EncryptionKeyAndTransform(key, "RSA/ECB/PKCS1Padding"); | ||
| } | ||
| public EncryptionKeyAndTransform getKeyForDecryption(Context context) { | ||
| return getHandler(false).getKeyOrGenerate(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant methods or clarify their purpose.
These methods accept a Context parameter but ignore it, using the instance's context instead. This is confusing and could lead to bugs.
Either remove these methods if they're not needed for backward compatibility, or update them to use the provided context:
-public EncryptionKeyAndTransform getKeyForEncryption(Context context) {
- return getHandler(true).getKeyOrGenerate();
-}
-
-public EncryptionKeyAndTransform getKeyForDecryption(Context context) {
- return getHandler(false).getKeyOrGenerate();
-}If backward compatibility is required, consider marking them as deprecated:
@Deprecated
public EncryptionKeyAndTransform getKeyForEncryption(Context context) {
// Use instance method instead
return getKeyForEncryption();
}🤖 Prompt for AI Agents
In app/src/org/commcare/utils/EncryptionKeyProvider.java around lines 51 to 57,
the methods getKeyForEncryption and getKeyForDecryption accept a Context
parameter but do not use it, which is confusing. To fix this, either remove
these methods if they are unnecessary or update them to use the provided Context
parameter properly. If these methods must remain for backward compatibility,
mark them as @Deprecated and modify their implementation to call the instance
methods without the Context parameter, clearly indicating their deprecated
status.
shubham1g5
force-pushed
the
androidKeyStoreEncryptionRefactor
branch
from
fae5d58 to
e9d8bd8
Compare
| @Override | ||
| public EncryptionKeyAndTransform getKey(Context context, boolean trueForEncrypt) | ||
| public EncryptionKeyAndTransform getKeyForEncryption() { | ||
| try { |
There was a problem hiding this comment.
Nit: Why not have getKey convert the NoSuchAlgorithmException to a RuntimeException to avoid duplicated code in these two functions?
| } | ||
| } | ||
|
|
||
| fun doesKeyExists(): Boolean { |
There was a problem hiding this comment.
Nit: Should be doesKeyExist (no 's' at the end).
| } | ||
| } | ||
|
|
||
| fun doesKeyExists(alias: String): Boolean { |
There was a problem hiding this comment.
Nit: Should be doesKeyExist (no 's' at the end).
| val key = if (entry is KeyStore.SecretKeyEntry) { | ||
| entry.secretKey | ||
| } else { | ||
| generateAesKey(alias, needsUserAuth) |
There was a problem hiding this comment.
Should this be "key = generateAesKey"?
There was a problem hiding this comment.
all of this is key = assignment in line 24 above.
| } | ||
| } | ||
| public EncryptionKeyProvider(Context context) { | ||
| this.context = context.getApplicationContext(); |
There was a problem hiding this comment.
@shubham1g5 context is already application, can be set directly
| public EncryptionKeyAndTransform getKeyForEncryption(Context context) { | ||
| return getHandler(true).getKeyOrGenerate(); |
| public EncryptionKeyAndTransform getKeyForDecryption(Context context) { | ||
| return getHandler(false).getKeyOrGenerate(); |
| object AndroidKeyStore { | ||
| const val ANDROID_KEY_STORE_NAME = "AndroidKeyStore" | ||
| val instance: KeyStore by lazy { | ||
| KeyStore.getInstance(ANDROID_KEY_STORE_NAME).apply { |
There was a problem hiding this comment.
we still need to call load on keystore to load it and being able to use it
|
|
||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { | ||
| builder.setUserAuthenticationRequired(needsUserAuth) | ||
| builder.setInvalidatedByBiometricEnrollment(needsUserAuth) |
There was a problem hiding this comment.
@shubham1g5 Just a question: Whenever biometric is altered, key becomes invalid, how does app recovers old keys to unlock DB?
There was a problem hiding this comment.
Currently we don't any keys that gets invalidated tied to DB - details in here
There was a problem hiding this comment.
@shubham1g5 No, I mean when biometric is altered, what will happen to key?
There was a problem hiding this comment.
accessing the key throws https://developer.android.com/reference/android/security/keystore/KeyPermanentlyInvalidatedException
There was a problem hiding this comment.
Yeah so how do we recover/decode the DB phrase which is already stored locally which was encrypted using key store previously?
| */ | ||
| object AndroidKeyStore { | ||
| const val ANDROID_KEY_STORE_NAME = "AndroidKeyStore" | ||
| val instance: KeyStore by lazy { |
|
@damagatchi retest this please |
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Nice to see the extra code in CommCareApplication go away!
|
@damagatchi retest this please |
Product Description
No user visible changes, Refactors the existing code around android key store to make it easier to extend.
Labels and Review