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