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

Build libsecret without gcrypt #756

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Build libsecret without gcrypt #756

wants to merge 10 commits into from

Conversation

bbhtt
Copy link
Contributor

@bbhtt bbhtt commented Sep 29, 2024

If you previously had basic storage, run it as flatpak run --env=SIGNAL_PASSWORD_STORE=gnome-libsecret org.signal.Signal and check if it's storing in the keyring correctly.

@bbhtt bbhtt marked this pull request as draft September 29, 2024 11:08
@flathubbot
Copy link
Contributor

Started test build 150490

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/133582/org.signal.Signal.flatpakref

@minosimo
Copy link
Contributor

On Fedora 40 this works.

@minosimo
Copy link
Contributor

minosimo commented Sep 29, 2024

We should use the keyring as default again, and if it's possible, also recover the key from the app_id org.signal.Signal entry if it exists.

@salim-b
Copy link

salim-b commented Sep 29, 2024

Nice! With this change, SIGNAL_PASSWORD_STORE=gnome-libsecret now works for me on Ubuntu 22.04 (gnome-keyring: 40.0).

secret-tool search application Signal now gives:

[/45]
label = Chromium Safe Storage
secret = yadayadayada
created = 2024-09-29 11:43:38
modified = 2024-09-29 11:43:38
schema = chrome_libsecret_os_crypt_password_v2
attribute.application = Signal

Does that mean, the underlying issue was on the libsecret side of things and not Electron's or Signal's fault?

@bermeitinger-b
Copy link
Collaborator

It's nice that it's working.

If it was not working with Gnome before, what's the point of flatpak? It's supposed to be agnostic and be able to run anywhere.

Doesn't Ubuntu provide this library, or is it outdated maybe?

I recommend a bit more testing, especially without the env var and its checking, so that it defaults to use the keyring if available, just like upstream.

@salim-b
Copy link

salim-b commented Sep 29, 2024

If it was not working with Gnome before, what's the point of flatpak? It's supposed to be agnostic and be able to run anywhere.

I guess this is just yet another part that is not (yet) agnostic.

Doesn't Ubuntu provide this library, or is it outdated maybe?

> apt info libsecret-1-0
Package: libsecret-1-0
Version: 0.20.5-2
Priority: optional
Section: libs
Source: libsecret
Origin: Ubuntu
Maintainer: Ubuntu Developers <[email protected]>
Original-Maintainer: Debian GNOME Maintainers <[email protected]>
Bugs: https://bugs.launchpad.net/ubuntu/+filebug
Installed-Size: 439 kB
Depends: libc6 (>= 2.14), libgcrypt20 (>= 1.9.0), libglib2.0-0 (>= 2.59.0), libsecret-common
Breaks: seahorse (<< 3.10)
Homepage: https://wiki.gnome.org/Projects/Libsecret
Task: ubuntu-desktop-minimal, ubuntu-desktop, ubuntu-desktop-raspi, kubuntu-desktop, kubuntu-full, xubuntu-core, xubuntu-desktop, lubuntu-desktop, ubuntustudio-desktop, ubuntukylin-desktop, ubuntu-mate-core, ubuntu-mate-desktop, ubuntu-budgie-desktop, ubuntu-budgie-desktop-raspi
Download-Size: 124 kB
APT-Manual-Installed: yes
APT-Sources: http://ch.archive.ubuntu.com/ubuntu jammy/main amd64 Packages
Description: Secret store
 Library for storing and retrieving passwords and other secrets.
 It communicates with the "Secret Service" using DBus.

@bbhtt
Copy link
Contributor Author

bbhtt commented Sep 29, 2024

libsecret uses a file based backend inside Flatpak specifically which is incompatible with how Chromium tries to use it. Disabling this compiles out that backend and it reverts to the usual DBus backend same as how it works outside Flatpak.

We should use the keyring as default again, and if it's possible, also recover the key from the app_id org.signal.Signal entry if it exists.

I don't think it's possible to do that

@bbhtt
Copy link
Contributor Author

bbhtt commented Sep 29, 2024

The library was already present through the electron baseapp btw but it isn't compiled with -Dgcrypt=false, it uses the upstream default.

@salim-b
Copy link

salim-b commented Sep 29, 2024

Great research @bbhtt, I guess!

libsecret uses a file based backend inside Flatpak specifically which is incompatible with how Chromium tries to use it.

Are the upstream (Electron/Chromium) devs aware of this?

The library was already present through the electron baseapp btw but it isn't compiled with -Dgcrypt=false, it uses the upstream default.

With "electron baseapp", you're referring to this repo? Any specific reason to not add the -Dgcrypt=false compilation flag to this base app rather than Signal here?

@flathubbot
Copy link
Contributor

Started test build 150509

@bbhtt
Copy link
Contributor Author

bbhtt commented Sep 29, 2024

I don't know if chromium/electron developers are aware, but libsecret has an issue open for a long time https://gitlab.gnome.org/GNOME/libsecret/-/issues/49 The fix is for Chromium to use the proper API.

With "electron baseapp", you're referring to this repo? Any specific reason to not add the -Dgcrypt=false compilation flag to this base app rather than Signal here?

There is no migration path between the backends, switching the backend will mean the app cannot retrieve the secrets anymore.

Electron baseapp was using gcrypt backend ever since its inception. Disabling it now and switching it will break any app that depends on it.

It doesn't matter for signal because it was using plain text before.

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/133602/org.signal.Signal.flatpakref

This uses the Dbus backend instead of the file based ones under Flatpak
@flathubbot
Copy link
Contributor

Started test build 150532

@pm4rcin
Copy link
Contributor

pm4rcin commented Sep 29, 2024

Not sure if it's related but Tutanota Flatpak that's based on Electron creates dummy entry in keyring because of the bug in Chromium. Not sure if it's related to this bug but here's the text of the entry:

Because of quirks in the gnome libsecret API, Chrome needs to store a dummy entry to guarantee that this keyring was properly unlocked. More details at http://crbug.com/660005.

@bbhtt
Copy link
Contributor Author

bbhtt commented Sep 29, 2024

No that's unrelated.

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/133625/org.signal.Signal.flatpakref

@salim-b
Copy link

salim-b commented Sep 29, 2024

I don't know if chromium/electron developers are aware, but libsecret has an issue open for a long time https://gitlab.gnome.org/GNOME/libsecret/-/issues/49 The fix is for Chromium to use the proper API.

The relevant discussion (this thread) happended almost 3 years ago. There was no further activity in the issue after that. Hence I'd consider it plausible that Chromium/Electron development never got a scent of this...

@rugk
Copy link

rugk commented Sep 30, 2024

So then open an issue with Electron or what is preventing you from doing that?

I don't know the technical details well enough here to describe the problem, so it would be great if one of you could do it.

@salim-b
Copy link

salim-b commented Sep 30, 2024

So then open an issue with Electron or what is preventing you from doing that?

Expertness, mainly. But I tried my best: https://issues.chromium.org/issues/370535829 (the original culprit is Chromium, not Electron, if I'm not mistaken).

@ramcq
Copy link

ramcq commented Sep 30, 2024

@bbhtt This works here. Thanks v. much!

@BentHaase
Copy link

@bbhtt Works for me, F40 👍

@mbridon
Copy link

mbridon commented Oct 17, 2024

So, Signal now works for me, but it takes insanely long to start, like, I started Signal about an hour ago and it's still not up with the window visible yet. 🤯

Running it in the command line I see this:

$ flatpak run org.signal.Signal 
Debug: Using password store: gnome-libsecret
Debug: Will run signal with the following arguments: --password-store=gnome-libsecret
Debug: Additionally, user gave: 
Set Windows Application User Model ID (AUMID) { AUMID: 'org.whispersystems.signal-desktop' }
NODE_ENV production
NODE_CONFIG_DIR /app/Signal/resources/app.asar/config
NODE_CONFIG {}
ALLOW_CONFIG_MUTATIONS undefined
HOSTNAME lappy
NODE_APP_INSTANCE undefined
SUPPRESS_NO_CONFIG_WARNING undefined
SIGNAL_ENABLE_HTTP undefined
userData: /home/bochecha/.var/app/org.signal.Signal/config/Signal
config/get: Successfully read user config file
config/get: Successfully read ephemeral config file
making app single instance
LaunchProcess: failed to execvp:
xdg-settings
LaunchProcess: failed to execvp:
xdg-settings
Gtk-Message: 10:13:01.677: Failed to load module "pk-gtk-module"
Gtk-Message: 10:13:01.679: Failed to load module "pk-gtk-module"

And that's pretty much it, it doesn't go any further than that, but sometimes I eventually see the window at some point. 😕

So I left it hanging for several hours and it wouldn't do anything, then hit ctrl+c to kill it and restarted the same command, and now the window opened just fine. 🤷

@minosimo
Copy link
Contributor

minosimo commented Oct 26, 2024

bot build

Aha, that doesn't work in PRs I guess. Are the expired test builds still available anywhere?

@RayJW
Copy link

RayJW commented Oct 26, 2024

Aha, that doesn't work in PRs I guess. Are the expired test builds still available anywhere?

bot, build

You need the comma for it to work.

@flathubbot
Copy link
Contributor

Queued test build for org.signal.Signal.

@flathubbot
Copy link
Contributor

Started test build 157016

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/140104/org.signal.Signal.flatpakref

@flathubbot
Copy link
Contributor

Started test build 157026

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/140113/org.signal.Signal.flatpakref

@flathubbot
Copy link
Contributor

Started test build 158290

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/141375/org.signal.Signal.flatpakref

@p-fruck
Copy link

p-fruck commented Nov 13, 2024

would be great if we could get this merged. There should be no downsides to non-gnome users I think, and gnome users will be able to finally utilize the safeStorage API. Thanks a lot for your investigation @bbhtt!

@flathubbot
Copy link
Contributor

Started test build 161704

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/144774/org.signal.Signal.flatpakref

@sebasmonia
Copy link

bot, build

@flathubbot
Copy link
Contributor

Queued test build for org.signal.Signal.

@flathubbot
Copy link
Contributor

Started test build 163206

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/146276/org.signal.Signal.flatpakref

@flathubbot
Copy link
Contributor

Started test build 163231

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/146301/org.signal.Signal.flatpakref

@calexandru2018
Copy link

calexandru2018 commented Nov 22, 2024

Indeed this solves the issue. Honestly have no clue why this wasn't considered in the very beginning. Most of the distros come either with gnome-keyring or KDE pre-installed. Leaving a simple text-file should've been only in the case that these systems are not support, as leaving everything unencrypted is a major privacy flaw. Can confirm that for me on F41 this issue is finally solved, thanks @bbhtt for fixing this, have build the signal app from this MR is and it's working flawlessly!

@pm4rcin
Copy link
Contributor

pm4rcin commented Nov 22, 2024

@bbhtt is there anything felt to do until it's merged? It appears that it works for people including me.

@flathubbot
Copy link
Contributor

Started test build 163943

@flathubbot
Copy link
Contributor

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

flatpak install --user https://dl.flathub.org/build-repo/147023/org.signal.Signal.flatpakref

@fluoriteByte
Copy link

this latest version is working fine, i wonder what's taking so long for this to get merged

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.