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

Enable database encryption for new logins on Nightly/PR builds. #2328

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

pixlwave
Copy link
Member

This PR essentially closes #441, although we're going to test on Nightly first to measure any potential performance impact. The following approach has been taken:

  • The RestorationToken now contains an optional passphrase string which is given to the SDK when restoring a session. Existing users won't have one set, so their accounts will continue to operate without encryption on the db.
  • When running a dev build (inc Nightly and PRs) the AuthenticationServiceProxy will generate a passphrase in the init (which we do for each login so a fresh key will be generated for each login). This passphrase is handed to the SDK which will create the stores, and when a sucessfull login is made, it is handed to the session store to add to the RestorationToken
  • OIDC sessions update the RestorationToken each time the restoration token is refreshed. We now need to fetch the token to preserve the passphrase and if it is missing the app will crash (which will likely result in the user being signed out on next launch but if this happened there wouldn't be much we could do app-side to fix it anyway).
  • I've tweaked the location where the pusherNotificationClientIdentifier is generated, as getting it from the ClientProxy through the restoration token is no longer possible, due to the proxy not knowing what the passphrase is.

Copy link

github-actions bot commented Jan 12, 2024

Warnings
⚠️ Some of the commits are missing ticket numbers. Please consider squashing all commits that don't have a tracking number.
⚠️

ElementX/Sources/Services/UserSession/UserSessionStore.swift#L126 - FIXMEs should be resolved (Quick and dirty fix for stoppi...). (todo)

Generated by 🚫 Danger Swift against 9e97be8

- Slightly reworks how the pusher client ID is generated.
@pixlwave pixlwave force-pushed the doug/441-encrypt-database branch from 178bc57 to c498260 Compare January 12, 2024 13:32
@pixlwave pixlwave marked this pull request as ready for review January 12, 2024 13:33
@pixlwave pixlwave requested a review from a team as a code owner January 12, 2024 13:33
@pixlwave pixlwave requested review from Velin92 and removed request for a team January 12, 2024 13:33
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: 43 lines in your changes are missing coverage. Please review.

Comparison is base (1001b5d) 72.53% compared to head (9e97be8) 72.44%.
Report is 1 commits behind head on develop.

Files Patch % Lines
...ources/Services/UserSession/UserSessionStore.swift 0.00% 23 Missing ⚠️
...Sources/Services/Keychain/KeychainController.swift 0.00% 8 Missing ⚠️
ElementX/Sources/Application/AppCoordinator.swift 0.00% 6 Missing ⚠️
...rces/Services/Keychain/EncryptionKeyProvider.swift 0.00% 5 Missing ⚠️
...eens/RoomScreen/RoomScreenInteractionHandler.swift 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2328      +/-   ##
===========================================
- Coverage    72.53%   72.44%   -0.10%     
===========================================
  Files          512      513       +1     
  Lines        35327    35334       +7     
  Branches     16998    16988      -10     
===========================================
- Hits         25626    25599      -27     
- Misses        9077     9113      +36     
+ Partials       624      622       -2     
Flag Coverage Δ
unittests 57.82% <6.52%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Velin92 Velin92 left a comment

Choose a reason for hiding this comment

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

I don't understand a lot of the new passphrase system works but... if we restore the encrypted db from the NSE... how does the NSE decrypt such db? I think we should decrypt it also in the NSE

Copy link
Member

@Velin92 Velin92 left a comment

Choose a reason for hiding this comment

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

NOICE

@pixlwave
Copy link
Member Author

I don't understand a lot of the new passphrase system works but... if we restore the encrypted db from the NSE... how does the NSE decrypt such db? I think we should decrypt it also in the NSE

Thank you 🙌 - I totally forgot about the NSE 🤦‍♂️

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@pixlwave pixlwave merged commit 01f4254 into develop Jan 12, 2024
9 checks passed
@pixlwave pixlwave deleted the doug/441-encrypt-database branch January 12, 2024 16:46
@manuroe manuroe added the Trigger-PR-Build Uses to trigger alpha builds for PRs label Jan 12, 2024
Copy link

📱 Scan the QR code below to install the build for this PR.
🔒 This build is for internal testing purpose. Only devices listed in the ad-hoc provisioning profile can install Element Alpha.

QR code

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/26waWp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Trigger-PR-Build Uses to trigger alpha builds for PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encrypt local databases
3 participants