VPN-6422: Migrate to XDG Secrets Portal #9922

oskirby · 2024-09-27T23:15:12Z

Description

We have a steady stream of users reporting that their Linux clients are frequently loosing their settings across reboot. This looks and smells like a failure in the LinuxCryptoSettings class to retrieve the settings encryption keys from the libsecret API. We have seen this before, and it typically results from the user's keyring being locked at the time that they launch the VPN client.

Fortunately, there is a better way to generate an application encryption key using the XDG Secrets Portal, and we already have it implemented for Flatpaks. This PR attempts to provide a migration strategy for users who have settings encrypted with libsecret to switch over to the XDG secrets instead.

We accomplish this by doing the following:

Extend the CryptoSettings::getKey() method to pass in the encrypted settings file version. This lets us distinguish if a file was encrypted with libsecret or the XDG portal.
Replace LinuxCryptoSettings with a wrapper that checks for a key in libsecret to use as a fallback if a V1 file is found, otherwise it just wraps XdgCryptoSettings instead.
Add some code to delete the keys in libsecret once the user attempts to write a file with an XDG secret.

Along the way, we found a bug (or quirk?). When the application is launched via the old /etc/xdg/autostart handlers, this results in the application being unassociated with a systemd scope. This means that any attempt to use an XDG portal results in the portals being unable to figure out the application ID. This can result in the application getting a different set of keys than when launched via the .desktop file. Different keys results in a failure to read the settings files, which results in an apparent loss of settings. Luckily for us, this can be addressed by using the XDG Background Portal for launching at boot... which we also have implemented as a part of the Flatpak work.

There is a potential bug in here though, or maybe a just a developer annoyance. Because the XDG secrets portal is sensitive to the application ID (which in turn is derived from the systemd scopes it is launched in). We wind up with different encryption keys if we launch it via the application menu (which winds up with the proper scopes and app IDs) vs. when we start it manually on the command line (which gets no scope).

And there seems to be something odd in the RPM build world that breaks when trying to include the Qt6 private GUI libraries (for the XDG Portal helpers). This doesn't seem to cause problems on Fedora 39 or later, so we have also included an update to the Fedora build images here as well (they were long overdue for an update anyways as the old versions have been EOL for a number of months).

Reference

JIRA Issue: VPN-6422
User bug reports:

Checklist

My code follows the style guidelines for this project
I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
I have performed a self review of my own code
I have commented my code PARTICULARLY in hard to understand areas
I have added thorough tests where needed

It turns out that the CryptoSettings class really does need to be a singleton, and we run into some gnarly memory corrpution bugs if there is more than one implementation kicking around. So, lets make LinuxCryptoSettings inherit XdgCryptoSettings instead.

For host applications (eg: not in a Flatpak/Snap sandbox), there is some guesswork applied in the XDG Portals to figure out the application ID. If that guess is wrong, then the secrets API will give us a different key because it thinks we are a different application. Unfortunately, this guess goes very wrong when launched via the old /etc/xdg/autostart handlers, which results in the keys getting wiped upon boot. Luckily for us, just switching to the XDG Background portal fixes it for us.

ahal · 2024-10-01T21:05:19Z

r+ for the taskcluster changes, though removing myself so this gets a proper review.

mcleinman

A bunch of questions, mostly to make sure I understand this area of the code and the tradeoffs made here.

I'd like this to be a +2 (2 VPN engineers sign off), given the security implications and my limited prior work in this area of the codebase. I'd be comfortable being one of the signoffs after I make sure I'm understanding this - happy to setup time to talk live about my questions, or you're welcome to answer them here - whatever is easier.

mcleinman · 2024-10-01T22:53:08Z

taskcluster/kinds/build/linux.yml

@@ -55,24 +55,24 @@ linux/noble:
        name: linux-noble
        type: build

-linux/fedora-fc37:
-    description: "Linux Build (Fedora/37)"
+linux/fedora-fc39:


Are we updating versions here?

This PR enables the XdgPortal class for all Linux builds (no longer just the Flatpaks), which has a dependency on Qt6::GuiPrivate in order to resolve the window handle. It turned out that this dependency doesn't build cleanly on Fedora 37/38.

These versions of Fedora have also been EOL for a couple of months now, so I figured the cleanest solution was just to do an upgrade... but I could be convinced to ignore the problem and do that upgrade in another PR.

Thanks. That makes sense. Do we need to ensure any help docs or download pages are updated w/ this info?

taskcluster/kinds/docker-image/kind.yml

src/platforms/linux/xdgportal.cpp

src/platforms/linux/xdgcryptosettings.cpp

mcleinman · 2024-10-01T23:02:12Z

src/platforms/linux/linuxcryptosettings.h

@@ -6,20 +6,22 @@
 #define LINUXCRYPTOSETTINGS_H

 #include "cryptosettings.h"


Can this line be removed now?

src/platforms/linux/linuxcryptosettings.cpp

mcleinman · 2024-10-01T23:27:15Z

src/platforms/linux/linuxcryptosettings.cpp

+  auto schema = cryptosettings_get_schema();
+  gchar* password = secret_password_lookup_sync(schema, nullptr, &err, nullptr);
+  if (err != nullptr) {
+    g_error_free(err);


Do we want to log the error in this block, as we do currently?

Sure, I can put the error message back.

src/cryptosettings.cpp

mcleinman

I've added @lesleyjanenorton to this review - Like I said earlier, given that this is security-related and something I'm not that familiar with, I'd love a second r+ on this one.

r+ from me, thanks for answering a bunch of questions as I get more familiar with this part of the code. I'm not marking "approve" yet so we can't merge it until we get a second reviewer who looks at it.

lesleyjanenorton

👍 On behalf of our futures selves, let's make sure to tell the folks in support about the launch quirk and maybe add something to the docs.

oskirby · 2024-10-08T18:13:45Z

After a bit of research, I think we could workaround the startup quirk by creating a Systemd scope on startup if we find that the application is launched outside of one. Basically we would need to invoke the startTransientUnit() method over D-Bus and request a scope name which matches the XDG desktop portal parser implementation

oskirby added 11 commits September 27, 2024 12:33

LinuxCryptoSettings: migrate users to XDG secrets portal

c003c51

Update unit tests and test the v1->v2 upgrade

9ce9b8c

Tidy up some debug

1a34808

Care for a wee spot of lint?

810fe92

Maybe fix build on Ubuntu/Focal

b3a1ead

Fix android compile error

307530f

Fix compile dangling reference warning in XdgCryptoSettings

1df0880

Remove /etc/xdg/autostart from RPM spec

f9bfb35

Bump Taskcluster RPM builds to Fedora 39 and 40

5ef63ad

oskirby requested a review from a team as a code owner September 28, 2024 02:28

oskirby requested review from ahal and mcleinman and removed request for a team September 28, 2024 02:28

ahal removed their request for review October 1, 2024 21:05

mcleinman reviewed Oct 1, 2024

View reviewed changes

mcleinman requested review from mcleinman and lesleyjanenorton October 2, 2024 22:57

mcleinman approved these changes Oct 2, 2024

View reviewed changes

Address review comments

736ddb5

lesleyjanenorton approved these changes Oct 8, 2024

View reviewed changes

oskirby merged commit 746b6c5 into main Oct 9, 2024
117 checks passed

oskirby deleted the vpn-6422-migrate-to-xdg-secrets branch October 9, 2024 15:52

oskirby mentioned this pull request Nov 6, 2024

Linux: Drop CAP_NET_RAW capability from binary #10016

Merged

5 tasks

oskirby mentioned this pull request Nov 27, 2024

VPN-6647: Ensure the app is started with a systemd application scope #10079

Merged

5 tasks

oskirby mentioned this pull request Jan 14, 2025

Remove legacy XDG autostart files #10187

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPN-6422: Migrate to XDG Secrets Portal #9922

VPN-6422: Migrate to XDG Secrets Portal #9922

oskirby commented Sep 27, 2024 •

edited

Loading

ahal commented Oct 1, 2024

mcleinman left a comment

mcleinman Oct 1, 2024

oskirby Oct 2, 2024

mcleinman Oct 2, 2024

mcleinman Oct 1, 2024

oskirby Oct 2, 2024

mcleinman Oct 1, 2024

oskirby Oct 2, 2024

mcleinman left a comment

lesleyjanenorton left a comment

oskirby commented Oct 8, 2024

		@@ -6,20 +6,22 @@
		#define LINUXCRYPTOSETTINGS_H

		#include "cryptosettings.h"

VPN-6422: Migrate to XDG Secrets Portal #9922

VPN-6422: Migrate to XDG Secrets Portal #9922

Conversation

oskirby commented Sep 27, 2024 • edited Loading

Description

Reference

Checklist

ahal commented Oct 1, 2024

mcleinman left a comment

Choose a reason for hiding this comment

mcleinman Oct 1, 2024

Choose a reason for hiding this comment

oskirby Oct 2, 2024

Choose a reason for hiding this comment

mcleinman Oct 2, 2024

Choose a reason for hiding this comment

mcleinman Oct 1, 2024

Choose a reason for hiding this comment

oskirby Oct 2, 2024

Choose a reason for hiding this comment

mcleinman Oct 1, 2024

Choose a reason for hiding this comment

oskirby Oct 2, 2024

Choose a reason for hiding this comment

mcleinman left a comment

Choose a reason for hiding this comment

lesleyjanenorton left a comment

Choose a reason for hiding this comment

oskirby commented Oct 8, 2024

oskirby commented Sep 27, 2024 •

edited

Loading