-
-
Notifications
You must be signed in to change notification settings - Fork 80
feat(datastore): add opt-in SQLCipher encryption support #584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
e886a48
67e9062
8603ab3
d2d797f
4597c2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,6 +125,20 @@ impl DatastoreWorker { | |
| DatastoreMethod::File(path) => { | ||
| Connection::open(path).expect("Failed to create datastore") | ||
| } | ||
| #[cfg(any(feature = "encryption", feature = "encryption-vendored"))] | ||
| DatastoreMethod::FileEncrypted(path, key) => { | ||
| let conn = Connection::open(path).expect("Failed to create encrypted datastore"); | ||
| conn.pragma_update(None, "key", key) | ||
| .expect("Failed to set SQLCipher encryption key"); | ||
| // PRAGMA key always succeeds even with a wrong passphrase; the | ||
| // first real SQL query is what fails. Read user_version immediately | ||
| // to surface an incorrect key as a clear error rather than an | ||
| // opaque panic later. | ||
| conn.pragma_query_value(None, "user_version", |row| row.get::<_, i64>(0)) | ||
| .expect("Failed to open encrypted database: wrong passphrase or not an encrypted database"); | ||
| info!("Opened encrypted database at {}", path); | ||
| conn | ||
| } | ||
|
Comment on lines
+128
to
+141
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Add an explicit verification step immediately after setting the key so the failure is diagnosed clearly: conn.pragma_update(None, "key", key)
.expect("Failed to set SQLCipher encryption key");
// Verify the key is correct — PRAGMA user_version is a simple read that
// will return an error if the key cannot decrypt the database header.
conn.pragma_query_value(None, "user_version", |r| r.get::<_, i32>(0))
.expect("SQLCipher key is incorrect or database is not encrypted");This produces a human-readable panic message instead of a cryptic SQLite error buried inside a thread.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 67e9062 — added |
||
| }; | ||
| let mut ds = DatastoreInstance::new(&conn, true).unwrap(); | ||
|
|
||
|
|
@@ -324,6 +338,16 @@ impl Datastore { | |
| Datastore::_new_internal(method, legacy_import) | ||
| } | ||
|
|
||
| /// Create an encrypted datastore using SQLCipher. | ||
| /// | ||
| /// Requires the `encryption` or `encryption-vendored` feature flag. | ||
| /// Build with: `cargo build --no-default-features --features encryption` | ||
| #[cfg(any(feature = "encryption", feature = "encryption-vendored"))] | ||
| pub fn new_encrypted(dbpath: String, key: String, legacy_import: bool) -> Self { | ||
| let method = DatastoreMethod::FileEncrypted(dbpath, key); | ||
| Datastore::_new_internal(method, legacy_import) | ||
| } | ||
|
|
||
| fn _new_internal(method: DatastoreMethod, legacy_import: bool) -> Self { | ||
| let (requester, responder) = | ||
| mpsc_requests::channel::<Command, Result<Response, DatastoreError>>(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -57,6 +57,13 @@ struct Opts { | |||||||||||||||||||||||||||||
| /// Don't import from aw-server-python if no aw-server-rust db found | ||||||||||||||||||||||||||||||
| #[clap(long)] | ||||||||||||||||||||||||||||||
| no_legacy_import: bool, | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /// Encryption key for the database (requires 'encryption' feature). | ||||||||||||||||||||||||||||||
| /// Can also be set via the AW_DB_PASSWORD environment variable. | ||||||||||||||||||||||||||||||
| /// WARNING: passing a password on the command line may expose it in process listings. | ||||||||||||||||||||||||||||||
| #[clap(long, env = "AW_DB_PASSWORD")] | ||||||||||||||||||||||||||||||
| #[cfg(any(feature = "encryption", feature = "encryption-vendored"))] | ||||||||||||||||||||||||||||||
| db_password: Option<String>, | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| #[rocket::main] | ||||||||||||||||||||||||||||||
|
|
@@ -141,10 +148,33 @@ async fn main() -> Result<(), rocket::Error> { | |||||||||||||||||||||||||||||
| device_id::get_device_id() | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| #[cfg(any(feature = "encryption", feature = "encryption-vendored"))] | ||||||||||||||||||||||||||||||
| let datastore = if let Some(key) = opts.db_password { | ||||||||||||||||||||||||||||||
| // Clear the env var immediately so child processes don't inherit the key. | ||||||||||||||||||||||||||||||
| std::env::remove_var("AW_DB_PASSWORD"); | ||||||||||||||||||||||||||||||
| info!("Using encrypted database (SQLCipher)"); | ||||||||||||||||||||||||||||||
| aw_datastore::Datastore::new_encrypted(db_path, key, legacy_import) | ||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||
| aw_datastore::Datastore::new(db_path, legacy_import) | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| #[cfg(not(any(feature = "encryption", feature = "encryption-vendored")))] | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| if std::env::var("AW_DB_PASSWORD").is_ok() { | ||||||||||||||||||||||||||||||
| panic!( | ||||||||||||||||||||||||||||||
| "AW_DB_PASSWORD is set but this binary was not compiled with encryption support. \ | ||||||||||||||||||||||||||||||
| Refusing to start with an unencrypted database when the user requested encryption. \ | ||||||||||||||||||||||||||||||
| Rebuild with the 'encryption' or 'encryption-vendored' feature, or unset \ | ||||||||||||||||||||||||||||||
| AW_DB_PASSWORD to use an unencrypted database." | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| #[cfg(not(any(feature = "encryption", feature = "encryption-vendored")))] | ||||||||||||||||||||||||||||||
| let datastore = aw_datastore::Datastore::new(db_path, legacy_import); | ||||||||||||||||||||||||||||||
|
Comment on lines
+160
to
+172
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the binary is built without the Add a runtime guard in the non-encryption path: #[cfg(not(any(feature = "encryption", feature = "encryption-vendored")))]
{
if std::env::var("AW_DB_PASSWORD").is_ok() {
warn!(
"AW_DB_PASSWORD is set but this binary was not compiled with \
encryption support (missing `encryption` feature). \
The database will NOT be encrypted and the password is ignored."
);
}
aw_datastore::Datastore::new(db_path, legacy_import)
}
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like having it set with encryption not built should trigger a panic... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the right call for a security-critical misconfiguration. A warning can be missed; a panic makes the failure impossible to ignore. Here's the updated suggestion:
Suggested change
The panic message explains both why it's failing and how to fix it, which is the important part — users who hit this will know exactly what to do. Tip: You can customize Greptile's behavior for this repo with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — addressed in 4597c2c. Changed |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| let server_state = endpoints::ServerState { | ||||||||||||||||||||||||||||||
| // Even if legacy_import is set to true it is disabled on Android so | ||||||||||||||||||||||||||||||
| // it will not happen there | ||||||||||||||||||||||||||||||
| datastore: Mutex::new(aw_datastore::Datastore::new(db_path, legacy_import)), | ||||||||||||||||||||||||||||||
| datastore: Mutex::new(datastore), | ||||||||||||||||||||||||||||||
| asset_resolver: endpoints::AssetResolver::new(asset_path), | ||||||||||||||||||||||||||||||
| device_id, | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The encryption key is held as a plain
StringinsideFileEncrypted. When theDatastoreMethodvalue is dropped (after the worker exits), the key bytes remain in process memory until overwritten by the allocator. For an encryption feature this is worth addressing withzeroize::Zeroizing<String>:This ensures the passphrase is securely wiped when the enum is dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged — I'll leave the
zeroizeimprovement as a follow-up. The key is stored inDatastoreMethodonly until the worker thread starts and extracts it at the top ofwork_loop, so the exposure window is short. But you're right thatZeroizing<String>would be the clean solution for a hardened build. I'll track it separately rather than block this PR on it.