caddy icon indicating copy to clipboard operation
caddy copied to clipboard

core: Add optional unix socket file permissions

Open emilylange opened this issue 3 years ago • 6 comments

Closes #4675

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

emilylange avatar Apr 28 '22 06:04 emilylange

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 28 '22 06:04 CLAassistant

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.

sybereal avatar Jul 07 '22 15:07 sybereal

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!

emilylange avatar Jul 07 '22 17:07 emilylange

@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?

mholt avatar Sep 13 '22 22:09 mholt

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

emilylange avatar Sep 13 '22 23:09 emilylange

@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!

mholt avatar Sep 14 '22 03:09 mholt

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

francislavoie avatar Jan 06 '23 20:01 francislavoie

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 avatar Jan 09 '23 19:01 emilylange

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 :^)

emilylange avatar Feb 08 '23 17:02 emilylange

Ah yeah :confused: Sorry about that... thanks, Windows.

mholt avatar Feb 08 '23 17:02 mholt

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

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

Saklad5 avatar Feb 08 '23 17:02 Saklad5

@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 avatar Apr 18 '23 07:04 jameshartig

@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?

emilylange avatar Apr 18 '23 16:04 emilylange

@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.

jameshartig avatar Apr 18 '23 16:04 jameshartig

@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.

gi-yt avatar May 20 '23 03:05 gi-yt

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?

mholt avatar May 20 '23 05:05 mholt

@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 :)

emilylange avatar May 20 '23 11:05 emilylange

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

francislavoie avatar May 20 '23 12:05 francislavoie

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.

jameshartig avatar May 22 '23 12:05 jameshartig

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

mholt avatar May 22 '23 15:05 mholt

Cool!! I will review this soon!

Dare we try to put this in 2.7?

mholt avatar Jun 22 '23 15:06 mholt

@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!

emilylange avatar Jun 22 '23 16:06 emilylange

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.

ueffel avatar Jun 22 '23 19:06 ueffel

No objections from me :^)

emilylange avatar Jun 23 '23 20:06 emilylange

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 😉

jameshartig avatar Jun 23 '23 20:06 jameshartig

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

mholt avatar Jun 23 '23 20:06 mholt