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.

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

Conversation

cjroth
Copy link

@cjroth cjroth commented Dec 23, 2020

This builds on @afiller's PR #597 and also adds optional encryption for iOS. My understanding is that disk encryption is not totally necessary for iOS but offers an additional layer of security.

Notes:

  • This uses FMDB from CocoaPods instead of hardwiring it.
    • EDIT: Looks like the desire is to include support for non CocoaPods users? I think the options are to either update the hardwired FMDB source to the one that supports SQLCipher or to just put in the docs that SQLCipher will only work for CocoaPods users. I'm happy to break this into a separate PR for the sake of keeping things organized if it makes sense to switch to Cocoapods. It seems like most RN libraries are 100% Cocoapods now, but maybe I'm getting ahead of the community? If desired, I am happy to update this PR / open a new one where I undo the removal of the hardwired FMDB.
  • This doesn't add password support for web. I don't see anything about encryption in LokiJS's docs for their various storage adapters. If it's used only for in-memory storage then it shouldn't really matter.
  • better-sqlite3 (what WatermelonDB uses for Node) does work with SQLCipher if you compile it with it enabled - it sounds like it shouldn't be super hard to get it working for that too, which may be good for the tests. However, I'm not sure if it's worth the effort, honestly, given that it seems like the Node adapter is mainly used for testing. I could be wrong.
  • We probably would want to add some tests for this - I need to think about what said tests might look like.

@cjroth
Copy link
Author

cjroth commented Dec 23, 2020

@radex I assume this will require some discussion and additional work but wanted to put it on your radar, so I went ahead and opened the PR. I'm not sure if this is a feature that you're even interested in merging. I assume we'd want to figure out how to only include the sqlcipher version of sqlite if the user opts in - not sure how to set that up in Kotlin atm. Also assuming this will require further documentation changes.

@@ -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.

@@ -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.

@@ -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

@@ -53,6 +53,7 @@ declare module '@nozbe/watermelondb/Model' {

public observe(): Observable<this>

// @ts-ignore
Copy link
Author

Choose a reason for hiding this comment

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

This was necessary to get TypeScript not to complain. Could add more descriptive comment if needed.

@@ -109,6 +109,9 @@ export default class LokiJSAdapter implements DatabaseAdapter {
this._dbName = dbName

if (process.env.NODE_ENV !== 'production') {
invariant('password' in options,
Copy link
Author

Choose a reason for hiding this comment

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

It would be bad if someone didn't realize that encryption wasn't supported, so best to safeguard.

@radex
Copy link
Collaborator

radex commented Jan 7, 2021

'm not sure if this is a feature that you're even interested in merging

it is! I'm not promising when I'll take a closer look at this, but I would very much like it as a built-in feature

@cjroth
Copy link
Author

cjroth commented Jan 7, 2021

@radex sounds good and let me know what I can do to help. I will try to work with you to get this done.

@klandell
Copy link

klandell commented Mar 11, 2021

Hello! Just wondering if there are any updates on this since the last comment in January. I'm thinking about migrating from Realm to Watermelon, but the current lack of encryption is a deal breaker.

Copy link
Collaborator

@radex radex left a comment

Choose a reason for hiding this comment

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

Excellent work! Thank you! I have somewhat mixed feelings about the approach of passing password through JavaScript into the native thread. I'm not saying it's necessarily a bad thing… What I'd prefer in my own apps is to keep encryption info purely in native code - to treat JavaScript as insecure, and shield encryption keys from it. Having said that, this would only be a small extra level of protection, considering JS has access to the data anyway, so rogue JS could still read and transmit all of it, and many/most users would not be able to figure out the native app code required to set this up. So I think it's acceptable.

Have you checked if native/iosTest works correctly? It didn't used to use WatermelonDB via CocoaPods but via old-school linking, so I'm not sure if we can make the breaking change of removing non-CP support without fixing this first

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

Comment on lines -82 to +95
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())
}
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?

@@ -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
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?

@@ -32,6 +34,7 @@ class Database {
}

func inTransaction(_ executeBlock: () throws -> Void) throws {
fmdb.setKey(self.password)
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


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?

@@ -27,7 +27,7 @@ let testSchema = """
"""

func newDatabase() -> DatabaseDriver {
return DatabaseDriver(dbName: ":memory:", setUpWithSchema: (version: 1, sql: testSchema))
return DatabaseDriver(dbName: ":memory:", password: "password", setUpWithSchema: (version: 1, sql: testSchema))
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to pass password here? Seems irrelevant to what we want to do in this test

Comment on lines -27 to -28
useWebWorker?: boolean
useIncrementalIndexedDB?: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

hm?

@@ -109,6 +109,9 @@ export default class LokiJSAdapter implements DatabaseAdapter {
this._dbName = dbName

if (process.env.NODE_ENV !== 'production') {
invariant(!('password' in options),
'LokiJSAdapter `password` option not supported. Encryption is only supported on mobile.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'LokiJSAdapter `password` option not supported. Encryption is only supported on mobile.',
'LokiJSAdapter `password` option not supported. Encryption is only supported in SQLiteAdapter.',

this.schema = schema
this.migrations = migrations
this._dbName = this._getName(dbName)

// Password is not supported in web/Node but we pass it through in order to keep the API
// consistent and we throw an error if a password has been provided in order to prevent mistakes.
this._password = password || ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we defaulting to '' instead of having password be a ?string ?

if (process.env.NODE_ENV !== 'production') {
invariant(
!password,
'SQLiteAdapter `password` option not supported. Encryption is only supported on mobile`',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'SQLiteAdapter `password` option not supported. Encryption is only supported on mobile`',
'`password` option not currently supported in Node implementation of SQLiteAdapter`',

@cjroth
Copy link
Author

cjroth commented Mar 24, 2021

Excellent work! Thank you! I have somewhat mixed feelings about the approach of passing password through JavaScript into the native thread. I'm not saying it's necessarily a bad thing… What I'd prefer in my own apps is to keep encryption info purely in native code - to treat JavaScript as insecure, and shield encryption keys from it. Having said that, this would only be a small extra level of protection, considering JS has access to the data anyway, so rogue JS could still read and transmit all of it, and many/most users would not be able to figure out the native app code required to set this up. So I think it's acceptable.

How do you get the password from the user to the native thread? Currently we still use a React Native TextInput for this and then pass it over to RNSensitiveInfo, so it does at one moment go through JS. I'm not sure what the alternative would be here - maybe if you could configure the specific encrypted keychain location to read the password from, which would then prompt the user to do a FaceID/Fingerprint/etc? This would require the user to have a keychain library installed and would be a bit of work to have the two native modules communicate. I think this JS concept is ok for now too for the reasons that you mentioned but I'm open to brainstorming more ideas on this.

Have you checked if native/iosTest works correctly? It didn't used to use WatermelonDB via CocoaPods but via old-school linking, so I'm not sure if we can make the breaking change of removing non-CP support without fixing this first

I ran the tests which are passing - does that run when you run yarn test or is there another step for that?

Thank you for the code review :) I'll review your specific comments when I get some time.

@radex
Copy link
Collaborator

radex commented Mar 24, 2021

Have you checked if native/iosTest works correctly? It didn't used to use WatermelonDB via CocoaPods but via old-school linking, so I'm not sure if we can make the breaking change of removing non-CP support without fixing this first

Yes, just today. It's gotten so out of date that I could no longer even build it correctly on an M1 Mac... I forgot that modern RN doesn't even support non-CP installs, so I think we're fine with just CP support. I don't want to specifically test for RN versions older than 0.62.

How do you get the password from the user to the native thread?

My assumption was that the password isn't going to be directly input by the user, but instead a system Keychain API would be used - either the biometrics+system password fallback combo or just random password generated and stored in the Keychain (I'm not super familiar with Android security model, but on iOS, keychain is far, far more secure than disk storage, since the former is managed by the SEP, and the second is only protected by sandboxing).

I think what I'd do is have an API exposed so that the app developer can hook up DatabaseDriver.passwordProvider = { dbName in .... add your custom password request implementation here ... }

Anyway. I don't think you have to worry about it. It's a fairly easy addition to make and shouldn't block this PR IMO.

@klandell
Copy link

For what it's worth to the above conversation:

Currently on Android I store the encryption key for Realm in Android's EncryptedSharedPreferences which is itself encrypted by a MasterKey managed by Android's keystore.

Realm's key is randomly generated on the Native Side on the first app open, then passed to the JS side to configure Realm, then passed back to be stored. Each time the app starts up, the value needs to be read from Native and sent to JS, where it's sent back to native I presume with all of the other Realm configs.

IF this could all be handled in native code in Android without all of the back and forth it would be really cool.

@radex
Copy link
Collaborator

radex commented Mar 25, 2021

@cjroth iosTest changes are merged to master, so when you merge in master here, CP-only is going to be fine.

PS: If you can configure the PR so that I can modify your branch, that will help me with review

@afiller
Copy link

afiller commented Mar 25, 2021

@cjroth and @radex thank you so much for taking on this task and your intensive discussions. My suggestion (PR) was just a proprietary solution because we needed it urgently in a project. 🙏 👍

@cjroth
Copy link
Author

cjroth commented Mar 26, 2021

PS: If you can configure the PR so that I can modify your branch, that will help me with review

@radex what do you mean exactly? Happy to do whatever helps you!

Sorry still haven't had much time to review all comments in detail, hoping to find some time soon. I also don't know Kotlin so it would be amazing if someone could help me out on that front.

@radex
Copy link
Collaborator

radex commented Mar 26, 2021

Sorry still haven't had much time

No rush!

@radex what do you mean exactly? Happy to do whatever helps you!

This checkbox: https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@cjroth
Copy link
Author

cjroth commented Mar 27, 2021

@radex
Copy link
Collaborator

radex commented May 7, 2021

@cjroth hey, are you planning to finish this in the near future? Conflicts are creating themselves :(

@cjroth
Copy link
Author

cjroth commented May 21, 2021

@radex hey I'm so sorry I have not had any time to work on this and probably won't any time soon :( I will see if one of my team members has any time to help though!

@dzcpy
Copy link

dzcpy commented Jul 30, 2021

Any updates?

@waqas19921
Copy link

@radex @cjroth Anyone who will resolve the conflicts and give it a final touch?

@dzcpy
Copy link

dzcpy commented Jan 26, 2022

Anyone's still interested in working on it? Maybe we can try fixing it

@radex
Copy link
Collaborator

radex commented Jan 26, 2022

@dzcpy There were some issues left - look at the comments; happy to review a PR with these issues fixed

@UnknownH
Copy link

Does anyone know what are the chances of this ever being implemented? Would it be possible to go back to #597 as an alternative (since iOS apparently didn't need this encryption to begin with), so that safe storage of data can also be unlocked/available on Android? We've been keen to implement a React Native project with WatermelonDB for the longest time but this is always stopping us from adoption. Thanks and regards.

@radex
Copy link
Collaborator

radex commented Jan 31, 2022

@UnknownH This is an open source project. If you need this PR, then use a fork of WatermelonDB and use it. Or contribute, help bring it up to the standard required to get it merged.

@ororsatti
Copy link

I have a PR open to do that for JSI, @radex if you can take a look that'd be awesome

@avinashlingaloo
Copy link

Is this PR still relevant ? I would need this feature. @radex

@Lemon-King
Copy link

Is this PR still relevant ? I would need this feature. @radex

I'm also interested in knowing if this is still being worked on or if there is an equivalent feature already in?

@ororsatti
Copy link

Is this PR still relevant ? I would need this feature. @radex

I'm also interested in knowing if this is still being worked on or if there is an equivalent feature already in?

@avinashlingaloo @Lemon-King

Is this PR still relevant ? I would need this feature. @radex

I'm also interested in knowing if this is still being worked on or if there is an equivalent feature already in?

I have a pr open on that, for the new arch, take a look: #1635
perhaps youll be able to use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.