From 89276a56b2267ca49041be0bd9c7be7c96d3b94d Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Wed, 26 Jun 2024 08:52:13 +0100 Subject: [PATCH] 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 --- changelog/27589.txt | 4 ++ command/server/listener_tcp_test.go | 70 +++++++++++++++++++++++++++++ command/server/listener_test.go | 7 ++- go.mod | 6 ++- go.sum | 4 +- helper/proxyutil/proxyutil.go | 7 ++- 6 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 changelog/27589.txt diff --git a/changelog/27589.txt b/changelog/27589.txt new file mode 100644 index 000000000000..844857ff1e77 --- /dev/null +++ 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. +``` \ No newline at end of file diff --git a/command/server/listener_tcp_test.go b/command/server/listener_tcp_test.go index dde225134075..6508e7a8afa4 100644 --- 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() + } +} diff --git a/command/server/listener_test.go b/command/server/listener_test.go index b1c6be73f7f8..cffba69eed02 100644 --- 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 { diff --git a/go.mod b/go.mod index 763fcd7d3c74..7f89722f8cb7 100644 --- 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 diff --git a/go.sum b/go.sum index 6c2c12a3018e..81be6ddfb7d9 100644 --- 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= diff --git a/helper/proxyutil/proxyutil.go b/helper/proxyutil/proxyutil.go index 7724dde2fd3a..5b0e523b51b3 100644 --- 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: