Closed thomcc closed 6 years ago
I have relatively strong feelings that a misconfiguration — trying to build a key-using Mentat application against a non-crypto Mentat — should fail to build or panic at launch (the behavior that will result with bad run-time linking) rather than yielding a runtime error; after all, we have a wonderful compiler to help us, and the consequences of accidentally muffling the error (especially if someone writes expedient fallback code!) are potentially severe.
I don’t think it’s too onerous to specify a cipher
feature when importing Mentat; indeed, we already expose a bundling feature that we pass through to rusqlite
and that we expect real-world callers to have to think about. (Firefox will specify non-bundled because SQLite is already linked into libxul
.)
An additional layer of runtime checks, as rusqlite
does for some features, seems like a good option to handle the dynamic linking risk in a descriptive way.
I have relatively strong feelings that a misconfiguration — trying to build a key-using Mentat application against a non-crypto Mentat — should fail to build...
I can get a patch up for this relatively easily because I did it this way first, before deciding that it was annoying. My concerns are basically:
Neither of these may actually matter, though.
An additional layer of runtime checks, as rusqlite does for some features, seems like a good option to handle the dynamic linking risk in a descriptive way.
I'm not sure how to check this at runtime in a way that gives a descriptive result and isn't sort of disgusting. I can think of:
dlopen
and dlsym
equivalents: I don't think we know we'll always dynamically link against sqlcipher though. The common case is probably static (complete guess without any evidence).#[cfg(feature = "sqlcipher")] extern "C" fn sqlite3_key(...)
in libsqlite3_sys or something similar. The main difficulty here is integrating with their build system, but maybe we could just -DSQLITE_HAS_CODEC
for bindgen or something? IDK.#[link]
in order to make it work in the dynamically linked case but don't know.It's likely that I'm either unaware of or forgetting a substantially easier way to do this. (Ideally there would be a way that just requires running SQL!)
Whelp, I have no clue how to get travis working with sqlcipher. AFAICT the version of ubuntu on travis means that apt-get install libsqlcipher-dev
is installing a version from 2013, which means it's too old for the sqlite that mentat needs.
For now I've abandoned testing on travis, and will file a bug about that.
Just FYI, this doesn't build with stable Rust. I'll fix and push follow-up.
Just FYI, this doesn't build with stable Rust. I'll fix and push follow-up.
Sorry, with Rust 1.25:
rustc 1.25.0 (84203cac6 2018-03-25)
Our minimum version is set in https://github.com/mozilla/mentat/blob/6a1a2658942b29fd4cd151fb1cd50f3a7f3855fb/build/version.rs#L22
Just FYI, this doesn't build with stable Rust. I'll fix and push follow-up.
I've addressed this in https://github.com/mozilla/mentat/commit/f041dfe509b40b1db85db42b75121866e1ab9bb4.
This adds a
sqlcipher
feature (which is incompatible with thebundled_sqlite3
) support for opening aStore
with a key, changing the key of an open Store, and support for it through the CLI and (in theory) the FFI. It's based some on @mhammond's branch, and on debugging we did on Wednesday.The additional APIs aren't behind
cfg(feature = "sqlcipher")
mostly because it seems like that's a big pain to use and requires callers add a lot of similarcfg
's. Instead, we emit aResult::Err
when you try to do something that would require sqlcipher when we weren't build with support for it. I could easily be convinced we should do things the other way.It's easy-ish to try on mac, since you can
brew install sqlcipher --with-fts
, thencargo build --no-default-features --features sqlcipher
. See caveat number 1 about building/running the cli or building the ffi, though (needs to be done from the directory in question due to a cargo bug). On other platforms this is trickier.Some caveats:
Until https://github.com/rust-lang/cargo/issues/5364 is fixed, building nested crates that depend on mentat itself (e.g. tools/cli and ffi) must be done inside the directory of those crates. Specifically
cargo build --no-default-features --features sqlcipher -p mentat_cli
will apply the--features
and--no-default-features
flags to the wrong package.cd tools/cli; cargo build --no-default-features --features sqlcipher; cd '-';
works instead. Ditto substitutingtools/cli
withffi
. Yes, this is annoying.PRAGMA page_size
doesn't seem to be valid in all cases when used in conjunction withPRAGMA cipher_page_size
. I can't find official documentation on this. What I have found is:page_size
but notcipher_page_size
seems never to be valid.cipher_page_size
is a positive integer multiple ofpage_size
(I didn't check this very thoroughly though).page_size
, thecipher_page_size
is not simply an optimization detail! Having the correctcipher_page_size
is required to read the contents of a database, e.g. if you open a DB with cipher_page_size at 8192 (for example) that's the value it must always have in order for reads/writes to be successful.I set it to the value we were using for
page_size
before, and made the code use bothcipher_page_size
andpage_size
when using sqlcipher. It's not at all clear to me this was the right choice, though.There are no tests outside the CLI parsing changes. I'm open to suggestions about how best to test this, though. Maybe adding an encrypted db in fixtures and making sure that we can open it would be enough?