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.

Sign up for GitHub

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

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

Merged
merged 10 commits into from
Jun 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bdef3c7
lowercase start of error message
Jun 14, 2024
6d959da
remove unused 'td' and general test cleanup using testify require
Jun 14, 2024
71c633d
timeout 'testListenerConnFn' waiting on the server connection after 3…
Jun 14, 2024
97dd479
return the invalid upstream error so the library knows not to stop li…
Jun 17, 2024
14df1b1
Merge remote-tracking branch 'origin/main' into peteski22/VAULT-6803/…
Jun 17, 2024
b2e666f
Merge remote-tracking branch 'origin/main' into peteski22/VAULT-6803/…
Jun 25, 2024
12bc4b9
update go-proxyproto to use fork/tag
Jun 25, 2024
a3b67c6
changelog
Jun 25, 2024
8f825e4
test that fails before library and code update, but passes afterwards
Jun 25, 2024
8e5f0e2
Match go doc name to test function name
Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/27589.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
```
70 changes: 70 additions & 0 deletions command/server/listener_tcp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}
7 changes: 6 additions & 1 deletion command/server/listener_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"io"
"net"
"testing"
"time"
)

type testListenerConnFn func(net.Listener) (net.Conn, error)
Expand Down Expand Up @@ -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:
}
Comment on lines +64 to +68
Copy link
Author

@peteski22 peteski22 Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Added a timeout to the tests as the forked library now never stops listening until it can return a valid connection.


if server == nil {
if !expectError {
Expand Down
6 changes: 5 additions & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Author

@peteski22 peteski22 Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Info on forked repo tag.

Keep listener listening if upstream connection address is not trusted (but close that connection).
Releases the changes requested on pires's go-proxyproto by pires/go-proxyproto#110.

Bumped to v1.0.0 which doesn't exist as a tag on the maintainer's repo but does on the forked repo. Suspect that if the maintainer accepts the PR and tags it, semver suggests the breaking change to Accept means v1.0.0 is likely.

Copy link
Author

Choose a reason for hiding this comment

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

The change here can now be updated and the replace removed, as the PR was accepted, merged and tagged:

https://github.com/pires/go-proxyproto/releases/tag/v0.8.0

image

4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand All @@ -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=
Expand Down
7 changes: 3 additions & 4 deletions helper/proxyutil/proxyutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

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

This is the change that keeps the listener waiting for a valid connection.

https://github.com/peteski22/go-proxyproto/blob/v1.0.0/protocol.go#L85-L88

},
}
default:
Expand Down
Loading