feat: Encrypted Reticulum identity key storage#276
Merged
torlando-tech merged 10 commits intomainfrom Feb 17, 2026
Merged
Conversation
❌ Threading Architecture Audit FailedView Audit ReportPlease fix the dispatcher violations before merging. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Implements layered AES-256-GCM encryption for identity key protection: - IdentityKeyEncryptor: Core encryption using Android Keystore (device key) with optional PBKDF2-based password protection layer - IdentityKeyMigrator: Silent auto-migration of unencrypted keys at startup - IdentityKeyProvider: Runtime decrypted key access with in-memory caching and lifecycle-aware cleanup when app goes to background Database changes: - Add encryptedKeyData, keyEncryptionVersion, passwordSalt, passwordVerificationHash columns to LocalIdentityEntity - Migration 30→31 for new columns Migration export/import: - Password-protected export encryption for portable backups - MigrationData version bumped to 7 Security properties: - Hardware-backed encryption via TEE/StrongBox - Random IV per encryption for forward secrecy - GCM authentication prevents tampering - Secure memory wiping after use Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace runBlocking with synchronous cache clearing in onStop() lifecycle callback. ConcurrentHashMap is thread-safe for this cleanup scenario. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fix compilation error caused by trailing lambda being interpreted as the new importPassword parameter instead of onProgress. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The test was flaky because yield() doesn't guarantee the collector is
subscribed before downloadRegion starts emitting statuses. Use onStart{}
with CompletableDeferred to ensure the collector is fully subscribed
before starting the download.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
0dd331a to
c0f8990
Compare
Owner
Author
Contributor
Greptile SummaryThis PR implements comprehensive at-rest encryption for Reticulum identity keys using AES-256-GCM with Android Keystore backing. The implementation follows security best practices with hardware-backed encryption (TEE/StrongBox), optional user password protection via PBKDF2 (600K iterations), and proper key lifecycle management. Key Strengths:
Architecture Notes:
Previously Reported Issues:
Both issues are documented with comments explaining the limitations. Confidence Score: 5/5
Important Files Changed
Flowchartflowchart TD
A[64-byte Reticulum Identity Key] --> B{Encryption Layer}
B --> C[Layer 1: Device Key<br/>AES-256-GCM + Android Keystore<br/>TEE/StrongBox backed]
C --> D{Password Protection?}
D -->|No| E[Version 1 Format<br/>0x01 + IV + Encrypted Data + Tag]
D -->|Yes| F[Layer 2: User Password<br/>PBKDF2 600K iterations]
F --> G[Version 2 Format<br/>0x02 + Salt + IV + Double-Encrypted + Tag]
E --> H[(Database Storage)]
G --> H
H --> I[Migration Export]
I --> J{Export Password?}
J -->|Yes| K[Portable Format<br/>Salt + IV + Encrypted + Tag]
J -->|No| L[Legacy Plaintext]
K --> M[Import on New Device]
L --> M
M --> N[Decrypt with Password]
N --> O[Re-encrypt with Device Key]
O --> H
H --> P[Runtime Access]
P --> Q[IdentityKeyProvider]
Q --> R{In Cache?}
R -->|Yes| S[Return Cached Key]
R -->|No| T[Decrypt from Storage]
T --> U[Cache Key]
U --> S
V[App Goes to Background] --> W[Lifecycle: onStop]
W --> X[Wipe Cache Memory]
X --> Y[Delete Temp Files]
Last reviewed commit: 6361427 |
Comment on lines
+227
to
+234
| // Remove password protection | ||
| val deviceOnlyEncrypted = encryptor.removePasswordProtection(encryptedData, currentPassword) | ||
|
|
||
| // Update database (clear password-related fields) | ||
| identityDao.updateEncryptedKeyData( | ||
| identityHash = identityHash, | ||
| encryptedKeyData = deviceOnlyEncrypted, | ||
| version = IdentityKeyEncryptor.VERSION_DEVICE_ONLY.toInt(), |
Contributor
There was a problem hiding this comment.
Stale password metadata after disabling protection
When disabling password protection, updateEncryptedKeyData only updates encryptedKeyData and keyEncryptionVersion, but does not clear passwordSalt and passwordVerificationHash. This leaves stale password-related data in the database after the password layer is removed. While this doesn't break decryption (since keyEncryptionVersion is correctly set to VERSION_DEVICE_ONLY), it leaves sensitive password-derived material in the database unnecessarily and could cause confusion if passwordSalt/passwordVerificationHash are checked independently of the version field elsewhere.
Consider either:
- Adding a dedicated DAO method that clears password fields when downgrading, or
- Using
updatePasswordProtectionwith null-able salt/hash parameters to clear them in the same update.
Suggested change
| // Remove password protection | |
| val deviceOnlyEncrypted = encryptor.removePasswordProtection(encryptedData, currentPassword) | |
| // Update database (clear password-related fields) | |
| identityDao.updateEncryptedKeyData( | |
| identityHash = identityHash, | |
| encryptedKeyData = deviceOnlyEncrypted, | |
| version = IdentityKeyEncryptor.VERSION_DEVICE_ONLY.toInt(), | |
| // Remove password protection | |
| val deviceOnlyEncrypted = encryptor.removePasswordProtection(encryptedData, currentPassword) | |
| // Update database (clear password-related fields by setting version to device-only) | |
| // Note: passwordSalt and passwordVerificationHash should also be cleared | |
| identityDao.updateEncryptedKeyData( | |
| identityHash = identityHash, | |
| encryptedKeyData = deviceOnlyEncrypted, | |
| version = IdentityKeyEncryptor.VERSION_DEVICE_ONLY.toInt(), | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: data/src/main/java/com/lxmf/messenger/data/crypto/IdentityKeyProvider.kt
Line: 227:234
Comment:
**Stale password metadata after disabling protection**
When disabling password protection, `updateEncryptedKeyData` only updates `encryptedKeyData` and `keyEncryptionVersion`, but does **not** clear `passwordSalt` and `passwordVerificationHash`. This leaves stale password-related data in the database after the password layer is removed. While this doesn't break decryption (since `keyEncryptionVersion` is correctly set to `VERSION_DEVICE_ONLY`), it leaves sensitive password-derived material in the database unnecessarily and could cause confusion if `passwordSalt`/`passwordVerificationHash` are checked independently of the version field elsewhere.
Consider either:
1. Adding a dedicated DAO method that clears password fields when downgrading, or
2. Using `updatePasswordProtection` with null-able salt/hash parameters to clear them in the same update.
```suggestion
// Remove password protection
val deviceOnlyEncrypted = encryptor.removePasswordProtection(encryptedData, currentPassword)
// Update database (clear password-related fields by setting version to device-only)
// Note: passwordSalt and passwordVerificationHash should also be cleared
identityDao.updateEncryptedKeyData(
identityHash = identityHash,
encryptedKeyData = deviceOnlyEncrypted,
version = IdentityKeyEncryptor.VERSION_DEVICE_ONLY.toInt(),
)
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+215
to
+216
| // Securely clear the password key from memory | ||
| secureWipe(passwordKey.encoded) |
Contributor
There was a problem hiding this comment.
secureWipe on .encoded only wipes a copy
SecretKeySpec.getEncoded() and SecretKey.getEncoded() return a clone of the internal byte array per the JCA spec. Calling secureWipe(passwordKey.encoded) allocates a new copy and wipes that copy — the key material inside the SecretKeySpec/SecretKey object remains in memory until GC. This same pattern appears at lines 216, 300, and 507.
This is a known JCA limitation and doesn't make the encryption less secure in practice (JVM GC makes deterministic memory wiping unreliable regardless), but it's worth noting that the secureWipe calls on .encoded provide less protection than they appear to. Consider adding a comment to document this JCA limitation so future readers don't assume the key material is actually zeroed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: data/src/main/java/com/lxmf/messenger/data/crypto/IdentityKeyEncryptor.kt
Line: 215:216
Comment:
**`secureWipe` on `.encoded` only wipes a copy**
`SecretKeySpec.getEncoded()` and `SecretKey.getEncoded()` return a **clone** of the internal byte array per the JCA spec. Calling `secureWipe(passwordKey.encoded)` allocates a new copy and wipes that copy — the key material inside the `SecretKeySpec`/`SecretKey` object remains in memory until GC. This same pattern appears at lines 216, 300, and 507.
This is a known JCA limitation and doesn't make the encryption less secure in practice (JVM GC makes deterministic memory wiping unreliable regardless), but it's worth noting that the `secureWipe` calls on `.encoded` provide less protection than they appear to. Consider adding a comment to document this JCA limitation so future readers don't assume the key material is actually zeroed.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Additional Comments (1)
After migration, if the identity file is lost/corrupted, this method will report "Identity file missing and no valid keyData backup available" even though the key data exists in encrypted form. This should fall back to Prompt To Fix With AIThis is a comment left during a code review.
Path: data/src/main/java/com/lxmf/messenger/data/repository/IdentityRepository.kt
Line: 335:345
Comment:
**File recovery broken after encryption migration**
`ensureIdentityFileExists` attempts to recover a missing identity file from `identity.keyData`, but after the encryption migration runs, `keyData` is set to `NULL` (via `clearUnencryptedKeyData`). This means the recovery path on line 335-345 will always fail for migrated identities, since it doesn't attempt to decrypt from `encryptedKeyData`.
After migration, if the identity file is lost/corrupted, this method will report "Identity file missing and no valid keyData backup available" even though the key data exists in encrypted form. This should fall back to `keyProvider.getDecryptedKeyData(identity.identityHash)` when `keyData` is null but `encryptedKeyData` is present.
How can I resolve this? If you propose a fix, please make it concise. |
IdentityRepository and MigrationImporter gained new constructor parameters (keyEncryptor, keyMigrator, keyProvider) during the rebase but two test files were not updated to pass them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
data/src/main/java/com/lxmf/messenger/data/repository/IdentityRepository.kt
Show resolved
Hide resolved
data/src/main/java/com/lxmf/messenger/data/repository/IdentityRepository.kt
Show resolved
Hide resolved
1. MigrationImporter.readMigrationBundle was catching data.crypto.WrongPasswordException but MigrationCrypto.decrypt throws migration.WrongPasswordException — added catch for both. 2. IdentityRepositoryDatabaseTest.createIdentity test now asserts encryptedKeyData (not deprecated keyData) since createIdentity encrypts keys before storage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
data/src/main/java/com/lxmf/messenger/data/crypto/IdentityKeyProvider.kt
Outdated
Show resolved
Hide resolved
- Add clearPasswordProtection DAO method to null out password metadata when disabling password protection (previously left stale salt/hash) - Replace coroutine Mutex with ReentrantLock in IdentityKeyProvider to fix race condition between suspend cache reads and onStop lifecycle callback running on main thread - Wire runEncryptionMigration into ColumbaApplication.onCreate so existing unencrypted keys are migrated on app startup - Fall back to keyProvider.getDecryptedKeyData in ensureIdentityFileExists when legacy plaintext keyData is null but encryptedKeyData exists - Add JCA limitation documentation to secureWipe explaining that SecretKeySpec.getEncoded() returns a clone - Fix test stubs for non-relaxed mocks (secureWipe, DAO mutations, password protection methods) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cancel viewModelScope in tearDown to prevent coroutines from leaking between tests, and stub deleteRegion to avoid unstubbed mock exceptions when cancelDownload() is invoked. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security Properties
Architecture
Files Changed
New files:
IdentityKeyEncryptor.kt- Core AES-256-GCM encryption with KeystoreIdentityKeyMigrator.kt- Auto-migration of unencrypted keysIdentityKeyProvider.kt- Runtime key access with cachingModified files:
LocalIdentityEntity.kt- Added encrypted storage columnsLocalIdentityDao.kt- New queries for encrypted key managementDatabaseModule.kt- Migration 30→31IdentityRepository.kt- Integration with encryption systemMigrationData/Exporter/Importer.kt- Password-protected exportTest Plan
Manual Testing
Prerequisites: A device/emulator with an existing install from
mainbranch with at least one identity created.1. Database Migration (38 → 39)
2. New Identity Creation Encrypts Keys
adb shell+ sqlite3, verifylocal_identities.encryptedKeyDatais populated andkeyDatais NULL for the new identity3. Export/Import Round-Trip
encryptedKeyDatapopulated (keys encrypted on import)4. Fresh Install
Not Yet Testable (backend only, no UI wired)
runEncryptionMigration()not called from app startup yet)Automated Tests
IdentityKeyEncryptorTest— password verification, export round-trip, version detection, salt extraction, secure wipe, input validationIdentityKeyMigratorTest— migration flow, edge cases (invalid key size, no key data), multi-identity, stats, resetIdentityKeyProviderTest— decrypt, password protection enable/disable/change, verification🤖 Generated with Claude Code