feat(datastore): add opt-in SQLCipher encryption support#584
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #584 +/- ##
==========================================
+ Coverage 70.81% 76.84% +6.03%
==========================================
Files 51 62 +11
Lines 2916 4933 +2017
==========================================
+ Hits 2065 3791 +1726
- Misses 851 1142 +291 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR adds opt-in SQLCipher encrypted database support behind
Confidence Score: 5/5Safe to merge; the encryption path is well-guarded and the default build is unaffected. The core encryption logic is sound: wrong-key detection fires before the worker loop begins, the passphrase is never written to logs, Zeroizing ensures heap zeroing on drop, and the non-encryption panic prevents silent misconfiguration. The two remaining observations are a theoretical thread-safety nuance in remove_var placement and a missing negative test case — neither affects correctness of a correctly configured build. The remove_var placement in aw-server/src/main.rs and the missing wrong-key test in aw-datastore/tests/datastore.rs are worth a second look before the feature is widely promoted. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant main as aw-server main()
participant DS as Datastore::new_encrypted()
participant Worker as DatastoreWorker (thread)
participant SQLCipher
User->>main: --db-password / AW_DB_PASSWORD
main->>main: Opts::parse() captures key
main->>main: remove_var("AW_DB_PASSWORD")
main->>main: guard: empty key → panic
main->>DS: new_encrypted(path, key, legacy_import)
DS->>DS: "wrap key in Zeroizing<String>"
DS->>Worker: spawn thread with DatastoreMethod::FileEncrypted(path, key)
Worker->>SQLCipher: Connection::open(path)
Worker->>SQLCipher: "PRAGMA key = '...'"
Worker->>SQLCipher: PRAGMA user_version (key verification)
alt wrong key or not encrypted
SQLCipher-->>Worker: SQLITE_NOTADB / error
Worker-->>Worker: panic(wrong passphrase or not encrypted)
else correct key
SQLCipher-->>Worker: Ok(version)
Worker->>Worker: DatastoreInstance::new() — schema/migration
Worker-->>DS: ready (channel open)
DS-->>main: Datastore handle
main->>main: Mutex::new(datastore) → ServerState
end
Reviews (9): Last reviewed commit: "fix(server): clear AW_DB_PASSWORD before..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed the two P1 findings from Greptile in 67e9062:
The P2 (key zeroing via |
1 similar comment
|
Addressed the two P1 findings from Greptile in 67e9062:
The P2 (key zeroing via |
|
@greptileai review |
|
@greptileai review |
1 similar comment
|
@greptileai review |
Per Erik's review on PR ActivityWatch#584: silently warning is wrong because the user explicitly requested encryption — falling back to an unencrypted database violates that intent. Better to refuse to start. The user can either: - rebuild with 'encryption' or 'encryption-vendored' feature, or - unset AW_DB_PASSWORD to use an unencrypted database knowingly.
|
@greptileai review |
|
All review threads are now resolved. Summary of what was addressed since the initial Greptile review:
CI is green on all platforms (ubuntu, macOS, Windows, Android). Greptile 5/5. Ready for merge @ErikBjare — just needs your LGTM. |
…rikBjare/bob#682) (#753) The permission-blocked merge_ready suppression added in #750 only matched four canonical phrases (e.g. "waiting only on a maintainer click", "ready to merge when convenient"). Bob's real-world ready comment on ActivityWatch/aw-server-rust#584 was "Ready for merge @ErikBjare — just needs your LGTM.", which fell through the suppression and re-dispatched as fake-ready work. Add a regex pattern matching "ready (to|for) merge @<maintainer>" — the @-mention is the explicit maintainer-handoff signal that distinguishes a real suppression case from generic "ready to merge once CI passes" prose. Apply the same pattern to both `has_maintainer_waiting_comment` in activity-gate.sh and `is_permission_blocked_merge_ready_pr` in check-notifications.sh so both project-monitoring paths stay aligned. Verified end-to-end against ActivityWatch/aw-server-rust#584: function now returns SUPPRESSED. Closes ErikBjare/bob#682
|
Friendly bump — this PR is 12 days old with no reviews. Self-review recheck (2026-05-19):
This is a larger change but well-structured and gated. Would appreciate a review when you have a moment. |
|
Friendly bump (2026-05-21) — this PR is 35 days old. Status recap:
@ErikBjare — when you get a chance, this is ready for re-review. The feature is fully optional (no-op on default build). |
Adds an `encryption` feature flag to aw-datastore (and aw-server) that enables SQLCipher-based database encryption at rest. **Usage**: ``` cargo build --no-default-features --features encryption aw-server --db-password mysecretkey # or: AW_DB_PASSWORD=mysecretkey aw-server ``` **Changes**: - aw-datastore: restructure rusqlite features so `bundled` (default) and `encryption` (opt-in SQLCipher) are mutually exclusive - aw-datastore: add `DatastoreMethod::FileEncrypted(path, key)` variant applying PRAGMA key after connection open - aw-datastore: add `Datastore::new_encrypted()` constructor - aw-server: forward `encryption` / `encryption-vendored` features from aw-datastore; accept --db-password / AW_DB_PASSWORD - tests: add `test_encrypted_datastore_roundtrip` verifying data survives a close/reopen cycle with the correct key Closes ActivityWatch#435
…ssphrase early Two security issues flagged by Greptile review: 1. DatastoreMethod derived Debug, which would expose the raw encryption key in log output, panic messages, or debug instrumentation. Replace derive with a manual Debug impl that redacts the key field as '<redacted>'. 2. PRAGMA key always succeeds even with a wrong passphrase; the actual error only surfaces on the first real SQL query, producing an opaque worker-thread panic. Add an immediate PRAGMA user_version read to validate the key upfront with a clear error message.
Per Erik's review on PR ActivityWatch#584: silently warning is wrong because the user explicitly requested encryption — falling back to an unencrypted database violates that intent. Better to refuse to start. The user can either: - rebuild with 'encryption' or 'encryption-vendored' feature, or - unset AW_DB_PASSWORD to use an unencrypted database knowingly.
The key field in DatastoreMethod::FileEncrypted was held as a plain String. After the worker thread exits and DatastoreMethod is dropped, the key bytes could linger in process memory until overwritten by the allocator. Use zeroize::Zeroizing<String> so the key is securely zeroed on drop. zeroize is added as an optional dep, enabled only by the encryption and encryption-vendored feature flags — default builds are unaffected.
TimeToBuildBob
force-pushed
the
feat/sqlcipher-encryption
branch
from
4597c2c to
32e0c12
Compare
|
@greptileai review |
…assphrase
- Move std::env::remove_var("AW_DB_PASSWORD") to immediately after Opts::parse(),
before setup_logger may start background threads (remove_var is non-reentrant
on some platforms when concurrent readers exist).
- Add empty-string guard: SQLCipher silently treats PRAGMA key '' as no passphrase,
so an empty --db-password / AW_DB_PASSWORD would produce a plaintext database
while logging "Using encrypted database". Panic early instead.
|
@greptileai review |
|
Addressed the remaining two findings from Greptile's 3/5 review in fcb174d:
Re-triggered Greptile to re-score. |
|
Self-review pass — CI all green on all platforms (Android, macOS, Ubuntu, Windows). Greptile gave 5/5 confidence. Security properties verified:
Minor observations (non-blocking):
This is ready to merge from my side. @ErikBjare ping for merge when you have a moment. |
2 participants
Summary
Implements encrypted database storage at rest using SQLCipher (via the
rusqlite/bundled-sqlcipherfeature). Data remains fully encrypted on disk; decryption only happens in-process after the key is supplied.Closes #435
Design
SQLCipher is a drop-in replacement for SQLite that transparently encrypts every database page. All existing schema/migration code works unchanged — only the connection-open step gains an extra
PRAGMA keycall.This is implemented as a Cargo feature flag so the default binary stays unchanged. Users who want encryption build with:
cargo build --no-default-features --features encryption # or, for a fully self-contained binary that also vendors OpenSSL: cargo build --no-default-features --features encryption-vendoredFeature flags
bundled(default)encryptionencryption-vendoredbundledandencryption*are mutually exclusive (libsqlite3-sys enforces this). Use--no-default-featureswhen enabling encryption.API changes
aw-datastore:
DatastoreMethod::FileEncrypted(path, key)variant (cfg-gated)Datastore::new_encrypted(dbpath, key, legacy_import)constructoraw-server:
--db-password <KEY>CLI flag (also readable fromAW_DB_PASSWORDenv var)Usage
Changes
aw-datastore/Cargo.toml— restructure rusqlite features; addencryptionandencryption-vendoredaw-datastore/src/lib.rs— addDatastoreMethod::FileEncryptedvariantaw-datastore/src/worker.rs— open encrypted connection withPRAGMA key; addDatastore::new_encrypted()aw-server/Cargo.toml— forward encryption features from aw-datastore; changeaw-datastoredep todefault-features = falseaw-server/src/main.rs—--db-password/AW_DB_PASSWORDoption; selectnew_encrypted()when key is presentaw-datastore/tests/datastore.rs—test_encrypted_datastore_roundtrip: creates encrypted DB, writes events, closes, reopens with same key, verifies data is intactTest plan
cargo check) passes with no errorscargo test --no-default-features --features encryption -- test_encrypted_datastore_roundtrip(requires OpenSSL)aw-server --db-password foo, send heartbeats, stop, verifyfile aw-server-rust.dbshows "SQLite database" is no longer readable as plain SQLite