From 5c3d435fe776c904039697a76f4d5da6d3eb9bbc Mon Sep 17 00:00:00 2001 From: pspieker-stripe <40726826+pspieker-stripe@users.noreply.github.com> Date: Fri, 16 Feb 2024 11:20:46 -0800 Subject: [PATCH] Add support for Smokescreen -> HTTPS CONNECT Proxy ACLs (#213) * Introduce CONNECT Proxy URL ACL Support Add gitignore debug changes WIP Basic concept working WIP Cleaned up some things prereview fixed tests Removed extraneous yaml file Add correctly failing test tmp WIP WIP WIP WIP WIP WIP * WIP * WIP * PR feedback 1 * Fixed tests * testing again * WIP * Added extra test --- .gitignore | 1 + pkg/smokescreen/acl/v1/acl.go | 39 +++++++-- pkg/smokescreen/acl/v1/acl_test.go | 4 +- pkg/smokescreen/acl/v1/yaml_loader.go | 16 ++-- pkg/smokescreen/smokescreen.go | 13 ++- pkg/smokescreen/smokescreen_test.go | 118 ++++++++++++++++++++++++++ pkg/smokescreen/testdata/acl.yaml | 18 ++++ 7 files changed, 189 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index e84f0ea6..dde12ee0 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ *.swp *.swo *.swn +*debug.test* diff --git a/pkg/smokescreen/acl/v1/acl.go b/pkg/smokescreen/acl/v1/acl.go index 3d29b0db..f5bf3a4a 100644 --- a/pkg/smokescreen/acl/v1/acl.go +++ b/pkg/smokescreen/acl/v1/acl.go @@ -9,7 +9,7 @@ import ( ) type Decider interface { - Decide(service, host string) (Decision, error) + Decide(service, host, connectProxyHost string) (Decision, error) } type ACL struct { @@ -22,9 +22,10 @@ type ACL struct { } type Rule struct { - Project string - Policy EnforcementPolicy - DomainGlobs []string + Project string + Policy EnforcementPolicy + DomainGlobs []string + ExternalProxyGlobs []string } type Decision struct { @@ -80,11 +81,12 @@ func (acl *ACL) Add(svc string, r Rule) error { } // Decide takes uses the rule configured for the given service to determine if -// 1. The host is in the rule's allowed domain -// 2. The host has been globally denied -// 3. The host has been globally allowed -// 4. There is a default rule for the ACL -func (acl *ACL) Decide(service, host string) (Decision, error) { +// 1. The CONNECT proxy host is in the rule's allowed domain +// 2. The host is in the rule's allowed domain +// 3. The host has been globally denied +// 4. The host has been globally allowed +// 5. There is a default rule for the ACL +func (acl *ACL) Decide(service, host, connectProxyHost string) (Decision, error) { var d Decision rule := acl.Rule(service) @@ -97,6 +99,25 @@ func (acl *ACL) Decide(service, host string) (Decision, error) { d.Project = rule.Project d.Default = rule == acl.DefaultRule + if connectProxyHost != "" { + shouldDeny := true + for _, dg := range rule.ExternalProxyGlobs { + if HostMatchesGlob(connectProxyHost, dg) { + shouldDeny = false + break + } + } + + // We can only break out early and return if we know that we should deny; + // at this point the host hasn't been allowed by the rule, so we need to + // continue to check it below (unless we know we should deny it already) + if shouldDeny { + d.Result = Deny + d.Reason = "connect proxy host not allowed in rule" + return d, nil + } + } + // if the host matches any of the rule's allowed domains, allow for _, dg := range rule.DomainGlobs { if HostMatchesGlob(host, dg) { diff --git a/pkg/smokescreen/acl/v1/acl_test.go b/pkg/smokescreen/acl/v1/acl_test.go index 1ba35b93..9539458a 100644 --- a/pkg/smokescreen/acl/v1/acl_test.go +++ b/pkg/smokescreen/acl/v1/acl_test.go @@ -186,7 +186,7 @@ func TestACLDecision(t *testing.T) { a.NoError(err) a.Equal(testCase.expectProject, proj) - d, err := acl.Decide(testCase.service, testCase.host) + d, err := acl.Decide(testCase.service, testCase.host, "") a.NoError(err) a.Equal(testCase.expectDecision, d.Result) a.Equal(testCase.expectDecisionReason, d.Reason) @@ -207,7 +207,7 @@ func TestACLUnknownServiceWithoutDefault(t *testing.T) { a.Equal("no rule for service: unk", err.Error()) a.Empty(proj) - d, err := acl.Decide("unk", "example.com") + d, err := acl.Decide("unk", "example.com", "") a.Equal(Deny, d.Result) a.False(d.Default) a.Nil(err) diff --git a/pkg/smokescreen/acl/v1/yaml_loader.go b/pkg/smokescreen/acl/v1/yaml_loader.go index a589dad7..a13a2376 100644 --- a/pkg/smokescreen/acl/v1/yaml_loader.go +++ b/pkg/smokescreen/acl/v1/yaml_loader.go @@ -26,10 +26,11 @@ type YAMLConfig struct { } type YAMLRule struct { - Name string `yaml:"name"` - Project string `yaml:"project"` // owner - Action string `yaml:"action"` - AllowedHosts []string `yaml:"allowed_domains"` + Name string `yaml:"name"` + Project string `yaml:"project"` // owner + Action string `yaml:"action"` + AllowedHosts []string `yaml:"allowed_domains"` + AllowedExternalProxyHosts []string `yaml:"allowed_external_proxies"` } func (yc *YAMLConfig) ValidateConfig() error { @@ -78,9 +79,10 @@ func (cfg *YAMLConfig) Load() (*ACL, error) { } r := Rule{ - Project: v.Project, - Policy: p, - DomainGlobs: v.AllowedHosts, + Project: v.Project, + Policy: p, + DomainGlobs: v.AllowedHosts, + ExternalProxyGlobs: v.AllowedExternalProxyHosts, } err = acl.Add(v.Name, r) diff --git a/pkg/smokescreen/smokescreen.go b/pkg/smokescreen/smokescreen.go index 919eae83..7283bf22 100644 --- a/pkg/smokescreen/smokescreen.go +++ b/pkg/smokescreen/smokescreen.go @@ -640,7 +640,8 @@ func handleConnect(config *Config, pctx *goproxy.ProxyCtx) (string, error) { } // checkIfRequestShouldBeProxied can return an error if either the resolved address is disallowed, - // or if there is a DNS resolution failure. + // or if there is a DNS resolution failure, or if the subsequent proxy host (specified by the + // X-Https-Upstream-Proxy header in the CONNECT request to _this_ proxy) is disallowed. sctx.Decision, sctx.lookupTime, pctx.Error = checkIfRequestShouldBeProxied(config, pctx.Req, destination) if pctx.Error != nil { // DNS resolution failure @@ -929,7 +930,15 @@ func checkACLsForRequest(config *Config, req *http.Request, destination hostport return decision } - ACLDecision, err := config.EgressACL.Decide(role, destination.Host) + // X-Upstream-Https-Proxy is a header that can be set by the client to specify + // a _subsequent_ proxy to use for the CONNECT request. This is used to allow traffic + // flow as in: client -(TLS)-> smokescreen -(TLS)-> external proxy -(TLS)-> destination. + // Without this header, there's no way for the client to specify a subsequent proxy. + // Also note - Get returns the first value for a given header, or the empty string, + // which is the behavior we want here. + connectProxyHost := req.Header.Get("X-Upstream-Https-Proxy") + + ACLDecision, err := config.EgressACL.Decide(role, destination.Host, connectProxyHost) decision.project = ACLDecision.Project decision.reason = ACLDecision.Reason if err != nil { diff --git a/pkg/smokescreen/smokescreen_test.go b/pkg/smokescreen/smokescreen_test.go index 1ccc9a61..d3764722 100644 --- a/pkg/smokescreen/smokescreen_test.go +++ b/pkg/smokescreen/smokescreen_test.go @@ -1236,6 +1236,124 @@ func TestCustomRequestHandler(t *testing.T) { }) } +func TestCONNECTProxyACLs(t *testing.T) { + t.Run("Blocks a non-approved proxy when the X-Upstream-Https-Proxy header is set", func(t *testing.T) { + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("OK")) + }) + r := require.New(t) + l, err := net.Listen("tcp", "localhost:0") + r.NoError(err) + cfg, err := testConfig("test-external-connect-proxy-blocked-srv") + r.NoError(err) + cfg.Listener = l + + err = cfg.SetAllowAddresses([]string{"127.0.0.1"}) + r.NoError(err) + + internalToStripeProxy := proxyServer(cfg) + logHook := proxyLogHook(cfg) + remote := httptest.NewTLSServer(h) + + client, err := proxyClientWithConnectHeaders(internalToStripeProxy.URL, http.Header{"X-Upstream-Https-Proxy": []string{"https://google.com"}}) + r.NoError(err) + + req, err := http.NewRequest("GET", remote.URL, nil) + r.NoError(err) + + client.Do(req) + + entry := findCanonicalProxyDecision(logHook.AllEntries()) + r.NotNil(entry) + r.Equal("connect proxy host not allowed in rule", entry.Data["decision_reason"]) + r.Equal("test-external-connect-proxy-blocked-srv", entry.Data["role"]) + r.Equal(false, entry.Data["allow"]) + }) + + t.Run("Allows an approved proxy when the X-Upstream-Https-Proxy header is set", func(t *testing.T) { + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("OK")) + }) + r := require.New(t) + l, err := net.Listen("tcp", "localhost:0") + r.NoError(err) + cfg, err := testConfig("test-external-connect-proxy-allowed-srv") + r.NoError(err) + cfg.Listener = l + + err = cfg.SetAllowAddresses([]string{"127.0.0.1"}) + r.NoError(err) + + proxy := proxyServer(cfg) + logHook := proxyLogHook(cfg) + + // The External proxy is a HTTPS proxy that will be used to connect to the remote server + externalProxy := httptest.NewUnstartedServer(BuildProxy(cfg)) + externalProxy.StartTLS() + + remote := httptest.NewTLSServer(h) + client, err := proxyClientWithConnectHeaders(proxy.URL, http.Header{"X-Upstream-Https-Proxy": []string{"myproxy.com"}}) + r.NoError(err) + + req, err := http.NewRequest("GET", remote.URL, nil) + r.NoError(err) + + client.Do(req) + + entry := findCanonicalProxyDecision(logHook.AllEntries()) + r.NotNil(entry) + r.Equal("host matched allowed domain in rule", entry.Data["decision_reason"]) + r.Equal("test-external-connect-proxy-allowed-srv", entry.Data["role"]) + r.Equal(true, entry.Data["allow"]) + }) + + t.Run("Allows multiple approved proxies when the X-Upstream-Https-Proxy header is set", func(t *testing.T) { + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("OK")) + }) + r := require.New(t) + l, err := net.Listen("tcp", "localhost:0") + r.NoError(err) + cfg, err := testConfig("test-external-connect-proxy-allowed-srv") + r.NoError(err) + cfg.Listener = l + + err = cfg.SetAllowAddresses([]string{"127.0.0.1"}) + r.NoError(err) + + proxy := proxyServer(cfg) + logHook := proxyLogHook(cfg) + + // The External proxy is a HTTPS proxy that will be used to connect to the remote server + externalProxy := httptest.NewUnstartedServer(BuildProxy(cfg)) + externalProxy.StartTLS() + + remote := httptest.NewTLSServer(h) + first_client, err := proxyClientWithConnectHeaders(proxy.URL, http.Header{"X-Upstream-Https-Proxy": []string{"myproxy.com"}}) + r.NoError(err) + + first_req, err := http.NewRequest("GET", remote.URL, nil) + r.NoError(err) + + first_client.Do(first_req) + + second_client, err := proxyClientWithConnectHeaders(proxy.URL, http.Header{"X-Upstream-Https-Proxy": []string{"myproxy2.com"}}) + r.NoError(err) + + second_req, err := http.NewRequest("GET", remote.URL, nil) + r.NoError(err) + + second_client.Do(second_req) + + entries := logHook.AllEntries() + r.Equal(2, len(entries)) + + first_entry := entries[0] + second_entry := entries[1] + r.Equal("host matched allowed domain in rule", first_entry.Data["decision_reason"]) + r.Equal("host matched allowed domain in rule", second_entry.Data["decision_reason"]) + }) +} func findCanonicalProxyDecision(logs []*logrus.Entry) *logrus.Entry { for _, entry := range logs { if entry.Message == CanonicalProxyDecision { diff --git a/pkg/smokescreen/testdata/acl.yaml b/pkg/smokescreen/testdata/acl.yaml index a55ffa41..5e7230a9 100644 --- a/pkg/smokescreen/testdata/acl.yaml +++ b/pkg/smokescreen/testdata/acl.yaml @@ -15,6 +15,24 @@ services: - name: test-open-srv project: security action: open + - name: test-external-connect-proxy-blocked-srv + project: security + action: enforce + allowed_domains: + - 127.0.0.1 + allowed_external_proxies: + - myproxy.com + - otherproxy.org + - name: test-external-connect-proxy-allowed-srv + project: security + action: enforce + allowed_domains: + - 127.0.0.1 + allowed_external_proxies: + - localhost + - myproxy.com + - myproxy2.com + - thisisaproxy.com global_deny_list: - stripe.com