Use SQLCipher to encrypt SQLite databases: datastores & indexes (second try) #112

indisoluble · 2015-03-05T12:54:03Z

What:
Provide a mechanism to cipher the SQLite databases in CDTDatastore with SQLCipher.

Why:
Some users can not rely on the encryption provided by default in iOS. We have to give an alternative solution for those that are in this situation.

How:
CDTDatastore accesses to SQLite databases with FMDB. This library is already able to use SQLCipher, we only have to provide a key to encrypt the database and compile the library with the correct subspec. However users should not have to care about 3rd party libraries so:

We will add a subspec to CDTDatastore.podspec that will import every necessary component to encrypt databases.
Only if the user set this new subspec, CDTDatastore will expose the new methods to create encrypted datastores. We will handle this defining the new methods in categories, only with the new subspec these categories will be imported into the project. This way, if an user does not change his/her Podfile, the API will remain the same and he/she will not be able to use the new methods by mistake.
To provide the key to encrypt the databases, a user will have to implement a class that conforms to a new protocol and pass an instance of the class to the new methods. Defining a protocol will allow us to implement later on a helper class to safely handle and store the key.

Tests:
CDTDatastore manages two different databases:

One with the data in the datastore
Other for the indexes

Each one is tested independently. Also the result of these tests depends on the library included, i.e. if SQLCipher is included or not.

datastore + SQLCipher
1. Create a non-encrypted db if no key is provided
2. Create an encrypted db if a key is provided
3. Do not open a non-encrypted db if a key is provided
4. Do not open an encrypted db if no key is provided
5. Do not open an encrypted db if the key provided is not the key used to encrypt the db
6. Do not open a non-encrypted db if a key is provided although the db was opened before (in the same session)
7. Do not open an encrypted db if no key is provided although the db was opened before (in the same session)
8. Do not open an encrypted db if the key provided is not the key used to encrypt the db although the db was opened before (in the same session)
datastore + SQLite
1. Create a non-encrypted db if no key is provided
2. Do not create a encrypted db if a key is provided
3. Do not open a non-encrypted db with a key
4. Do not open an encrypted db with a key (although it is the right key)
5. Do not open an encrypted db if no key is provided
6. Do not open a non-encrypted db with a key although the db was opened before (in the same session)
index + SQLCipher
1. Create an non-encrypted db with a non-encrypted datastore
2. Create an encrypted db with an encrypted datastore
3. Do not open an encrypted db with a non-encrypted datastore
4. Do not open a non-encrypted db with an encrypted datastore
5. Do not open an encrypted db with an encrypted datastore if they do not share the same key
index + SQLite
1. Create an non-encrypted db with a non-encrypted datastore
2. Do not open an encrypted db with a non-encrypted datastore

reviewer @mikerhodes
reviewer @emlaver

mikerhodes · 2015-03-09T14:00:28Z

Main things:

key provider not retriever. The implementor of the protocol need not retrieve a key from anywhere (e.g., your hard-coded implementations in the tests).
Your tests don't appear to check that the database file is actually encrypted. Given you're providing a fixed key, you should be able to check the first say 50 bytes of the file against a canned set of bytes.
09b74c2 has a bunch of stuff related to deleting databases in it. Should these be in the other commit?
I don't think that the createDatabase=true option in the replicator should allow creating databases with encryption keys. Extra complexity isn't worth it.
I'd like to understand why the pattern of a "dummy" key provider rather than just allowing a nil provider?

indisoluble · 2015-03-10T19:01:12Z

Changed:

Protocol renamed: key provider not retriever.
Tests check that the database (datastore and index) is encrypted or no depending on the case.
Replicators do not use (or make reference) to encryption key. They are created with already open databases, so they do not need the encryption key.

Not changed:

It was necessary to modify the delete method because it opened the databases before deleting them, therefore it needed the encryption key. To avoid that, I check if the database is already open because in the case I do not need the encryption key and if it is not open, no other object is related to it so I can simply delete the files from disk.
I do not allow nil as a provider because in that case I have to do twice validations. First to check if there is an object and second to check if I get an encryption key. Of course, if I send a method to nil, I'll get nil as a response but if I try to put the provider in an array or a dictionary, the app will crash.
Some classes implement NSCopying protocol but they do not copy the object, instead they alloc a new object; that is because they inherit from NSObject.

Also:

Renamed dummy provider to nil provider.
Methods in CDT-classes related to encrypt the database have been moved to categories, so if a user is not interested in this functionality, he/she does not have to bother about it.
Clarified code in 'TD_DatabaseManager:deleteDatabaseNamed:error:'

Pending:

Rewrite history before reviewing changes.

mikerhodes · 2015-03-16T17:34:11Z

Classes/common/CDTEncryptionKey/CDTDatastore+EncryptionKey.h

+
+#import "CDTDatastore.h"
+
+@protocol CDTEncryptionKeyProviding;


I think this should be CDTEncryptionKeyProvider

Fixed in 66c19fb

mikerhodes · 2015-03-25T10:00:20Z

Several of the error and other log messages in -openFMDBWithEncryptionKeyProvider: need more context to them, for example not all include the path or are difficult to understand out of context, such as @"Wrong key". At least include the database name.

Also remember log messages are read by a developer, so messages like Is DB at path %@ encrypted? are read as questions to them, where as the log message's meaning is more like "You didn't provide a key. Checking whether database is encrypted.".

mikerhodes · 2015-03-25T10:02:46Z

Classes/common/touchdb/TD_Database.m

+        result = (queue != nil);
+    }
+
+    // Cipher database


Comment seems inaccurate -- this checks we can use the key provided with the database file?

Fixed in bc0c17e

mikerhodes · 2015-03-25T10:32:24Z

I think you could split up 9127d10, it's over 3000 lines in a single commit. Could you have something like:

Update podspec to allow building with encryption support -- this would allow you to explain how the subspecs work in the commit comment.
Allow user to provide key to encrypt JSON data -- which would just be the code the get the encryption enabled on the main database with the JSON docs in.
Encrypt database indexes -- which shows how the key is carried into the indexing code (this would be particularly useful when updating Cloudant Query to support this after the merge).
Add tests to confirm databases encrypted -- adding the new project and tests (which is a large number of lines for not that many changes, and would be helpful separated).

mikerhodes · 2015-03-25T10:40:44Z

Classes/common/Indexing/CDTIndexManager.m

+    }
+
+    return success;
+}


This has far fewer checks than the version inside TD_Database. E.g.:

No check for whether the database is encrypted if there is no key provided.

You don't do the query to sqlite_master to make sure the key actually works.

It might be worth pulling out a utility class which does all these checks so both TD_Database and CDTIndexManager (and the Query index manager) can use it.

But an index manager is created with an datastore. If the datastore is correctly initialised, this code will not fail. It will fail if the datastore is encrypted but the index is not, which means that the user copied in the folder an index created with a different datastore. Do we need to add defensive code for a situation like this?

I don't think you should assume that the data on disk is consistent with each other. As you say, it's easy enough for an index database to be copied in whose encryption or lack thereof doesn't match the one on the main database.

In addition, in making the assumption that the TD_Database object does all the checks you need, you're increasing coupling between the two objects and the knowledge that someone maintaining the code needs to have of the wider system.

Fixed in dc84c4b

mikerhodes · 2015-03-25T10:44:37Z

Classes/common/CDTEncryptionKey/CDTDatastoreManager+EncryptionKey.h

+ @return a datastore for the given name
+
+ @warning *Warning:* Encryption is an experimental feature, use with caution. It won't work unless
+ you use subspec 'CDTDatastore/SQLCipher'


Is encryption experimental? I was hoping not, though the warning to use CDTDatastore/SQLCipher is useful.

No, it is not experimental but at the moment I wrote this comment I considered this development part of a bigger one so it was experimental in the sense that there were still more changes to deploy.

Fixed in ad3debd

mikerhodes · 2015-04-02T15:16:21Z

A +1 from me, awaiting @emlaver

emlaver · 2015-04-06T01:58:27Z

+1

Details: - Conform to protocol CDTEncryptionKeyProvider to inform a key to cipher databases - If a CDTDatastore is created with a key, the CDTIndexManager based on that CDTDatastore uses the same key to encrypt its database. - New subspec in CDTDatastore.podspec. Use 'CDTDatastore/SQLCipher' to get a version of the library capable of encrypting the databases. - Added new tests specific for the new funcitonality. Updated .travis.yml to run the tests in Travis CI

CDTDatastoreManager opens a database in order to delete it. If the database is not open yet, do not allocate memory and delete it from disk

Read-only property 'database' in CDTDatastore already ensures that the database is open, i.e. do not open database again in methods in CDDatastore+Conflicts

CDTDatastoreManager deletes databases from disk, however TD_DatabaseManager does not release them. More exactly, CDTDatastoreManager creates databases through TD_DatabaseManager but it deletes them on its own, so TD_DatabaseManager never releases them

indisoluble · 2015-04-07T15:39:57Z

Not merged to master, new branch feature-encryption created to hold whole feature while allowing smaller PRs as development occurs

indisoluble force-pushed the 43714-SQLCipher-iOS-02 branch 11 times, most recently from 37b0cf7 to 069245d Compare March 16, 2015 15:21

mikerhodes reviewed Mar 16, 2015
View reviewed changes

mikerhodes reviewed Mar 25, 2015
View reviewed changes

indisoluble force-pushed the 43714-SQLCipher-iOS-02 branch from eb282de to 347585c Compare April 7, 2015 11:50

indisoluble added 4 commits April 7, 2015 13:58

Do not open a TD_Database to delete it

39e7db3

CDTDatastoreManager opens a database in order to delete it. If the database is not open yet, do not allocate memory and delete it from disk

Remove redundancy: do not open db with 'database'

7ea7582

Read-only property 'database' in CDTDatastore already ensures that the database is open, i.e. do not open database again in methods in CDDatastore+Conflicts

indisoluble force-pushed the 43714-SQLCipher-iOS-02 branch from 347585c to 62650b2 Compare April 7, 2015 13:56

indisoluble closed this Apr 7, 2015

rhyshort mentioned this pull request Jun 17, 2016

No concurrency control for operations in database files: 'open', 'close' & 'delete' #273

Closed

rhyshort deleted the 43714-SQLCipher-iOS-02 branch January 13, 2017 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use SQLCipher to encrypt SQLite databases: datastores & indexes (second try) #112

Use SQLCipher to encrypt SQLite databases: datastores & indexes (second try) #112

indisoluble commented Mar 5, 2015

mikerhodes commented Mar 9, 2015

indisoluble commented Mar 10, 2015

mikerhodes Mar 16, 2015

indisoluble Mar 19, 2015

mikerhodes commented Mar 25, 2015

mikerhodes Mar 25, 2015

indisoluble Mar 27, 2015

mikerhodes commented Mar 25, 2015

mikerhodes Mar 25, 2015

indisoluble Mar 25, 2015

mikerhodes Mar 25, 2015

indisoluble Mar 29, 2015

mikerhodes Mar 25, 2015

indisoluble Mar 25, 2015

indisoluble Mar 27, 2015

mikerhodes commented Apr 2, 2015

emlaver commented Apr 6, 2015

indisoluble commented Apr 7, 2015


		#import "CDTDatastore.h"

		@protocol CDTEncryptionKeyProviding;

Use SQLCipher to encrypt SQLite databases: datastores & indexes (second try) #112

Use SQLCipher to encrypt SQLite databases: datastores & indexes (second try) #112

Conversation

indisoluble commented Mar 5, 2015

mikerhodes commented Mar 9, 2015

indisoluble commented Mar 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikerhodes commented Mar 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikerhodes commented Mar 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikerhodes commented Apr 2, 2015

emlaver commented Apr 6, 2015

indisoluble commented Apr 7, 2015