feat: add configurable socket permissions

JP-Ellis commented 5 months ago

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:

[x] I included a new entry to the CHANGELOG.md.
[x] 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 commented 5 months ago

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[bot] commented 5 months ago

Test Results

3 files 22 suites 3m 2s :stopwatch: 154 tests 154 :white_check_mark: 0 :zzz: 0 :x: 328 runs 328 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 1fd2f645.

:recycle: This comment has been updated with latest results.

Nukesor commented 5 months ago

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 commented 5 months ago

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

Nukesor commented 5 months ago

Do you need any further help :)?

JP-Ellis commented 5 months ago

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 commented 5 months ago

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

JP-Ellis commented 4 months ago

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:

It introduces a dependency on libc (though it appears pueue already dependency on libc transitively).
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?

JP-Ellis commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 :sweat_smile:

JP-Ellis commented 4 months ago

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[bot] commented 4 months ago

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 :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JP-Ellis commented 4 months ago

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[^1]

[^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 commented 4 months ago

Awesome, thanks for the contribution :)

Nukesor / pueue