SQLCipher disk encryption for Android + iOS

[cjroth]

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]

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

[radex]

'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]

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

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.

[cjroth]

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]

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]

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]

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

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

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]

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]

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

Not sure why but I don't see this option:

[radex]

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

[cjroth]

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

[yf-hk]

Any updates?

[waqas19921]

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

[yf-hk]

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

[radex]

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

[UnknownH]

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]

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

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