flathub / org.signal.Signal

https://flathub.org/apps/details/org.signal.Signal
62 stars 38 forks source link

Add --talk-name=org.freedesktop.secrets so it can talk to local secre… #729

Closed Justinzobel closed 1 week ago

Justinzobel commented 1 week ago

…ts storage providers

flathubbot commented 1 week ago

Started test build 149340

flathubbot commented 1 week ago

Build 149340 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/132432/org.signal.Signal.flatpakref
cyrneko commented 1 week ago

Not sure why this wasn't done earlier. Anyways I'm 90% sure it should be org.freedesktop.Secrets (capitalised) and not org.freedesktop.secrets, at least my testing leads me to believe that.

Justinzobel commented 1 week ago

Not sure why this wasn't done earlier. Anyways I'm 90% sure it should be org.freedesktop.Secrets (capitalised) and not org.freedesktop.secrets, at least my testing leads me to believe that.

I don't think it matters. I copied this from another Flatpak that uses freedesktop secrets already.

cyrneko commented 1 week ago

Not sure why this wasn't done earlier. Anyways I'm 90% sure it should be org.freedesktop.Secrets (capitalised) and not org.freedesktop.secrets, at least my testing leads me to believe that.

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 🤔

Justinzobel commented 1 week ago

Not sure why this wasn't done earlier. Anyways I'm 90% sure it should be org.freedesktop.Secrets (capitalised) and not org.freedesktop.secrets, at least my testing leads me to believe that.

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.

Justinzobel commented 1 week ago

bot, build

flathubbot commented 1 week ago

Queued test build for org.signal.Signal.

flathubbot commented 1 week ago

Started test build 149362

flathubbot commented 1 week ago

Started test build 149363

flathubbot commented 1 week ago

Build 149362 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/132454/org.signal.Signal.flatpakref
flathubbot commented 1 week ago

Build 149363 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/132455/org.signal.Signal.flatpakref
barthalion commented 1 week ago

Fairly sure it should be lowercase… Please test whether data is correctly encrypted once it gets published.

Justinzobel commented 1 week ago

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.

barthalion commented 1 week ago

Yeah, gonna add it to the linter once you or @cyrneko confirm.

Altonss commented 1 week ago

Just out of curiosity, why does Signal need to talk to org.freedesktop.Secrets, or what does it need to store there?

pm4rcin commented 1 week ago

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.

pm4rcin commented 1 week ago

@barthalion as for the capitalization it's the lowercase. An example app that requests access to keyring is Element. PS. If anyone uses keepassxc as secret storage provider like me please confirm if it works with Signal for you as I have no luck with that.

Altonss commented 1 week ago

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.

Thanks for the info, I found an official comment about that here https://github.com/signalapp/Signal-Desktop/issues/6944#issuecomment-2243704263 :)

Justinzobel commented 1 week ago

Yeah, gonna add it to the linter once you or @cyrneko confirm.

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.

Altonss commented 1 week ago

I still get getSQLKey: using legacy key when running latest version, on Fedora Workstation (40).

Justinzobel commented 1 week ago

I still get getSQLKey: using legacy key when running latest version, on Fedora Workstation (40).

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.

Altonss commented 1 week ago

If you remove the flatpak data first and setup again fresh does it still show the message?

Does this mean also deleting all user data? :thinking: Ideally this should be migrated automatically for existing installations...

Justinzobel commented 1 week ago

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.

pm4rcin commented 1 week ago

You can try testing in a VM by adding it as a new device from the phone app.

Altonss commented 1 week ago

As for automatic migration, I'm not sure how that would work. It would have to be something within Signal.

AFAIU migration from legacy to safeStorage has been implemented, so it should migrate automatically :)

Quote from https://github.com/signalapp/Signal-Desktop/pull/6849#issuecomment-2218845070:

In addition to migrating to encrypted/keystore-backed local database encryption keys on supported platforms, our implementation also includes some additional troubleshooting steps and a temporary fallback option that will allow users to recover their message database using their legacy database encryption key if something goes wrong. This should help minimize data loss if any edge cases or other keystore-related bugs are discovered during the migration process and production rollout. The temporary fallback and legacy key will both be removed after everything has been tested and deployed on a wide variety of devices across various operating systems and OS versions.

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

Altonss commented 1 week ago

I still get getSQLKey: using legacy key when running latest version, on Fedora Workstation (40).

Just a note of what I tried so far, and didn't work, so be carefull!

bbhtt commented 1 week ago

This is in lowercase the second commit is incorrect. You can check the name with

dbus-send --print-reply --dest=org.freedesktop.DBus /org/freedesktop/DBus org.freedesktop.DBus.ListNames|grep secrets
      string "org.freedesktop.secrets"

I've added a lint rule for this and reverted the second commit.

bermeitinger-b commented 1 week ago

I need to add --password-store=gnome-libsecret as an argument so that it converts and uses the host's keyring.

This seems to be an issue of Signal, not Flatpak, though.

Justinzobel commented 1 week ago

I need to add --password-store=gnome-libsecret as an argument so that it converts and uses the host's keyring.

This seems to be an issue of Signal, not Flatpak, though.

I assume it would be --password-store=kwallet for KDE Plasma then?

bermeitinger-b commented 1 week ago

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.

ben-alkov commented 1 week ago

I need to add --password-store=gnome-libsecret as an argument so that it converts and uses the host's keyring. This seems to be an issue of Signal, not Flatpak, though.

I assume it would be --password-store=kwallet for KDE Plasma then?

Plasma 6, flatpak, Signal version "7.24.1 production" here. I got an error about the keyring backend, suggesting to use --password-store="kwallet6" via CLI (which took a bit of doing, as I'm not very familiar with the guts of Flatpak). Figured it out, added the flag to the end of the "Exec" line in the .desktop file, and got Signal running again with no data loss.

salim-b commented 1 week ago

Figured it out, added the flag to the end of the "Exec" line in the .desktop file, and got Signal running again with no data loss.

Instead of modifying your .desktop files you can set a permanent env var override with the latest version via:

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 SIGNAL_PASSWORD_STORE env var in the flatpak run invocation).

Justinzobel commented 1 week ago

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!