feat: add configurable socket permissions

[JP-Ellis]

Summary

Have pueue set the permissions of the socket created. Previously, the permissions where unspecified and (at least for me) defaulted to 755. The permissions can be configured through the new unix_socket_permissions setting.

As part of this change, the default settings are moving to 700 which is more restricted than the (possibly system-dependent) previous default. The more restricted permissions should not have any practical impact as users without write permissions would not be able to use the socket, though removing read access to others may possibly improve security.

Checklist

Please make sure the PR adheres to this project's standards:

• I included a new entry to the CHANGELOG.md.
• I checked cargo clippy and cargo fmt. The CI will fail otherwise anyway.
• (If applicable) I added tests for this feature or adjusted existing tests.
• (If applicable) I checked if anything in the wiki needs to be changed.

[JP-Ellis]

A couple of things outstanding for this PR before merging:

• I have not added a test yet. You have quite an exhaustive test suite, so I would like you to point me in the right direction as to what tests to add.
• There is a test failing when it comes to the backwards compatibility. It wasn't immediately clear to me how this should be resolved.

[github-actions]

Test Results

  3 files   22 suites   3m 2s ⏱️
154 tests 154 ✅ 0 💤 0 ❌
328 runs  328 ✅ 0 💤 0 ❌

Results for commit 1fd2f64.

♻️ This comment has been updated with latest results.

[Nukesor]

Regarding tests, the way to do it would be to start a daemon with settings that sets specific socket permissions.
A good test that can be used for some guidance is tests/daemon/integration/environment_variables.rs.

1. Modify the settings as you like before passing them to standalone_daemon.
2. Wait for the unix socket to show up (settings.shared.unix_socket_path())
3. Check its file permissions.

I think it would be nice to have a new unix_socket.rs test file.

[Nukesor]

I have a few waiting functions in tests/helpers/wait.rs.
Just take one of those and modify them to wait for a file to show up :)

I also edited my original comment to contain a bit more info, but I noticed you already read it before my edit. You're too quick :D

[Nukesor]

Also, a new lint popped up due to Rust 1.79.
It's already fixed on main :)

[Nukesor]

Do you need any further help :)?

[JP-Ellis]

Just need a bit more time 😆 I've had a busy few days lately, but I should have time by the end of the week to finish it up 👍

[Nukesor]

Don't worry, take your time. Just wanted to check in if you need anything :)

[JP-Ellis]

So I finally got back to this PR, and I have some not-so-great news.

Unfortunately the PermissionsExt from the standard library does not work as expected, with set_mode have no effect on the socket. This incompatibility has been reported by others.

The solution in their case is to use the unsafe libc::umask method which is unfortunate for two reasons:

1. It introduces a dependency on libc (though it appears pueue already dependency on libc transitively).
2. It introduces unsafe code.

I have added a separate commit implementing this, and while it works well for the hardcoded default value, I was unable to get the test with the modified value to work. For some reason, it always defaults back to the value in pueue_lib/src/settings.rs irrespective of being modified in the test. I might be missing something simple?

[Nukesor]

After thinking about the possible side-effects of using umask, I'm not convinced that this is the solution we should use, especially since we're dealing with a multi-threaded environment that spawns subprocesses and reads/writes files.

I just played around with the unix socket on my local PC and I could easily change the permissions of the socket via chmod 777, so there's theoretically no need to set the umask.

There's obviously that bug in the stdlib, but isn't there another way to set file permissions?

For example, the nix crate uses fchmod:
https://docs.rs/nix/latest/src/nix/sys/stat.rs.html#272-276

[JP-Ellis]

After thinking about the possible side-effects of using umask, I'm not convinced that this is the solution we should use, especially since we're dealing with a multi-threaded environment that spawns subprocesses and reads/writes files.

Completely agree with your assessment. I wasn't too happy with using umask myself and the need to set/reset this while creating the socket. I still wanted to test it and share my findings :)

There's obviously that bug in the stdlib, but isn't there another way to set file permissions?

For example, the nix crate uses fchmod: https://docs.rs/nix/latest/src/nix/sys/stat.rs.html#272-276

More than happy to use other crates, I just wanted to avoid unnecessarily introducing new dependencies (unless they exist transitively).

I've updated the PR to make use of the nix crate.

PS: I'm still not sure why the code is not respecting the modified Unix permissions, and unfortunately I seem unable to step through this with a debugger :/

[Nukesor]

This thread is pretty informative:
https://stackoverflow.com/a/74329441

It would explain why stdlib didn't work as expected.

Still, I'm curious why I'm able to change permissions on the unix file socket from the outside.
From what I understand, chmod 777 $XDG_RUNTIME_DIR/pueue_$USER.socket should be a no-op/throw an exception. But it works just fine.

Is that only forbidden for the currently attached process? I'm a bit confused.
Which Operating System are you on right now? The behavior seems to differ in different environments. I'm on Arch Linux right now.

[Nukesor]

Ok, in theory, we can create our own unix socket, set its permission and attach a UnixListener to it.

Tokio allows the creation of a UnixListener from a stdlib UnixListener over here.

And a stdlib UnixListener can be created via a socket_addr.

We just have to find out how to create raw unix sockets.

I'll be able to take a closer look at this at the end of next week, I'm pretty busy right now 😅

[JP-Ellis]

Wow! That's some good digging!

I certainly could implement the logic you're talking about whereby the socket is created first, then chmod, and finally bound with tokio. I honestly would not have thought of that first.

I'll look at raising a ticket to request setting permissions with the upstream libraries. It seems like a bit of a gap given what you have found.

Lastly, I've been doing this testing in macOS, though I also run Arch Linux (and would need this feature on my small Arch 'server').

Thanks so much for the help though!

[codecov]

Codecov Report

Attention: Patch coverage is 57.89474% with 8 lines in your changes missing coverage. Please review.

Project coverage is 79.49%. Comparing base (0e22596) to head (1fd2f64).
Report is 3 commits behind head on main.

Files	Patch %	Lines
pueue_lib/src/network/socket/unix.rs	52.94%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
- Coverage   79.57%   79.49%   -0.09%     
==========================================
  Files          75       75              
  Lines        5556     5573      +17     
==========================================
+ Hits         4421     4430       +9     
- Misses       1135     1143       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JP-Ellis]

Finally got it to work as intended!

Ultimately needed to use tokio's UnixSocket to create the socket, and then fchmodat to set the permissions on the path before listening on the socket.

From a pure security perspective, this implementation does have a very minor vulnerability in the time between the binding of the socket to a path, and the subsequent setting of the permissions. I doubt this will be an issue, but I wanted to flag that just in case. I did try setting the permissions on the socket's underlying RawFd before binding it to a path, but this did not work as intended¹

Footnotes

1. On macOS, the chmod call resulted in an error; and on the Ubuntu runners, the chmod call resulted in a silent no-op. ↩

[Nukesor]

Awesome, thanks for the contribution :)