Skip to content
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.

Sign up for GitHub

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

SQLCipher disk encryption for Android + iOS #907

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

SQLCipher disk encryption for Android + iOS #907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
820db09
merge afiller:features/encrypted-android-db-pull-request
cjroth Dec 19, 2020
9c8de8f
update a few more locations where password is required (native only)
cjroth Dec 19, 2020
6074631
use FMDB from cocoapods instead of hard coding it
cjroth Dec 19, 2020
e4832cc
wip - get sqlcipher working for fmdb
cjroth Dec 19, 2020
82dd122
accept password form javascriptland
cjroth Dec 22, 2020
577d2e5
fix typescript issues in development
cjroth Dec 22, 2020
1799b96
ignore eslint warning that password argument is not used in makeDispa…
cjroth Dec 23, 2020
43c643a
bump sqlcipher version on android
cjroth Dec 23, 2020
a9bdd9d
update changelog
cjroth Dec 23, 2020
cb480db
remove password from web/node dispatcher
cjroth Dec 23, 2020
78b8fae
fix kotlin typo
cjroth Dec 23, 2020
3237c52
Revert "fix kotlin typo"
cjroth Dec 23, 2020
08bd625
revert typescript fix
cjroth Dec 23, 2020
7bb0341
update node dispatcher to match native, tests passing
cjroth Dec 23, 2020
91216e2
safeguard against accidental usage of password with LokiJSAdapter
cjroth Dec 23, 2020
d68c04c
ensure that password is never provided when encryption is not support…
cjroth Dec 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,5 @@ coverage/
# docs/CHANGELOG.md
# docs/README.md
docs-build/

test*.db
1 change: 1 addition & 0 deletions CHANGELOG-Unreleased.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- [adapters] `onSetUpError: Error => void` option is added to both `SQLiteAdapter` and `LokiJSAdapter`. Supply this option to catch initialization errors and offer the user to reload or log out
- [LokiJS] new `extraLokiOptions` and `extraIncrementalIDBOptions` options
- [Android] Autolinking is now supported (v0.20 is insufficient)
- [adapters] Add optional `password` parameter to `SQLiteAdapter` to support SQLCipher on iOS and Android (must be separately configured in native settings)

### Performance

Expand Down
6 changes: 6 additions & 0 deletions docs-master/Installation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,12 @@ npm install @nozbe/with-observables
pod 'React-jsi', :path => '../node_modules/react-native/ReactCommon/jsi', :modular_headers => true
```

3. Finally, add this line:

```ruby
pod 'FMDB/SQLCipher', :modular_headers => true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be required. Seems appropriate to me to add to docs-master/Advanced a new .md for how to set up encryption. Alternatively, just put this at the end and specify that this is optional

```

Note that Xcode 9.4 and a deployment target of at least iOS 9.0 is required (although Xcode 11.5+ and iOS 12.0+ are recommended).

### Android (React Native)
Expand Down
2 changes: 2 additions & 0 deletions native/android/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ dependencies {
//noinspection GradleDynamicVersion
implementation 'com.facebook.react:react-native:+'
implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
implementation "net.zetetic:android-database-sqlcipher:4.4.2"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we'd want to branch these based on the user's desire to use SQLCipher or not. If not, I imagine we'd want to use the native implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I feel a little uncomfortable forcing this dependency. android.database.sqlite.SQLiteDatabase is built in, requires no extra code, and "just works".

Can we easily make it so that it's optional? For example, there would be a separate .kt file which adds to Database support for android-database-sqlicipher, and unless we explicitly add this to app's setup we fall back to standard implementation.

I'm not super familiar with ins and outs of Android. @rozPierog ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no clear way to create dependency that's conditional that I know of.
What I would do is to move and duplicate Database.kt to separate modules: standard, cipher and require users to include another path in settings.gradle+build.gradle to one or the other. That way person using library can have full control what is in their app.
In theory if both are included it will throw error on compile that there are two classes with same name/package
In theory if non are included it will throw error on compile that there is no such class
Database initialization would need to be handled via Class.forName

To make it easier for maintenance there should be another module common that has common code from both Database files so that any change shouldn't be duplicated. (But I don't know how to cleanly handle type of SQLiteDatabase [ import android.database.sqlite.SQLiteDatabase vs import net.sqlcipher.database.SQLiteDatabase])

Disclaimer: I have no idea what I'm doing. I've never attempted stuff like this and I'm not familiar with pitfalls of modularization like that.

@cjroth Can you give your input here? How would you handle this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disclaimer: I have no idea what I'm doing. I've never attempted stuff like this and I'm not familiar with pitfalls of modularization like that.

Haha! Me neither, I don't know even know Kotlin :)

I think I agree with both of your comments though - I'm not aware of any way to do optional dependencies, so separating into modules seems like the best option. I do think this is kind of a bummer for users that don't need to use SQLCipher since it adds one more step to setup, but maybe it's worth it.

implementation "androidx.sqlite:sqlite:2.0.1"
}
repositories {
mavenCentral()
Expand Down
24 changes: 16 additions & 8 deletions native/android/src/main/java/com/nozbe/watermelondb/Database.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,17 @@ package com.nozbe.watermelondb

import android.content.Context
import android.database.Cursor
import android.database.sqlite.SQLiteDatabase
import net.sqlcipher.database.SQLiteDatabase
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

import java.io.File

class Database(private val name: String, private val context: Context) {
class Database(private val name: String, private var password: String?, private val context: Context) {

private val db: SQLiteDatabase by lazy {
if (password.equals("")) {
password = null
}

SQLiteDatabase.loadLibs(context)
SQLiteDatabase.openOrCreateDatabase(
// TODO: This SUCKS. Seems like Android doesn't like sqlite `?mode=memory&cache=shared` mode. To avoid random breakages, save the file to /tmp, but this is slow.
// NOTE: This is because Android system SQLite is not compiled with SQLITE_USE_URI=1
Expand All @@ -18,6 +23,7 @@ class Database(private val name: String, private val context: Context) {
} else
// On some systems there is some kind of lock on `/databases` folder ¯\_(ツ)_/¯
context.getDatabasePath("$name.db").path.replace("/databases", ""),
password,
null)
}

Expand Down Expand Up @@ -79,12 +85,14 @@ class Database(private val name: String, private val context: Context) {
private fun getAllTables(): ArrayList<String> {
val allTables: ArrayList<String> = arrayListOf()
rawQuery(Queries.select_tables).use {
it.moveToFirst()
val index = it.getColumnIndex("name")
if (index > -1) {
do {
allTables.add(it.getString(index))
} while (it.moveToNext())
if (it.count > 0) {
it.moveToFirst()
val index = it.getColumnIndex("name")
if (index > -1) {
do {
allTables.add(it.getString(index))
} while (it.moveToNext())
}
Comment on lines -82 to +95
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this necessary?

}
}
return allTables
Expand Down
8 changes: 7 additions & 1 deletion native/android/src/main/java/com/nozbe/watermelondb/DatabaseBridge.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class DatabaseBridge(private val reactContext: ReactApplicationContext) :
fun initialize(
tag: ConnectionTag,
databaseName: String,
password: String,
schemaVersion: Int,
promise: Promise
) {
Expand All @@ -43,6 +44,7 @@ class DatabaseBridge(private val reactContext: ReactApplicationContext) :
driver = DatabaseDriver(
context = reactContext,
dbName = databaseName,
password = password,
schemaVersion = schemaVersion
)
)
Expand All @@ -66,6 +68,7 @@ class DatabaseBridge(private val reactContext: ReactApplicationContext) :
fun setUpWithSchema(
tag: ConnectionTag,
databaseName: String,
password: String,
schema: SQL,
schemaVersion: SchemaVersion,
promise: Promise
Expand All @@ -74,6 +77,7 @@ class DatabaseBridge(private val reactContext: ReactApplicationContext) :
driver = DatabaseDriver(
context = reactContext,
dbName = databaseName,
password = password,
schema = Schema(
version = schemaVersion,
sql = schema
Expand All @@ -86,6 +90,7 @@ class DatabaseBridge(private val reactContext: ReactApplicationContext) :
fun setUpWithMigrations(
tag: ConnectionTag,
databaseName: String,
password: String,
migrations: SQL,
fromVersion: SchemaVersion,
toVersion: SchemaVersion,
Expand All @@ -101,7 +106,8 @@ class DatabaseBridge(private val reactContext: ReactApplicationContext) :
from = fromVersion,
to = toVersion,
sql = migrations
)
),
password = password
),
promise = promise
)
Expand Down
14 changes: 7 additions & 7 deletions native/android/src/main/java/com/nozbe/watermelondb/DatabaseDriver.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import com.facebook.react.bridge.WritableArray
import java.lang.Exception
import java.util.logging.Logger

class DatabaseDriver(context: Context, dbName: String) {
class DatabaseDriver(context: Context, dbName: String, password: String) {
sealed class Operation {
class Execute(val table: TableName, val query: SQL, val args: QueryArgs) : Operation()
class Create(val table: TableName, val id: RecordID, val query: SQL, val args: QueryArgs) :
Expand All @@ -24,25 +24,25 @@ class DatabaseDriver(context: Context, dbName: String) {
class SchemaNeededError : Exception()
data class MigrationNeededError(val databaseVersion: SchemaVersion) : Exception()

constructor(context: Context, dbName: String, schemaVersion: SchemaVersion) :
this(context, dbName) {
constructor(context: Context, dbName: String, password: String, schemaVersion: SchemaVersion) :
this(context, dbName, password) {
when (val compatibility = isCompatible(schemaVersion)) {
is SchemaCompatibility.NeedsSetup -> throw SchemaNeededError()
is SchemaCompatibility.NeedsMigration ->
throw MigrationNeededError(compatibility.fromVersion)
}
}

constructor(context: Context, dbName: String, schema: Schema) : this(context, dbName) {
constructor(context: Context, dbName: String, password: String, schema: Schema) : this(context, dbName, password) {
unsafeResetDatabase(schema)
}

constructor(context: Context, dbName: String, migrations: MigrationSet) :
this(context, dbName) {
constructor(context: Context, dbName: String, password: String, migrations: MigrationSet) :
this(context, dbName, password) {
migrate(migrations)
}

private val database: Database = Database(dbName, context)
private val database: Database = Database(dbName, password, context)

private val log: Logger? = if (BuildConfig.DEBUG) Logger.getLogger("DB_Driver") else null

Expand Down
12 changes: 11 additions & 1 deletion native/ios/WatermelonDB/Database.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ class Database {

private let fmdb: FMDatabase
private let path: String
private let password: String

init(path: String) {
init(path: String, password: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't password be optional?

self.path = path
self.password = password
fmdb = FMDatabase(path: path)
open()
}
Expand All @@ -32,6 +34,7 @@ class Database {
}

func inTransaction(_ executeBlock: () throws -> Void) throws {
fmdb.setKey(self.password)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me why it was necessary to call setKey every time and I worry about performance with this. I think we want to figure this out before merging. Originally I tried calling setKey only in the open method but it did not encrypt the database - the method gets called, but it is effectively ignored by SQLite.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, agreed, should be figured out before merge, I don't like this either

guard fmdb.beginTransaction() else { throw fmdb.lastError() }

do {
Expand All @@ -44,17 +47,20 @@ class Database {
}

func execute(_ query: SQL, _ args: QueryArgs = []) throws {
fmdb.setKey(self.password)
try fmdb.executeUpdate(query, values: args)
}

/// Executes multiple queries separated by `;`
func executeStatements(_ queries: SQL) throws {
fmdb.setKey(self.password)
guard fmdb.executeStatements(queries) else {
throw fmdb.lastError()
}
}

func queryRaw(_ query: SQL, _ args: QueryArgs = []) throws -> AnyIterator<FMResultSet> {
fmdb.setKey(self.password)
let resultSet = try fmdb.executeQuery(query, values: args)

return AnyIterator {
Expand All @@ -69,6 +75,7 @@ class Database {

/// Use `select count(*) as count`
func count(_ query: SQL, _ args: QueryArgs = []) throws -> Int {
fmdb.setKey(self.password)
let result = try fmdb.executeQuery(query, values: args)
defer { result.close() }

Expand All @@ -86,18 +93,21 @@ class Database {
var userVersion: Int {
get {
// swiftlint:disable:next force_try
fmdb.setKey(self.password)
let result = try! fmdb.executeQuery("pragma user_version", values: [])
result.next()
defer { result.close() }
return result.long(forColumnIndex: 0)
}
set {
fmdb.setKey(self.password)
// swiftlint:disable:next force_try
try! execute("pragma user_version = \(newValue)")
}
}

func unsafeDestroyEverything() throws {
fmdb.setKey(self.password)
// NOTE: Deleting files by default because it seems simpler, more reliable
// But sadly this won't work for in-memory (shared) databases
if isInMemoryDatabase {
Expand Down
3 changes: 3 additions & 0 deletions native/ios/WatermelonDB/DatabaseBridge.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,20 @@ @interface RCT_EXTERN_REMAP_MODULE(DatabaseBridge, DatabaseBridge, NSObject)

WMELON_BRIDGE_METHOD(initialize,
databaseName:(nonnull NSString *)name
password:(nonnull NSString *)password
schemaVersion:(nonnull NSNumber *)version
)

WMELON_BRIDGE_METHOD(setUpWithSchema,
databaseName:(nonnull NSString *)name
password:(nonnull NSString *)password
schema:(nonnull NSString *)schema
schemaVersion:(nonnull NSNumber *)version
)

WMELON_BRIDGE_METHOD(setUpWithMigrations,
databaseName:(nonnull NSString *)name
password:(nonnull NSString *)password
migrations:(nonnull NSString *)migrationSQL
fromVersion:(nonnull NSNumber *)version
toVersion:(nonnull NSNumber *)version
Expand Down
27 changes: 18 additions & 9 deletions native/ios/WatermelonDB/DatabaseBridge.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,16 @@ final public class DatabaseBridge: NSObject {
// MARK: - Asynchronous connections

extension DatabaseBridge {
@objc(initialize:databaseName:schemaVersion:resolve:reject:)
@objc(initialize:databaseName:password:schemaVersion:resolve:reject:)
func initialize(tag: ConnectionTag,
databaseName: String,
password: String,
schemaVersion: NSNumber,
resolve: RCTPromiseResolveBlock,
reject: RCTPromiseRejectBlock) {
do {
try assertNoConnection(tag)
let driver = try DatabaseDriver(dbName: databaseName, schemaVersion: schemaVersion.intValue)
let driver = try DatabaseDriver(dbName: databaseName, password: password, schemaVersion: schemaVersion.intValue)
connections[tag.intValue] = .connected(driver: driver, synchronous: false)
resolve(["code": "ok"])
} catch _ as DatabaseDriver.SchemaNeededError {
Expand All @@ -48,22 +49,24 @@ extension DatabaseBridge {
}
}

@objc(setUpWithSchema:databaseName:schema:schemaVersion:resolve:reject:)
@objc(setUpWithSchema:databaseName:password:schema:schemaVersion:resolve:reject:)
func setUpWithSchema(tag: ConnectionTag,
databaseName: String,
password: String,
schema: Database.SQL,
schemaVersion: NSNumber,
resolve: RCTPromiseResolveBlock,
reject: RCTPromiseRejectBlock) {
let driver = DatabaseDriver(dbName: databaseName,
let driver = DatabaseDriver(dbName: databaseName, password: password,
setUpWithSchema: (version: schemaVersion.intValue, sql: schema))
connectDriverAsync(connectionTag: tag, driver: driver)
resolve(true)
}

@objc(setUpWithMigrations:databaseName:migrations:fromVersion:toVersion:resolve:reject:)
@objc(setUpWithMigrations:databaseName:password:migrations:fromVersion:toVersion:resolve:reject:)
func setUpWithMigrations(tag: ConnectionTag, // swiftlint:disable:this function_parameter_count
databaseName: String,
password: String,
migrations: Database.SQL,
fromVersion: NSNumber,
toVersion: NSNumber,
Expand All @@ -72,6 +75,7 @@ extension DatabaseBridge {
do {
let driver = try DatabaseDriver(
dbName: databaseName,
password: password,
setUpWithMigrations: (from: fromVersion.intValue, to: toVersion.intValue, sql: migrations)
)
connectDriverAsync(connectionTag: tag, driver: driver)
Expand All @@ -94,14 +98,15 @@ extension DatabaseBridge {
}
}

@objc(initializeSynchronous:databaseName:schemaVersion:)
@objc(initializeSynchronous:databaseName:password:schemaVersion:)
func initializeSynchronous(tag: ConnectionTag,
databaseName: String,
password: String,
schemaVersion: NSNumber) -> NSDictionary {
return synchronously {
do {
try assertNoConnection(tag)
let driver = try DatabaseDriver(dbName: databaseName, schemaVersion: schemaVersion.intValue)
let driver = try DatabaseDriver(dbName: databaseName, password: password, schemaVersion: schemaVersion.intValue)
connections[tag.intValue] = .connected(driver: driver, synchronous: true)
return ["code": "ok"]
} catch _ as DatabaseDriver.SchemaNeededError {
Expand All @@ -115,30 +120,34 @@ extension DatabaseBridge {
}
}

@objc(setUpWithSchemaSynchronous:databaseName:schema:schemaVersion:)
@objc(setUpWithSchemaSynchronous:databaseName:password:schema:schemaVersion:)
func setUpWithSchemaSynchronous(tag: ConnectionTag,
databaseName: String,
password: String,
schema: Database.SQL,
schemaVersion: NSNumber) -> NSDictionary {
return synchronously {
try assertNoConnection(tag)
let driver = DatabaseDriver(dbName: databaseName,
password: password,
setUpWithSchema: (version: schemaVersion.intValue, sql: schema))
connections[tag.intValue] = .connected(driver: driver, synchronous: true)
return true
}
}

@objc(setUpWithMigrationsSynchronous:databaseName:migrations:fromVersion:toVersion:)
@objc(setUpWithMigrationsSynchronous:databaseName:password:migrations:fromVersion:toVersion:)
func setUpWithMigrationsSynchronous(tag: ConnectionTag,
databaseName: String,
password: String,
migrations: Database.SQL,
fromVersion: NSNumber,
toVersion: NSNumber) -> NSDictionary {
return synchronously {
try assertNoConnection(tag)
let driver = try DatabaseDriver(
dbName: databaseName,
password: password,
setUpWithMigrations: (from: fromVersion.intValue, to: toVersion.intValue, sql: migrations)
)
connections[tag.intValue] = .connected(driver: driver, synchronous: true)
Expand Down
Loading