Encrypt device reports using Android Keystore#3563
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a centralized encryption key management system using Android Keystore and updates encryption method signatures throughout the codebase. A new SessionManager class provides static accessors for a session encryption key, eliminating the need for local key storage. Encryption-related methods in EncryptionIO and FormUploadUtil are expanded to accept cipher transformation strings and boolean flags indicating whether keys originate from Android Keystore. IV (initialization vector) extraction and handling logic is added for keystore-backed keys. Multiple call sites are updated to pass additional parameters, and DeviceReportRecord is refactored to retrieve encryption credentials from SessionManager instead of maintaining a local key field. The overall encryption flow and control paths remain structurally similar, with logic adjustments to support dynamic transformations and IV handling. Sequence DiagramsequenceDiagram
participant Client as Task/Widget
participant SessionMgr as SessionManager
participant EncryptIO as EncryptionIO
participant Keystore as Android Keystore
participant Storage as File Storage
Client->>SessionMgr: getEncryptionKey()
SessionMgr->>Keystore: retrieve/generate session_encryption_key
Keystore-->>SessionMgr: Key
SessionMgr-->>Client: Key
Client->>SessionMgr: getKeyTransformation()
SessionMgr-->>Client: transformation (e.g., "AES/GCM/NoPadding")
Client->>EncryptIO: encryptFile(path, destPath, key, transformation, isKeyFromKeystore=true)
alt isKeyFromKeystore is true
EncryptIO->>Storage: open output stream to destPath
EncryptIO->>Storage: write IV length
EncryptIO->>Storage: write IV bytes
end
EncryptIO->>EncryptIO: initialize Cipher with transformation and key
EncryptIO->>EncryptIO: wrap file stream in CipherOutputStream
EncryptIO->>Storage: encrypt and write file data
EncryptIO-->>Client: encryption complete
Client->>EncryptIO: createFileOutputStream(filename, key, transformation, isKeyFromKeystore=true)
EncryptIO->>EncryptIO: initialize Cipher with dynamic transformation
alt isKeyFromKeystore is true
EncryptIO->>Storage: read IV length
EncryptIO->>Storage: read IV bytes
EncryptIO->>EncryptIO: initialize Cipher with IvParameterSpec
end
EncryptIO-->>Client: CipherOutputStream
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/src/org/commcare/android/javarosa/DeviceReportRecord.java (1)
20-22:⚠️ Potential issue | 🟡 MinorUpdate class Javadoc to reflect keystore-managed keys.
The comment still says the record stores the key, but the key now comes from SessionManager. Please update the description to avoid confusion.
Also applies to: 68-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/android/javarosa/DeviceReportRecord.java` around lines 20 - 22, Update the Javadoc for the DeviceReportRecord class to remove the claim that the record "stores the key" and instead state that encryption keys are managed by the keystore via SessionManager (i.e., the record only stores disk location and metadata, not raw keys). Edit the class-level/comment blocks referencing key storage (including the block at the top and the block around lines 68-74) to clearly note that keys are keystore-managed and retrieved from SessionManager when needed.app/src/org/commcare/network/EncryptedFileBody.java (1)
52-73:⚠️ Potential issue | 🟠 MajorWrap FileInputStream in try-with-resources and validate IV reads.
The current code has three critical issues:
FileInputStreamat line 52 is never closed if an exception occurs before line 63, causing a resource leak.fis.read()can return-1on EOF, which becomes 255 when masked (-1 & 0xFF), creating a bogus IV. Additionally,fis.read(iv, 0, ivLength)can perform a short read without throwing, leaving the IV partially uninitialized.FormUploadUtil.getDecryptCipher()returnsnullon cipher initialization failure (see line 89 in FormUploadUtil.java), but the code passes this null directly toCipherInputStream, causing an NPE.Wrap the
FileInputStreamin try-with-resources, fully validate the IV length and contents, and explicitly check that the cipher was initialized before proceeding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/network/EncryptedFileBody.java` around lines 52 - 73, Wrap the FileInputStream (fis) in a try-with-resources block, and when isKeyFromAndroidKeyStore is true validate the initial length byte from fis.read() is not -1 before masking, reject zero or implausible ivLength, then fully populate iv using a loop or DataInput.readFully-style logic and fail with a clear IOException if the read is short or EOF occurs; call FormUploadUtil.getDecryptCipher(key, transformation, iv) and explicitly check the returned cipher is non-null (throw a descriptive IOException if null) before creating CipherInputStream, then proceed to StreamsUtil.writeFromInputToOutputUnmanaged(cis, sink.outputStream()) and preserve the existing InputIOException and OutputIOException handling behavior.app/src/org/commcare/utils/FormUploadUtil.java (1)
73-90:⚠️ Potential issue | 🟠 MajorAvoid returning null on cipher init failure.
Returning null here leads to NPEs downstream. In
EncryptedFileBody.java(line 62) andFormRecordProcessor.java(lines 74, 227, 262), the cipher is passed directly toCipherInputStreamwithout null checks. IfgetDecryptCipherreturns null,CipherInputStreamwill fail when attempting cipher operations. Fail fast with a clear exception instead, consistent with how other crypto operations in the codebase handle initialization failures (e.g.,TemplatePrinterUtils,EncryptionIO).🛠️ Proposed fix
- } catch (NoSuchAlgorithmException | - NoSuchPaddingException | InvalidKeyException | InvalidAlgorithmParameterException e) { - e.printStackTrace(); - } - return null; + } catch (NoSuchAlgorithmException | + NoSuchPaddingException | InvalidKeyException | InvalidAlgorithmParameterException e) { + Logger.log(LogTypes.TYPE_ERROR_CRYPTO, "Failed to init decrypt cipher: " + e.getMessage()); + throw new IllegalStateException("Failed to init decrypt cipher", e); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/utils/FormUploadUtil.java` around lines 73 - 90, Replace the current null-return behavior in getDecryptCipher(Key key, String transformation, byte[] iv) with a fail-fast unchecked exception: catch the NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException and InvalidAlgorithmParameterException and throw a new IllegalStateException (or a project-standard unchecked crypto init exception) with a clear message like "Failed to initialize decrypt Cipher" and include the caught exception as the cause; remove the final return null so callers such as EncryptedFileBody and FormRecordProcessor (which pass the result to CipherInputStream) never receive null and will fail fast with the original cause.
🧹 Nitpick comments (1)
app/src/org/commcare/tasks/LogSubmissionTask.java (1)
369-380: Remove the now-unreachablegetDecryptCiphermethod.No call site for
private static Cipher getDecryptCipher(SecretKeySpec key)exists in this class after the refactor. Removing it also allows theimport javax.crypto.spec.SecretKeySpecat line 40 to be cleaned up.🗑️ Proposed removal
- private static Cipher getDecryptCipher(SecretKeySpec key) { - Cipher cipher; - try { - cipher = Cipher.getInstance("AES"); - cipher.init(Cipher.DECRYPT_MODE, key); - return cipher; - //TODO: Something smart here; - } catch (NoSuchAlgorithmException | InvalidKeyException | NoSuchPaddingException e) { - e.printStackTrace(); - } - return null; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/tasks/LogSubmissionTask.java` around lines 369 - 380, Remove the unused private static method getDecryptCipher(SecretKeySpec key) from LogSubmissionTask.java since it is no longer referenced; after deleting the method also remove the now-unused import javax.crypto.spec.SecretKeySpec (and any other crypto imports that become unused) to clean up imports and avoid dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/org/commcare/models/encryption/EncryptionIO.java`:
- Around line 61-68: The code calls cipher.getIV() after cipher.init(...) and
then uses iv.length without checking for null, which can NPE if the
transformation has no IV; update the isKeyFromAndroidKeyStore branch (the
Cipher.getInstance / cipher.init / isKeyFromAndroidKeyStore block) to retrieve
iv = cipher.getIV(), validate iv != null before writing (fos.write(iv.length);
fos.write(iv);) and if iv is null throw or log a clear, descriptive
exception/message indicating the cipher/transformation does not provide an IV so
the keystore-based encryption cannot proceed.
In `@app/src/org/commcare/tasks/LogSubmissionTask.java`:
- Around line 295-304: The call to FormUploadUtil.createEncryptedFilePart in
LogSubmissionTask is passing SessionManager.getEncryptionKey() directly, which
can be null and cause silent fallback to unencrypted uploads; before calling
createEncryptedFilePart (in the method that builds parts inside
LogSubmissionTask), explicitly check SessionManager.getEncryptionKey() (and
optionally SessionManager.getKeyTransformation()) for null and fail fast: if the
key is null, log an error via the existing logger/process flow and abort the
submission (throw or return an error) rather than proceeding to add the part;
reference the exact call site where
parts.add(FormUploadUtil.createEncryptedFilePart(...)) is invoked to apply this
guard.
- Around line 295-304: Old device-report files copied by
UserSandboxUtils.migrateDeviceReports() are still in the pre-IV format, but
LogSubmissionTask.java now calls FormUploadUtil.createEncryptedFilePart(...)
with isKeyFromAndroidKeyStore = true which expects an IV-prefixed file; fix by
updating UserSandboxUtils.migrateDeviceReports() to detect legacy-format files,
decrypt them using the old decryption path, and re-encrypt/write them in the new
IV-prefixed format (so EncryptedFileBody.writeTo and
FormUploadUtil.createEncryptedFilePart remain correct), or alternatively
implement a clear legacy-decryption path in EncryptedFileBody.writeTo to handle
files missing the IV prefix and convert them to the new format during migration;
reference UserSandboxUtils.migrateDeviceReports(), EncryptedFileBody.writeTo,
and FormUploadUtil.createEncryptedFilePart/LogSubmissionTask.java when applying
the change.
---
Outside diff comments:
In `@app/src/org/commcare/android/javarosa/DeviceReportRecord.java`:
- Around line 20-22: Update the Javadoc for the DeviceReportRecord class to
remove the claim that the record "stores the key" and instead state that
encryption keys are managed by the keystore via SessionManager (i.e., the record
only stores disk location and metadata, not raw keys). Edit the
class-level/comment blocks referencing key storage (including the block at the
top and the block around lines 68-74) to clearly note that keys are
keystore-managed and retrieved from SessionManager when needed.
In `@app/src/org/commcare/network/EncryptedFileBody.java`:
- Around line 52-73: Wrap the FileInputStream (fis) in a try-with-resources
block, and when isKeyFromAndroidKeyStore is true validate the initial length
byte from fis.read() is not -1 before masking, reject zero or implausible
ivLength, then fully populate iv using a loop or DataInput.readFully-style logic
and fail with a clear IOException if the read is short or EOF occurs; call
FormUploadUtil.getDecryptCipher(key, transformation, iv) and explicitly check
the returned cipher is non-null (throw a descriptive IOException if null) before
creating CipherInputStream, then proceed to
StreamsUtil.writeFromInputToOutputUnmanaged(cis, sink.outputStream()) and
preserve the existing InputIOException and OutputIOException handling behavior.
In `@app/src/org/commcare/utils/FormUploadUtil.java`:
- Around line 73-90: Replace the current null-return behavior in
getDecryptCipher(Key key, String transformation, byte[] iv) with a fail-fast
unchecked exception: catch the NoSuchAlgorithmException, NoSuchPaddingException,
InvalidKeyException and InvalidAlgorithmParameterException and throw a new
IllegalStateException (or a project-standard unchecked crypto init exception)
with a clear message like "Failed to initialize decrypt Cipher" and include the
caught exception as the cause; remove the final return null so callers such as
EncryptedFileBody and FormRecordProcessor (which pass the result to
CipherInputStream) never receive null and will fail fast with the original
cause.
---
Nitpick comments:
In `@app/src/org/commcare/tasks/LogSubmissionTask.java`:
- Around line 369-380: Remove the unused private static method
getDecryptCipher(SecretKeySpec key) from LogSubmissionTask.java since it is no
longer referenced; after deleting the method also remove the now-unused import
javax.crypto.spec.SecretKeySpec (and any other crypto imports that become
unused) to clean up imports and avoid dead code.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/src/org/commcare/activities/components/ImageCaptureProcessing.javaapp/src/org/commcare/android/javarosa/DeviceReportRecord.javaapp/src/org/commcare/models/FormRecordProcessor.javaapp/src/org/commcare/models/database/HybridFileBackedSqlStorage.javaapp/src/org/commcare/models/encryption/EncryptionIO.javaapp/src/org/commcare/network/EncryptedFileBody.javaapp/src/org/commcare/services/SessionManager.ktapp/src/org/commcare/tasks/DumpTask.javaapp/src/org/commcare/tasks/FormRecordToFileTask.javaapp/src/org/commcare/tasks/LogSubmissionTask.javaapp/src/org/commcare/tasks/SaveToDiskTask.javaapp/src/org/commcare/utils/FormUploadUtil.javaapp/src/org/commcare/views/widgets/MediaWidget.java
| Cipher cipher = Cipher.getInstance(Objects.requireNonNullElse(transformation, "AES")); | ||
| cipher.init(Cipher.ENCRYPT_MODE, symmetricKey); | ||
| byte[] iv; | ||
| if (isKeyFromAndroidKeyStore) { | ||
| iv = cipher.getIV(); | ||
| fos.write(iv.length); | ||
| fos.write(iv); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/src/org/commcare/models/encryption/EncryptionIO.java | sed -n '50,80p'Repository: dimagi/commcare-android
Length of output: 1970
🏁 Script executed:
fd checkstyle.xmlRepository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
web_search
Cipher.getIV() return null Android Java documentationRepository: dimagi/commcare-android
Length of output: 261
🏁 Script executed:
find . -name "*checkstyle*" -type fRepository: dimagi/commcare-android
Length of output: 99
🌐 Web query:
Java Cipher.getIV() null return value documentation
💡 Result:
javax.crypto.Cipher.getIV() is documented to return null in these cases:
- The cipher’s transformation/mode doesn’t use an IV (e.g., ECB).
- The IV “has not yet been set” (e.g., before
init(...), or when decrypting if you didn’t supply IV/parameters and the provider can’t/doesn’t derive them).
Oracle Javadoc (Java SE) states: it returns “the initialization vector in a new buffer, or null if this cipher does not use an IV, or if the IV has not yet been set.” [1] (same wording in older Java docs [2]).
🏁 Script executed:
cat ./.github/linters/checkstyle.xml | head -100Repository: dimagi/commcare-android
Length of output: 5041
🏁 Script executed:
rg "createFileOutputStream" --type java -A 2 -B 2Repository: dimagi/commcare-android
Length of output: 3275
🏁 Script executed:
# Check the line length of the proposed fix to ensure it complies with checkstyle (115 char limit)
cat > /tmp/check_fix.java << 'EOF'
byte[] iv;
if (isKeyFromAndroidKeyStore) {
iv = cipher.getIV();
if (iv == null || iv.length == 0) {
throw new IllegalStateException(
"Keystore encryption requires an IV-producing transformation");
}
fos.write(iv.length);
fos.write(iv);
}
EOF
awk '{print NR": " $0 " [len:" length($0) "]"}' /tmp/check_fix.javaRepository: dimagi/commcare-android
Length of output: 467
🏁 Script executed:
rg "transformation" app/src/org/commcare/models/encryption/EncryptionIO.java -B 3 -A 3Repository: dimagi/commcare-android
Length of output: 1372
Add null check before accessing IV from keystore cipher.
The code at line 65 calls cipher.getIV() without null-checking the result. Per Java documentation, Cipher.getIV() returns null when the transformation doesn't use an IV. Accessing iv.length on line 66 will cause a NullPointerException if this occurs. Add a guard with a clear error message before attempting to write the IV:
Proposed fix
byte[] iv;
if (isKeyFromAndroidKeyStore) {
iv = cipher.getIV();
+ if (iv == null || iv.length == 0) {
+ throw new IllegalStateException(
+ "Keystore encryption requires an IV-producing transformation");
+ }
fos.write(iv.length);
fos.write(iv);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/models/encryption/EncryptionIO.java` around lines 61 -
68, The code calls cipher.getIV() after cipher.init(...) and then uses iv.length
without checking for null, which can NPE if the transformation has no IV; update
the isKeyFromAndroidKeyStore branch (the Cipher.getInstance / cipher.init /
isKeyFromAndroidKeyStore block) to retrieve iv = cipher.getIV(), validate iv !=
null before writing (fos.write(iv.length); fos.write(iv);) and if iv is null
throw or log a clear, descriptive exception/message indicating the
cipher/transformation does not provide an IV so the keystore-based encryption
cannot proceed.
| parts.add( | ||
| FormUploadUtil.createEncryptedFilePart( | ||
| "xml_submission_file", | ||
| f, | ||
| "text/xml", | ||
| SessionManager.getEncryptionKey(), | ||
| SessionManager.getKeyTransformation(), | ||
| true | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Guard against SessionManager.getEncryptionKey() returning null.
EncryptionIO.createFileOutputStream silently falls back to a plain, unencrypted FileOutputStream when symmetricKey == null. If SessionManager has not yet initialised its Keystore-backed key (e.g., first boot, Keystore failure), device report logs will be transmitted in plaintext to the server.
Add an explicit null check and fail fast rather than silently degrading encryption:
🛡️ Proposed guard
+ Key sessionKey = SessionManager.getEncryptionKey();
+ if (sessionKey == null) {
+ Logger.log(LogTypes.TYPE_ERROR_CRYPTO, "Aborting log submission: session encryption key unavailable.");
+ return LogSubmitOutcomes.ERROR;
+ }
parts.add(
FormUploadUtil.createEncryptedFilePart(
"xml_submission_file",
f,
"text/xml",
- SessionManager.getEncryptionKey(),
+ sessionKey,
SessionManager.getKeyTransformation(),
true
)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parts.add( | |
| FormUploadUtil.createEncryptedFilePart( | |
| "xml_submission_file", | |
| f, | |
| "text/xml", | |
| SessionManager.getEncryptionKey(), | |
| SessionManager.getKeyTransformation(), | |
| true | |
| ) | |
| ); | |
| Key sessionKey = SessionManager.getEncryptionKey(); | |
| if (sessionKey == null) { | |
| Logger.log(LogTypes.TYPE_ERROR_CRYPTO, "Aborting log submission: session encryption key unavailable."); | |
| return LogSubmitOutcomes.ERROR; | |
| } | |
| parts.add( | |
| FormUploadUtil.createEncryptedFilePart( | |
| "xml_submission_file", | |
| f, | |
| "text/xml", | |
| sessionKey, | |
| SessionManager.getKeyTransformation(), | |
| true | |
| ) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/tasks/LogSubmissionTask.java` around lines 295 - 304,
The call to FormUploadUtil.createEncryptedFilePart in LogSubmissionTask is
passing SessionManager.getEncryptionKey() directly, which can be null and cause
silent fallback to unencrypted uploads; before calling createEncryptedFilePart
(in the method that builds parts inside LogSubmissionTask), explicitly check
SessionManager.getEncryptionKey() (and optionally
SessionManager.getKeyTransformation()) for null and fail fast: if the key is
null, log an error via the existing logger/process flow and abort the submission
(throw or return an error) rather than proceeding to add the part; reference the
exact call site where parts.add(FormUploadUtil.createEncryptedFilePart(...)) is
invoked to apply this guard.
🧩 Analysis chain
🏁 Script executed:
# Search for migration code and DeviceReportRecord references
fd -type f -name "*.java" -o -name "*.kt" | xargs grep -l "MigrationTo30\|DbMigration.*30\|migration.*v30\|DB_VERSION.*30" 2>/dev/null | head -20Repository: dimagi/commcare-android
Length of output: 237
🏁 Script executed:
# Search for DeviceReportRecord and migration-related code
rg -n "DeviceReportRecord|aesKey|reencrypt|re.encrypt|MigrationTo30" --type java -A 3 -B 1 | head -200Repository: dimagi/commcare-android
Length of output: 20486
🏁 Script executed:
# Find the migration class and version constants
rg -n "DB_VERSION|DbMigration|MigrationTo" --type java -A 2 | head -150Repository: dimagi/commcare-android
Length of output: 6637
🏁 Script executed:
# Search for EncryptedFileBody to understand writeTo behavior
fd -type f -name "*.java" | xargs grep -l "EncryptedFileBody" | head -10Repository: dimagi/commcare-android
Length of output: 237
🏁 Script executed:
# Read the migrateDeviceReports method from UserSandboxUtils
cat -n app/src/org/commcare/models/database/user/UserSandboxUtils.java | sed -n '97,106p'Repository: dimagi/commcare-android
Length of output: 640
🏁 Script executed:
# Search for EncryptedFileBody implementation
fd -type f -name "*.java" | xargs grep -l "EncryptedFileBody" | head -5Repository: dimagi/commcare-android
Length of output: 237
🏁 Script executed:
# Search for the actual v30 migration or staged migration
rg -n "version.*30|30.*migration|v30|V30|MigrationTo30" --type java -A 3 -B 1 | head -100Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check FormUploadUtil.createEncryptedFilePart to understand the isKeyFromAndroidKeyStore parameter
rg -n "createEncryptedFilePart|isKeyFromAndroidKeyStore" -A 5 --type java | head -150Repository: dimagi/commcare-android
Length of output: 6722
🏁 Script executed:
# Look at DeviceReportRecord class definition
fd -type f -name "DeviceReportRecord.java" -exec cat -n {} \;Repository: dimagi/commcare-android
Length of output: 237
🏁 Script executed:
# Check HybridFileBackedSqlStorage - how are per-record AES keys retrieved and used?
cat -n app/src/org/commcare/models/database/HybridFileBackedSqlStorage.java | sed -n '140,170p'Repository: dimagi/commcare-android
Length of output: 1448
🏁 Script executed:
# Search for any legacy decryption or old key handling
rg -n "aesKey|per.record|legacy|old.*encrypt" app/src/org/commcare/models/database/ -A 2 -B 1Repository: dimagi/commcare-android
Length of output: 6299
🏁 Script executed:
# Check DeviceReportRecord to see if it stores per-record encryption keys
fd -name "DeviceReportRecord.java" -exec cat {} \;Repository: dimagi/commcare-android
Length of output: 297
🏁 Script executed:
# Check if there's version detection or migration flag in DeviceReportRecord
rg -n "class DeviceReportRecord|version|migrat" -A 10 app/src/org/commcare/android/javarosa/Repository: dimagi/commcare-android
Length of output: 1100
🏁 Script executed:
# Read more context of LogSubmissionTask to see how file f is obtained
cat -n app/src/org/commcare/tasks/LogSubmissionTask.java | sed -n '280,310p'Repository: dimagi/commcare-android
Length of output: 1306
🏁 Script executed:
# Search for where the DeviceReportRecord file is read/obtained before submission
rg -n "submitReports|getFilePath" app/src/org/commcare/tasks/LogSubmissionTask.java -A 5 -B 2Repository: dimagi/commcare-android
Length of output: 1621
🏁 Script executed:
# Check if there's a v29→v30 migration being staged or planned
rg -n "USER_DB_VERSION|29|30" app/src/org/commcare/models/database/user/UserDatabaseSchemaManager.java -A 3 -B 3Repository: dimagi/commcare-android
Length of output: 818
🏁 Script executed:
# Search for any code that removes aesKey from database or migration logic
rg -n "aesKey|AES_COL|removeColumn|dropColumn|migration" app/src/org/commcare/models/database/user/ -iRepository: dimagi/commcare-android
Length of output: 3711
🏁 Script executed:
# Check UserDatabaseUpgrader to see how it handles DeviceReportRecord during migrations
rg -n "DeviceReportRecord|version.*29\|29.*version" app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java -A 3 -B 1Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Look at the key difference - when reading DeviceReportRecord for storage operations vs submission
rg -n "getInputStreamFromFile|SessionManager.getEncryptionKey|getEncryptionKey" app/src/org/commcare/ -B 2 -A 2Repository: dimagi/commcare-android
Length of output: 5508
🏁 Script executed:
# Check the actual DB schema - does it still have AES_COL for DeviceReportRecord?
rg -n "DeviceReportRecord|AES_COL|FILE_COL" app/src/org/commcare/models/database/user/UserDatabaseSchemaManager.java -A 2 -B 1Repository: dimagi/commcare-android
Length of output: 421
🏁 Script executed:
# Check if there's any indication of v30 migration code or staged changes
rg -n "version.*30|VERSION.*30|v30|V30|MigrationTo30" app/src/Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Look at how DeviceReportRecord.write() actually writes files - is it really using SessionManager key?
cat -n app/src/org/commcare/android/javarosa/DeviceReportRecord.java | sed -n '65,85p'Repository: dimagi/commcare-android
Length of output: 502
🏁 Script executed:
# Important: When old DeviceReportRecords are stored, what key do they use? Check serializeLogs
cat -n app/src/org/commcare/tasks/LogSubmissionTask.java | sed -n '175,235p'Repository: dimagi/commcare-android
Length of output: 4315
🏁 Script executed:
# Check how the file is written in serializeLogs - what encryption context is used?
rg -n "serializeLogs|write.*record|getOutputFileStream" app/src/org/commcare/tasks/LogSubmissionTask.java -A 8 -B 2Repository: dimagi/commcare-android
Length of output: 1780
🏁 Script executed:
# Check git history to see when LogSubmissionTask was changed to use SessionManager.getEncryptionKey with true flag
git log --oneline -20 app/src/org/commcare/tasks/LogSubmissionTask.javaRepository: dimagi/commcare-android
Length of output: 111
🏁 Script executed:
# Check if there's any migration step between v28 and v29 that handles DeviceReportRecord
rg -n "v28|v29|version.*28|version.*29" app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java -A 5 -B 1Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check the old encryption scheme - was there a different path before?
rg -n "ECB|PKCS5Padding|AES/ECB" app/src/org/commcare/ --type javaRepository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Look for any code that handles pre-migration records or version detection
rg -n "isKeyFromAndroidKeyStore|version.*flag|record.*version" app/src/org/commcare/android/javarosa/ -A 3 -B 1Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check when the isKeyFromAndroidKeyStore=true parameter was added to LogSubmissionTask
git log -p --all -- app/src/org/commcare/tasks/LogSubmissionTask.java | grep -A 10 -B 10 "isKeyFromAndroidKeyStore" | head -80Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check if there are any other places where DeviceReportRecord encryption is handled
rg -n "DeviceReportRecord" app/src/org/commcare/ -A 3 | grep -i "encrypt\|key\|aes"Repository: dimagi/commcare-android
Length of output: 1143
🏁 Script executed:
# Verify the actual behavior: check if HybridFileBackedSqlStorage has a special case for DeviceReportRecord
rg -n "class.*Storage.*DeviceReport\|extends.*HybridFileBackedSqlStorage" app/src/org/commcare/ -A 5Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check git history of DeviceReportRecord to see when isKeyFromAndroidKeyStore was introduced
git log -p --all -- app/src/org/commcare/android/javarosa/DeviceReportRecord.java | grep -A 15 -B 5 "openOutputStream\|isKeyFromAndroidKeyStore\|true" | head -100Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check SessionManager to see if getEncryptionKey was recently changed
git log -p --all -- app/src/org/commcare/services/SessionManager.kt | head -150Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# One final check: verify that the review comment's premise about per-record keys is correct
# by looking at the old DB schema or comments about it
rg -n "aesKey|per.record|per-record|key.*record" app/src/org/commcare/models/database/ -i -B 2 -A 2 | head -100Repository: dimagi/commcare-android
Length of output: 10707
🏁 Script executed:
# Check if there's any fallback or try-catch logic when IV reading fails
cat -n app/src/org/commcare/network/EncryptedFileBody.java | sed -n '45,75p'Repository: dimagi/commcare-android
Length of output: 1539
Pre-migration unsubmitted DeviceReportRecord files will fail to submit due to encryption format mismatch.
The submission code (lines 295–304) now passes isKeyFromAndroidKeyStore = true, instructing EncryptedFileBody.writeTo to read an IV-length byte + IV from the front of the file before decrypting. However, the migration in UserSandboxUtils.migrateDeviceReports() (lines 103–104) only copies files via FileUtil.copyFile() without re-encrypting them, despite its misleading comment claiming otherwise.
If old DeviceReportRecord files were previously encrypted without an IV prefix, the submission attempt will misinterpret the first bytes as an IV length, producing garbled decryption output and silent failure.
The migration should either:
- Re-encrypt on-disk log files with the new encryption format, or
- Detect and discard pre-migration records on upgrade, or
- Provide a legacy decryption code path for files missing the IV prefix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/tasks/LogSubmissionTask.java` around lines 295 - 304,
Old device-report files copied by UserSandboxUtils.migrateDeviceReports() are
still in the pre-IV format, but LogSubmissionTask.java now calls
FormUploadUtil.createEncryptedFilePart(...) with isKeyFromAndroidKeyStore = true
which expects an IV-prefixed file; fix by updating
UserSandboxUtils.migrateDeviceReports() to detect legacy-format files, decrypt
them using the old decryption path, and re-encrypt/write them in the new
IV-prefixed format (so EncryptedFileBody.writeTo and
FormUploadUtil.createEncryptedFilePart remain correct), or alternatively
implement a clear legacy-decryption path in EncryptedFileBody.writeTo to handle
files missing the IV prefix and convert them to the new format during migration;
reference UserSandboxUtils.migrateDeviceReports(), EncryptedFileBody.writeTo,
and FormUploadUtil.createEncryptedFilePart/LogSubmissionTask.java when applying
the change.
There was a problem hiding this comment.
Db migration will be handled in a separate PR
There was a problem hiding this comment.
@avazirna, understood! I'll note that the migration handling will be addressed separately.
Would you like me to create a follow-up issue to track the migration work for pre-existing DeviceReportRecord files? This would ensure the encryption format compatibility concern is captured for the upcoming PR.
Also, I wanted to check: did you get a chance to review the first concern about adding a null check for SessionManager.getEncryptionKey() to prevent silent fallback to unencrypted uploads? That one could potentially be addressed in this PR since it's a simpler guard condition.
🧠 Learnings used
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T13:40:19.645Z
Learning: PR `#3048` introduces a comprehensive messaging system in the Connect feature, implementing secure encryption using AES-GCM for message content, proper channel management with consent flows, and a well-designed UI separation between sent and received messages with real-time notification integration.
avazirna
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Can we update the safety story with specific steps taken to be confident in these changes? The current safety story description is very vague and does not give a good sense of how exactly this was tested successfully
Very true, let me correct that. |
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
A couple suggestions about code arrangement but otherwise all looks good to me and I didn't encounter any issues in basic testing.
| ".html", ".rtf", ".txt", ".docx", ".xlsx", ".msg"}; | ||
|
|
||
| public static Cipher getDecryptCipher(SecretKeySpec key) { | ||
| public static Cipher getDecryptCipher(Key key, String transformation, byte[] iv) { |
There was a problem hiding this comment.
Seeing 6 callers to this function and all but one pass "null, null" for the new parameters, thinking keeping the original function signature and having it call the new one would reduce the number of files touched by this PR as those callers could remain unchanged.
| public class EncryptionIO { | ||
|
|
||
| public static void encryptFile(String sourceFilePath, String destPath, SecretKeySpec symetricKey) throws IOException { | ||
| public static void encryptFile(String sourceFilePath, String destPath, SecretKeySpec symmetricKey, String transformation, boolean isKeyFromKeystore) throws IOException { |
There was a problem hiding this comment.
The new parameters are always "null, false" in practice (2 callers), why expose the option?
There was a problem hiding this comment.
Yes, this is because of the db migration, I've pushed that in a separate PR. But yes, unnecessary here.
| public static OutputStream createFileOutputStream(String filename, | ||
| SecretKeySpec symetricKey) | ||
| throws FileNotFoundException { | ||
| Key symmetricKey, |
There was a problem hiding this comment.
Similar to another comment, I suggest:
- Keeping the existing function signature
- Creating a new createKeyStoreBackedFileOuptStream with the extra transformation string
- Have the above two both redirect to a private function with the new signature proposed above (including isKeyFromKeystore)
That saves unaffected code from needing to change
There was a problem hiding this comment.
I think method overloading is sufficient to reduce blast radius here. I plan to refactor this in the migration PR but around the cipher initiation which is real reason why we need these extra fields.
There was a problem hiding this comment.
can we have the refactoring in this PR itself as it feels important for this review, I also think any migrations for this work should preceed this work or part of this PR itself.
@conroy-ricketts I've added some notes around the testing. |
avazirna
force-pushed
the
vaz/encrypt-device-reports-with-android-keystore
branch
4 times, most recently
from
d5e3c6b to
d592a94
Compare
avazirna
force-pushed
the
vaz/encrypt-device-reports-with-android-keystore
branch
from
d592a94 to
4c8d8c8
Compare
|
|
||
|
|
||
| public static MultipartBody.Part createEncryptedFilePart(String partName, File file, String contentType, SecretKeySpec key) { | ||
| public static MultipartBody.Part createEncryptedFilePart( |
There was a problem hiding this comment.
This may be another good place to do method overloading similarly to EncryptionIO.createFileOutputStream()
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Have not reviewed it fully but made some comments around the approach on this PR.
| * Currently used to manage the session Android keystore backed encryption key, | ||
| */ | ||
| @RequiresApi(Build.VERSION_CODES.M) | ||
| class SessionManager { |
There was a problem hiding this comment.
@RequiresApi(Build.VERSION_CODES.M) is not required given that is already the minSdk we support.
| * Currently used to manage the session Android keystore backed encryption key, | ||
| */ | ||
| @RequiresApi(Build.VERSION_CODES.M) | ||
| class SessionManager { |
There was a problem hiding this comment.
Think the naming here should not be coupled with any session manangement and we should rename this CommCareKeyManager or similar.
| "xml_submission_file", | ||
| f, | ||
| "text/xml", | ||
| SessionManager.getEncryptionKey(), |
There was a problem hiding this comment.
think we should fallback to the old mechanism using the slr.getKey() here if SessionManager.getEncryptionKey()==null. This is because we don't know for sure if the keystore encryption is supported on all devices used by CommCare and given CommCare is used on a much larger variety of devices, think we should be doing this implementation in a fail-safe way.
4 participants
Product Description
This PR is part of series related to the retirement of the CommCareSessionService. This PR covers the encryption of Device report files using an Android Keystore-backed AES key rather than a per-record randomly generated key stored in the database. This removes the need to persist raw key material in user storage.
Technical Summary
Previously, each
DeviceReportRecordgenerated and stored its own AES key, then used it to encrypt the Device report file. This meant raw key bytes were serialized into the database alongside the file path.This PR replaces that approach with a shared session key managed by the new
SessionManagerclass, which wraps an Android Keystore-backed AES/CBC key (viaAesKeyStoreHandler). The Keystore key generates a fresh IV on each encryption; the IV is prepended to the ciphertext and read back during decryption.Key changes:
SessionManager— new Kotlin class providing a lazily-initialised Keystore-backedAES/CBC/PKCS7key viagetEncryptionKey()/getKeyTransformation()DeviceReportRecord— removesaesKeyfield and per-record key generation;openOutputStream()now delegates toSessionManagerEncryptionIO.createFileOutputStream— updated signature to acceptKey,transformation, andisKeyFromAndroidKeyStore; writes IV to stream when using a Keystore keyEncryptedFileBody.writeTo— reads IV from stream before decryption whenisKeyFromAndroidKeyStore=trueFormUploadUtil.getDecryptCipher/createEncryptedFilePart— updated signatures to propagate transformation and IV; correctcipher.initoverload used based on IV presenceSaveToDiskTask,HybridFileBackedSqlStorage,FormRecordProcessor, etc.) updated to passnull, false— no behaviour change for those pathsNote: Database migration to convert existing Device report files into this model will be submitted in a separate PR.
Safety Assurance
Safety story
Successfully tested locally. Steps taken were:
201;Automated test coverage
There are no tests covering this at the moment. I'm inclined to add them in a separate PR.
QA Plan
Labels and Review