Apply changes to Database Encryption API #103
Conversation
pasin
commented
Mar 23, 2021
•
pasin
commented
- Applied changes to Database Encryption API.
- Kept encryptionKey inside CBLDatabase for returning it back when calling CBLDatabase_Config.
- Added a test for testing database encryption.
- Fixed EE exported symbol file settings.
pasin
force-pushed
the
feature/encryption-api
branch
3 times, most recently
from
c9adb6b
to
d74dad1
Compare
|
Windows test hang is fixed in #108. |
include/cbl/CBLDatabase.h
Outdated
| @@ -55,10 +52,9 @@ typedef struct CBLEncryptionKey { | |||
| /** Database configuration options. */ | |||
| typedef struct { | |||
| FLString directory; ///< The parent directory of the database | |||
| CBLEncryptionKey* encryptionKey; ///< The database's encryption key (if any) | |||
| CBLEncryptionKey* encryptionKey; ///< (Enterprise Edition Only) The database's encryption key (if any) | |||
There was a problem hiding this comment.
This is compiled out of CE builds on other platforms
There was a problem hiding this comment.
First, I was thinking about putting ifdef COUCHBASE_ENTERPRISE there, but then unpleasant code changes (e.g. branching constructor and so on) are needed so I stopped (but that shouldn't be the reason not to do it).
I'm not quite understand about putting ifdef COUCHBASE_ENTERPRISE in the public header files as users will then need a defined COUCHBASE_ENTERPRISE (maybe need to define it in an EE header?). I plan to have some discussion about this when doing EE repo. Let's chat about this next week.
src/CBLDatabase_Internal.hh
Outdated
| @@ -352,6 +356,7 @@ private: | |||
|
|
|||
| litecore::access_lock<Retained<C4Database>> _c4db; | |||
| alloc_slice const _dir; | |||
| CBLEncryptionKey* _key; | |||
There was a problem hiding this comment.
Using a bare pointer here is not good practice ... who owns this key? It looks like this points directly to the caller-provided key, so if the app used a stack variable, it will be overwritten with garbage very soon. I think this should be a `CBLEncryptionKey struct.
* Applied changes to Database Encryption API. * Kept encryptionKey inside CBLDatabase for returning it back when calling CBLDatabase_Config. * Added a test for testing database encryption.
* Reverted CBLEncryptionAlgorithm to include kCBLEncryptionNone. With this small compromise, we could keep CBLDatabaseConfiguration.encryptionKey in both CE and EE. Also, implementation wise, when copying CBLEncryptionKey, kCBLEncryptionNone helps to know whether there is an encryption or not. * Made CBLDatabaseConfiguration.encryptionKey const pointer to avoiding changing the bytes of the encryptionKey getting from database’s config. * In CBLDatabase, copy encryption key instead of keeping the pointer.
pasin
force-pushed
the
feature/encryption-api
branch
from
d74dad1
to
3e8ff6c
Compare
|
I have made changes as suggested by the code review. I have also reverted CBLEncryptionAlgorithm to the original which includes kCBLEncryptionNone which helps simplifying CE and EE availability of the functionality as well as some part of the implementation (Need to update API Spec). |
* Made CBLDatabaseConfiguration.encryptionKey and CBLEncryptionKey inside ifdef COUCHBASE_ENTERPRISE. * Updated the other parts of the code correspondingly.
include/cbl/CBLDatabase.h
Outdated
| CBLEncryptionKey* encryptionKey; ///< The database's encryption key (if any) | ||
| FLString directory; ///< The parent directory of the database | ||
| #ifdef COUCHBASE_ENTERPRISE | ||
| const CBLEncryptionKey* encryptionKey; ///< The database's encryption key (if any) |
There was a problem hiding this comment.
There doesn't seem to be a reason anymore to have this be a pointer. Why not just put the key struct itself in the configuration? If you zero-initialize the config (CBLDatabaseConfig config {};) the key will be set to none.
Made CBLConfiguration.encryptionKey a non-pointer value per code review.
* Apply changes to Database Encryption API * Applied changes to Database Encryption API. * Kept encryptionKey inside CBLDatabase for returning it back when calling CBLDatabase_Config. * Added a test for testing database encryption. * Fix setting exported symbol file for EE * Fix based on code review * Reverted CBLEncryptionAlgorithm to include kCBLEncryptionNone. With this small compromise, we could keep CBLDatabaseConfiguration.encryptionKey in both CE and EE. Also, implementation wise, when copying CBLEncryptionKey, kCBLEncryptionNone helps to know whether there is an encryption or not. * Made CBLDatabaseConfiguration.encryptionKey const pointer to avoiding changing the bytes of the encryptionKey getting from database’s config. * In CBLDatabase, copy encryption key instead of keeping the pointer. * Make CBLDatabaseConfiguration.encryptionKey EE feature * Made CBLDatabaseConfiguration.encryptionKey and CBLEncryptionKey inside ifdef COUCHBASE_ENTERPRISE. * Updated the other parts of the code correspondingly. * Make CBLConfiguration.encryptionKey a non-pointer value Made CBLConfiguration.encryptionKey a non-pointer value per code review.
