core: Add optional unix socket file permissions #4741

emilylange · 2022-04-28T06:38:41Z

Allows one to specify the wanted socket permissions like so:

unix//path/to/unix.sock|0666 (unix socket at /path/to/unix.sock with 0666 (ugw=rw)
The leading 0 is optional. unix//path/to/unix.sock|666 would yield the same permissions.
Using :u=rw,r=rw,=o=rw will not work. This could, however, be added fairly easily later.

This commit also changes the default unix socket file permissions to u=rw,g=rw,o= (0660).
It used to default to the shell's umask.

However, the fact that caddy used to default to the umask hasn't been documented anywhere as far as I am aware.
The only mentions I know of are my two comments in https://caddy.community/t/new-user-cant-get-darkweb-site-host-working/15491/12 and #4675.
So I would argue this isn't strictly a deprecation 🤔
Though changing the default to 0600 might be more on par with the umask:

Excerpt taken from the linked issue:

❯ umask -S # symbolic
u=rwx,g=rx,o=rx
ℹ️ A user only needs rw (read/write) access to a socket to be able to use it. x (execute) is not required.

I've also added a unit test for the new function splitUnixSocketPermissionsBits().
I am not too happy with its name, but I think it gets the point across anyway 🤷‍♀️

An addition integration test might make sense, since the actual os.Chmod() only happens at runtime.

CLAassistant · 2022-04-28T06:38:45Z

All committers have signed the CLA.

mholt

This is great, thank you for implementing it! Sorry I sat on this for so long... am finally starting to catch up on things.

LGTM overall. Did I miss why the default perms should be 660 instead of 600? 600 seems safer, like for the admin endpoint for example.

listeners_test.go

sybereal · 2022-07-07T15:42:25Z

Forgive me if I'm forgetting something, but is there a reason to expose the full array of access modes? From what I gather by reading unix(7), the only permission that matters is write access. While it is possible at the file system–level to set other permissions on sockets, they don't seem to have any effect.

I tested this locally as well, by using netcat to listen on a Unix socket and trying to connect to it with another netcat from a different terminal window. I tested the modes 0000 (---), 0100 (--x), 0200 (-w-), 0400 (r--), 0500 (r-x), and only the write bit granted the ability to connect; all other combinations returned "Permission denied.

emilylange · 2022-07-07T17:12:38Z

I am genuinely confused about that.
I tested it on a Linux 5.15.51 with systemd 250.4, and can confirm @sybereal's findings 👀

Literally every single socket implementation and mentioning of them I've seen so far uses read/write not just write.

So I did some more research, I noticed some hosted versions of the unix(7) man page stated¹²

Connecting to the socket object requires read/write permission.

while others didn't.³⁴

As it turns out, quite a few host outdated man pages.
There is a handy mirror, which I used to see when this statement might have been removed and why:

mkerrisk/man-pages@b1ef409 (year 2016)

unix.7: Fix statement about permissions needed to connect to a UNIX domain socket
Read permission is not required (verified by experiment).

and a follow-up commit the same day mkerrisk/man-pages@7578ea2

unix.7: Expand discussion of socket permissions

My whole life has been a lie /s

I guess we should lower the permissions down to write-only 😄

Thanks @sybereal!

mholt

Neat, LGTM! Thanks for looking into this so thoroughly.

mholt · 2022-09-13T22:57:46Z

@IndeedNotJames Did we want to try to merge this for 2.6 or wait? I can't recall if you wanted to do more with this, or just merge it for now and iterate on it later?

emilylange · 2022-09-13T23:21:40Z

I did resolve the merge conflict locally and started working on #4869, just before I went on vacation (back now).
Though I figured v2.6.0 is feature frozen by now 👀

Originally, I wanted to defer any additional features and just have it merged.
But since #4869 got opened, I think it's best to implement that too and merge everything as a single PR.

I should be able to have it review-ready (again) by the day after tomorrow with group names as requested in #4869 :)

mholt · 2022-09-14T03:36:23Z

@IndeedNotJames Cool.

Now that you say that, and if we're going to just do one PR, maybe it would be better to put this in after 2.6 since I don't want to break anything more than I already have 😶 haha. 👍

Thank you for working on this!

francislavoie · 2023-01-06T20:46:44Z

@IndeedNotJames I think we can probably merge this soon, could you fix the conflicts?

emilylange · 2023-01-09T19:00:47Z

I originally stopped working on it again after @mholt's comment suggesting postponing it until after v2.6

But I also get that this is, as of now, the oldest open PR in this repo.

Will prefix the PR with WIP for now.
But I am also open to reduce the scope to its original, so this can be merged, and I do the group thingy in another follow-up PR sometime in the future

emilylange · 2023-02-08T17:44:24Z

Just as a note to myself:
Unix sockets addresses on Windows may include a drive letter (e.g. unix/c:\absolute\path.sock) since #5114, which makes parsing unix//path/to/unix.sock:0666 slightly more complex :^)

mholt · 2023-02-08T17:48:26Z

Ah yeah 😕 Sorry about that... thanks, Windows.

Saklad5 · 2023-02-08T17:49:33Z

Ah yeah 😕 Sorry about that... thanks, Windows.

This is not the first time someone has said that, and it will not be the last.

jameshartig · 2023-04-18T07:30:27Z

@IndeedNotJames any time to wrap this up in the coming months? Anything I can help with? Is the Windows parsing the last outstanding issue?

emilylange · 2023-04-18T16:03:21Z

@jameshartig I'd love to hear about your use-case :)

I've eventually felt like this feature is incredibly niche and essentially no one wants it.
So I lost interest.

I will pick this PR back up if this isn't the case.

Just out of interest: Do you just need chmod (#4675) or chown (#4869) as well?

jameshartig · 2023-04-18T16:11:24Z

@IndeedNotJames Our use-case is that we want to set the perms for the admin unix socket to 600 but set more relaxed permissions for other unix sockets that might be used by client applications 666 or 660. For those purposes we don't need any chown and can get by with just chmod for now. If there's another way to do that I'd be happy to hear.

gi-yt · 2023-05-20T03:23:59Z

@IndeedNotJames Our use-case for this is that we have a master caddy that serves each user's website by reverse-proxying the unix-socket of the user's local caddy instance.
For this, the chmod of the user's local caddy's unix-socket needs to be pretty open, in our case 777.
To achieve this we are currently using a very hacky solution of chmod 777ing every user's unix-socket every 5 seconds using a cronjob.
Having this builtin to caddy would make it a lot more cleaner and easier.

mholt · 2023-05-20T05:13:11Z

For those who replied above with use cases: does every Unix socket need its own, separate permissions? Or can they all have the same permission bits?

emilylange · 2023-05-20T11:55:51Z

@gi-yt FYI a better workaround (for now) would be to modify the systemd service's UMask.
My comment in #4675 (comment) partially applies if you need a pointer on how that could be done :)

francislavoie · 2023-05-20T12:02:58Z

We talked on slack about using | as a separator for the path|bits|user:group as syntax

jameshartig · 2023-05-22T12:49:13Z

does every Unix socket need its own, separate permissions? Or can they all have the same permission bits?

@mholt see my earlier reply:

Our use-case is that we want to set the perms for the admin unix socket to 600 but set more relaxed permissions for other unix sockets that might be used by client applications 666 or 660.

mholt · 2023-05-22T15:55:38Z

Oops, yeah... I'm going blind. Thanks for highlighting that.

This commit also changes the default unix socket file permissions to `u=w,g=,o=` (octal: `0200`). It used to default to the shell's umask (usually `u=rwx,g=rx,o=rx`, octal: `0755`). `/run/caddy.sock` -> `/run/caddy.sock` with `0200` default perms `/run/caddy.sock|0222` -> `/run/caddy.sock` with `0222` perms `|` instead of `:` is used as a separator, to account for the `:` in Windows drive letters (e.g. `C:\absolute\path.sock`) Fun fact: The old unix(7) man page (pre Jun 2016) stated a socket needs both read and write perms. Turns out, only write perms are needed. Corrected in mkerrisk/man-pages@7578ea2 Despite this, most implementations still default to read+write to this date.

mholt · 2023-06-22T15:09:25Z

Cool!! I will review this soon!

Dare we try to put this in 2.7?

emilylange · 2023-06-22T16:59:19Z

@ueffel friendly ping, if you happen to have some spare time to help to test this change on Windows, considering your work in #5114 :)

Would be very helpful and appreciated. Thank you!

ueffel · 2023-06-22T19:09:44Z

It seems to work fine on windows with relative and absolute paths.
For the record: write permission is sufficient on windows as well for the UDSes to work.

So the sockets do not work without write permission and they can also not be deleted by caddy when exiting if there is no write permission (obviously). Question is, if we want to protect against that. Meaning require at least write permissions on one of the permission bits or always on the owner bit? Just some food for thought.

Also I would like some test cases with windows paths. Meaning with backslashes and with an absolute path. Just to be safe.

mholt

Awesome! Is this good to go then?

Maybe we can sneak it in for 2.7 😶 Any objections?

emilylange · 2023-06-23T20:32:54Z

No objections from me :^)

jameshartig · 2023-06-23T20:46:55Z

Maybe we can sneak it in for 2.7 😶 Any objections?

Yay! This is currently a blocker for us rolling out Caddy more broadly internally so I would love for it to be in 2.7 😉

mholt · 2023-06-23T20:50:03Z

You got it. Thanks @emilylange for the great enhancement.

francislavoie added the feature ⚙️ New feature or request label Apr 28, 2022

francislavoie added this to the v2.6.0 milestone Apr 28, 2022

emilylange force-pushed the feature/unix-socket-perms branch 3 times, most recently from 8284a18 to db1cdc4 Compare April 30, 2022 00:15

mholt approved these changes Jul 6, 2022

View reviewed changes

listeners_test.go Outdated Show resolved Hide resolved

emilylange mentioned this pull request Jul 7, 2022

Allow one to specify file permissions for unix sockets #4675

Closed

mholt approved these changes Jul 7, 2022

View reviewed changes

emilylange mentioned this pull request Jul 12, 2022

Allow setting the owning group for Unix listeners #4869

Open

mholt modified the milestones: v2.6.0, v2.7.0 Sep 15, 2022

emilylange changed the title ~~core: Add optional unix socket file permissions~~ WIP: core: Add optional unix socket file permissions Jan 9, 2023

francislavoie marked this pull request as draft January 9, 2023 20:18

mholt modified the milestones: v2.7.0, v2.8.0 May 13, 2023

emilylange force-pushed the feature/unix-socket-perms branch from d5c2bb1 to 7ef6206 Compare June 22, 2023 14:38

emilylange marked this pull request as ready for review June 22, 2023 14:39

emilylange requested a review from mholt June 22, 2023 14:39

emilylange changed the title ~~WIP: core: Add optional unix socket file permissions~~ core: Add optional unix socket file permissions Jun 22, 2023

emilylange added 2 commits June 23, 2023 01:50

Add cases with Windows paths to test

5e0c45e

Require write perms for the owning user

49895e8

mholt approved these changes Jun 23, 2023

View reviewed changes

mholt merged commit 22927e2 into caddyserver:master Jun 23, 2023

mholt modified the milestones: v2.8.0, v2.7.0 Jun 23, 2023

emilylange deleted the feature/unix-socket-perms branch June 23, 2023 20:51

bones-was-here mentioned this pull request Aug 4, 2023

admin api unix socket permissions cause config reload to fail #5694

Closed

emilylange mentioned this pull request Nov 21, 2023

unix socket support matrix-org/sliding-sync#378

Merged

1 task

francislavoie mentioned this pull request May 13, 2024

logging: Allow setting log file permissions #6314

Merged

Uh oh!

core: Add optional unix socket file permissions #4741

core: Add optional unix socket file permissions #4741

Uh oh!

Conversation

emilylange commented Apr 28, 2022 • edited by francislavoie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mholt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sybereal commented Jul 7, 2022

Uh oh!

emilylange commented Jul 7, 2022

Footnotes

Uh oh!

mholt left a comment

Choose a reason for hiding this comment

Uh oh!

mholt commented Sep 13, 2022

Uh oh!

emilylange commented Sep 13, 2022

Uh oh!

mholt commented Sep 14, 2022

Uh oh!

francislavoie commented Jan 6, 2023

Uh oh!

emilylange commented Jan 9, 2023

Uh oh!

emilylange commented Feb 8, 2023

Uh oh!

mholt commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Saklad5 commented Feb 8, 2023

Uh oh!

jameshartig commented Apr 18, 2023

Uh oh!

emilylange commented Apr 18, 2023

Uh oh!

jameshartig commented Apr 18, 2023

Uh oh!

gi-yt commented May 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mholt commented May 20, 2023

Uh oh!

emilylange commented May 20, 2023

Uh oh!

francislavoie commented May 20, 2023

Uh oh!

jameshartig commented May 22, 2023

Uh oh!

mholt commented May 22, 2023

Uh oh!

mholt commented Jun 22, 2023

Uh oh!

emilylange commented Jun 22, 2023

Uh oh!

ueffel commented Jun 22, 2023

Uh oh!

mholt left a comment

Choose a reason for hiding this comment

Uh oh!

emilylange commented Jun 23, 2023

Uh oh!

jameshartig commented Jun 23, 2023

Uh oh!

mholt commented Jun 23, 2023

Uh oh!

Uh oh!

emilylange commented Apr 28, 2022 •

edited by francislavoie

Loading

CLAassistant commented Apr 28, 2022 •

edited

Loading

mholt commented Feb 8, 2023 •

edited

Loading

gi-yt commented May 20, 2023 •

edited

Loading