-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add --talk-name=org.freedesktop.secrets so it can talk to local secre… #729
Conversation
…ts storage providers
Started test build 149340 |
Build 149340 successful
|
Not sure why this wasn't done earlier. Anyways I'm 90% sure it should be |
I don't think it matters. I copied this from another Flatpak that uses freedesktop secrets already. |
On my Fedora Kinoite system it didn't access the keyring until I manually set it to uppercase 🤔 |
Interesting, thanks. I've updated it now. |
bot, build |
Queued test build for org.signal.Signal. |
Started test build 149362 |
Started test build 149363 |
Build 149362 successful
|
Build 149363 successful
|
Fairly sure it should be lowercase… Please test whether data is correctly encrypted once it gets published. |
+1 point for pre-build linting. Confirm finishing arguments are valid and appropriately cased. |
Yeah, gonna add it to the linter once you or @cyrneko confirm. |
Just out of curiosity, why does Signal need to talk to org.freedesktop.Secrets, or what does it need to store there? |
Because otherwise the key is stored in plaintext. Recently there was a drama and they have fixed the hole that allowed to steal your desktop session with malicious program without you ever noticing. |
@barthalion as for the capitalization it's the lowercase. An example app that requests access to keyring is Element. |
Thanks for the info, I found an official comment about that here signalapp/Signal-Desktop#6944 (comment) :) |
Oh I don't use Signal, just came here to fix this as I saw some whining on the Fediverse and figured it should be an easy fix. |
I still get |
If you remove the flatpak data first and setup again fresh does it still show the message? It may have chosen the key storage location based on the first time you ran the flatpak. |
Does this mean also deleting all user data? 🤔 Ideally this should be migrated automatically for existing installations... |
It would involve deleting all user data. However, you can (and should) backup the data first just in case you need to restore. As for automatic migration, I'm not sure how that would work. It would have to be something within Signal. |
You can try testing in a VM by adding it as a new device from the phone app. |
AFAIU migration from legacy to safeStorage has been implemented, so it should migrate automatically :) Quote from signalapp/Signal-Desktop#6849 (comment):
See relevant implementation here https://github.com/signalapp/Signal-Desktop/blob/96a2d05906351b2c7caf6ee3d5d3292404606919/app/main.ts#L1641 and https://github.com/signalapp/Signal-Desktop/blob/96a2d05906351b2c7caf6ee3d5d3292404606919/app/main.ts#L1697 |
Just a note of what I tried so far, and didn't work, so be carefull!
|
This is in lowercase the second commit is incorrect. You can check the name with
I've added a lint rule for this and reverted the second commit. |
I need to add This seems to be an issue of Signal, not Flatpak, though. |
I assume it would be |
Yes, but this again seems to be either a bug in Electron or your host system. It should detect the currently running instance automatically. You can overwrite this by using this cli argument. |
Plasma 6, flatpak, Signal version "7.24.1 production" here. I got an error about the keyring backend, suggesting to use |
Instead of modifying your flatpak override org.signal.Signal --user --env=SIGNAL_PASSWORD_STORE=kwallet6 This will then become the default for your user, i.e. apply always, regardless how you start the Signal Flatpak (unless you explicitly override the |
Was happy to help with the initial change but as I don't use Signal I'm going to unsubscribe from this. Happy encrypted messaging to all! |
…ts storage providers