Add option to bind to a Unix socket instead of a TCP port #703
Conversation
Add a -socket option that configures the server to listen on a Unix-domain socket or Windows named pipe instead of a TCP port. This allows webhook to be used behind a reverse proxy on multi-tenant shared hosting without the need to choose (and the permission to bind to) a free port number. On Windows, -socket is expected to be a named pipe such as \\.\pipe\webhook, and the code uses https://github.com/microsoft/go-winio to bind the listening socket. On other platforms, -socket is the path to a Unix domain socket such as /tmp/webhook.sock, or an abstract socket name starting with @, bound using the regular net.Listen function with the "network" parameter set to "unix". Note: this pushes our minimum Go version up to 1.21 as that is what go-winio requires, but that is already the minimum version against which we are testing in the CI matrix.
Refactored webhook_test so that the test HTTP requests are made using an explicitly-provided http.Client, so we can run at least one test with the server bound to a socket instead of a port number, using an http.Client whose transport has been configured with a suitable Unix-domain or Windows named pipe dialer function.
This should ensure that, even if a developer or CI server has multiple versions of go installed, the version used to build the tools under test will be the same version that is running the test harness.
If webhook is restarted with the same settings but the socket file has not been deleted, webhook will be unable to bind and will exit with an error.
- README mentions the idea of using webhook behind a reverse proxy, including with the -socket flag - added a note in Hook-Rules that the ip-whitelist rule type does not work as expected behind a reverse proxy, and you should configure IP restrictions at the proxy level instead
| -http-methods string | ||
| globally restrict allowed HTTP methods; separate methods with comma | ||
| set default allowed HTTP methods (ie. "POST"); separate methods with comma |
There was a problem hiding this comment.
This isn't something I've changed, just something where the snapshot of the -help message in this file had drifted out of date with respect to the code.
| -socket string | ||
| path to a Unix socket (e.g. /tmp/webhook.sock) or Windows named pipe (e.g. \\.\pipe\webhook) to use instead of listening on an ip and port; if specified, the ip and port options are ignored |
There was a problem hiding this comment.
The actual -help message is different on different platforms, I manually merged the two for the documentation.
| @@ -43,6 +45,15 @@ func watchForSignals() { | |||
| log.Print(err) | |||
| } | |||
| } | |||
| if socket != "" && !strings.HasPrefix(socket, "@") { | |||
There was a problem hiding this comment.
If you give a socket path starting with @ to net.Listen("unix", path) then it is treated as an "abstract" socket, a separate namespace that doesn't have a direct representation as an object in the filesystem, in which case there's no socket file to remove.
| // we've been listening on a named Unix socket, delete it | ||
| // before we exit so subsequent runs can re-bind the same | ||
| // socket path | ||
| err := os.Remove(socket) |
There was a problem hiding this comment.
This will almost certainly error out in combination with -setuid, since the socket is created before dropping privileges. But you don't need to be root to bind a Unix socket (as long as you have write access to the parent directory) so this situation should rarely arise.
There was a problem hiding this comment.
Maybe we should do it after dropping the privileges then?
There was a problem hiding this comment.
I did wonder about either delaying opening the socket until after setuid or just forbidding the use of both -socket and -setuid at the same time. Like I say the only valid use case for combining -socket with -setuid would be if you wanted to bind to a socket file in a directory to which the target uid doesn't have write access, in which case moving the bind to happen after privileges have been dropped will fail the same way as this cleanup would in the current design.
My gut feeling is that the cleanest option is to make it an error if both -socket and -setuid are set. If you need to run webhook as a regular unprivileged user but listen on a socket as root, then I have another PR ready to go that stacks on top of this one that would make webhook compatible with systemd socket activation - that would mean that systemd itself manages the socket (as root, meaning it can be a Unix socket in a locked down directory or a TCP socket on a privileged port) and it can spawn webhook as an unprivileged user with the socket already open.
There was a problem hiding this comment.
Ah, ok, I was assuming if you agreed with my analysis then I’d implement that change before you merged… I’ll prepare the systemd PR and roll this fix into that.
There was a problem hiding this comment.
This looks like a big diff but it's largely whitespace - I've refactored the test function into a variable outside the inner for, which means it's pulled everything one tab to the left.
|
|
||
| cmd := exec.Command("go", "build", "-o", binPath, "test/hookecho.go") | ||
| gobin := filepath.Join(runtime.GOROOT(), "bin", "go") | ||
| cmd := exec.Command(gobin, "build", "-o", binPath, "test/hookecho.go") |
There was a problem hiding this comment.
I have several different go versions installed on my development machine, so I wanted to be sure the go that builds the test code was the same go that runs the tests.
|
Very nice PR! |
Summary
Adds a new command line flag
-socketas an alternative to-ipand-port, to cause webhook to bind a Unix domain socket instead of a TCP port, providing more flexibility for users who need to run webhook behind a reverse proxy.Motivation
Popular reverse proxy servers like Apache httpd and nginx have the ability to use a Unix domain socket as their upstream, instead of or alongside a regular TCP network connection. In particular, I have a use case where I want to use webhook on a multi-tenant shared hosting service that does not allow users to run their own services listening on TCP ports, but does offer the option to run services listening on a Unix socket and put those behind their reverse proxy. Even in cases where one could bind to a port number on localhost, using Unix sockets means there is no risk of accidental remote exposure of the listener, and different services on the same machine do not need to co-ordinate their port number choices - multiple users all running webhook cannot all use the default port 9000, but they could all use Unix sockets at
$HOME/www/webhook.sock.Implementation
I've added a new command line flag
-socket=/path/to/webhook.sockgiving the path to the desired socket. If-socketis provided,-ipand-portare ignored and webhook instead creates and binds a Unix socket at the given path. It is technically possible to combine-socketwith the TLS options but I'm not sure whether this combination would be supported by any downstream servers. The socket file is deleted when the process exits.Testing
I've refactored
TestWebhookso that after running the full suite of tests against the normal TCP server, it re-runs just one of them against a-socketserver instead of an-ip/-portone. If you prefer I could make it run every test twice, once against each server type, but that feels like it'd just slow the test suite down for no particular gain in safety.Windows notes
Unix sockets are not supported on Windows, but there is a similar concept there of "named pipes", that exist in a namespace
\\.\pipe\<name>. On Windows, the-socketparameter is interpreted as the path to a named pipe, and webhook acts as a server on that pipe.To support this I've had to add a dependency on https://github.com/microsoft/go-winio, which in turn means bumping the minimum supported go version of this project to 1.21, but I figured that wasn't a big issue given 1.21 is the earliest release that the CI workflows test on. However I'm not sure whether any reverse proxy implementations would actually be able to make use of named pipes so if you prefer I can just take the
-socketfeature out altogether on Windows, remove the extra dependency, and go back to a minimum of 1.20 (the current effective minimum version as required by gorilla/mux).