fix(proxy): return 502 when forwarding to upstream fails

proxy/proxy.go

@@ -380,6 +380,7 @@ func (p *Server) forwardRequest(conn net.Conn, req *http.Request, https bool, se
 	newReq, err := http.NewRequest(req.Method, targetURL.String(), body)
 	if err != nil {
 		p.logger.Error("can't create http request", "error", err)
+		p.writeBadGatewayResponse(conn, req, err)
 		return
 	}
 
@@ -403,6 +404,7 @@ func (p *Server) forwardRequest(conn net.Conn, req *http.Request, https bool, se
 	resp, err := client.Do(newReq)
 	if err != nil {
 		p.logger.Error("Failed to forward HTTPS request", "error", err)
+		p.writeBadGatewayResponse(conn, req, err)
 		return
 	}
 
@@ -418,6 +420,8 @@ func (p *Server) forwardRequest(conn net.Conn, req *http.Request, https bool, se
 	bodyBytes, err := io.ReadAll(resp.Body)
 	if err != nil {
 		p.logger.Error("can't read response body", "error", err)
+		_ = resp.Body.Close()
+		p.writeBadGatewayResponse(conn, req, err)
 		return
 	}
 	resp.Header.Add("Content-Length", strconv.Itoa(len(bodyBytes)))
@@ -503,6 +507,30 @@ For more help: https://github.com/coder/boundary
 	p.logger.Debug("Successfully wrote to connection")
 }
 
+// writeBadGatewayResponse tells the client that an allowed request could not
+// be forwarded. Every request needs a response: in a CONNECT tunnel the
+// client would otherwise wait for one while the proxy waits for the next
+// request.
+func (p *Server) writeBadGatewayResponse(conn net.Conn, req *http.Request, forwardErr error) {
+	body := fmt.Sprintf("Boundary could not forward the request to %s: %v\n", req.Host, forwardErr)
+
+	resp := &http.Response{
+		Status:        "502 Bad Gateway",
+		StatusCode:    http.StatusBadGateway,
+		Proto:         "HTTP/1.1",
+		ProtoMajor:    1,
+		ProtoMinor:    1,
+		Header:        make(http.Header),
+		Body:          io.NopCloser(strings.NewReader(body)),
+		ContentLength: int64(len(body)),
+	}
+	resp.Header.Set("Content-Type", "text/plain")
+
+	if err := resp.Write(conn); err != nil {
+		p.logger.Error("Failed to write bad gateway response", "error", err)
+	}
+}
+
 // connectionWrapper lets us "unread" the peeked byte
 type connectionWrapper struct {
 	net.Conn

proxy/proxy_upstream_error_test.go

@@ -0,0 +1,89 @@
+package proxy
+
+import (
+	"io"
+	"net"
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+// unreachableAddr returns a loopback address that refuses connections.
+func unreachableAddr(t *testing.T) string {
+	t.Helper()
+
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	addr := ln.Addr().String()
+	require.NoError(t, ln.Close())
+	return addr
+}
+
+// TestProxyServerUpstreamUnreachable verifies that an allowed request whose
+// upstream cannot be reached gets a 502 Bad Gateway instead of an empty
+// reply or a hung connection.
+func TestProxyServerUpstreamUnreachable(t *testing.T) {
+	addr := unreachableAddr(t)
+
+	pt := NewProxyTest(t,
+		WithCertManager(t.TempDir()),
+		WithAllowedDomain("127.0.0.1"),
+	).Start()
+	defer pt.Stop()
+
+	expectBadGateway := func(t *testing.T, client *http.Client, url, host string) {
+		t.Helper()
+
+		req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, url, nil)
+		require.NoError(t, err)
+		req.Host = host
+
+		resp, err := client.Do(req)
+		require.NoError(t, err)
+		defer resp.Body.Close() //nolint:errcheck
+
+		require.Equal(t, http.StatusBadGateway, resp.StatusCode)
+		body, err := io.ReadAll(resp.Body)
+		require.NoError(t, err)
+		require.Contains(t, string(body), addr)
+	}
+
+	t.Run("TransparentHTTP", func(t *testing.T) {
+		expectBadGateway(t, pt.client, "http://localhost:8080/", addr)
+	})
+
+	t.Run("TransparentHTTPS", func(t *testing.T) {
+		expectBadGateway(t, pt.client, "https://localhost:8080/", addr)
+	})
+
+	t.Run("ExplicitProxyHTTP", func(t *testing.T) {
+		expectBadGateway(t, pt.proxyClient, "http://"+addr+"/", "")
+	})
+
+	// The CONNECT tunnel serves many requests on one connection, so a
+	// failed forward must still produce a response. Otherwise the client
+	// waits for it while the proxy waits for the next request.
+	t.Run("CONNECTTunnel", func(t *testing.T) {
+		// The CONNECT target only sets the TLS server name, which must be a
+		// hostname. The Host header below picks the upstream.
+		tunnel, err := pt.establishExplicitCONNECT("localhost:443")
+		require.NoError(t, err)
+		defer tunnel.close() //nolint:errcheck
+		require.NoError(t, tunnel.tlsConn.SetDeadline(time.Now().Add(5*time.Second)))
+
+		// Two requests show that the tunnel stays usable after a failure.
+		for range 2 {
+			_, err := tunnel.tlsConn.Write([]byte("GET / HTTP/1.1\r\nHost: " + addr + "\r\n\r\n"))
+			require.NoError(t, err)
+
+			resp, err := http.ReadResponse(tunnel.reader, nil)
+			require.NoError(t, err)
+			_, err = io.Copy(io.Discard, resp.Body)
+			require.NoError(t, err)
+			require.NoError(t, resp.Body.Close())
+			require.Equal(t, http.StatusBadGateway, resp.StatusCode)
+		}
+	})
+}