Move socket into separate directory #8030

WhyNotHugo · 2022-05-09T10:13:04Z

This is mostly to ease setup and configuration with sandboxed browsers.

The socket currently existing in $XDG_RUNTIME_DIR. When sandboxing a
browser, it would be unsafe to mount this directory inside the sandbox.
Mounting the socket into the sandbox's filesystem is also not possible
in cases where KeePassXC is [re]started after the browser has started.

This commit moves the socket into its own isolated subdirectory, which
can be safely mounted into sandboxes. Sandbox engines can create the
directory themselves (in case the browser starts before KeePassXC). Both
Flatpak and Firejail support this configuration.

A symlink is also created, linking the previous location to the new
location. This is meant for backwards compatibility and should
eventually be dropped.

The directory can't be named org.keepassxc.KeePassXC.BrowserServer,
since that would collide with the symlink. Instead, the directory has
been created to match the format used for Flatpak builds, which make it
a bit less of a snowflake build, while following accepted conventions.

Fixes: #8018

Testing strategy

Backwards compatibility

Try using browser integration on an existing, working, setup. It should all work without any issues.

For sandboxed applications

See #6741 (reply in thread), without tampering with keepassxc.local.

Type of change

✅ Bug fix (non-breaking change that fixes an issue)
✅ New feature (change that adds functionality)

Notes

It might make sense to drop this bit, since now it matches the default behaviour:

keepassxc/src/browser/BrowserShared.cpp

Lines 35 to 37 in 736517f

    
           #elif defined(KEEPASSXC_DIST_FLATPAK) 
        
                   return QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation) + "/app/" + "org.keepassxc.KeePassXC" 
        
                          + serverName;

Not sure if it's fine to do this on the PR of it it's best to do that separately.

codecov-commenter · 2022-05-09T10:28:35Z

Codecov Report

Merging #8030 (86f3b1d) into develop (a4d4adb) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 86f3b1d differs from pull request most recent head 4ebfc7e. Consider uploading reports for the commit 4ebfc7e to get more accurate results

@@             Coverage Diff             @@
##           develop    #8030      +/-   ##
===========================================
- Coverage    64.29%   64.27%   -0.02%     
===========================================
  Files          339      339              
  Lines        43431    43446      +15     
===========================================
  Hits         27923    27923              
- Misses       15508    15523      +15

Impacted Files	Coverage Δ
src/browser/BrowserShared.cpp	`0.00% <0.00%> (ø)`
src/core/FileWatcher.cpp	`86.75% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4d4adb...4ebfc7e. Read the comment docs.

droidmonkey · 2022-05-09T10:33:15Z

Nice!

WhyNotHugo · 2022-05-10T19:17:07Z

Squashed follow-up commits. Formatting issues are all fixed. Ready for review/feedback.

droidmonkey · 2022-05-10T20:53:14Z

Interesting, our code format check seems to be broken @phoerious. We shouldn't be passing when there are if statements without { } blocks.

src/browser/BrowserShared.cpp

This is mostly to ease setup and configuration with sandboxed browsers. The socket currently existing in `$XDG_RUNTIME_DIR`. When sandboxing a browser, it would be unsafe to mount this directory inside the sandbox. Mounting the socket into the sandbox's filesystem is also not possible in cases where KeePassXC is [re]started after the browser has started. This commit moves the socket into its own isolated subdirectory, which can be safely mounted into sandboxes. Sandbox engines can create the directory themselves (in case the browser starts before KeePassXC). Both Flatpak and Firejail support this configuration. A symlink is also created, linking the previous location to the new location. This is meant for backwards compatibility and should eventually be dropped. The directory can't be named `org.keepassxc.KeePassXC.BrowserServer`, since that would collide with the symlink. Instead, the directory has been created to match the format used for Flatpak builds, which make it a bit less of a snowflake build, while following accepted conventions. Given that the preferred path now matches what Flatpak uses, the block handling Flatpak and non-Flatpak is now the same. If `$XDG_RUNTIME_DIR` is undefined, the temporary directory is used, though reading the socket from this location is discouraged. Closes: #8018 References: #6741

WhyNotHugo · 2022-05-11T16:10:50Z

Looking at the logs for the build, I noticed this:

[13:52:59][Step 3/4] QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-keepassxc'

It turns out that if XDG_RUNTIME_DIR is unset, then QStandardsPaths::RuntimeLocation returns fallback a path in /tmp. This implies that it only returns empty if a path inside /tmp was unusable, so the fallback code that's already present in src/browser/BrowserShared.cpp is wrong, since it'll try to use /tmp in cases when /tmp is unusable.

The fallback should be dropped. It's clear that nobody is relying on this fallback for the extension tho, because, unless I'm missing something, it cannot work.

Edit: implemented in a455ee4

That aside, I've no idea why the Ubuntu build is failing. Is this a fluke? I'll try to re-trigger CI and see if it passes.

Edit: it passed.

It if XDG_RUNTIME_DIR is unset, then QStandardsPaths::RuntimeLocation returns fallback a path in /tmp. This implies that the fallback code remove here is wrong, since the fallback to /tmp is already handled by Qt and doesn't need to be done again on the KeePassXC side. It's clear that nobody is relying on the existing fallback for the extension, since the code-path that creates triggers this fallback was unreachable.

droidmonkey · 2022-05-11T23:02:41Z

Agree the fallback is silly. Perhaps we can issue a warning to the console if the socket cannot be created.

WhyNotHugo · 2022-05-17T16:03:46Z

Agree the fallback is silly. Perhaps we can issue a warning to the console if the socket cannot be created.

Can we address it separately? I'm not really sure how to do that, and I think this PR is ready as-is.

droidmonkey · 2022-05-17T18:28:11Z

Yah no worries

droidmonkey · 2022-05-24T21:19:16Z

I'll be looking at prs later this week

src/browser/BrowserShared.cpp

WhyNotHugo · 2022-05-28T22:22:18Z

Thanks!

This is mostly to ease setup and configuration with sandboxed browsers. The socket currently existing in `$XDG_RUNTIME_DIR`. When sandboxing a browser, it would be unsafe to mount this directory inside the sandbox. Mounting the socket into the sandbox's filesystem is also not possible in cases where KeePassXC is [re]started after the browser has started. This commit moves the socket into its own isolated subdirectory, which can be safely mounted into sandboxes. Sandbox engines can create the directory themselves (in case the browser starts before KeePassXC). Both Flatpak and Firejail support this configuration. A symlink is also created, linking the previous location to the new location. This is meant for backwards compatibility and should eventually be dropped. The directory can't be named `org.keepassxc.KeePassXC.BrowserServer`, since that would collide with the symlink. Instead, the directory has been created to match the format used for Flatpak builds, which make it a bit less of a snowflake build, while following accepted conventions. Given that the preferred path now matches what Flatpak uses, the block handling Flatpak and non-Flatpak is now the same. If `$XDG_RUNTIME_DIR` is undefined, the temporary directory is used, though reading the socket from this location is discouraged. Closes: keepassxreboot#8018 References: https://github.com/keepassxreboot/keepassxc/discussions/6741

WhyNotHugo · 2022-06-15T17:15:46Z

Hmmm, the directory is created with permission 755, but keepass refuses to create the socket due to directory permissions (requires them to be 700).

I'm not that familiar with Qt, can we change mkdir so that is uses 0700, or should I just add a chmod?

droidmonkey · 2022-06-15T20:55:04Z

Use https://doc.qt.io/qt-5/qfileinfo.html#permission

WhyNotHugo · 2022-06-16T21:07:34Z

Oh, never mind. I was running in a sandbox and the permissions of $XDG_RUNTIME_DIR were wrong. Nothing to be fixed here, my bad.

This is mostly to ease setup and configuration with sandboxed browsers. The socket currently existing in `$XDG_RUNTIME_DIR`. When sandboxing a browser, it would be unsafe to mount this directory inside the sandbox. Mounting the socket into the sandbox's filesystem is also not possible in cases where KeePassXC is [re]started after the browser has started. This commit moves the socket into its own isolated subdirectory, which can be safely mounted into sandboxes. Sandbox engines can create the directory themselves (in case the browser starts before KeePassXC). Both Flatpak and Firejail support this configuration. A symlink is also created, linking the previous location to the new location. This is meant for backwards compatibility and should eventually be dropped. The directory can't be named `org.keepassxc.KeePassXC.BrowserServer`, since that would collide with the symlink. Instead, the directory has been created to match the format used for Flatpak builds, which make it a bit less of a snowflake build, while following accepted conventions. Given that the preferred path now matches what Flatpak uses, the block handling Flatpak and non-Flatpak is now the same. If `$XDG_RUNTIME_DIR` is undefined, the temporary directory is used, though reading the socket from this location is discouraged. Closes: #8018 References: #6741

This is mostly to ease setup and configuration with sandboxed browsers. The socket currently existing in `$XDG_RUNTIME_DIR`. When sandboxing a browser, it would be unsafe to mount this directory inside the sandbox. Mounting the socket into the sandbox's filesystem is also not possible in cases where KeePassXC is [re]started after the browser has started. This commit moves the socket into its own isolated subdirectory, which can be safely mounted into sandboxes. Sandbox engines can create the directory themselves (in case the browser starts before KeePassXC). Both Flatpak and Firejail support this configuration. A symlink is also created, linking the previous location to the new location. This is meant for backwards compatibility and should eventually be dropped. The directory can't be named `org.keepassxc.KeePassXC.BrowserServer`, since that would collide with the symlink. Instead, the directory has been created to match the format used for Flatpak builds, which make it a bit less of a snowflake build, while following accepted conventions. Given that the preferred path now matches what Flatpak uses, the block handling Flatpak and non-Flatpak is now the same. If `$XDG_RUNTIME_DIR` is undefined, the temporary directory is used, though reading the socket from this location is discouraged. Closes: keepassxreboot#8018 References: keepassxreboot#6741

This is mostly to ease setup and configuration with sandboxed browsers. The socket currently existing in `$XDG_RUNTIME_DIR`. When sandboxing a browser, it would be unsafe to mount this directory inside the sandbox. Mounting the socket into the sandbox's filesystem is also not possible in cases where KeePassXC is [re]started after the browser has started. This commit moves the socket into its own isolated subdirectory, which can be safely mounted into sandboxes. Sandbox engines can create the directory themselves (in case the browser starts before KeePassXC). Both Flatpak and Firejail support this configuration. A symlink is also created, linking the previous location to the new location. This is meant for backwards compatibility and should eventually be dropped. The directory can't be named `org.keepassxc.KeePassXC.BrowserServer`, since that would collide with the symlink. Instead, the directory has been created to match the format used for Flatpak builds, which make it a bit less of a snowflake build, while following accepted conventions. Given that the preferred path now matches what Flatpak uses, the block handling Flatpak and non-Flatpak is now the same. If `$XDG_RUNTIME_DIR` is undefined, the temporary directory is used, though reading the socket from this location is discouraged. Closes: #8018 References: #6741

To avoiding mounting everything into Flatpak sandbox, the socket was moved into a separate path under Linux [1][2]. Note this does not affect other distributions under Linux as there is a symbolic link from the old path [3]. [1] keepassxreboot/keepassxc#8030 [2] keepassxreboot/keepassxc@1009650 [3] https://github.com/keepassxreboot/keepassxc/blob/2.7.4/src/browser/BrowserShared.cpp#L49

WhyNotHugo mentioned this pull request May 9, 2022

Move socket into separate directory for sandbox friendliness #8018

Closed

droidmonkey added platform: Linux feature: Browser backport-pending labels May 10, 2022

droidmonkey requested changes May 10, 2022

View reviewed changes

src/browser/BrowserShared.cpp Outdated Show resolved Hide resolved

src/browser/BrowserShared.cpp Outdated Show resolved Hide resolved

src/browser/BrowserShared.cpp Outdated Show resolved Hide resolved

src/browser/BrowserShared.cpp Outdated Show resolved Hide resolved

WhyNotHugo requested a review from droidmonkey May 18, 2022 14:29

droidmonkey requested changes May 28, 2022

View reviewed changes

src/browser/BrowserShared.cpp Show resolved Hide resolved

droidmonkey reviewed May 28, 2022

View reviewed changes

src/browser/BrowserShared.cpp Show resolved Hide resolved

droidmonkey added this to the v2.7.2 milestone May 28, 2022

droidmonkey approved these changes May 28, 2022

View reviewed changes

droidmonkey merged commit 1009650 into keepassxreboot:develop May 28, 2022

WhyNotHugo deleted the sandbox-friendly-socket branch May 28, 2022 22:22

WhyNotHugo mentioned this pull request May 30, 2022

Socket has moved on latest keepassxc master varjolintu/keepassxc-proxy-rust#8

Closed

WhyNotHugo mentioned this pull request Jun 15, 2022

keepassxc: please provide more options or more guidance netblue30/firejail#3329

Open

droidmonkey added backported and removed backport-pending labels Jun 27, 2022

varjolintu mentioned this pull request Jul 4, 2022

Containerized Chrome, extension does not connect to keepass anymore keepassxreboot/keepassxc-browser#1665

Closed

droidmonkey mentioned this pull request Oct 24, 2022

After upgrade to 2.7.3 on Linux, cannot connect to browser #8634

Closed

varjolintu mentioned this pull request Oct 25, 2022

Fix deleting existing socket file before making a new symbolic link #8656

Merged

stefan-matic mentioned this pull request Oct 26, 2022

KeePassXC update 2.7.2/2.7.3 breaks connection to Unix socket Frederick888/git-credential-keepassxc#53

Closed

droidmonkey mentioned this pull request Oct 29, 2022

Browser: second fix for linked socket path #8704

Merged

github-actions bot mentioned this pull request Oct 31, 2022

Refactor get_socket_path() and support KPXC 2.7.4 for Flatpak Frederick888/git-credential-keepassxc#55

Merged

3 tasks

This was referenced Nov 6, 2022

Symlinking from /run/user/1000 breaks other flatpacked apps related to KeePassXC and flatpacked KeePassXC #8757

Open

Access KeePassXC installed as a snap or a Flatpak package purejava/keepassxc-proxy-access#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move socket into separate directory #8030

Move socket into separate directory #8030

WhyNotHugo commented May 9, 2022 •

edited

Loading

codecov-commenter commented May 9, 2022 •

edited

Loading

droidmonkey commented May 9, 2022

WhyNotHugo commented May 10, 2022

droidmonkey commented May 10, 2022

WhyNotHugo commented May 11, 2022 •

edited

Loading

droidmonkey commented May 11, 2022

WhyNotHugo commented May 17, 2022

droidmonkey commented May 17, 2022

droidmonkey commented May 24, 2022

WhyNotHugo commented May 28, 2022

WhyNotHugo commented Jun 15, 2022

droidmonkey commented Jun 15, 2022

WhyNotHugo commented Jun 16, 2022

	#elif defined(KEEPASSXC_DIST_FLATPAK)
	return QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation) + "/app/" + "org.keepassxc.KeePassXC"
	+ serverName;

Move socket into separate directory #8030

Move socket into separate directory #8030

Conversation

WhyNotHugo commented May 9, 2022 • edited Loading

Testing strategy

Backwards compatibility

For sandboxed applications

Type of change

Notes

codecov-commenter commented May 9, 2022 • edited Loading

Codecov Report

droidmonkey commented May 9, 2022

WhyNotHugo commented May 10, 2022

droidmonkey commented May 10, 2022

WhyNotHugo commented May 11, 2022 • edited Loading

droidmonkey commented May 11, 2022

WhyNotHugo commented May 17, 2022

droidmonkey commented May 17, 2022

droidmonkey commented May 24, 2022

WhyNotHugo commented May 28, 2022

WhyNotHugo commented Jun 15, 2022

droidmonkey commented Jun 15, 2022

WhyNotHugo commented Jun 16, 2022

WhyNotHugo commented May 9, 2022 •

edited

Loading

codecov-commenter commented May 9, 2022 •

edited

Loading

WhyNotHugo commented May 11, 2022 •

edited

Loading