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

[Desktop] Secure saved credentials. #3733

Closed

Tracked by

bedhub opened this issue Dec 14, 2021 · 3 comments · Fixed by #3833 or #3866

Closed

Tracked by

[Desktop] Secure saved credentials. #3733

bedhub opened this issue Dec 14, 2021 · 3 comments · Fixed by #3833 or #3866

Labels

desktop state:done state:tested

Milestone

Contributor

bedhub commented Dec 14, 2021 •

edited

Loading

This is a subtask of #3444 for the desktop client to secure the stored credentials with a device key.
There should be only the option "Automatic" which creates a key, encrypts the credentials and stores that key in the secure storage of the device. As discussed in #3444 there should be no other biometric or system password option.

Implementation hints
Usually the keychain should be unlocked after the user logged in to their accounts but we have to consider the case that the keychain is still locked. In that case we need to show a dialog to prompt for unlocking the keychain.

There is currently a problem when using keytar because the cancel path is broken: atom/node-keytar#422

We like to replace keytar with electrons api anyway so we need to do this task first:
#3676

bedhub mentioned this issue

Provide option to lock saved credentials with PIN/biometrics #3444

Closed

3 tasks

bedhub added this to the Next release milestone

bedhub changed the title ~~Desktop~~ [Desktop] Secure saved credentials.

charlag added the desktop label

charlag assigned ganthern

ganthern added a commit that referenced this issue


          [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 mentioned this issue

[desktop] use electron safeStorage to store device keys #3742

Closed

Contributor

ganthern commented Dec 21, 2021 •

edited

Loading

the electron part is blocked on this (#3676 (comment)) unless we do something stupid like open an invisible BrowserWindow as soon as app is ready (which could even be too late since we want encryption pretty early).

ganthern added a commit that referenced this issue


          [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


          [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

ganthern added a commit that referenced this issue


          [desktop/linux] propagate keytar cancellation from KeytarSecretStorage

9dfea52

#3733

ganthern mentioned this issue

[desktop/linux] propagate keytar cancellation from KeytarSecretStorage #3773

Merged

charlag mentioned this issue

use electron safeStorage #3676

Closed

ganthern added the state:done label

ganthern added a commit that referenced this issue


          [desktop] use login keychain to secure saved credentials

88cc505

close #3733

ganthern mentioned this issue

[desktop] encrypt credentials #3833

Merged

ganthern added a commit that referenced this issue


          [desktop] use login keychain to secure saved credentials

83f0312

close #3733

ganthern removed the state:done label

ganthern added a commit that referenced this issue


          [desktop] changes after review by ivk

86e3970

don't show credential encryption options if there are
no options

#3733

bedhub mentioned this issue

Encrypt persistent email cache for desktop client #3816

Closed

5 tasks

ganthern added a commit that referenced this issue


          [desktop] use login keychain to secure saved credentials

8ee2c72

close #3733

ganthern added a commit that referenced this issue


          [desktop] changes after review by ivk

4ca9d47

don't show credential encryption options if there are
no options

#3733

Contributor

ganthern commented Feb 1, 2022 •

edited by charlag

Loading

Testing

log in with the old client, save the credentials, update tutanota and then check that the credentials still work.
- Cancellation on Linux:
  - run gnome-keyring-daemon -f -r
  - run client, cancel all the prompts
  - no error should be shown (besides "secure storage not available")
  - try to use saved credentials, another prompt should be shown, cancel, should see no error
  - try to use saved credentials, unlock prompt, should be logged in
  - log out, try to use credentials again, they should unlock you
- Cancellation on macOS:
  - Run client, unlock all prompts
  - open Keychain Access, find tutanota-credentials entry in login keychain, change its properties to not always unlock for the app and to ask password each time
  - open client again, try to log in and cancel, should see no error
  - log in and unlock prompt
  - log out, log in, should see prompt again and it should let you log in again
on linux, delete all credentials, then restart tutanota, then lock the login key chain (using seahorse). Login and make sure the credentials get stored and are available after another restart.
on mac, delete all credentials, then restart tutanota, then lock the login key chain (using keychain access). Login and make sure the credentials get stored and are available after another restart.

charlag added the state:done label

bedhub mentioned this issue

Provide option to protect saved credentials with application pin #3853

Closed

17 tasks

charlag modified the milestones: Next release, 3.91.X

charlag pushed a commit that referenced this issue


          [desktop] use login keychain to secure saved credentials

3c396c9

close #3733

charlag pushed a commit that referenced this issue


          [desktop] changes after review by ivk

03350b2

don't show credential encryption options if there are
no options

#3733

charlag pushed a commit that referenced this issue


          [desktop] use login keychain to secure saved credentials

cab263d

close #3733

charlag pushed a commit that referenced this issue


          [desktop] changes after review by ivk

e3bf47d

don't show credential encryption options if there are
no options

#3733

charlag closed this as completed in #3833

charlag pushed a commit that referenced this issue


          [desktop] use login keychain to secure saved credentials

6bc1af3

close #3733

charlag pushed a commit that referenced this issue


          [desktop] changes after review by ivk

don't show credential encryption options if there are
no options

#3733

charlag unassigned ganthern

charlag mentioned this issue

[desktop/linux] fix & upgrade keytar #3809

Closed

3 tasks

Contributor

charlag commented Feb 4, 2022

I couldn't get it to work yet. Here's what I did:

run gnome-keyring-daemon -f -r to effectively "lock" the keychain
Start the app
See keyring prompt, cancel it
See another keyring prompt, cancel it
client shows "could not access secret storage" dialog, see from output that client treats it like cancellation (like it should)
try to select stored credentials
See 2 other prompts and cancel them
client output "key credentials-device-lock-key not found, generating a new one" which shouldn't actually happen. Also 2 error dialogs (native and in app)

charlag reopened this

charlag added a commit that referenced this issue


          [desktop] Fix accessing keychain and cancellation for it, fix #3733

839e162

There were few issues:

1. We would try to resolve any given key only once. If it fails, it
fails until we restart the app. Now we only cache successful
resolutions3

2. We would cache any of the keys forever, including credentials key.
We actually don't want to cache credential keys.

3. Using defer() led to a situation where we would treat error an as a
non-existing key and try to overwrite it. This meant that during
cancellation we would try to rewrite the key (which is bad by itself
and is exactly what we tried to avoid) and would also trigger another
prompt to unlock keychain.

4. Cancellation would not be propagated correctly and would lead to an
error on the client.

Now we only cache device key, we don't cache errors, and we don't
re-wrap errors (so that cancellation is propagated).

charlag added a commit that referenced this issue


          [desktop] Fix accessing keychain and cancellation for it, fix #3733

a1efaf2

There were few issues:

1. We would try to resolve any given key only once. If it fails, it
fails until we restart the app. Now we only cache successful
resolutions3

2. We would cache any of the keys forever, including credentials key.
We actually don't want to cache credential keys.

3. Using defer() led to a situation where we would treat error an as a
non-existing key and try to overwrite it. This meant that during
cancellation we would try to rewrite the key (which is bad by itself
and is exactly what we tried to avoid) and would also trigger another
prompt to unlock keychain.

4. Cancellation would not be propagated correctly and would lead to an
error on the client.

5. macOS would spam user with prompts for password event after other
changes. User has no idea what they give access to.

Now we only cache device key, we don't cache errors, and we don't
re-wrap errors (so that cancellation is propagated). We introduces
second service name for credentials so that user can differentiate
keys better (that's what macOS shows in prompt and in keychain list
instead of account name).

charlag mentioned this issue

Fix desktop cred encryption #3866

Merged

charlag closed this as completed in #3866

charlag added a commit that referenced this issue


          [desktop] Fix accessing keychain and cancellation for it, fix #3733

d71d8ee

There were few issues:

1. We would try to resolve any given key only once. If it fails, it
fails until we restart the app. Now we only cache successful
resolutions3

2. We would cache any of the keys forever, including credentials key.
We actually don't want to cache credential keys.

3. Using defer() led to a situation where we would treat error an as a
non-existing key and try to overwrite it. This meant that during
cancellation we would try to rewrite the key (which is bad by itself
and is exactly what we tried to avoid) and would also trigger another
prompt to unlock keychain.

4. Cancellation would not be propagated correctly and would lead to an
error on the client.

5. macOS would spam user with prompts for password event after other
changes. User has no idea what they give access to.

Now we only cache device key, we don't cache errors, and we don't
re-wrap errors (so that cancellation is propagated). We introduces
second service name for credentials so that user can differentiate
keys better (that's what macOS shows in prompt and in keychain list
instead of account name).

charlag added the state:tested label

This was referenced Feb 15, 2022

Additional Permissions flathub/com.tutanota.Tutanota#79

Closed

Unexpected error message: GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown #3900

Closed

ganthern added a commit that referenced this issue


          [desktop] use electron safeStorage to store device keys

* 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


          [desktop] use electron safeStorage to store device keys

* 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


          [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


          [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


          [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


          [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


          [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


          [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


          [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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment