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

peteski22 · 2024-06-25T09:21:43Z

Description

This PR corrects an issue introduced when supported was added for PROXY v2 on TCP listeners using proxy_protocol_behavior.

This issue was introduced in Vault v1.10.0 and has impacted all subsequent versions other than v1.16.x (due to a bug parsing this config setting).

Any listener configured to deny_unauthorized will still close a connection when the upstream proxy is untrusted, but it will now continue to listen for connections until a valid one is found. It does this by returning ErrInvalidUpstream to the proxy library.

NOTE: the issue is only present if this config setting proxy_protocol_behavior is enabled and set to deny_unauthorized.

HashiCorp checklist

Labels: If this PR is the CE portion of an ENT change, and that ENT change is
getting backported to N-2, use the new style backport/ent/x.x.x+ent labels
instead of the old style backport/x.x.x labels.
Labels: If this PR is a CE only change, it can only be backported to N, so use
the normal backport/x.x.x label (there should be only 1).
ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
Jira: If this change has an associated Jira, it's referenced either
in the PR description, commit message, or branch name.
RFC: If this change has an associated RFC, please link it in the description.
ENT PR: If this change has an associated ENT PR, please link it in the
description. Also, make sure the changelog is in this PR, not in your ENT PR.

… secs

…stening/serving

…listener-proxy-behavior-bug-allow-deny

github-actions · 2024-06-25T09:25:13Z

CI Results:
All Go tests succeeded! ✅

github-actions · 2024-06-25T09:45:55Z

Build Results:
All builds succeeded! ✅

peteski22 · 2024-06-25T09:57:11Z

command/server/listener_test.go

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


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

peteski22 · 2024-06-25T09:58:16Z

helper/proxyutil/proxyutil.go

@@ -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


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

peteski22 · 2024-06-25T10:00:38Z

go.mod

+
+// 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


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.

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

VioletHynes

Is it possible to add a test that fails before this change but passes afterwards? I'm not particularly familiar with this area of the code so it would especially help increase my confidence both now and in the future if we could exercise this code path and bug fix in a test

peteski22 · 2024-06-25T13:15:33Z

Is it possible to add a test that fails before this change but passes afterwards? I'm not particularly familiar with this area of the code so it would especially help increase my confidence both now and in the future if we could exercise this code path and bug fix in a test

I had been testing this manually using https://github.com/muhtasimmaleque/Vault-docker-listener-bug and Linux Vault binaries I'd built before/after the change.

VioletHynes · 2024-06-25T13:21:22Z

Sorry, I think I could have worded that better: is it possible to add an automated Go test for this bug? That would also increase confidence if/when we move from the fork that it still works, without needing to re-do the manual testing

peteski22 · 2024-06-25T16:17:00Z

Sorry, I think I could have worded that better: is it possible to add an automated Go test for this bug? That would also increase confidence if/when we move from the fork that it still works, without needing to re-do the manual testing

👍🏼 added https://github.com/hashicorp/vault/pull/27589/files#diff-b14d5cc2a3b63b6d1e6990ec3933ac2339bfe5a825ac166048387614a4b6862cR440

This fails before the change, and passes afterwards. Between this and the manual testing steps in the Jira associated with the branch we should be able to prevent a regression and reproduce any issues.

VioletHynes

This looks great! Thanks for adding a nice test for this too, it'll help if/when we move from the fork.

Awesome job with this one -- complicated area of code!

Peter Wilson added 7 commits June 14, 2024 16:16

lowercase start of error message

bdef3c7

remove unused 'td' and general test cleanup using testify require

6d959da

timeout 'testListenerConnFn' waiting on the server connection after 3…

71c633d

… secs

return the invalid upstream error so the library knows not to stop li…

97dd479

…stening/serving

Merge remote-tracking branch 'origin/main' into peteski22/VAULT-6803/…

14df1b1

…listener-proxy-behavior-bug-allow-deny

Merge remote-tracking branch 'origin/main' into peteski22/VAULT-6803/…

b2e666f

…listener-proxy-behavior-bug-allow-deny

update go-proxyproto to use fork/tag

12bc4b9

peteski22 added this to the 1.17.2 milestone Jun 25, 2024

changelog

a3b67c6

peteski22 marked this pull request as ready for review June 25, 2024 09:25

peteski22 requested review from a team, ncabatoff and hghaf099 June 25, 2024 09:50

peteski22 commented Jun 25, 2024

View reviewed changes

peteski22 requested review from VioletHynes and biazmoreira June 25, 2024 12:31

VioletHynes reviewed Jun 25, 2024

View reviewed changes

test that fails before library and code update, but passes afterwards

8f825e4

peteski22 requested a review from VioletHynes June 25, 2024 16:17

VioletHynes approved these changes Jun 25, 2024

View reviewed changes

Match go doc name to test function name

8e5f0e2

peteski22 enabled auto-merge (squash) June 26, 2024 07:35

peteski22 merged commit 89276a5 into main Jun 26, 2024
83 checks passed

peteski22 deleted the peteski22/VAULT-6803/listener-proxy-behavior-bug-allow-deny branch June 26, 2024 07:52

hc-github-team-secure-vault-core mentioned this pull request Jun 26, 2024

Backport of VAULT-6803: fix listener issue if using proxy_protocol_behavior with deny_unauthorized for untrusted upstream connections into release/1.17.x #27608

Merged

6 tasks

peteski22 added backport/ent/1.15.x+ent backport/ent/1.16.x+ent Changes are backported to 1.16.x+ent and removed backport/ent/1.15.x+ent backport/ent/1.16.x+ent Changes are backported to 1.16.x+ent labels Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

peteski22 commented Jun 25, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024

peteski22 Jun 25, 2024 •

edited

Loading

peteski22 Jun 25, 2024

peteski22 Jun 25, 2024 •

edited

Loading

peteski22 Oct 10, 2024

VioletHynes left a comment

peteski22 commented Jun 25, 2024

VioletHynes commented Jun 25, 2024 •

edited

Loading

peteski22 commented Jun 25, 2024

VioletHynes left a comment

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

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

Conversation

peteski22 commented Jun 25, 2024 • edited Loading

Description

HashiCorp checklist

github-actions bot commented Jun 25, 2024 • edited Loading

github-actions bot commented Jun 25, 2024

peteski22 Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

peteski22 Jun 25, 2024

Choose a reason for hiding this comment

peteski22 Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

peteski22 Oct 10, 2024

Choose a reason for hiding this comment

VioletHynes left a comment

Choose a reason for hiding this comment

peteski22 commented Jun 25, 2024

VioletHynes commented Jun 25, 2024 • edited Loading

peteski22 commented Jun 25, 2024

VioletHynes left a comment

Choose a reason for hiding this comment

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

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

peteski22 commented Jun 25, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024 •

edited

Loading

peteski22 Jun 25, 2024 •

edited

Loading

peteski22 Jun 25, 2024 •

edited

Loading

VioletHynes commented Jun 25, 2024 •

edited

Loading