Allow restricting redirect destinations (rakyll#97)

The `/redirect-to` endpoint currently acts as an open redirect, which is
bad for any go-httpbin instance exposed to the public internet.

This allows configuring an allowlist of domains to which traffic can be
redirected.

README.md

@@ -11,24 +11,17 @@ A reasonably complete and well-tested golang port of [Kenneth Reitz][kr]'s
 
 ## Usage
 
+### Docker
 
-### Configuration
-
-go-httpbin can be configured via either command line arguments or environment
-variables (or a combination of the two):
-
-| Argument| Env var | Documentation | Default |
-| - | - | - | - |
-| `-host` | `HOST` | Host to listen on | "0.0.0.0" |
-| `-https-cert-file` | `HTTPS_CERT_FILE` | HTTPS Server certificate file | |
-| `-https-key-file` | `HTTPS_KEY_FILE` | HTTPS Server private key file | |
-| `-max-body-size` | `MAX_BODY_SIZE` | Maximum size of request or response, in bytes | 1048576 |
-| `-max-duration` | `MAX_DURATION` | Maximum duration a response may take | 10s |
-| `-port` | `PORT` | Port to listen on | 8080 |
-| `-use-real-hostname` | `USE_REAL_HOSTNAME` | Expose real hostname as reported by os.Hostname() in the /hostname endpoint | false |
+Docker images are published to [Docker Hub][docker-hub]:
 
-**Note:** Command line arguments take precedence over environment variables.
+```bash
+# Run http server
+$ docker run -P mccutchen/go-httpbin
 
+# Run https server
+$ docker run -e HTTPS_CERT_FILE='/tmp/server.crt' -e HTTPS_KEY_FILE='/tmp/server.key' -p 8080:8080 -v /tmp:/tmp mccutchen/go-httpbin
+```
 
 ### Standalone binary
 
@@ -48,18 +41,6 @@ $ openssl req -new -x509 -sha256 -key server.key -out server.crt -days 3650
 $ go-httpbin -host 127.0.0.1 -port 8081 -https-cert-file ./server.crt -https-key-file ./server.key
 ```
 
-### Docker
-
-Docker images are published to [Docker Hub][docker-hub]:
-
-```bash
-# Run http server
-$ docker run -P mccutchen/go-httpbin
-
-# Run https server
-$ docker run -e HTTPS_CERT_FILE='/tmp/server.crt' -e HTTPS_KEY_FILE='/tmp/server.key' -p 8080:8080 -v /tmp:/tmp mccutchen/go-httpbin
-```
-
 ### Unit testing helper library
 
 The `github.com/mccutchen/go-httpbin/httpbin/v2` package can also be used as a
@@ -95,16 +76,26 @@ func TestSlowResponse(t *testing.T) {
 }
 ```
 
+### Configuration
 
-## Custom instrumentation
+go-httpbin can be configured via either command line arguments or environment
+variables (or a combination of the two):
 
-If you're running go-httpbin in your own infrastructure and would like custom
-instrumentation (metrics, structured logging, request tracing, etc), you'll
-need to wrap this package in your own code and use the included
-[Observer][observer] mechanism to instrument requests as necessary.
+| Argument| Env var | Documentation | Default |
+| - | - | - | - |
+| `-allowed-redirect-domains` | `ALLOWED_REDIRECT_DOMAINS` | Comma-separated list of domains the /redirect-to endpoint will allow | |
+| `-host` | `HOST` | Host to listen on | "0.0.0.0" |
+| `-https-cert-file` | `HTTPS_CERT_FILE` | HTTPS Server certificate file | |
+| `-https-key-file` | `HTTPS_KEY_FILE` | HTTPS Server private key file | |
+| `-max-body-size` | `MAX_BODY_SIZE` | Maximum size of request or response, in bytes | 1048576 |
+| `-max-duration` | `MAX_DURATION` | Maximum duration a response may take | 10s |
+| `-port` | `PORT` | Port to listen on | 8080 |
+| `-use-real-hostname` | `USE_REAL_HOSTNAME` | Expose real hostname as reported by os.Hostname() in the /hostname endpoint | false |
 
-See [examples/custom-instrumentation][custom-instrumentation] for an example
-that instruments every request using DataDog.
+**Notes:**
+- Command line arguments take precedence over environment variables.
+- See [Production considerations] for recommendations around safe configuration
+  of public instances of go-httpbin
 
 
 ## Installation
@@ -122,6 +113,66 @@ go install github.com/mccutchen/go-httpbin/v2/cmd/go-httpbin
 ```
 
 
+## Production considerations
+
+Before deploying an instance of go-httpbin on your own infrastructure on the
+public internet, consider tuning it appropriately:
+
+1. **Restrict the domains to which the `/redirect-to` endpoint will send
+   traffic to avoid the security issues of an open redirect**
+
+   Use the `-allowed-redirect-domains` CLI argument or the
+   `ALLOWED_REDIRECT_DOMAINS` env var to configure an appropriate allowlist.
+
+2. **Tune per-request limits**
+
+   Because go-httpbin allows clients send arbitrary data in request bodies and
+   control the duration some requests (e.g. `/delay/60s`), it's important to
+   properly tune limits to prevent misbehaving or malicious clients from taking
+   too many resources.
+
+   Use the `-max-body-size`/`MAX_BODY_SIZE` and `-max-duration`/`MAX_DURATION`
+   CLI arguments or env vars to enforce appropriate limits on each request.
+
+3. **Decide whether to expose real hostnames in the `/hostname` endpoint**
+
+   By default, the `/hostname` endpoint serves a dummy hostname value, but it
+   can be configured to serve the real underlying hostname (according to
+   `os.Hostname()`) using the `-use-real-hostname` CLI argument or the
+   `USE_REAL_HOSTNAME` env var to enable this functionality.
+
+   Before enabling this, ensure that your hostnames do not reveal too much
+   about your underlying infrastructure.
+
+4. **Add custom instrumentation**
+
+   By default, go-httpbin logs basic information about each request. To add
+   more detailed instrumentation (metrics, structured logging, request
+   tracing), you'll need to wrap this package in your own code, which you can
+   then instrument as you would any net/http server. Some examples:
+
+   - [examples/custom-instrumentation] instruments every request using DataDog,
+     based on the built-in [Observer] mechanism.
+
+   - [mccutchen/httpbingo.org] is the code that powers the public instance of
+     go-httpbin deployed to [httpbingo.org], which adds customized structured
+     logging using [zerolog] and further hardens the HTTP server against
+     malicious clients by tuning lower-level timeouts and limits.
+
+## Development
+
+```bash
+# local development
+make
+make test
+make testcover
+make run
+
+# building & pushing docker images
+make image
+make imagepush
+```
+
 ## Motivation & prior art
 
 I've been a longtime user of [Kenneith Reitz][kr]'s original
@@ -148,24 +199,14 @@ Compared to [ahmetb/go-httpbin][ahmet]:
  - More complete implementation of endpoints
 
 
-## Development
-
-```bash
-# local development
-make
-make test
-make testcover
-make run
-
-# building & pushing docker images
-make image
-make imagepush
-```
-
-[kr]: https://github.com/kennethreitz
-[httpbin-org]: https://httpbin.org/
-[httpbin-repo]: https://github.com/kennethreitz/httpbin
 [ahmet]: https://github.com/ahmetb/go-httpbin
 [docker-hub]: https://hub.docker.com/r/mccutchen/go-httpbin/
-[observer]: https://pkg.go.dev/github.com/mccutchen/go-httpbin/v2/httpbin#Observer
-[custom-instrumentation]: ./examples/custom-instrumentation/
+[examples/custom-instrumentation]: ./examples/custom-instrumentation/
+[httpbin-org]: https://httpbin.org/
+[httpbin-repo]: https://github.com/kennethreitz/httpbin
+[httpbingo.org]: https://httpbingo.org/
+[kr]: https://github.com/kennethreitz
+[mccutchen/httpbingo.org]: https://github.com/mccutchen/httpbingo.org
+[Observer]: https://pkg.go.dev/github.com/mccutchen/go-httpbin/v2/httpbin#Observer
+[Production considerations]: #production-considerations
+[zerolog]: https://github.com/rs/zerolog

cmd/go-httpbin/main.go

@@ -10,6 +10,7 @@ import (
 	"os"
 	"os/signal"
 	"strconv"
+	"strings"
 	"syscall"
 	"time"
 
@@ -22,23 +23,25 @@ const (
 )
 
 var (
-	host            string
-	port            int
-	maxBodySize     int64
-	maxDuration     time.Duration
-	httpsCertFile   string
-	httpsKeyFile    string
-	useRealHostname bool
+	allowedRedirectDomains string
+	host                   string
+	httpsCertFile          string
+	httpsKeyFile           string
+	maxBodySize            int64
+	maxDuration            time.Duration
+	port                   int
+	useRealHostname        bool
 )
 
 func main() {
-	flag.StringVar(&host, "host", defaultHost, "Host to listen on")
+	flag.BoolVar(&useRealHostname, "use-real-hostname", false, "Expose value of os.Hostname() in the /hostname endpoint instead of dummy value")
+	flag.DurationVar(&maxDuration, "max-duration", httpbin.DefaultMaxDuration, "Maximum duration a response may take")
+	flag.Int64Var(&maxBodySize, "max-body-size", httpbin.DefaultMaxBodySize, "Maximum size of request or response, in bytes")
 	flag.IntVar(&port, "port", defaultPort, "Port to listen on")
+	flag.StringVar(&allowedRedirectDomains, "allowed-redirect-domains", "", "Comma-separated list of domains the /redirect-to endpoint will allow")
+	flag.StringVar(&host, "host", defaultHost, "Host to listen on")
 	flag.StringVar(&httpsCertFile, "https-cert-file", "", "HTTPS Server certificate file")
 	flag.StringVar(&httpsKeyFile, "https-key-file", "", "HTTPS Server private key file")
-	flag.Int64Var(&maxBodySize, "max-body-size", httpbin.DefaultMaxBodySize, "Maximum size of request or response, in bytes")
-	flag.DurationVar(&maxDuration, "max-duration", httpbin.DefaultMaxDuration, "Maximum duration a response may take")
-	flag.BoolVar(&useRealHostname, "use-real-hostname", false, "Expose value of os.Hostname() in the /hostname endpoint instead of dummy value")
 	flag.Parse()
 
 	// Command line flags take precedence over environment vars, so we only
@@ -97,6 +100,16 @@ func main() {
 		useRealHostname = true
 	}
 
+	var allowedRedirectDomainsList []string
+	if allowedRedirectDomains == "" && os.Getenv("ALLOWED_REDIRECT_DOMAINS") != "" {
+		allowedRedirectDomains = os.Getenv("ALLOWED_REDIRECT_DOMAINS")
+	}
+	for _, domain := range strings.Split(allowedRedirectDomains, ",") {
+		if strings.TrimSpace(domain) != "" {
+			allowedRedirectDomainsList = append(allowedRedirectDomainsList, strings.TrimSpace(domain))
+		}
+	}
+
 	logger := log.New(os.Stderr, "", 0)
 
 	// A hacky log helper function to ensure that shutdown messages are
@@ -123,6 +136,9 @@ func main() {
 		}
 		opts = append(opts, httpbin.WithHostname(hostname))
 	}
+	if len(allowedRedirectDomainsList) > 0 {
+		opts = append(opts, httpbin.WithAllowedRedirectDomains(allowedRedirectDomainsList))
+	}
 	h := httpbin.New(opts...)
 
 	listenAddr := net.JoinHostPort(host, strconv.Itoa(port))

httpbin/handlers.go

@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"net/url"
 	"strconv"
 	"strings"
 	"time"
@@ -375,13 +376,25 @@ func (h *HTTPBin) AbsoluteRedirect(w http.ResponseWriter, r *http.Request) {
 func (h *HTTPBin) RedirectTo(w http.ResponseWriter, r *http.Request) {
 	q := r.URL.Query()
 
-	url := q.Get("url")
-	if url == "" {
+	inputURL := q.Get("url")
+	if inputURL == "" {
 		http.Error(w, "Missing URL", http.StatusBadRequest)
 		return
 	}
 
-	var err error
+	u, err := url.Parse(inputURL)
+	if err != nil {
+		http.Error(w, "Invalid URL", http.StatusBadRequest)
+		return
+	}
+
+	if u.IsAbs() && len(h.AllowedRedirectDomains) > 0 {
+		if _, ok := h.AllowedRedirectDomains[u.Hostname()]; !ok {
+			http.Error(w, "Forbidden redirect URL. Be careful with this link.", http.StatusForbidden)
+			return
+		}
+	}
+
 	statusCode := http.StatusFound
 	rawStatusCode := q.Get("status_code")
 	if rawStatusCode != "" {
@@ -392,7 +405,7 @@ func (h *HTTPBin) RedirectTo(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	w.Header().Set("Location", url)
+	w.Header().Set("Location", u.String())
 	w.WriteHeader(statusCode)
 }
 

httpbin/handlers_test.go

@@ -1218,9 +1218,11 @@ func TestRedirectTo(t *testing.T) {
 		url            string
 		expectedStatus int
 	}{
-		{"/redirect-to", http.StatusBadRequest},
-		{"/redirect-to?status_code=302", http.StatusBadRequest},
-		{"/redirect-to?url=foo&status_code=418", http.StatusBadRequest},
+		{"/redirect-to", http.StatusBadRequest},                                               // missing url
+		{"/redirect-to?status_code=302", http.StatusBadRequest},                               // missing url
+		{"/redirect-to?url=foo&status_code=201", http.StatusBadRequest},                       // invalid status code
+		{"/redirect-to?url=foo&status_code=418", http.StatusBadRequest},                       // invalid status code
+		{"/redirect-to?url=http%3A%2F%2Ffoo%25%25bar&status_code=418", http.StatusBadRequest}, // invalid URL
 	}
 	for _, test := range badTests {
 		test := test
@@ -1233,6 +1235,32 @@ func TestRedirectTo(t *testing.T) {
 			assertStatusCode(t, w, test.expectedStatus)
 		})
 	}
+
+	allowListHandler := New(
+		WithAllowedRedirectDomains([]string{"httpbingo.org", "example.org"}),
+		WithObserver(StdLogObserver(log.New(io.Discard, "", 0))),
+	).Handler()
+
+	allowListTests := []struct {
+		url            string
+		expectedStatus int
+	}{
+		{"/redirect-to?url=http://httpbingo.org", http.StatusFound},                // allowlist ok
+		{"/redirect-to?url=https://httpbingo.org", http.StatusFound},               // scheme doesn't matter
+		{"/redirect-to?url=https://example.org/foo/bar", http.StatusFound},         // paths don't matter
+		{"/redirect-to?url=https://foo.example.org/foo/bar", http.StatusForbidden}, // subdomains of allowed domains do not match
+		{"/redirect-to?url=https://evil.com", http.StatusForbidden},                // not in allowlist
+	}
+	for _, test := range allowListTests {
+		test := test
+		t.Run("allowlist"+test.url, func(t *testing.T) {
+			t.Parallel()
+			r, _ := http.NewRequest("GET", test.url, nil)
+			w := httptest.NewRecorder()
+			allowListHandler.ServeHTTP(w, r)
+			assertStatusCode(t, w, test.expectedStatus)
+		})
+	}
 }
 
 func TestCookies(t *testing.T) {

httpbin/httpbin.go

@@ -101,6 +101,9 @@ type HTTPBin struct {
 	// Default parameter values
 	DefaultParams DefaultParams
 
+	// Set of hosts to which the /redirect-to endpoint will allow redirects
+	AllowedRedirectDomains map[string]struct{}
+
 	// The hostname to expose via /hostname.
 	hostname string
 }
@@ -274,3 +277,15 @@ func WithObserver(o Observer) OptionFunc {
 		h.Observer = o
 	}
 }
+
+// WithAllowedRedirectDomains limits the domains to which the /redirect-to
+// endpoint will redirect traffic.
+func WithAllowedRedirectDomains(hosts []string) OptionFunc {
+	return func(h *HTTPBin) {
+		hostSet := make(map[string]struct{}, len(hosts))
+		for _, host := range hosts {
+			hostSet[host] = struct{}{}
+		}
+		h.AllowedRedirectDomains = hostSet
+	}
+}