-
Notifications
You must be signed in to change notification settings - Fork 156
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
[RFC] Minimal SQLCipher support #77
Conversation
@zauguin I have no experiment with SQLCipher. Here is @polesapart comment about this issue (from #10)
|
hdr/sqlite_modern_cpp.h
Outdated
|
||
void set_key(const std::string &key) { | ||
if(auto ret = sqlite3_key(_db.get(), key.data(), key.size())) | ||
exceptions::throw_sqlite_error(ret); |
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.
Please add a custom exception class for sqlcipher.
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.
There is no such thing.
SQLCipher isn't very good at error reporting.
For example if the key is wrong, it still returns SQLITE_OK and fails only if you try to access the database for the first time with the existing SQLITE_NOTADB.
The key and rekey functions only fail, if you pass them nullpointers, but this can't happen here.
So actually this are noops, but the functions are a part of sqlites more generic codec interface and other implementations may choose to fail here, so we should at least check the return code.
hdr/sqlite_modern_cpp.h
Outdated
database(const std::string &db_name, const std::string &key): database(db_name) { | ||
set_key(key); | ||
} | ||
database(const std::u16string &db_name, const std::string &key): database(db_name) { |
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.
hdr/sqlite_modern_cpp.h
Outdated
#ifdef SQLITE_HAS_CODEC | ||
if(!config.key.empty()) set_key(config.key); | ||
#else | ||
assert(config.key.empty() && "Encryption supported is disabled."); |
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.
We could use an exception instead, but passing a key without encryption support is IMO a strong indication, that the application is compiled with the wrong settings.
I use sqlcipher - by including it's header instead of sqlite and arranging proper ldflags - and using PRAGMAS instead of it's extended C api. While I don't see any problems with conditionally adding that support to sqlite_modern_cpp, it's not strictly necessary either. Please note that sqlcipher is not the only project using SQLITE_HAS_CODEC and relying on it for autodetection may lead to incompatibilities in case someone uses one of these other extensions to sqlite. |
SQLITE_HAS_CODEC is used for autodetection, because I think this works for every system using SQLITE_HAS_CODEC. |
At least "see" (sqlite's own encrypted version, which is proprietary and requires license) uses SQLITE_HAS_CODEC for the same purposes. One wouldn't be able to use "see" transparently with this library, as it has no 1:1 API compatibility with sqlcipher (altough there's a common subset). I suggest you disable autodetection and require the user to pass a specific define, i.e. HAVE_SQLCIPHER or something. |
@polesapart @zauguin We are adding new I wonder if you think there is an easy way to include new |
@zauguin How about |
I like this approach, but if we want to add additional features like this we may need virtual inheritance so that the features can be combined. |
@zauguin Nicely done! Looks good to me 👍 |
@zauguin Is it possible (& easy) to write a test for this? |
Writing a test is easy but running it isn't. We could either require SQLCipher for our test suite or only test it if SQLCipher is available. I prefer the second option, but I don't know how to implement it with autotools. We might have to change our testing system too, because the other test should not run against SQLCipher. |
@zauguin I see that you have already implemented Skipped tests to Makefile.in. Add a test for sqlcipher and skip it by default in Makefile.in. |
I added the test which only executes if ENABLE_SQLCIPHER_TESTS is defined and added a hack for travis to check using SQLite3 and SQLCipher. |
@aminroosta I came late to the party, but everything seems ok from where I stand. Thanks to @zauguin for his efforts! |
Is there any interest in SQLCipher support?
How should this be tested? I don't really want to introduce a SQLCipher dependency just for the tests.