go-connections
go-connections copied to clipboard
docker
Implement AF_UNIX sockets on Windows
Gave CI a kick after the last push; failure is in TestUnixSocketWithOpts, but also curious if TestNewUnixSocket needs a change in constraints (currently gated by "must be root", which may need a slightly different check on Windows likely)
=== RUN TestNewUnixSocket
unix_socket_test.go:39: requires root
--- SKIP: TestNewUnixSocket (0.00s)
=== RUN TestUnixSocketWithOpts
unix_socket_test_windows.go:11: The process cannot access the file because it is being used by another process.
Also look like staticcheck is complaining because on Windows it's a stub, which, well, is the intent here 🤔
I guess a //nolint:staticcheck // Ignore SA4017, which is a stub on Windows, and has no side-effects could work
Alternatively, we could extract the whole block and have a separate Windows and Linux implementation / wrapper for l, err := net.Listen("unix", path) (on Windows skipping the umask)
That whole umask is hairy in either case 😬
That whole umask is hairy in either case
Yes, feels like a race condition waiting to be exploited.
Okay, I think I'm done here.
Alternatively, we could extract the whole block and have a separate Windows and Linux implementation / wrapper
Yep, I went this route.
@slonopotamus Could you sign-off your commit? The DCO check is failing.
Oh! It's the "accept suggestions" option on GitHub. That one is always a pain, because it's too easy to just "click the button", and now having an incorrect commit.
I can do a quick rebase and squash that one into the first commit.
I think this PR should be OK to merge, BUT will need some follow-ups. Some quick blurbs;
- There was discussion around this PR, and whether we should bring it in, knowing that the
go-connectionspackage as a whole needs to be re-evaluated; it originated from very far in the history of the Docker "project", not very well designed, and has quite some bad parts. - Then again; it's what we currently have, and while we should look at the overall issue, I don't think merging this PR brings us in a worse state.
- The
umaskmonstrosity is existing code (we should explore if we can find alternatives and/or propose changes in Go's stdlib or golang.org/x/sys), but this PR only moves that code, so no change there - AFAICS, no existing code is impacted (only moved), so should have no consequences for existing use.
That said (as mentioned) there's some bits that must be addressed before we tag a new release;
- First of all, verify this code in codebase(s) that use it; we should probably hold-off tagging a release before that's all verified (there may be ugly corners hiding)
- :warning: given that
go-connectionsis used to (among others) create the/var/run/docker.socksocket, access to that socket is important. Currently, there's code to set permissions, which I think are currently silently ignored on Windows; we must make sure that these functions;- have alternatives / equivalents on Windows
- produces an error for those that cannot be supported as-is on Windows; setting permissions (and ownership) should not be silent
https://github.com/docker/go-connections/blob/5df8d2b30ca886f2d94740ce3c54abd58a5bb2c9/sockets/unix_socket.go#L60-L78
I believe on Windows the for the created socket would follow the rest how filesystem permissions work there, i.e. hierarchical depending on the directory it's in. But maybe someone more knowledgeable would know better 😅
We should probably look at what we do for other parts /var/lib/docker and the named pipe. I think we set a docker group on that on Windows, but it could be SysAdmin or something along those lines.
A quick follow-up should be to make the existing functions (for now) return an error if they cannot set permissions (so that it's not silently ignored).
I went to look a bit into this, and from the Microsoft post about AF_UNIX support on Windows:
Communication over unix sockets can be secured by controlling the file (or directory) permissions on the pathname sockets (or the parent directory). For example, the bind socket API creates a ‘socket’ file with the given pathname. The creation of the new socket file will fail if the calling process does not has write permission on the directory where the file is being created. Similarly, for connecting to a stream socket, the connecting process should have write permission on the socket. The same level of security is available and enforced on the Windows unix socket implementation.
We're not ignoring any errors from that (on the go-connections side anyway), and are setting file ownership/permissions.
However, umask isn't a supported syscall on windows, and beyond that, the os.Chmod docs state that:
On Windows, only the 0o200 bit (owner writable) of mode is used; it controls whether the file's read-only attribute is set or cleared. The other bits are currently unused. For compatibility with Go 1.12 and earlier, use a non-zero mode. Use mode 0o400 for a read-only file and 0o600 for a readable+writable file.
Given that, I guess we're dependant on the hierarchical directory permissions on Windows from preventing other users from accessing the socket.
We're not ignoring any errors from that (on the go-connections side anyway), and are setting file ownership/permissions.
Yeah, if memory serves me well, some of those functions in Go stdlib are not implemented on Windows, and silently discarded (uid/gid being ignored). But we should check for sure.
Yup! I had written some of that in that reply:
the os.Chmod docs state that:
On Windows, only the 0o200 bit (owner writable) of mode is used; it controls whether the file's read-only attribute is set or cleared. The other bits are currently unused. For compatibility with Go 1.12 and earlier, use a non-zero mode. Use mode 0o400 for a read-only file and 0o600 for a readable+writable file.