[expo-sqlite] Updated to allow usage with encrypted SQLite files #8242

gurs1kh · 2020-05-11T05:02:08Z

Why

There is a need to be able to use encrypted SQLite files in Expo as expressed in several places, like: Expo feature requests (Canny), the Expo forums, StackOverflow, etc.
Here's a few examples:
https://expo.canny.io/feature-requests/p/sqlite-encrypted-database
https://forums.expo.io/t/expo-sqlite-encrypted-support/19493
https://forums.expo.io/t/storing-sqlite-file-securely/10377
https://stackoverflow.com/questions/58688148/encrypt-database-in-react-with-expo

How

In order to be able to use encrypted databases, I integrated SQLCipher into the existing expo-sqlite package.

Test Plan

I've tested added tests, but sadly I wasn't able to run them properly on my machine. I think there was some sort of clash with the installed Expo Client. I'd appreciate if someone could confirm that these work as expected.
Along with tests, I managed to get this to work with a personal project and the expo examples/with-sqlite project (though I had to eject to be able to use my updated package).

akaldesign · 2020-05-11T15:20:13Z

Very important to be resolved for the future of our app roadmap.
We cannot migrate our apps to RN.

gurs1kh · 2020-05-12T03:56:51Z

@sjchmiela @mczernek I've gotten the builds succeeding to the best of my abilities. The remaining failures are both due to missing environment variables.
Can one of you take a look at them?

Here's the output on CircleCI:

#!/bin/bash -eo pipefail
yarn danger ci
yarn run v1.22.4
$ /home/circleci/project/docs/node_modules/.bin/danger ci
The DANGER_GITHUB_API_TOKEN/DANGER_BITBUCKETSERVER_HOST/DANGER_GITLAB_API_TOKEN environmental variable is missing
Without an api token, danger will be unable to comment on a PR
Error:  Error: Cannot use authenticated API requests.
    at Object.getPlatformForEnv (/home/circleci/project/docs/node_modules/danger/distribution/platforms/platform.js:76:11)
    at Object.<anonymous> (/home/circleci/project/docs/node_modules/danger/distribution/commands/ci/runner.js:81:57)
    at step (/home/circleci/project/docs/node_modules/danger/distribution/commands/ci/runner.js:32:23)
    at Object.next (/home/circleci/project/docs/node_modules/danger/distribution/commands/ci/runner.js:13:53)
    at fulfilled (/home/circleci/project/docs/node_modules/danger/distribution/commands/ci/runner.js:4:58)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Exited with code exit status 1

sjchmiela

Apart from this couple of comments I'm afraid of:

app size impact
WebSQL compatibility — big feature of expo-sqlite and Expo modules as a whole is cross-platform compatibility — as far as I know it's currently not possible to use encrypted SQLite databases on Web. What do you think about it @ide?

packages/expo-sqlite/android/build.gradle

packages/expo-sqlite/android/src/main/java/expo/modules/sqlite/SQLiteModule.java

CHANGELOG.md

packages/expo-sqlite/ios/EXSQLite/EXSQLite.m

packages/expo-sqlite/ios/EXSQLite.podspec

ide · 2020-05-22T19:27:19Z

Will need to think about the implications some more but I probably won't get around to this soon. Ease of installation and size are both important. Also want to think about the threat model here. Is there a way for an app to say to the OS that all of its data should be encrypted? If so, then encrypting just the SQLite DB is superficial.

…of params

# Why I wanted to clear the way for #8242 to be landed (or not). I noticed that web support doesn't look like it should work and that it isn't tested anywhere. # How - added SQLite test suite to set run on web - changed the way we fetch values in test suite from using `._array[0]` to `.item(0)` - added explanations in `SQLite.types.ts` about why we may need a copy of `@types/websql` - added `Window` declaration to types declarations - removed `@types/websql` dependency as it's not even importable - fixed web counterpart — `SQLite.web.ts` — by fixing the method header and gracefully handling situation where `openDatabase` is not available (I think). # Test Plan Test suite passes.

sjchmiela

Thank you for this contribution. A couple of things still left to find out:

how does this affect app size? If noticeably, we could make it an optional addition
strip the key when run on Web
add tests to test-suite run on all platforms that will let us ensure that both openDatabase(string) & openDatabase({…}) work and that an encrypted database is not readable when an invalid key is provided

packages/expo-sqlite/ios/EXSQLite/EXSQLite.m

packages/expo-sqlite/CHANGELOG.md

packages/expo-sqlite/android/build.gradle

docs/pages/versions/unversioned/sdk/sqlite.md

CHANGELOG.md

sjchmiela · 2020-06-01T17:21:33Z

packages/expo-sqlite/src/SQLite.ts

@@ -81,16 +90,28 @@ function addExecMethod(db: any): WebSQLDatabase {
 }

 export function openDatabase(
-  name: string,
+  fileInfo: SQLiteFileInfo | string,


Could you also add support for this to SQLite.web.ts so it strips key from the fileInfo and uses only name if an object is provided?

I have made the change, but would appreciate if you could double-check SQLite.web.ts to see if I did what you were expecting

gurs1kh · 2020-06-03T00:16:11Z

how does this affect app size? If noticeably, we could make it an optional addition

Do you know of a good way to check this?

add tests to test-suite run on all platforms that will let us ensure that both openDatabase(string) & openDatabase({…}) work and that an encrypted database is not readable when an invalid key is provided

I've already added these tests. Is there anything specific that I missed?

Co-authored-by: Stanisław Chmiela <sjchmiela@users.noreply.github.com>

gurs1kh · 2020-06-23T02:14:19Z

how does this affect app size? If noticeably, we could make it an optional addition

Do you know of a good way to check this?

@sjchmiela Just thought I'd give you a reminder

sjchmiela

When it comes to measuring app size impact:

first, assemble an APK release of bare-expo on master, without your changes (you may need to use some temporary key to sign the app)
then, assemble an APK release of bare-expo on your branch, with the changes
see the difference in APK sizes
in Xcode — archive the project on master, note the size of the xcarchive
archive the project on your branch, see the difference.

Sorry for the delay! 😕

sjchmiela · 2020-06-04T16:28:36Z

packages/expo-sqlite/src/SQLite.web.ts

@@ -1,2 +1,11 @@
-export const { openDatabase } = global;
+export function openDatabase(fileInfo, ...args) {


Ah, so… could you please rebase this PR on top of master? Before getting back to you I've ensured that SQLite works on Web properly and changed this file a little bit — #8518.

sjchmiela · 2020-06-04T16:31:29Z

packages/expo-sqlite/ios/EXSQLite/EXSQLite.m

@@ -58,6 +58,14 @@ - (NSValue *)openDatabase:(NSString *)dbName
    if (sqlite3_open([path UTF8String], &db) != SQLITE_OK) {
      return nil;
    };
+
+    if (!dbKey) {


Suggested change

if (!dbKey) {

if (dbKey) {

Sorry for making that suggestion before — I misunderstood dbKey != NULL and proposed !dbKey changing the if completely. dbKey != NULL means if dbKey is non-empty which should translate to Obj-C-y if (dbKey) (if dbKey is truthy)

sjchmiela · 2020-06-04T16:31:36Z

packages/expo-sqlite/ios/EXSQLite/EXSQLite.m

+
+    if (!dbKey) {
+      const char *key = [dbKey UTF8String];
+      if (!key) {


Suggested change

if (!key) {

if (key) {

gurs1kh · 2020-09-06T23:50:37Z

@sjchmiela I'm running into too many issues when trying to build the app. I think it might be easier if someone who already has expo properly set up could do it. I've put up comment asking someone to help on https://expo.canny.io/feature-requests/p/sqlite-encrypted-database, but I'm not sure how soon we can get someone to do it.

gurs1kh · 2020-12-08T05:35:16Z

@sjchmiela I managed to work through the issues I faced building the apps. Here is what I found:

	Old size	New size	Size increase
Android (APK)	41.6 MB	47.4 MB	13.9%
iOS (Archive)	263.6 MB	281.8 MB	6.9%

I can go through and rebase the PR off of the latest commit in master, but I just wanted to get your thoughts on the app size increase before I do so. Just thought it'd be good to see if it's even worth going through the work of doing that or not.

ide · 2020-12-09T01:22:25Z

A 14% increase is probably too much, especially coming from a proprietary library that we'll end up wanting to remove. I think the path forward is to use the OS's own encryption capabilities where possible.

For example, this might be the appropriate approach on iOS: https://developer.apple.com/documentation/uikit/protecting_the_user_s_privacy/encrypting_your_app_s_files. Android also has some disk encryption too: https://source.android.com/security/encryption/file-based.

The threat model we'd consider is an attacker getting physical access to someone else's device, especially if the device is off. We can't really prevent a user accessing data on their own device and consider that scenario to be out of scope. Fundamentally speaking, if an app has access to data, then the user using that app has access to the data -- to prevent a user from accessing data, it needs not to be sent to the device in the first place.

The disk-level encryption on Android and iOS seem reasonable to adopt. Perhaps we need to identify a set of options that work across platforms (e.g. disallow access until first unlock) that map to Android and iOS's options, and then create a universal API to expose those options and apply them to the SQLite file.

gurs1kh · 2020-12-11T06:30:27Z

Though I would say that there definitely are reasons to encrypt the sqlite DB itself, I do understand how the app size increase is problematic. Anyways, thanks for entertaining the possibility of adding this feature. Guess we'll just have to use a bare workflow.

gurs1kh requested review from mczernek and sjchmiela as code owners May 11, 2020 05:02

gurs1kh force-pushed the master branch from e252797 to be86ed2 Compare May 11, 2020 07:41

gurs1kh force-pushed the master branch from 20c1392 to fb99693 Compare May 12, 2020 02:48

sjchmiela reviewed May 22, 2020

View reviewed changes

gurs1kh force-pushed the master branch from ea1b9d5 to c1fad68 Compare May 27, 2020 03:50

gurs1kh added 12 commits May 26, 2020 20:52

[expo-sqlite] updated js to work with SQLCipher

385fc69

[expo-sqlite][android] integrated SQLCipher

fcae382

[expo-sqlite][ios] integrated SQLCipher

10f374f

[expo-sqlite][tests] updated expo-sqlite test to test multiple types …

95cd030

…of params

[expo-sqlite][docs] updated expo-sqlite docs to explain fileInfo param

3bdc315

[expo-sqlite] updated change logs

530a52e

[expo-sqlite] fixed lint/prettier violations

a136cef

[expo-sqlite] moved contents of SQLCipher subspec to top level

412e10b

[expo-sqlite] updated bare-expo Pods folder

c66473d

[expo-sqlite] simplified parameters in test

f03c8d8

[expo-sqlite] added tests for opening encrypted db

32a5682

[expo-sqlite][ios] addressed code review feedback

50544de

gurs1kh force-pushed the master branch from c1fad68 to 24eb9be Compare May 27, 2020 03:56

[expo-sqlite] fixes to changelog changes

b9b93b5

gurs1kh force-pushed the master branch from 24eb9be to b9b93b5 Compare May 27, 2020 03:57

[expo-sqlite][android] addressed code review feedback

503e599

sjchmiela mentioned this pull request May 27, 2020

[expo-sqlite] Fix Web support #8518

Merged

sjchmiela reviewed Jun 1, 2020

View reviewed changes

Apply suggestions from code review

c964941

Co-authored-by: Stanisław Chmiela <sjchmiela@users.noreply.github.com>

gurs1kh force-pushed the master branch from fcf20c2 to 4ed99f1 Compare June 3, 2020 00:54

[expo-sqlite][web] updated to ignore key on web

457ae2c

gurs1kh force-pushed the master branch from 4ed99f1 to 457ae2c Compare June 3, 2020 00:55

sjchmiela reviewed Jun 26, 2020

View reviewed changes

gurs1kh closed this Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[expo-sqlite] Updated to allow usage with encrypted SQLite files #8242

[expo-sqlite] Updated to allow usage with encrypted SQLite files #8242

gurs1kh commented May 11, 2020

akaldesign commented May 11, 2020

gurs1kh commented May 12, 2020

sjchmiela left a comment

ide commented May 22, 2020

sjchmiela left a comment

sjchmiela Jun 1, 2020

gurs1kh Jun 3, 2020

gurs1kh commented Jun 3, 2020

gurs1kh commented Jun 23, 2020

sjchmiela left a comment

sjchmiela Jun 4, 2020

sjchmiela Jun 4, 2020

sjchmiela Jun 4, 2020

gurs1kh commented Sep 6, 2020

gurs1kh commented Dec 8, 2020 •

edited

ide commented Dec 9, 2020

gurs1kh commented Dec 11, 2020

		@@ -1,2 +1,11 @@
		export const { openDatabase } = global;
		export function openDatabase(fileInfo, ...args) {

[expo-sqlite] Updated to allow usage with encrypted SQLite files #8242

[expo-sqlite] Updated to allow usage with encrypted SQLite files #8242

Conversation

gurs1kh commented May 11, 2020

Why

How

Test Plan

akaldesign commented May 11, 2020

gurs1kh commented May 12, 2020

sjchmiela left a comment

Choose a reason for hiding this comment

ide commented May 22, 2020

sjchmiela left a comment

Choose a reason for hiding this comment

sjchmiela Jun 1, 2020

Choose a reason for hiding this comment

gurs1kh Jun 3, 2020

Choose a reason for hiding this comment

gurs1kh commented Jun 3, 2020

gurs1kh commented Jun 23, 2020

sjchmiela left a comment

Choose a reason for hiding this comment

sjchmiela Jun 4, 2020

Choose a reason for hiding this comment

sjchmiela Jun 4, 2020

Choose a reason for hiding this comment

sjchmiela Jun 4, 2020

Choose a reason for hiding this comment

gurs1kh commented Sep 6, 2020

gurs1kh commented Dec 8, 2020 • edited

ide commented Dec 9, 2020

gurs1kh commented Dec 11, 2020

gurs1kh commented Dec 8, 2020 •

edited