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 generic secrets in keytar via ipc #1286

Closed

Conversation

Nils1729
Copy link

@Nils1729 Nils1729 commented Oct 20, 2023

This PR implements an extended IPC interface (element-hq/element-web#26405) between the electron platform and application. The interface can be used to set, retrieve and destroy arbitrary secrets, similar to saving pickle keys. Notably, these changes allow setting a value for a secret instead of just generating a random one and retrieving it. See also matrix-org/matrix-react-sdk#11776.

Checklist

  • Ensure your code works with manual testing
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Notes: none


This change has no change notes, so will not be included in the changelog.

@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Oct 20, 2023
@Nils1729 Nils1729 force-pushed the feature/keep-local-backup-key branch from e57ac17 to 421dfe6 Compare November 18, 2023 10:50
@Nils1729 Nils1729 marked this pull request as ready for review November 18, 2023 10:58
@Nils1729 Nils1729 requested review from a team as code owners November 18, 2023 10:58
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

It'd be good to avoid using keytar for new secrets code, can you use https://www.electronjs.org/docs/latest/api/safe-storage instead?

@Nils1729
Copy link
Author

It'd be good to avoid using keytar for new secrets code, can you use https://www.electronjs.org/docs/latest/api/safe-storage instead?

I can try to do that from the current branch, but the implementation would greatly benefit from parts of #1087. Is there any timeline for that PR?

@t3chguy
Copy link
Member

t3chguy commented Nov 20, 2023

@Johennes is currently looking into scheduling for that work to continue, keep in mind that PR hasn't yet been tested, otherwise feel free to cargocult whatever you deem useful

@Johennes
Copy link
Contributor

Sadly, the keytar deprecation topic sits further down in our backlog right now. We'll probably not be able to pick this up ourselves in the near future but contributions would certainly be welcome.

@Nils1729 Nils1729 force-pushed the feature/keep-local-backup-key branch from 421dfe6 to 83135ea Compare November 26, 2023 20:06
@Nils1729
Copy link
Author

I switched to a minimal setup with safeStorage. Note that the code uses a different store from global.store because the security assumptions are different and explicit exceptions to the global.store.clear() on logout would probably not play nicely with the rest.

@@ -0,0 +1,81 @@
/*
Copyright 2023 New Vector Ltd
Copy link
Member

Choose a reason for hiding this comment

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

This should be your copyright

src/secrets.ts Outdated
/**
* Save secrets on a local machine.
* These secrets are associated with a local machine user, not an Element user.
* CAUTION: They are not cleared when a user logs out of Element.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, what is the reason for this? This doesn't seem desirable, we go out of our way to clear all data when you log out using electron-clear-data via the clearStorage IPC

Copy link
Author

Choose a reason for hiding this comment

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

Of couse we can only do this on a trusted platform/filesystem, I am aware this is against the spirit of the remaining code base, thus the comment.

The feature of this PR is intended for use in matrix-org/matrix-react-sdk#11776 (Store the ssss key on the platform so it is easier to use and harder to lose). Not clearing the key reduces friction, as it enables a user to log into Element again and have the device automatically verified.

What if we make clearing the secrets a feature flag that explicitly warn about the implications?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah as it stands this would need product buy-in, right now its intentional that a log out wipes as much as possible.

@andybalaam andybalaam removed their request for review December 7, 2023 09:55
Signed-off-by: Nils Hanff <nils.hanff@giz.berlin>
@Nils1729 Nils1729 force-pushed the feature/keep-local-backup-key branch from 83135ea to 503c42f Compare December 20, 2023 14:02
@Nils1729 Nils1729 marked this pull request as draft February 14, 2024 16:46
@Nils1729 Nils1729 closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants