From 6f1bd0b09bbdf9b04980233ffb5c13002bcdc610 Mon Sep 17 00:00:00 2001 From: Patrick Spieker Date: Wed, 6 Dec 2023 10:31:21 -0800 Subject: [PATCH 1/8] 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 --- .gitignore | 1 + pkg/smokescreen/acl/v1/acl.go | 40 ++++++++++---- pkg/smokescreen/acl/v1/acl_test.go | 4 +- pkg/smokescreen/acl/v1/yaml_loader.go | 16 +++--- pkg/smokescreen/smokescreen.go | 20 ++++++- pkg/smokescreen/smokescreen_test.go | 79 ++++++++++++++++++++++++++- pkg/smokescreen/testdata/acl.yaml | 14 +++++ 7 files changed, 151 insertions(+), 23 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..3ad31937 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 { @@ -39,7 +40,6 @@ func New(logger *logrus.Logger, loader Loader, disabledActions []string) (*ACL, if err != nil { return nil, err } - err = acl.DisablePolicies(disabledActions) if err != nil { return nil, err @@ -80,11 +80,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 +98,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..518cefbd 100644 --- a/pkg/smokescreen/smokescreen.go +++ b/pkg/smokescreen/smokescreen.go @@ -639,8 +639,11 @@ func handleConnect(config *Config, pctx *goproxy.ProxyCtx) (string, error) { return "", pctx.Error } - // checkIfRequestShouldBeProxied can return an error if either the resolved address is disallowed, - // or if there is a DNS resolution failure. + /* + checkIfRequestShouldBeProxied can return an error if either the resolved address is disallowed, + 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 +932,18 @@ 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. + var connectProxyHost string + if len(req.Header["X-Upstream-Https-Proxy"]) > 0 { + connectProxyHost = req.Header["X-Upstream-Https-Proxy"][0] + } else { + connectProxyHost = "" + } + + 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..27f93bf8 100644 --- a/pkg/smokescreen/smokescreen_test.go +++ b/pkg/smokescreen/smokescreen_test.go @@ -1236,6 +1236,79 @@ 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")) + }) + // a := assert.New(t) + 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")) + }) + // a := assert.New(t) + 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) + + // TODO: figure out how to change the rules at runtime here + 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{"localhost"}}) + 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-allowed-srv", entry.Data["role"]) + r.Equal(true, entry.Data["allow"]) + }) +} func findCanonicalProxyDecision(logs []*logrus.Entry) *logrus.Entry { for _, entry := range logs { if entry.Message == CanonicalProxyDecision { @@ -1255,6 +1328,10 @@ func findCanonicalProxyClose(logs []*logrus.Entry) *logrus.Entry { } func testConfig(role string) (*Config, error) { + return testConfigFromACLFile(role, "testdata/acl.yaml") +} + +func testConfigFromACLFile(role string, filepath string) (*Config, error) { conf := NewConfig() if err := conf.SetAllowRanges(allowRanges); err != nil { @@ -1264,7 +1341,7 @@ func testConfig(role string) (*Config, error) { conf.ExitTimeout = 10 * time.Second conf.AdditionalErrorMessageOnDeny = "Proxy denied" conf.Resolver = &net.Resolver{} - conf.SetupEgressAcl("testdata/acl.yaml") + conf.SetupEgressAcl(filepath) conf.RoleFromRequest = func(req *http.Request) (string, error) { return role, nil } diff --git a/pkg/smokescreen/testdata/acl.yaml b/pkg/smokescreen/testdata/acl.yaml index a55ffa41..e08379f3 100644 --- a/pkg/smokescreen/testdata/acl.yaml +++ b/pkg/smokescreen/testdata/acl.yaml @@ -15,6 +15,20 @@ 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 + - name: test-external-connect-proxy-allowed-srv + project: security + action: enforce + allowed_domains: + - 127.0.0.1 + allowed_external_proxies: + - localhost global_deny_list: - stripe.com From 7c9175cecbb679e1ee0f9808ef8fcab39749790b Mon Sep 17 00:00:00 2001 From: Patrick Spieker Date: Wed, 14 Feb 2024 14:20:10 -0800 Subject: [PATCH 2/8] WIP --- pkg/smokescreen/smokescreen_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/smokescreen/smokescreen_test.go b/pkg/smokescreen/smokescreen_test.go index 27f93bf8..0f2e05ff 100644 --- a/pkg/smokescreen/smokescreen_test.go +++ b/pkg/smokescreen/smokescreen_test.go @@ -1275,7 +1275,6 @@ func TestCONNECTProxyACLs(t *testing.T) { h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("OK")) }) - // a := assert.New(t) r := require.New(t) l, err := net.Listen("tcp", "localhost:0") r.NoError(err) @@ -1286,9 +1285,9 @@ func TestCONNECTProxyACLs(t *testing.T) { err = cfg.SetAllowAddresses([]string{"127.0.0.1"}) r.NoError(err) - // TODO: figure out how to change the rules at runtime here 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() @@ -1304,7 +1303,7 @@ func TestCONNECTProxyACLs(t *testing.T) { entry := findCanonicalProxyDecision(logHook.AllEntries()) r.NotNil(entry) - // r.Equal("connect proxy host not allowed in rule", entry.Data["decision_reason"]) + 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"]) }) From 6bf0712b9c5388ac47630b4dc488a4eccebf9a70 Mon Sep 17 00:00:00 2001 From: Patrick Spieker Date: Wed, 14 Feb 2024 14:33:01 -0800 Subject: [PATCH 3/8] WIP --- pkg/smokescreen/smokescreen_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/smokescreen/smokescreen_test.go b/pkg/smokescreen/smokescreen_test.go index 0f2e05ff..0131572e 100644 --- a/pkg/smokescreen/smokescreen_test.go +++ b/pkg/smokescreen/smokescreen_test.go @@ -1241,7 +1241,6 @@ func TestCONNECTProxyACLs(t *testing.T) { h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("OK")) }) - // a := assert.New(t) r := require.New(t) l, err := net.Listen("tcp", "localhost:0") r.NoError(err) From 4363cdb099bb474b71a2ea50f3acd4af6446f142 Mon Sep 17 00:00:00 2001 From: Patrick Spieker Date: Thu, 15 Feb 2024 01:20:49 -0800 Subject: [PATCH 4/8] PR feedback 1 --- pkg/smokescreen/smokescreen.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/pkg/smokescreen/smokescreen.go b/pkg/smokescreen/smokescreen.go index 518cefbd..84fe37cc 100644 --- a/pkg/smokescreen/smokescreen.go +++ b/pkg/smokescreen/smokescreen.go @@ -639,11 +639,9 @@ func handleConnect(config *Config, pctx *goproxy.ProxyCtx) (string, error) { return "", pctx.Error } - /* - checkIfRequestShouldBeProxied can return an error if either the resolved address is disallowed, - 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. - */ + // checkIfRequestShouldBeProxied can return an error if either the resolved address is disallowed, + // 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 @@ -937,10 +935,8 @@ func checkACLsForRequest(config *Config, req *http.Request, destination hostport // 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. var connectProxyHost string - if len(req.Header["X-Upstream-Https-Proxy"]) > 0 { - connectProxyHost = req.Header["X-Upstream-Https-Proxy"][0] - } else { - connectProxyHost = "" + if connectProxyHostSlice := req.Header.Get("X-Upstream-Https-Proxy"); len(connectProxyHostSlice) > 0 { + connectProxyHost = string(connectProxyHostSlice[0]) } ACLDecision, err := config.EgressACL.Decide(role, destination.Host, connectProxyHost) From 1eceba796d9df36c70904ee3230c85f393e00e3f Mon Sep 17 00:00:00 2001 From: Patrick Spieker Date: Thu, 15 Feb 2024 01:30:56 -0800 Subject: [PATCH 5/8] Fixed tests --- pkg/smokescreen/acl/v1/acl.go | 1 + pkg/smokescreen/smokescreen.go | 7 +++---- pkg/smokescreen/testdata/acl.yaml | 2 ++ 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/smokescreen/acl/v1/acl.go b/pkg/smokescreen/acl/v1/acl.go index 3ad31937..f5bf3a4a 100644 --- a/pkg/smokescreen/acl/v1/acl.go +++ b/pkg/smokescreen/acl/v1/acl.go @@ -40,6 +40,7 @@ func New(logger *logrus.Logger, loader Loader, disabledActions []string) (*ACL, if err != nil { return nil, err } + err = acl.DisablePolicies(disabledActions) if err != nil { return nil, err diff --git a/pkg/smokescreen/smokescreen.go b/pkg/smokescreen/smokescreen.go index 84fe37cc..7283bf22 100644 --- a/pkg/smokescreen/smokescreen.go +++ b/pkg/smokescreen/smokescreen.go @@ -934,10 +934,9 @@ func checkACLsForRequest(config *Config, req *http.Request, destination hostport // 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. - var connectProxyHost string - if connectProxyHostSlice := req.Header.Get("X-Upstream-Https-Proxy"); len(connectProxyHostSlice) > 0 { - connectProxyHost = string(connectProxyHostSlice[0]) - } + // 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 diff --git a/pkg/smokescreen/testdata/acl.yaml b/pkg/smokescreen/testdata/acl.yaml index e08379f3..ad4df89c 100644 --- a/pkg/smokescreen/testdata/acl.yaml +++ b/pkg/smokescreen/testdata/acl.yaml @@ -22,6 +22,7 @@ services: - 127.0.0.1 allowed_external_proxies: - myproxy.com + - otherproxy.org - name: test-external-connect-proxy-allowed-srv project: security action: enforce @@ -29,6 +30,7 @@ services: - 127.0.0.1 allowed_external_proxies: - localhost + - thisisaproxy.com global_deny_list: - stripe.com From 995a427e0609b390e3aabe8c7b87e316bfc389c1 Mon Sep 17 00:00:00 2001 From: Patrick Spieker Date: Thu, 15 Feb 2024 19:59:35 -0800 Subject: [PATCH 6/8] testing again --- pkg/smokescreen/smokescreen_test.go | 150 ++++++++++++++-------------- 1 file changed, 74 insertions(+), 76 deletions(-) diff --git a/pkg/smokescreen/smokescreen_test.go b/pkg/smokescreen/smokescreen_test.go index 0131572e..fdbb3eb7 100644 --- a/pkg/smokescreen/smokescreen_test.go +++ b/pkg/smokescreen/smokescreen_test.go @@ -1236,77 +1236,79 @@ 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{"localhost"}}) - 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"]) - }) -} +// func TestCONNECTProxyACLsDeny(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"]) +// //}) +// } + +// func TestCONNECTProxyACLsAllow(t *testing.T) { +// //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{"localhost"}}) +// 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"]) +// //}) +// } func findCanonicalProxyDecision(logs []*logrus.Entry) *logrus.Entry { for _, entry := range logs { if entry.Message == CanonicalProxyDecision { @@ -1326,10 +1328,6 @@ func findCanonicalProxyClose(logs []*logrus.Entry) *logrus.Entry { } func testConfig(role string) (*Config, error) { - return testConfigFromACLFile(role, "testdata/acl.yaml") -} - -func testConfigFromACLFile(role string, filepath string) (*Config, error) { conf := NewConfig() if err := conf.SetAllowRanges(allowRanges); err != nil { @@ -1339,7 +1337,7 @@ func testConfigFromACLFile(role string, filepath string) (*Config, error) { conf.ExitTimeout = 10 * time.Second conf.AdditionalErrorMessageOnDeny = "Proxy denied" conf.Resolver = &net.Resolver{} - conf.SetupEgressAcl(filepath) + conf.SetupEgressAcl("testdata/acl.yaml") conf.RoleFromRequest = func(req *http.Request) (string, error) { return role, nil } From 61eb3805e4bc9ac7f4a551d0a94ed9a8acac2470 Mon Sep 17 00:00:00 2001 From: Patrick Spieker Date: Thu, 15 Feb 2024 20:01:47 -0800 Subject: [PATCH 7/8] WIP --- pkg/smokescreen/smokescreen_test.go | 144 ++++++++++++++-------------- 1 file changed, 71 insertions(+), 73 deletions(-) diff --git a/pkg/smokescreen/smokescreen_test.go b/pkg/smokescreen/smokescreen_test.go index fdbb3eb7..da9814a4 100644 --- a/pkg/smokescreen/smokescreen_test.go +++ b/pkg/smokescreen/smokescreen_test.go @@ -1236,79 +1236,77 @@ func TestCustomRequestHandler(t *testing.T) { }) } -// func TestCONNECTProxyACLsDeny(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"]) -// //}) -// } - -// func TestCONNECTProxyACLsAllow(t *testing.T) { -// //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{"localhost"}}) -// 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"]) -// //}) -// } +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{"localhost"}}) + 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"]) + }) +} func findCanonicalProxyDecision(logs []*logrus.Entry) *logrus.Entry { for _, entry := range logs { if entry.Message == CanonicalProxyDecision { From 412128e2b8c3a80e8297641a332d57ddaa56b4af Mon Sep 17 00:00:00 2001 From: Patrick Spieker Date: Fri, 16 Feb 2024 11:11:32 -0800 Subject: [PATCH 8/8] Added extra test --- pkg/smokescreen/smokescreen_test.go | 49 ++++++++++++++++++++++++++++- pkg/smokescreen/testdata/acl.yaml | 2 ++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/pkg/smokescreen/smokescreen_test.go b/pkg/smokescreen/smokescreen_test.go index da9814a4..d3764722 100644 --- a/pkg/smokescreen/smokescreen_test.go +++ b/pkg/smokescreen/smokescreen_test.go @@ -1292,7 +1292,7 @@ func TestCONNECTProxyACLs(t *testing.T) { externalProxy.StartTLS() remote := httptest.NewTLSServer(h) - client, err := proxyClientWithConnectHeaders(proxy.URL, http.Header{"X-Upstream-Https-Proxy": []string{"localhost"}}) + 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) @@ -1306,6 +1306,53 @@ func TestCONNECTProxyACLs(t *testing.T) { 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 { diff --git a/pkg/smokescreen/testdata/acl.yaml b/pkg/smokescreen/testdata/acl.yaml index ad4df89c..5e7230a9 100644 --- a/pkg/smokescreen/testdata/acl.yaml +++ b/pkg/smokescreen/testdata/acl.yaml @@ -30,6 +30,8 @@ services: - 127.0.0.1 allowed_external_proxies: - localhost + - myproxy.com + - myproxy2.com - thisisaproxy.com global_deny_list: