Skip to content

Desktop: Windows portable: Fix keychain-backed storage incorrectly enabled #10942

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

Conversation

personalizedrefrigerator
Copy link
Collaborator

Summary

This pull request reverts a behavior change from #10535 --- previously, keychain support was disabled in the Windows portable application. After #10535, Electron safeStorage is used to encrypt secure settings in the Windows portable app before saving them in Joplin's database. This may break the portability of Joplin Portable.

Notes:

Note

The new portable app behavior introduced by #10535 may be desired. Consider closing this pull request without merging.

Testing plan

This pull request includes automated tests that:

  • Verify that settings not present in the database are loaded from the KeychainService, if available.
  • Verify that passwords are saved to the database (without processing by KeychainService) when KeychainService is read-only.
  • In the case where KeychainService is read-only, verifies that passwords saved using the default setting storage are preferred to those stored in the keychain.

Manual testing (in progress):

  1. Build the Windows portable app.
  2. Download v3.1.4 of the Windows portable app.
  3. Start the just-downloaded version of Joplin.
  4. Open the development tools. Run Setting.value('keychain.supported') and verify that the output is 1.
  5. Enable the backup plugin and restart Joplin.
  6. Set the backup password to testing and enable password-protected backups.
    • testing must be set in both the password and confirm inputs.
  7. Save settings and quit Joplin.
  8. Replace the downloaded JoplinPortable.exe with the built JoplinPortable.exe.
  9. Start the built JoplinPortable.exe.
  10. Verify that the backup password is still set to testing (in settings, using dev tools).
  11. Change the backup password to test and save.
    • test must be set in both the password and confirm inputs.
  12. Restart Joplin.
  13. Verify that the backup password is still set to test.
  14. Go to Help > About and verify that keychain support is listed as No.
  15. Start a development build of Joplin (Windows 11).
  16. Go to Help > About and verify that keychain support is listed as Yes.
  17. Open the development console and run Setting.setValue('keychain.supported', -1).
  18. Restart Joplin.
  19. Go to Help > About and verify that keychain support is listed as Yes.
  20. Verify that the development tools contain tried to set and get password. Result was: mytest.

@laurent22
Copy link
Owner

After #10535, Electron safeStorage is used to encrypt secure settings in the Windows portable app before saving them in Joplin's database. This may break the portability of Joplin Portable.

It's not clear to me in which way that would break the portability? If the secure settings are still saved to the Joplin's database, then it's still portable?

@personalizedrefrigerator
Copy link
Collaborator Author

It's not clear to me in which way that would break the portability? If the secure settings are still saved to the Joplin's database, then it's still portable?

Secure settings are encrypted using safeStorage before being stored in the database (which, if I recall correctly, internally uses Windows' DPAPI). This may be expected behavior. However, it prevents secure settings from being read after transferring Joplin to a different computer. Secure settings were readable after moving the Windows portable profile to a different computer in Joplin 3.0.

Here are two alternatives to this pull request:

  • Add an "encrypt secure keys" setting (only available to users of the portable app).
  • Allow portable app users to provide a custom secure setting encryption password.

@laurent22 laurent22 merged commit fd06c18 into laurent22:dev Sep 2, 2024
10 checks passed
@laurent22
Copy link
Owner

Thanks for clarifying. So this pull request is good then - on the portable app we don't encrypt settings. We won't do more development to encrypt them because I'd expect any user would already encrypt their drive using Veracrypt or some other solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants