Encrypt FHIR engine database via SQLCipher #787

stevenckngaa · 2021-09-15T22:27:27Z

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #392

Description
Here are the changes:

Introduce a new dependency: SQLCipher for database encryption
Generate a private key in Android keystore for signing messages using HMAC SHA256.
Sign an empty string with the private key in 2. Then use the signed message as the SQLCipher security key.

Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Database encryption: SQLCipher is the one sqlite3 encryption library that is available.
Key encryption:

Implement an encryption wrapper around Datastore
Pros:
- Avoid shared preferences issue, such as blocking disk I/O on UI thread, lack of type safety, inefficient thread usage / disk usage, no guarantee of data written after apply() etc
Cons:
- more work
- we don't benefit from the goodies of data store that much because we are only storing one key.
EncryptedSharedPreferences
Pros:
- less work. We don't need to maintain the encryption code.
Cons:
- See Datastore pros above
- Only support Android M or above
Use HMAC SHA256 signing algorithm to sign an empty string. Then use the signed byte array as database key (chosen)
Pros:
- Easy to implement
- No need to generate key in the application layer. Avoid a potential attack vector.
- No need to store the encrypted database key to the disk. No need to handle I/O error if the encrypted database key isn't available.
Cons:
- Since this solution is backed by Android keystore. To avoid known keystore security issues, we only support Android M or above.

Type
Feature

Screenshots (if applicable)
N/A

Checklist

I have read and acknowledged the Code of conduct
I have read How to Contribute
I have read the Developer's guide
I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA
I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach
I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project
I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally
I have built and run the reference app(s) to verify my change fixes the issue and/or does not break the reference app(s)

Also store the key in EncryptedSharedPreference, which is encrypted by an application main key from the Android key store.

Also 1. Keep minSdk to 21 2. Add interface for key storage operations 3. Implement a KeyStore backed by EncryptedSharedPreferences 4. Generate key with vary length ranging 16 to 31 bytes 5. Add unit tests

engine/src/main/java/com/google/android/fhir/FhirServices.kt

codecov-commenter · 2021-09-20T23:27:40Z

Codecov Report

Merging #787 (650ac9a) into master (6e722c6) will decrease coverage by 0.38%.
The diff coverage is 68.00%.

@@             Coverage Diff              @@
##             master     #787      +/-   ##
============================================
- Coverage     88.68%   88.30%   -0.39%     
- Complexity      469      489      +20     
============================================
  Files           107      110       +3     
  Lines          9147     9309     +162     
  Branches        614      635      +21     
============================================
+ Hits           8112     8220     +108     
- Misses          704      751      +47     
- Partials        331      338       +7

Impacted Files	Coverage Δ
...gle/android/fhir/db/DatabaseEncryptionException.kt	`0.00% <0.00%> (ø)`
.../main/java/com/google/android/fhir/FhirServices.kt	`75.00% <57.14%> (-25.00%)`	⬇️
...java/com/google/android/fhir/FhirEngineProvider.kt	`63.63% <65.00%> (-11.37%)`	⬇️
...gle/android/fhir/db/impl/SQLCipherSupportHelper.kt	`71.42% <71.42%> (ø)`
...roid/fhir/db/impl/DatabaseEncryptionKeyProvider.kt	`72.72% <72.72%> (ø)`
...va/com/google/android/fhir/db/impl/DatabaseImpl.kt	`88.88% <94.44%> (+5.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e722c6...650ac9a. Read the comment docs.

engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt

1. Load database key in RoomDatabase background thread 2. Add more tests

jmarkoff

Mostly needs more Keystore specific error handling, otherwise LGTM.

engine/src/main/java/com/google/android/fhir/security/StorageKeyProvider.kt

engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt

Also 1. Rename StorageKeyProvider to DatabaseEncryptionKeyProvider and make it private to database/impl package so that it is only used in FHIR database 2. better handle database corruption at open

reference/src/main/java/com/google/android/fhir/reference/FhirApplication.kt

engine/src/androidTest/java/com/google/android/fhir/db/impl/EncryptedDatabaseErrorTest.kt

stevenckngaa · 2021-10-18T21:45:52Z

engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt

+ val databaseErrorStrategy: DatabaseErrorStrategy
+)
+
+suspend fun <R> RoomDatabase.withWrappedTransaction(


@yigit @jingtang10 In the case of keystore busy error, we would like to get the key from the keystore again. Is there any concern of adding this wrapper around RoomDatabase.withTransaction to fetch the key again with an exponential backoff?

Also, I am not sure how to test key store error because there is no robolectric shadow for keystore. I could write a fake DatabaseEncryptionKeyProvider and then use it in JUnit test. Any suggestion?

You can test with an emulator as well, but I'm not sure of a good way to test the keystore without an emulator or real device unfortunately. You would need to build out some stubs. For Jetpack Security, we just require that you test on a device (real or virtual).

this is going to be easy to get wrong. as in you are not really convering all of the access to room database.

Any reason why we cannot do this when reading the key in open helper callback instead of here? I mean, do the retry in the open helper callback (and literally block by blocking the thread so that the rest of the database access do not get complicated?)

Good point @yigit. The only reason I put the retry here is to utilize the suspend function. Given that we are okay to block whatever thread the getWriteableDatabase, I have moved the retry to SQLCipherSupportHelper.

@yigit do you think I should write a fake for DatabaseEncryptionKeyProvider in order to stimulate the key store error?

buildSrc/src/main/kotlin/Dependencies.kt

engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt

engine/src/main/java/com/google/android/fhir/FhirServices.kt

engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt

jingtang10 · 2021-11-04T13:09:50Z

engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt

+ FhirServices.builder(context.applicationContext)
+ .apply {
+ if (fhirEngineConfiguration.enableEncryption) enableEncryption()
+ setDatabaseErrorStrategy(fhirEngineConfiguration.databaseErrorStrategy)
+ }


the more i look at this block the more i feel FhirServices should just be inlined in this class.. not sure how you feel about that? no need to do that in this PR but i think this can be a follow-up.

@yigit

not sure what you mean by inlined?

engine/src/main/java/com/google/android/fhir/db/DatabaseEncryptionException.kt

reference/src/main/java/com/google/android/fhir/reference/FhirApplication.kt

engine/src/main/java/com/google/android/fhir/db/impl/SQLCipherSupportHelper.kt

engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt

engine/src/androidTest/java/com/google/android/fhir/db/impl/DatabaseImplTest.kt

yigit · 2021-11-04T23:51:43Z

engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt

+ FhirServices.builder(context.applicationContext)
+ .apply {
+ if (fhirEngineConfiguration.enableEncryption) enableEncryption()
+ setDatabaseErrorStrategy(fhirEngineConfiguration.databaseErrorStrategy)
+ }


not sure what you mean by inlined?

engine/src/main/java/com/google/android/fhir/FhirServices.kt

engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt

yigit · 2021-11-05T00:06:24Z

engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt

+ val databaseErrorStrategy: DatabaseErrorStrategy
+)
+
+suspend fun <R> RoomDatabase.withWrappedTransaction(


this is going to be easy to get wrong. as in you are not really convering all of the access to room database.

Any reason why we cannot do this when reading the key in open helper callback instead of here? I mean, do the retry in the open helper callback (and literally block by blocking the thread so that the rest of the database access do not get complicated?)

engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt

engine/src/main/java/com/google/android/fhir/db/impl/SQLCipherSupportHelper.kt

reference/src/main/java/com/google/android/fhir/reference/FhirApplication.kt

jingtang10

I ❤️ this. This is #lit.

Thanks @stevenckngaa for this exemplary work!

Left some comments but mostly cosmetic.

engine/src/main/java/com/google/android/fhir/db/impl/DatabaseEncryptionKeyProvider.kt

engine/src/main/java/com/google/android/fhir/db/DatabaseEncryptionException.kt

engine/build.gradle.kts

buildSrc/src/main/kotlin/Dependencies.kt

engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt

engine/src/main/java/com/google/android/fhir/db/impl/SQLCipherSupportHelper.kt

engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt

buildSrc/src/main/kotlin/Dependencies.kt

stevenckngaa added 3 commits September 15, 2021 21:34

Import SQLCipher dependencies

e3f45e4

Generate a 16 bytes key for database encryption

9e43cb9

Also store the key in EncryptedSharedPreference, which is encrypted by an application main key from the Android key store.

Merge branch 'master' into ckng/db_encryption

b0d3388

stevenckngaa requested review from aditya-07, deepankarb, jingtang10, kevinmost, pankajkatoch and yigit as code owners September 15, 2021 22:27

stevenckngaa changed the title ~~[WIP] Introduce a SQLCipher encryption to FHIR engine database~~ [WIP] Encrypt FHIR engine database via SQLCipher Sep 15, 2021

Apply spotless

75a5006

stevenckngaa marked this pull request as draft September 15, 2021 22:32

Tarun-Bhardwaj added the Q3 2021 label Sep 20, 2021

Tarun-Bhardwaj added this to In progress in FHIR engine library via automation Sep 20, 2021

Allow database encryption on Android 6.0 or above

5b03a8a

Also 1. Keep minSdk to 21 2. Add interface for key storage operations 3. Implement a KeyStore backed by EncryptedSharedPreferences 4. Generate key with vary length ranging 16 to 31 bytes 5. Add unit tests

stevenckngaa changed the title ~~[WIP] Encrypt FHIR engine database via SQLCipher~~ Encrypt FHIR engine database via SQLCipher Sep 20, 2021

stevenckngaa marked this pull request as ready for review September 20, 2021 23:10

stevenckngaa assigned jingtang10 Sep 20, 2021

stevenckngaa commented Sep 20, 2021

View reviewed changes

engine/src/main/java/com/google/android/fhir/FhirServices.kt Outdated Show resolved Hide resolved

stevenckngaa added 3 commits September 30, 2021 16:00

Merge branch 'master' into ckng/db_encryption

4d1a969

Use a HMAC 256 signed empty string as the database key

b44b9c8

Enable database encryption in the reference app by default

a9c1b94

stevenckngaa commented Oct 7, 2021

View reviewed changes

engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt Show resolved Hide resolved

stevenckngaa added 2 commits October 9, 2021 00:26

Load database key in RoomDatabase background thread

ce43106

1. Load database key in RoomDatabase background thread 2. Add more tests

Merge branch 'master' into ckng/db_encryption

98fa50d

jmarkoff reviewed Oct 12, 2021

View reviewed changes

stevenckngaa added 2 commits October 18, 2021 22:30

Add retry when Android keystore is busy

2746cc3

Also 1. Rename StorageKeyProvider to DatabaseEncryptionKeyProvider and make it private to database/impl package so that it is only used in FHIR database 2. better handle database corruption at open

Reenable some tests

a941caa

stevenckngaa commented Oct 18, 2021

View reviewed changes

reference/src/main/java/com/google/android/fhir/reference/FhirApplication.kt Outdated Show resolved Hide resolved

stevenckngaa commented Oct 18, 2021

View reviewed changes

engine/src/androidTest/java/com/google/android/fhir/db/impl/EncryptedDatabaseErrorTest.kt Outdated Show resolved Hide resolved

stevenckngaa commented Oct 18, 2021

View reviewed changes

stevenckngaa added 4 commits October 27, 2021 19:32

Merge branch 'master' into ckng/db_encryption

d5ab15f

Remove outdated TODO

bf9105c

Use a different database name for unencrypted and encrypted database

7335ec8

Update documentation

956f019

jingtang10 requested changes Nov 4, 2021

View reviewed changes

FHIR engine library automation moved this from In progress to Review in progress Nov 4, 2021

yigit requested changes Nov 5, 2021

View reviewed changes

stevenckngaa added 8 commits November 5, 2021 12:43

Merge branch 'master' into ckng/db_encryption

68e0e52

Move keystore timeout retry to SQLCipherSupportHelper

d7f7dc1

Update kdoc

e4d009b

Update documentation

6137ba6

Merge branch 'master' into ckng/db_encryption

386808a

Apply spotless

d54b91c

Allow SQLCipher license because it's effectively BSD-3

73441e5

Fix SQLCipher license allowed dependency

00b2b9e

Tarun-Bhardwaj added Beta and removed Q3 2021 labels Nov 23, 2021

jingtang10 approved these changes Nov 25, 2021

View reviewed changes

stevenckngaa added 2 commits November 26, 2021 11:59

Merge branch 'master' into ckng/db_encryption

b00b47c

Address Jing's comments

650ac9a

jingtang10 approved these changes Nov 26, 2021

View reviewed changes

yigit approved these changes Nov 29, 2021

View reviewed changes

buildSrc/src/main/kotlin/Dependencies.kt Show resolved Hide resolved

FHIR engine library automation moved this from Review in progress to Reviewer approved Nov 29, 2021

stevenckngaa merged commit 596683b into master Nov 29, 2021

stevenckngaa deleted the ckng/db_encryption branch November 29, 2021 22:08

FHIR engine library automation moved this from Reviewer approved to Done Nov 29, 2021

This was referenced Nov 29, 2021

Investigate if FHIR library can request users to enter screen lock challenge #944

Closed

Add documentation for using FHIR database encryption #945

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encrypt FHIR engine database via SQLCipher #787

Encrypt FHIR engine database via SQLCipher #787

stevenckngaa commented Sep 15, 2021 •

edited

Loading

codecov-commenter commented Sep 20, 2021 •

edited by codecov bot

Loading

jmarkoff left a comment

stevenckngaa Oct 18, 2021

stevenckngaa Oct 29, 2021

jmarkoff Nov 4, 2021

yigit Nov 5, 2021

stevenckngaa Nov 5, 2021

jingtang10 Nov 4, 2021

yigit Nov 4, 2021

yigit Nov 4, 2021

yigit Nov 5, 2021

jingtang10 left a comment

Encrypt FHIR engine database via SQLCipher #787

Encrypt FHIR engine database via SQLCipher #787

Conversation

stevenckngaa commented Sep 15, 2021 • edited Loading

codecov-commenter commented Sep 20, 2021 • edited by codecov bot Loading

Codecov Report

jmarkoff left a 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

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

jingtang10 left a comment

Choose a reason for hiding this comment

stevenckngaa commented Sep 15, 2021 •

edited

Loading

codecov-commenter commented Sep 20, 2021 •

edited by codecov bot

Loading