Personal ID: Biometric secret Key Invalidation #3169
Conversation
📝 WalkthroughWalkthroughThis set of changes introduces biometric key invalidation handling and related error messaging for Personal ID authentication. The Android KeyStore handler interfaces and implementations are updated to provide a new Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdManager
participant EncryptionKeyProvider
participant KeyStoreHandler
participant UI
User->>PersonalIdManager: Attempt biometric login
PersonalIdManager->>EncryptionKeyProvider: isKeyValid()
EncryptionKeyProvider->>KeyStoreHandler: isKeyValid()
alt Key valid
PersonalIdManager->>PersonalIdManager: Proceed with authentication
else Key invalid
PersonalIdManager->>UI: Show biometric invalidated error
PersonalIdManager->>GlobalErrors: Log PERSONALID_BIOMETRIC_INVALIDATED_ERROR
PersonalIdManager-->>User: Abort authentication
end
sequenceDiagram
participant User
participant PersonalIdBiometricConfigFragment
participant EncryptionKeyProvider
participant KeyStoreHandler
User->>PersonalIdBiometricConfigFragment: Configure biometric or PIN
PersonalIdBiometricConfigFragment->>EncryptionKeyProvider: Create/retrieve BIOMETRIC_INVALIDATION_KEY
EncryptionKeyProvider->>KeyStoreHandler: getKeyOrGenerate()
KeyStoreHandler-->>EncryptionKeyProvider: Return key
EncryptionKeyProvider-->>PersonalIdBiometricConfigFragment: Key stored
PersonalIdBiometricConfigFragment->>User: Navigate forward
Suggested labels
Suggested reviewers
✨ 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: 0
🧹 Nitpick comments (1)
app/src/org/commcare/android/security/KeyStoreHandler.kt (1)
4-4: Remove unused import.The
javax.crypto.SecretKeyimport is not used anywhere in this interface file.-import javax.crypto.SecretKey
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/res/values/strings.xml(1 hunks)app/src/org/commcare/android/security/AesKeyStoreHandler.kt(2 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/PersonalIdManager.java(7 hunks)app/src/org/commcare/connect/database/ConnectDatabaseHelper.java(1 hunks)app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java(2 hunks)app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java(5 hunks)app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java(2 hunks)app/src/org/commcare/utils/EncryptionKeyProvider.java(1 hunks)app/src/org/commcare/utils/GlobalErrors.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (1)
DatabaseConnectOpenHelper(39-165)
app/src/org/commcare/utils/EncryptionKeyProvider.java (3)
app/src/org/commcare/android/security/KeyStoreHandler.kt (1)
isKeyValid(8-8)app/src/org/commcare/android/security/AesKeyStoreHandler.kt (1)
isKeyValid(38-47)app/src/org/commcare/android/security/RsaKeyStoreHandler.kt (1)
isKeyValid(33-35)
app/src/org/commcare/android/security/AesKeyStoreHandler.kt (1)
app/src/org/commcare/utils/EncryptionKeyAndTransform.java (1)
EncryptionKeyAndTransform(10-39)
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (1)
app/src/org/commcare/CommCareApplication.java (1)
CommCareApplication(154-1246)
🔇 Additional comments (26)
app/src/org/commcare/android/security/KeyStoreHandler.kt (1)
8-8: LGTM: Clean interface extension for key validation.The new
isKeyValid()method provides a standardized way to check key validity across different KeyStore handler implementations.app/src/org/commcare/utils/EncryptionKeyProvider.java (1)
49-51: LGTM: Proper delegation to handler implementation.The method correctly delegates to the appropriate KeyStore handler in decryption mode, which is the appropriate context for discovering key invalidation.
app/src/org/commcare/connect/database/ConnectDatabaseHelper.java (1)
52-54: LGTM: Good refactoring to centralize context management.Removing the Context parameter and delegating to the static
DatabaseConnectOpenHelper.dbExists()method centralizes context access and simplifies the API.app/src/org/commcare/android/security/RsaKeyStoreHandler.kt (1)
33-35: LGTM: Appropriate implementation for RSA keys.For RSA keys, checking existence is sufficient for validity since RSA keys don't get invalidated by biometric changes like AES keys do. This implementation correctly delegates to the existing
doesKeyExist()method.app/src/org/commcare/utils/GlobalErrors.java (1)
7-8: LGTM: Error constants properly updated for biometric invalidation feature.The addition of
PERSONALID_BIOMETRIC_INVALIDATED_ERRORand the update to use a more specific message forPERSONALID_LOST_CONFIGURATION_ERRORcorrectly support the biometric key invalidation functionality. The implementation follows the existing enum pattern.app/res/values/strings.xml (1)
333-334: LGTM: User-facing messages are clear and actionable.The updated recovery message provides specific guidance about Personal ID account recovery, and the new biometric invalidation message clearly explains the issue and next steps to users. Both messages support good user experience.
app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (2)
17-17:Details
✅ Verification successful
Verify consistency of database helper context management.
The removal of the context parameter from
ConnectDatabaseHelper.dbExists()suggests that context management has been centralized within the helper class. Ensure this pattern is consistently applied across all database operations.
🏁 Script executed:
#!/bin/bash # Description: Verify that ConnectDatabaseHelper.dbExists() method signature and usage is consistent across the codebase # Check for any remaining calls to dbExists() with context parameter rg -A 3 -B 3 "ConnectDatabaseHelper\.dbExists\(" --type java # Check the actual method signature in ConnectDatabaseHelper ast-grep --pattern $'class ConnectDatabaseHelper { $$$ public static $_ dbExists($_) { $$$ } $$$ }'Length of output: 1456
🏁 Script executed:
#!/bin/bash # Locate ConnectDatabaseHelper.java and inspect the dbExists method signature file=$(rg -l "class ConnectDatabaseHelper" -g "*.java") echo "File: $file" rg -n "public static .*dbExists" "$file" rg -n "dbExists(" "$file"Length of output: 569
🏁 Script executed:
#!/bin/bash # List all occurrences of dbExists( across the Java codebase to verify no calls with parameters rg -n "dbExists\\(" --type javaLength of output: 605
Consistency Verified: Context Management Centralized in dbExists
All references to
ConnectDatabaseHelper.dbExists()have been updated to the no-argument version, and the underlying helper also manages context internally:
- ConnectDatabaseHelper.java (line 52):
public static boolean dbExists() { … }- DatabaseConnectOpenHelper.java (line 72):
public static boolean dbExists() { … }- ConnectUserDatabaseUtil.java (line 17):
if (!ConnectDatabaseHelper.dbExists())- PersonalIdManager.java (line 247):
if (ConnectDatabaseHelper.dbExists())No further changes are needed—the refactoring is consistent and complete.
50-66:Details
✅ Verification successful
Verify database deletion method consistency.
The
forgetUser()method has been changed to remove the context parameter and callsDatabaseConnectOpenHelper.deleteDb()without arguments. Ensure this change is consistent with the helper class implementation.
🏁 Script executed:
#!/bin/bash # Description: Verify DatabaseConnectOpenHelper.deleteDb() method signature and usage consistency # Check all calls to deleteDb() to ensure they match the new signature rg -A 3 -B 3 "DatabaseConnectOpenHelper\.deleteDb\(" --type java # Check the actual method signature in DatabaseConnectOpenHelper ast-grep --pattern $'class DatabaseConnectOpenHelper { $$$ public static $_ deleteDb($_) { $$$ } $$$ }'Length of output: 950
🏁 Script executed:
#!/bin/bash # Locate the deleteDb method definition in DatabaseConnectOpenHelper rg -n "deleteDb" app/src/org/commcare/connect/database/DatabaseConnectOpenHelper.javaLength of output: 194
🏁 Script executed:
#!/bin/bash # 1. Find the file defining DatabaseConnectOpenHelper fd -e java DatabaseConnectOpenHelper.java # 2. Search for the class declaration if the filename differs rg -n "class DatabaseConnectOpenHelper" --type java # 3. Locate the deleteDb method signature across all Java files rg -n "public static .* deleteDb" --type javaLength of output: 481
deleteDb() signature update verified
Verified that
DatabaseConnectOpenHelper.deleteDb()is now defined as a parameterless static method in
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java:76, and its invocation in
ConnectUserDatabaseUtil.forgetUser()matches this signature. No further changes required.app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java (2)
246-246: LGTM: Biometric key generation properly placed.The call to
storeBiometricInvalidationKey()is correctly placed after confirming that biometric or PIN authentication is configured, ensuring the key is only generated when needed.
260-267:Details
✅ Verification successful
LGTM: Secure biometric key generation implementation.
The implementation correctly:
- Checks for minimum API level (Nougat) for biometric key invalidation support
- Uses
EncryptionKeyProviderwith biometric flag enabled for hardware-backed security- Only generates the key when authentication is properly configured
🏁 Script executed:
#!/bin/bash # Description: Verify that the biometric key validation logic properly integrates with this key generation # Search for usage of BIOMETRIC_INVALIDATION_KEY constant rg -A 5 -B 5 "BIOMETRIC_INVALIDATION_KEY" --type java # Search for key validation logic that might use this key rg -A 5 -B 5 "isKeyValid" --type javaLength of output: 7837
Secure biometric key generation and validation are correctly integrated
- BIOMETRIC_INVALIDATION_KEY is used consistently in storeBiometricInvalidationKey() and in PersonalIdManager.isKeyValid().
- EncryptionKeyProvider(requireContext(), true, BIOMETRIC_INVALIDATION_KEY) ensures hardware-backed security and invalidation on biometric enrollment.
- API-level check (>= N) guards against unsupported platforms.
No further changes required.
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java (4)
9-9: LGTM: Import supports the refactoring.The import of
CommCareApplicationis necessary for the singleton-based database path access.
68-70: LGTM: Clean refactoring to use singleton pattern.The method correctly uses
CommCareApplication.instance().getDatabasePath()to centralize database path management. This eliminates the need for passingContextparameters while maintaining the same functionality.
72-74: LGTM: Consistent with the refactoring pattern.The method signature change aligns with the centralized database path approach and correctly delegates to the refactored
getDbFile()method.
76-78: LGTM: Consistent with the refactoring pattern.The method signature change aligns with the centralized database path approach and correctly delegates to the refactored
getDbFile()method.app/src/org/commcare/android/security/AesKeyStoreHandler.kt (6)
5-5: LGTM: Required import for key validation.The
KeyPermanentlyInvalidatedExceptionimport is necessary for detecting invalidated biometric keys.
10-10: LGTM: Required import for cipher operations.The
Cipherimport is necessary for the key validation implementation.
23-25: LGTM: Good practice to use constants.Extracting the transformation string to a constant improves maintainability and reduces the risk of typos.
27-36: LGTM: Clean refactoring with helper method.The refactored method improves readability by separating key retrieval logic from key generation. The use of the
TRANSFORMconstant is consistent.
38-47: LGTM: Correct implementation of key validation.The method properly implements biometric key validation by:
- Checking if the key exists first
- Attempting to initialize a cipher with the key
- Catching
KeyPermanentlyInvalidatedExceptionto detect invalidated keys- Returning appropriate boolean values
This follows Android KeyStore best practices for biometric key validation.
49-58: LGTM: Proper keystore key retrieval implementation.The helper method correctly:
- Checks if the alias exists in the keystore
- Verifies the entry type is
SecretKeyEntry- Returns the secret key or null appropriately
The implementation is safe and follows Android KeyStore patterns.
app/src/org/commcare/connect/PersonalIdManager.java (6)
3-3: LGTM: Required import for error handling.The static import of
PERSONALID_BIOMETRIC_INVALIDATED_ERRORis necessary for the new error handling functionality.
30-30: LGTM: Required imports for new functionality.The imports support the new biometric key validation and error handling features:
GlobalErrorRecordfor error loggingEncryptionKeyProviderfor key validationGlobalErrorUtilfor error managementGlobalErrorsfor error definitionsAlso applies to: 49-51
71-71: LGTM: Well-defined constant.The
BIOMETRIC_INVALIDATION_KEYconstant provides a clear identifier for the biometric key used for validation purposes.
187-193: LGTM: Proper key validation integration.The key validation check is correctly placed before biometric authentication and properly handles the failure case by:
- Recording a global error for tracking
- Displaying a user-friendly error message
- Aborting the authentication flow
This prevents authentication attempts with invalidated biometric keys.
227-230: LGTM: Clean key validation implementation.The private method properly delegates to
EncryptionKeyProviderwith the appropriate parameters:
- Uses the parent activity context
- Enables user authentication requirement
- Uses the defined biometric invalidation key identifier
247-247: LGTM: Consistent with database helper refactoring.The method calls are updated to match the new parameterless signatures in the database helper classes, maintaining consistency with the overall refactoring.
Also applies to: 250-250
| GlobalErrorUtil.addError( | ||
| new GlobalErrorRecord(new Date(), PERSONALID_BIOMETRIC_INVALIDATED_ERROR.ordinal())); |
There was a problem hiding this comment.
@OrangeAndGreen I am not sure what's the behaviour we want here, in other places of global errors we are crashing the CommCare but changing biometric should not really cause a crash.
I am also not sure if we want this to be a global error like this. Furthermore, do we want to call forgetUser for this error ? I think not, because then there is no point of having a different key. But if we don't then the behaviour is weird as we are asking user to recover without really signing out of the device.
There was a problem hiding this comment.
I had a conversation with Clayton about this last week and it sounds like there are two approaches we could take:
- If we want to be safe, we can just silently log the error for now so we can see how prevalent this is before deciding on how to react to it. If we do that though, the token will need to automatically be re-established once invalidated so we're able to see if the same user is triggering the event more than once (otherwise the token would only be configured during the device configuration workflow)
- The alternative would be some kind of deconfiguration so the user has to recover their account to regain trust. In the long run I think this is one place where we might use "soft recovery" (i.e. a simpler recovery process like just OTP or backup code). But for this release, the way we're handling those is with crashDb and an error message when the app starts up next time.
My preference is for the first option.
There was a problem hiding this comment.
#1 doesn't seem to add any security value and I would avoid doing more immediate work here at the end of the QA cycle for something that doesn't add any definite value to the release
#2 Given changing biometric is a practical alid scenario, I doin't really want to crash here.
I am more inclined to call forget User when this happens and direct user to Login page thereafter in that case. Any concerns with that ?
There was a problem hiding this comment.
But if we're going with the second option, I think we should use crashDb and let it add the global error, rather than simply adding the error directly here (we also want the user to be deconfigured and kicked out of whatever activity they're currently doing)
There was a problem hiding this comment.
I don't understand why would we like to crash app in a genuine case that we can handle without crashing.
There was a problem hiding this comment.
Seems like this could be worth bringing to the team as a whole to make sure we have a decision everyone is on board with.
Regarding the latest commit, it seems that once the Keystore gets invalidated the code will then log an analytics event every time the user unlocks going forward, correct? Seems like we need a reset mechanism, to setup a new keystore entry so we only log another event if the new entry gets invalidated (i.e. logging each newly invalidated token rather than logging one invalidated entry a bunch of times).
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdBiometricConfigFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/models/database/connect/DatabaseConnectOpenHelper.java
Show resolved
Hide resolved
| GlobalErrorUtil.addError( | ||
| new GlobalErrorRecord(new Date(), PERSONALID_BIOMETRIC_INVALIDATED_ERROR.ordinal())); |
There was a problem hiding this comment.
But if we're going with the second option, I think we should use crashDb and let it add the global error, rather than simply adding the error directly here (we also want the user to be deconfigured and kicked out of whatever activity they're currently doing)
Product Description
https://dimagi.atlassian.net/browse/CCCT-1148
Technical Summary
Some context here
Labels and Review