firebase-ios-sdk icon indicating copy to clipboard operation
firebase-ios-sdk copied to clipboard

Make use of SQLITE_OPEN_FILEPROTECTION_COMPLETEUNTILFIRSTUSERAUTHENTICATION conditional.

Open cprince-foreflight opened this issue 11 months ago • 9 comments

https://github.com/firebase/firebase-ios-sdk/issues/10884

Testing

  • I ran Firebase remote config tests locally.

cprince-foreflight avatar Mar 13 '24 21:03 cprince-foreflight

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Mar 13 '24 21:03 google-cla[bot]

Thanks! This is a much simpler solution than completely replacing sqlite. What is the implication of running without the SQLITE_OPEN_FILEPROTECTION_COMPLETEUNTILFIRSTUSERAUTHENTICATION when it's not available?

paulb777 avatar Mar 13 '24 22:03 paulb777

Thanks! This is a much simpler solution than completely replacing sqlite. What is the implication of running without the SQLITE_OPEN_FILEPROTECTION_COMPLETEUNTILFIRSTUSERAUTHENTICATION when it's not available?

Well, good question :). I see basically no docs for this. E.g., I wouldn't call this docs. I'm assuming from the naming that it relates to standard concepts of file protection/encryption with iOS.

I'm about to test this in our app (hence the DRAFT status), and hopefully my colleague aaron-foreflight will add his 2 cents.

cprince-foreflight avatar Mar 15 '24 21:03 cprince-foreflight

@cprince-foreflight, thanks! I think one case worth checking will be the following:

  1. use Remote Config without this change to initialize DB
  2. try to access the same DB again (without deleting the app) with the change This could help rule out the risk of the pre-change DB becoming inaccessible when it is accessed with one less flag after upgrading.

ncooke3 avatar Mar 15 '24 21:03 ncooke3

@cprince-foreflight, thanks! I think one case worth checking will be the following:

  1. use Remote Config without this change to initialize DB
  2. try to access the same DB again (without deleting the app) with the change This could help rule out the risk of the pre-change DB becoming inaccessible when it is accessed with one less flag after upgrading.

Definitely. And I think it's particularly important for step 2), when SQLITE_OPEN_FILEPROTECTION_COMPLETEUNTILFIRSTUSERAUTHENTICATION is not defined. Shall have to stare at this a bit to see if I can test it. Our use isn't exactly simple :).

cprince-foreflight avatar Mar 15 '24 22:03 cprince-foreflight

Thanks! This is a much simpler solution than completely replacing sqlite. What is the implication of running without the SQLITE_OPEN_FILEPROTECTION_COMPLETEUNTILFIRSTUSERAUTHENTICATION when it's not available?

I don't know the answer to this. A brave soul could dig through this aborted patch from Apple and maybe figure it out: https://www2.sqlite.org/src/info/c8ade949d4a2eb3b

aaron-foreflight avatar Mar 21 '24 16:03 aaron-foreflight

@cprince-foreflight, thanks! I think one case worth checking will be the following:

  1. use Remote Config without this change to initialize DB
  2. try to access the same DB again (without deleting the app) with the change This could help rule out the risk of the pre-change DB becoming inaccessible when it is accessed with one less flag after upgrading.

Definitely. And I think it's particularly important for step 2), when SQLITE_OPEN_FILEPROTECTION_COMPLETEUNTILFIRSTUSERAUTHENTICATION is not defined. Shall have to stare at this a bit to see if I can test it. Our use isn't exactly simple :).

Well. Hmmm. My first thought to test this was to put together a demo/test app that creates a RCNConfigDBManager instance to try the tests above. However, it seems like RCNConfigDBManager isn't in scope when using the package imports.

So, I had to hack a bit, but got there. I added in a "testDb" static method to the "RemoteConfig" class which I can access from my app. That method is defined as:

+ (void) testDb {
    RCNConfigDBManager *manager = [[RCNConfigDBManager alloc] init];
}

And I call this from my test app that I run an iOS simulator.

Test 1

To start with, I deleted the app.

This has the original flags code:

 int flags = SQLITE_OPEN_CREATE | SQLITE_OPEN_READWRITE |
              SQLITE_OPEN_FILEPROTECTION_COMPLETEUNTILFIRSTUSERAUTHENTICATION |
              SQLITE_OPEN_FULLMUTEX;

I set various breakpoints after that and when I run my test, I get to: Screenshot 2024-04-02 at 3 09 19 PM

That is here.

After it gets to this breakpoint, I let it continue execution.

Test 2

I then stop the app. I do not remove it. I change to the new flag code:

int flags = SQLITE_OPEN_CREATE | SQLITE_OPEN_READWRITE | SQLITE_OPEN_FULLMUTEX; #ifdef SQLITE_OPEN_FILEPROTECTION_COMPLETEUNTILFIRSTUSERAUTHENTICATION flags |= SQLITE_OPEN_FILEPROTECTION_COMPLETEUNTILFIRSTUSERAUTHENTICATION; #endif

And I re-run/re-launch the app.I get to the same place again. That is here. Screenshot 2024-04-02 at 3 05 28 PM

Conclusion

I think this is a successful (non-failure) result. Happy to upload the full test code, but I've got the sdk inside of it, so it's somewhat large (122MB). Please let me know.

cprince-foreflight avatar Apr 02 '24 21:04 cprince-foreflight

I repro'd those test steps on my iPhone hardware too. Between Test 1 and Test 2 I restarted my device to ensure that I had to sign back in. Same results as on simulator.

cprince-foreflight avatar Apr 04 '24 13:04 cprince-foreflight

I ran another test sequence, on my iPhone. Test 1 was the same with this setup: Test1 Test 2 had: Test2

Between Test 1 and Test 2 I powered down my iPhone and powered it backup to ensure it was locked prior to Test 2. Same (positive) results. On both tests: Result

cprince-foreflight avatar Apr 05 '24 14:04 cprince-foreflight