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.

Sign up for GitHub

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

Jump to bottom

use electron safeStorage #3676

Closed
ganthern opened this issue Nov 16, 2021 · 8 comments · Fixed by #6185 or #6360
Closed

use electron safeStorage #3676

ganthern opened this issue Nov 16, 2021 · 8 comments · Fixed by #6185 or #6360
Labels
desktop Desktop client related issues improvement nice-to-haves that are not impeding usage of any features state:tested We tested it and are about to release it
Milestone
3.121.0

Comments

@ganthern
Copy link
Contributor

ganthern commented Nov 16, 2021 •
edited
Loading

once #3658 lands, we could use the new safeStorage API to encrypt the device key and store it in a file.

This would offload the device key management to electron and make our direct keytar dependency obsolete.
@ganthern ganthern added improvement nice-to-haves that are not impeding usage of any features desktop Desktop client related issues labels Nov 16, 2021
@johnbotris johnbotris mentioned this issue Nov 25, 2021
Closed
7 tasks
@ganthern ganthern self-assigned this Dec 14, 2021
@bedhub bedhub mentioned this issue Dec 14, 2021
Closed
@bedhub bedhub added this to the Next release milestone Dec 14, 2021
ganthern added a commit that referenced this issue Dec 16, 2021
@ganthern
[desktop] use electron safeStorage to store device keys
456fc5b 
* renames PathUtils.swapFilename to replaceLastComponent
* provides a new impl for SecretStorage
* uses electron.safeStorage to encrypt the device key that then gets
stored in a file in app.getPath('userData')/safe_storage/
* from now on, per-user data is encrypted with a per-user key,
even for per-machine installs.

#3733
close #3676
@ganthern ganthern mentioned this issue Dec 16, 2021
Closed
@ganthern
Copy link
Contributor Author

ganthern commented Dec 16, 2021 •
edited
Loading

Testing

  • previously set alarms get rescheduled even when offline after the app is restarted
  • the app, if saved credentials are present, uses the same sseinfo after the update (terminal output doesn't say anything about sseInfo corrupted or not present)

@ganthern
Copy link
Contributor Author

safeStorage passes through a Chromium API and that doesn't get initialized until a BrowserWindow gets created:
electron/electron#32206

ganthern added a commit that referenced this issue Dec 21, 2021
@ganthern
[desktop] use electron safeStorage to store device keys
275fec9 
* renames PathUtils.swapFilename to replaceLastComponent
* provides a new impl for SecretStorage
* uses electron.safeStorage to encrypt the device key that then gets
stored in a file in app.getPath('userData')/safe_storage/
* from now on, per-user data is encrypted with a per-user key,
even for per-machine installs.

#3733
close #3676
ganthern added a commit that referenced this issue Jan 6, 2022
@ganthern
[desktop] use electron safeStorage to store device keys
c853f68 
* renames PathUtils.swapFilename to replaceLastComponent
* provides a new impl for SecretStorage
* uses electron.safeStorage to encrypt the device key that then gets
stored in a file in app.getPath('userData')/safe_storage/
* from now on, per-user data is encrypted with a per-user key,
even for per-machine installs.

#3733
close #3676
@charlag
Copy link
Contributor

charlag commented Jan 19, 2022

We decided to not user Chromium APIs because they are coupled to window pretty tightly and we have no control over it
see #3733

@charlag charlag closed this as completed Jan 19, 2022
@charlag charlag removed this from the Next release milestone Jan 19, 2022
@charlag charlag added the state:wontfix issues that are not significant enough to invest in or that are intended behaviour label Jan 19, 2022
@bedhub bedhub mentioned this issue Feb 17, 2022
Closed
ganthern added a commit that referenced this issue Feb 24, 2022
@ganthern
[desktop] use electron safeStorage to store device keys
2860589 
* renames PathUtils.swapFilename to replaceLastComponent
* provides a new impl for SecretStorage
* uses electron.safeStorage to encrypt the device key that then gets
stored in a file in app.getPath('userData')/safe_storage/
* from now on, per-user data is encrypted with a per-user key,
even for per-machine installs.

#3733
close #3676
ganthern added a commit that referenced this issue Mar 1, 2022
@ganthern
[desktop] use electron safeStorage to store device keys
6342150 
* renames PathUtils.swapFilename to replaceLastComponent
* provides a new impl for SecretStorage
* uses electron.safeStorage to encrypt the device key that then gets
stored in a file in app.getPath('userData')/safe_storage/
* from now on, per-user data is encrypted with a per-user key,
even for per-machine installs.

#3733
close #3676
ganthern added a commit that referenced this issue Mar 3, 2022
@ganthern
[desktop] use electron safeStorage to store device keys
7cfcf05 
* renames PathUtils.swapFilename to replaceLastComponent
* provides a new impl for SecretStorage
* uses electron.safeStorage to encrypt the device key that then gets
stored in a file in app.getPath('userData')/safe_storage/
* from now on, per-user data is encrypted with a per-user key,
even for per-machine installs.

#3733
close #3676
ganthern added a commit that referenced this issue Mar 10, 2022
@ganthern
[desktop] use electron safeStorage to store device keys
f6c8834 
* renames PathUtils.swapFilename to replaceLastComponent
* provides a new impl for SecretStorage
* uses electron.safeStorage to encrypt the device key that then gets
stored in a file in app.getPath('userData')/safe_storage/
* from now on, per-user data is encrypted with a per-user key,
even for per-machine installs.

#3733
close #3676
ganthern added a commit that referenced this issue Mar 15, 2022
@ganthern
[desktop] use electron safeStorage to store device keys
b7f9d76 
* renames PathUtils.swapFilename to replaceLastComponent
* provides a new impl for SecretStorage
* uses electron.safeStorage to encrypt the device key that then gets
stored in a file in app.getPath('userData')/safe_storage/
* from now on, per-user data is encrypted with a per-user key,
even for per-machine installs.

#3733
close #3676
ganthern added a commit that referenced this issue Mar 22, 2022
@ganthern
[desktop] use electron safeStorage to store device keys
6aa6787 
* renames PathUtils.swapFilename to replaceLastComponent
* provides a new impl for SecretStorage
* uses electron.safeStorage to encrypt the device key that then gets
stored in a file in app.getPath('userData')/safe_storage/
* from now on, per-user data is encrypted with a per-user key,
even for per-machine installs.

#3733
close #3676
ganthern added a commit that referenced this issue Mar 24, 2022
@ganthern
[desktop] use electron safeStorage to store device keys
2db3283 
* renames PathUtils.swapFilename to replaceLastComponent
* provides a new impl for SecretStorage
* uses electron.safeStorage to encrypt the device key that then gets
stored in a file in app.getPath('userData')/safe_storage/
* from now on, per-user data is encrypted with a per-user key,
even for per-machine installs.

#3733
close #3676
ganthern added a commit that referenced this issue Apr 26, 2022
@ganthern
[desktop] use electron safeStorage to store device keys
10fa589 
* renames PathUtils.swapFilename to replaceLastComponent
* provides a new impl for SecretStorage
* uses electron.safeStorage to encrypt the device key that then gets
stored in a file in app.getPath('userData')/safe_storage/
* from now on, per-user data is encrypted with a per-user key,
even for per-machine installs.

#3733
close #3676
ganthern added a commit that referenced this issue May 4, 2022
@ganthern
[desktop] use electron safeStorage to store device keys
593c622 
* renames PathUtils.swapFilename to replaceLastComponent
* provides a new impl for SecretStorage
* uses electron.safeStorage to encrypt the device key that then gets
stored in a file in app.getPath('userData')/safe_storage/
* from now on, per-user data is encrypted with a per-user key,
even for per-machine installs.

#3733
close #3676
@ganthern ganthern removed their assignment Aug 16, 2023
@ganthern ganthern removed the state:wontfix issues that are not significant enough to invest in or that are intended behaviour label Nov 23, 2023
@ganthern ganthern reopened this Nov 23, 2023
@charlag
Copy link
Contributor

charlag commented Nov 23, 2023

Some points to check:

  • Does it handle cancellation?
  • How does it handle failed authentication?
  • When does it prompt the user?
  • Does it work with freedesktop portal/flatpak?
  • do we need to select backend explicitly for linux? Does it allow us to get rid of "secret storage" requirement on linux?

@ganthern
Copy link
Contributor Author

ganthern commented Nov 23, 2023 •
edited
Loading

MacOS:

  • cancellation is handled in the usual way (we get the key, it fails, we show an error dialog with a link to the FAQ)
  • wrong passwords just cause the dialog to stay until it's cancelled or the correct password is entered.
  • user is prompted on startup for each key the app did not create itself. at most 3 times - safe storage key, tutanota-vault, tutanota-credentials. The latter two are only requested because we want to migrate them. This could be reduced by only doing the migration once, currently it's unconditional.

Linux (GNOME/Seahorse/libsecret):

Windows:

  • seems to just work, could not get it to ask for any confirmation?

@ganthern ganthern linked a pull request Nov 24, 2023 that will close this issue
Arm support 2625 #6185
Merged
@ganthern
Copy link
Contributor Author

ganthern commented Nov 24, 2023 •
edited
Loading

I'm unsure how to test this for flatpak. we can easily make a test build (or several) once it's released, but the build process expects the prebuilt client to be on github. Not sure where to put test builds for it to pick up.

maybe attach it to an old release and hardcode the URL? Is there a way to build this locally?

E: flatpak-builder apparently

@charlag
Copy link
Contributor

charlag commented Nov 24, 2023

yeah I'd try just using flatpak builder

ganthern added a commit that referenced this issue Nov 27, 2023
@ganthern
[keytar] always use the dummy if loading keytar would crash the app
2220ddd 
#3676
#6050
@ganthern
Copy link
Contributor Author

It just works and defaults to gnome-libsecret backend if we just update tutanota to use safeStorage. Still needs libsecret.

ganthern added a commit that referenced this issue Nov 27, 2023
@ganthern
[keytar] always use the dummy if loading keytar would crash the app
5408751 
#3676
#6050
murilopereirame pushed a commit that referenced this issue Nov 27, 2023
@ganthern @murilopereirame
[keytar] always use the dummy if loading keytar would crash the app
69ec143 
#3676
#6050
murilopereirame pushed a commit that referenced this issue Nov 27, 2023
@ganthern @murilopereirame
[keytar] always use the dummy if loading keytar would crash the app
a15569b 
#3676
#6050
@ganthern ganthern linked a pull request Jan 10, 2024 that will close this issue
tutanota-error package, remove keytar, update electron #6360
Merged
@ganthern ganthern added this to the 3.120.6 milestone Jan 10, 2024
@wrdhub wrdhub modified the milestones: 3.120.x, 3.120.6 Jan 15, 2024
@ganthern ganthern added the state:tested We tested it and are about to release it label Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
desktop Desktop client related issues improvement nice-to-haves that are not impeding usage of any features state:tested We tested it and are about to release it
Projects
None yet
Milestone
3.121.0
5 participants
@charlag @bedhub @ganthern @murilopereirame @wrdhub