caddy
caddy copied to clipboard
core: Add optional unix socket file permissions
Closes #4675
Allows one to specify the wanted socket permissions like so:
unix//path/to/unix.sock:0666(unix socket at/path/to/unix.sockwith0666(ugw=rw) The leading0is optional.unix//path/to/unix.sock:666would yield the same permissions. Using:u=rw,r=rw,=o=rwwill 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 :thinking:
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:information_source: 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 :woman_shrugging:
An addition integration test might make sense, since the actual os.Chmod() only happens at runtime.
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.
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 :eyes:
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^1
Connecting to the socket object requires read/write permission.
while others didn't.^3
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:
https://github.com/mkerrisk/man-pages/commit/b1ef409dc7dd412e459bc120ddd31faf5ce374da (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 https://github.com/mkerrisk/man-pages/commit/7578ea2f85b272363d22680d69e7d32f0b59c83b
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 :smile:
Thanks @sybereal!
@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?
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 :eyes:
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 :)
@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 :no_mouth: haha. :+1:
Thank you for working on this!
@IndeedNotJames I think we can probably merge this soon, could you fix the conflicts?
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
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 :^)
Ah yeah :confused: Sorry about that... thanks, Windows.
Ah yeah 😕 Sorry about that... thanks, Windows.
This is not the first time someone has said that, and it will not be the last.
@IndeedNotJames any time to wrap this up in the coming months? Anything I can help with? Is the Windows parsing the last outstanding issue?
@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?
@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.
@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.
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?
@gi-yt FYI a better workaround (for now) would be to modify the systemd service's UMask.
My comment in https://github.com/caddyserver/caddy/issues/4675#issuecomment-1176977921 partially applies if you need a pointer on how that could be done :)
We talked on slack about using | as a separator for the path|bits|user:group as syntax
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.
Oops, yeah... I'm going blind. Thanks for highlighting that.
Cool!! I will review this soon!
Dare we try to put this in 2.7?
@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!
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.
No objections from me :^)
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 😉
You got it. Thanks @emilylange for the great enhancement.