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

@@ -3,3 +3,4 @@
 *.swp
 *.swo
 *.swn
+*debug.test*

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) {

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)

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)

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 {

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 {

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