VAULT-6803: fix listener issue if using proxy_protocol_behavior with deny_unauthorized for untrusted upstream connections (#27589)

* timeout 'testListenerConnFn' waiting on the server connection after 3 secs * return the invalid upstream error so the library knows not to stop listening/serving * update go-proxyproto to use fork/tag * test that fails before library and code update, but passes afterwards
2025-10-29 09:42:25 +00:00 · 2024-06-26 08:52:13 +01:00
parent aa828f1e29
commit 89276a56b2
6 changed files with 90 additions and 8 deletions
--- a/changelog/27589.txt
+++ b/changelog/27589.txt
@@ -0,0 +1,4 @@
+```release-note:bug
+core/config: fix issue when using `proxy_protocol_behavior` with `deny_unauthorized`, 
+which causes the Vault TCP listener to close after receiving an untrusted upstream proxy connection.
+```
--- a/command/server/listener_tcp_test.go
+++ b/command/server/listener_tcp_test.go
@@ -432,3 +432,73 @@ func TestTCPListener_proxyProtocol(t *testing.T) {
 		})
 	}
 }
+
+// TestTCPListener_proxyProtocol_keepAcceptingOnInvalidUpstream ensures that the server side listener
+// never returns an error from the listener.Accept method if the error is that the
+// upstream proxy isn't trusted. If an error is returned, underlying Go HTTP native
+// libraries may close down a server and stop listening.
+func TestTCPListener_proxyProtocol_keepAcceptingOnInvalidUpstream(t *testing.T) {
+	timeout := 3 * time.Second
+
+	// Configure proxy so we hit the deny unauthorized behavior.
+	header := &proxyproto.Header{
+		Version:           1,
+		Command:           proxyproto.PROXY,
+		TransportProtocol: proxyproto.TCPv4,
+		SourceAddr: &net.TCPAddr{
+			IP:   net.ParseIP("10.1.1.1"),
+			Port: 1000,
+		},
+		DestinationAddr: &net.TCPAddr{
+			IP:   net.ParseIP("20.2.2.2"),
+			Port: 2000,
+		},
+	}
+
+	var authAddrs []*sockaddr.SockAddrMarshaler
+	sockAddr, err := sockaddr.NewSockAddr("10.0.0.1/32")
+	require.NoError(t, err)
+	authAddrs = append(authAddrs, &sockaddr.SockAddrMarshaler{SockAddr: sockAddr})
+
+	ln, _, _, err := tcpListenerFactory(&configutil.Listener{
+		Address:                      "127.0.0.1:0",
+		TLSDisable:                   true,
+		ProxyProtocolBehavior:        "deny_unauthorized",
+		ProxyProtocolAuthorizedAddrs: authAddrs,
+	}, nil, cli.NewMockUi())
+	require.NoError(t, err)
+
+	// Kick off setting up server side, if we ever accept a connection send it out
+	// via a channel.
+	serverConnCh := make(chan net.Conn, 1)
+	go func() {
+		serverConn, err := ln.Accept()
+		// We shouldn't ever have an error if the problem was only that the upstream
+		// proxy wasn't trusted.
+		// An error would lead to the http.Serve closing the listener and giving up.
+		require.NoError(t, err, "server side listener errored")
+		serverConnCh <- serverConn
+	}()
+
+	// Now try to connect as the client.
+	d := net.Dialer{Timeout: timeout}
+	clientConn, err := d.Dial("tcp", ln.Addr().String())
+	require.NoError(t, err)
+	defer clientConn.Close()
+	_, err = header.WriteTo(clientConn)
+	require.NoError(t, err)
+
+	// Wait for the server to have accepted a connection, or we time out.
+	select {
+	case <-time.After(timeout):
+		// The server still hasn't accepted any valid client connection.
+		// Try to write another header using the same connection which should have
+		// been closed by the server, we expect that this client side connection was
+		// closed as it us untrusted,
+		_, err = header.WriteTo(clientConn)
+		require.Error(t, err, "reused a rejected connection without error")
+	case serverConn := <-serverConnCh:
+		require.NotNil(t, serverConn)
+		defer serverConn.Close()
+	}
+}
--- a/command/server/listener_test.go
+++ b/command/server/listener_test.go
@@ -9,6 +9,7 @@ import (
 	"io"
 	"net"
 	"testing"
+	"time"
 )

 type testListenerConnFn func(net.Listener) (net.Conn, error)
@@ -60,7 +61,11 @@ func testListenerImpl(t *testing.T, ln net.Listener, connFn testListenerConnFn,
 		}
 	}

-	server := <-serverCh
+	var server net.Conn
+	select {
+	case <-time.After(3 * time.Second):
+	case server = <-serverCh:
+	}

 	if server == nil {
 		if !expectError {
--- a/go.mod
+++ b/go.mod
@@ -187,7 +187,7 @@ require (
 	github.com/ory/dockertest v3.3.5+incompatible
 	github.com/ory/dockertest/v3 v3.10.0
 	github.com/patrickmn/go-cache v2.1.0+incompatible
-	github.com/pires/go-proxyproto v0.7.0
+	github.com/pires/go-proxyproto v1.0.0
 	github.com/pkg/errors v0.9.1
 	github.com/posener/complete v1.2.3
 	github.com/pquerna/otp v1.2.1-0.20191009055518-468c2dd2b58d
@@ -542,3 +542,7 @@ require (
 )

 replace github.com/ma314smith/signedxml v1.1.1 => github.com/moov-io/signedxml v1.1.1
+
+// Support using the forked repository until https://github.com/pires/go-proxyproto/pull/110 merges
+// and is released.
+replace github.com/pires/go-proxyproto v1.0.0 => github.com/peteski22/go-proxyproto v1.0.0
--- a/go.sum
+++ b/go.sum
@@ -1980,6 +1980,8 @@ github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/9
 github.com/peterbourgon/diskv v2.0.1+incompatible/go.mod h1:uqqh8zWWbv1HBMNONnaR/tNboyR3/BZd58JJSHlUSCU=
 github.com/petermattis/goid v0.0.0-20180202154549-b0b1615b78e5 h1:q2e307iGHPdTGp0hoxKjt1H5pDo6utceo3dQVK3I5XQ=
 github.com/petermattis/goid v0.0.0-20180202154549-b0b1615b78e5/go.mod h1:jvVRKCrJTQWu0XVbaOlby/2lO20uSCHEMzzplHXte1o=
+github.com/peteski22/go-proxyproto v1.0.0 h1:838NKdKEeViAMkaz08Pe+lvvAnGLYhZ0M0z246iCYv0=
+github.com/peteski22/go-proxyproto v1.0.0/go.mod h1:iknsfgnH8EkjrMeMyvfKByp9TiBZCKZM0jx2xmKqnVY=
 github.com/phpdave11/gofpdf v1.4.2/go.mod h1:zpO6xFn9yxo3YLyMvW8HcKWVdbNqgIfOOp2dXMnm1mY=
 github.com/phpdave11/gofpdi v1.0.12/go.mod h1:vBmVV0Do6hSBHC8uKUQ71JGW+ZGQq74llk/7bXwjDoI=
 github.com/phpdave11/gofpdi v1.0.13/go.mod h1:vBmVV0Do6hSBHC8uKUQ71JGW+ZGQq74llk/7bXwjDoI=
@@ -1988,8 +1990,6 @@ github.com/pierrec/lz4 v2.6.1+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi
 github.com/pierrec/lz4/v4 v4.1.15/go.mod h1:gZWDp/Ze/IJXGXf23ltt2EXimqmTUXEy0GFuRQyBid4=
 github.com/pierrec/lz4/v4 v4.1.18 h1:xaKrnTkyoqfh1YItXl56+6KJNVYWlEEPuAQW9xsplYQ=
 github.com/pierrec/lz4/v4 v4.1.18/go.mod h1:gZWDp/Ze/IJXGXf23ltt2EXimqmTUXEy0GFuRQyBid4=
-github.com/pires/go-proxyproto v0.7.0 h1:IukmRewDQFWC7kfnb66CSomk2q/seBuilHBYFwyq0Hs=
-github.com/pires/go-proxyproto v0.7.0/go.mod h1:Vz/1JPY/OACxWGQNIRY2BeyDmpoaWmEP40O9LbuiFR4=
 github.com/pjbgf/sha1cd v0.3.0 h1:4D5XXmUUBUl/xQ6IjCkEAbqXskkq/4O7LmGn0AqMDs4=
 github.com/pjbgf/sha1cd v0.3.0/go.mod h1:nZ1rrWOcGJ5uZgEEVL1VUM9iRQiZvWdbZjkKyFzPPsI=
 github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4/go.mod h1:4OwLy04Bl9Ef3GJJCoec+30X3LQs/0/m4HFRt/2LUSA=
--- a/helper/proxyutil/proxyutil.go
+++ b/helper/proxyutil/proxyutil.go
@@ -4,15 +4,14 @@
 package proxyutil

 import (
-	"errors"
 	"fmt"
 	"net"
 	"sync"
 	"time"

 	"github.com/hashicorp/go-secure-stdlib/parseutil"
-	sockaddr "github.com/hashicorp/go-sockaddr"
-	proxyproto "github.com/pires/go-proxyproto"
+	"github.com/hashicorp/go-sockaddr"
+	"github.com/pires/go-proxyproto"
 )

 // ProxyProtoConfig contains configuration for the PROXY protocol
@@ -72,7 +71,7 @@ func WrapInProxyProto(listener net.Listener, config *ProxyProtoConfig) (net.List
 					return proxyproto.IGNORE, nil
 				}

-				return proxyproto.REJECT, errors.New(`upstream connection not trusted proxy_protocol_behavior is "deny_unauthorized"`)
+				return proxyproto.REJECT, proxyproto.ErrInvalidUpstream
 			},
 		}
 	default: