Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to bind to a Unix socket instead of a TCP port #703

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ianroberts
Copy link

Summary

Adds a new command line flag -socket as an alternative to -ip and -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.sock giving the path to the desired socket. If -socket is provided, -ip and -port are ignored and webhook instead creates and binds a Unix socket at the given path. It is technically possible to combine -socket with 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 TestWebhook so that after running the full suite of tests against the normal TCP server, it re-runs just one of them against a -socket server instead of an -ip/-port one. 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 -socket parameter 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 -socket feature 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).

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
Comment on lines 16 to +17
-http-methods string
globally restrict allowed HTTP methods; separate methods with comma
set default allowed HTTP methods (ie. "POST"); separate methods with comma
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +40 to +41
-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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, "@") {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +49 to +52
// 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)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 216 to +218

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")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant