acl: pass *http.Request through Decide via DecideArgs

pkg/smokescreen/acl/v1/acl.go

@@ -2,14 +2,26 @@ package acl
 
 import (
 	"fmt"
+	"net/http"
 	"strings"
 
 	"github.com/sirupsen/logrus"
 	"github.com/stripe/smokescreen/pkg/smokescreen/hostport"
 )
 
+// DecideArgs holds the arguments for an ACL decision. Using a struct keeps the
+// Decider interface stable as new fields are added (e.g. Req for request-aware
+// authorization in custom Decider implementations). ACL.Decide does not
+// inspect Req; it is provided solely for use by custom implementations.
+type DecideArgs struct {
+	Req              *http.Request
+	Service          string
+	Host             string
+	ConnectProxyHost string
+}
+
 type Decider interface {
-	Decide(service, host, connectProxyHost string) (Decision, error)
+	Decide(args DecideArgs) (Decision, error)
 }
 
 type ACL struct {
@@ -101,10 +113,10 @@ func (acl *ACL) Add(svc string, r Rule) error {
 //  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) {
+func (acl *ACL) Decide(args DecideArgs) (Decision, error) {
 	var d Decision
 
-	rule := acl.Rule(service)
+	rule := acl.Rule(args.Service)
 	if rule == nil {
 		d.Result = Deny
 		d.Reason = "no rule matched"
@@ -114,10 +126,10 @@ func (acl *ACL) Decide(service, host, connectProxyHost string) (Decision, error)
 	d.Project = rule.Project
 	d.Default = rule == acl.DefaultRule
 
-	if connectProxyHost != "" {
+	if args.ConnectProxyHost != "" {
 		shouldDeny := true
 		for _, dg := range rule.ExternalProxyGlobs {
-			if HostMatchesGlob(connectProxyHost, dg) {
+			if HostMatchesGlob(args.ConnectProxyHost, dg) {
 				shouldDeny = false
 				break
 			}
@@ -135,11 +147,11 @@ func (acl *ACL) Decide(service, host, connectProxyHost string) (Decision, error)
 
 	// if the host matches any of the rule's allowed domains, allow
 	for _, dg := range rule.DomainGlobs {
-		if HostMatchesGlob(host, dg) {
+		if HostMatchesGlob(args.Host, dg) {
 			d.Result, d.Reason = Allow, "host matched allowed domain in rule"
 			// Check if we can find a matching MITM config
 			for _, dg := range rule.MitmDomains {
-				if HostMatchesGlob(host, dg.Domain) {
+				if HostMatchesGlob(args.Host, dg.Domain) {
 					d.MitmConfig = &MitmConfig{
 						AddHeaders:                  dg.AddHeaders,
 						DetailedHttpLogs:            dg.DetailedHttpLogs,
@@ -154,15 +166,15 @@ func (acl *ACL) Decide(service, host, connectProxyHost string) (Decision, error)
 
 	// if the host matches any of the global deny list, deny
 	for _, dg := range acl.GlobalDenyList {
-		if HostMatchesGlob(host, dg) {
+		if HostMatchesGlob(args.Host, dg) {
 			d.Result, d.Reason = Deny, "host matched rule in global deny list"
 			return d, nil
 		}
 	}
 
 	// if the host matches any of the global allow list, allow
 	for _, dg := range acl.GlobalAllowList {
-		if HostMatchesGlob(host, dg) {
+		if HostMatchesGlob(args.Host, dg) {
 			d.Result, d.Reason = Allow, "host matched rule in global allow list"
 			return d, nil
 		}
@@ -178,7 +190,7 @@ func (acl *ACL) Decide(service, host, connectProxyHost string) (Decision, error)
 		d.Result, d.Reason = Allow, "rule has open enforcement policy"
 	default:
 		d.Result, d.Reason = Deny, "unexpected policy value"
-		err = fmt.Errorf("unexpected policy value for (%s -> %s): %d", service, host, rule.Policy)
+		err = fmt.Errorf("unexpected policy value for (%s -> %s): %d", args.Service, args.Host, rule.Policy)
 	}
 
 	if d.Default {

pkg/smokescreen/acl/v1/acl_test.go

@@ -4,6 +4,7 @@
 package acl
 
 import (
+	"net/http"
 	"path"
 	"testing"
 
@@ -186,7 +187,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(DecideArgs{Service: testCase.service, Host: testCase.host})
 			a.NoError(err)
 			a.Equal(testCase.expectDecision, d.Result)
 			a.Equal(testCase.expectDecisionReason, d.Reason)
@@ -207,7 +208,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(DecideArgs{Service: "unk", Host: "example.com"})
 	a.Equal(Deny, d.Result)
 	a.False(d.Default)
 	a.Nil(err)
@@ -375,7 +376,7 @@ func TestMitmComfig(t *testing.T) {
 	a.NoError(err)
 	a.Equal("usersec", proj)
 
-	d, err := acl.Decide(mitmService, "example-mitm.com", "")
+	d, err := acl.Decide(DecideArgs{Service: mitmService, Host: "example-mitm.com"})
 	a.NoError(err)
 	a.Equal(Allow, d.Result)
 	a.Equal("host matched allowed domain in rule", d.Reason)
@@ -478,15 +479,48 @@ func TestDecisionWithProxyHost(t *testing.T) {
 	}
 
 	// Allowed proxy and target
-	decision, err := acl.Decide("proxy-service", "example.com", "proxy.example.com")
+	decision, err := acl.Decide(DecideArgs{Service: "proxy-service", Host: "example.com", ConnectProxyHost: "proxy.example.com"})
 	assert.NoError(t, err)
 	assert.Equal(t, Allow, decision.Result)
-	
-	// Disallowed proxy
-	decision, err = acl.Decide("proxy-service", "example.com", "bad.proxy.com")
+
+	// Disallowed proxy host → deny regardless of target
+	decision, err = acl.Decide(DecideArgs{Service: "proxy-service", Host: "example.com", ConnectProxyHost: "bad.proxy.com"})
 	assert.NoError(t, err)
 	assert.Equal(t, Deny, decision.Result)
 	assert.Equal(t, "connect proxy host not allowed in rule", decision.Reason)
+
+	// Allowed proxy but disallowed target → deny
+	decision, err = acl.Decide(DecideArgs{Service: "proxy-service", Host: "notallowed.com", ConnectProxyHost: "proxy.example.com"})
+	assert.NoError(t, err)
+	assert.Equal(t, Deny, decision.Result)
+}
+
+// TestDecideReqIgnored verifies that ACL.Decide produces identical results
+// regardless of whether Req is nil or a real *http.Request. ACL.Decide does
+// not inspect Req; it exists solely for use by custom Decider implementations.
+func TestDecideReqIgnored(t *testing.T) {
+	a := assert.New(t)
+
+	testACL := &ACL{
+		Rules: map[string]Rule{
+			"svc": {
+				Policy:      Enforce,
+				DomainGlobs: []string{"allowed.com"},
+			},
+		},
+	}
+
+	dNil, err := testACL.Decide(DecideArgs{Service: "svc", Host: "allowed.com"})
+	a.NoError(err)
+
+	req, _ := http.NewRequest("GET", "http://allowed.com", nil)
+	req.Header.Set("X-Custom-Header", "should-be-ignored")
+	dWithReq, err := testACL.Decide(DecideArgs{Req: req, Service: "svc", Host: "allowed.com"})
+	a.NoError(err)
+
+	a.Equal(dNil.Result, dWithReq.Result)
+	a.Equal(dNil.Reason, dWithReq.Reason)
+	a.Equal(dNil.Project, dWithReq.Project)
 }
 
 func TestDefaultRuleValidationWithDisableActions(t *testing.T) {

pkg/smokescreen/smokescreen.go

@@ -1381,7 +1381,12 @@ func checkACLsForRequest(config *Config, sctx *SmokescreenContext, req *http.Req
 		clientProvidedProxy = connectProxyUrl.Hostname()
 	}
 
-	ACLDecision, err := config.EgressACL.Decide(role, destination.Host, clientProvidedProxy)
+	ACLDecision, err := config.EgressACL.Decide(acl.DecideArgs{
+		Req:              req,
+		Service:          role,
+		Host:             destination.Host,
+		ConnectProxyHost: clientProvidedProxy,
+	})
 	decision.Project = ACLDecision.Project
 	decision.Reason = ACLDecision.Reason
 	decision.MitmConfig = ACLDecision.MitmConfig

pkg/smokescreen/smokescreen_test.go

@@ -757,59 +757,6 @@ func TestInvalidHost(t *testing.T) {
 	}
 }
 
-var hostSquareBracketsCases = []struct {
-	scheme    string
-	proxyType string
-	hostname  string
-	msg       string
-}{
-	{"http", "http", "[stripe.com]", "invalid domain \"[stripe.com]\": disallowed rune U+005B"},
-	{"https", "connect", "[stripe.com]", "host matched rule in global deny list"},
-	{"http", "http", "[[stripe.com]]", "invalid domain \"[[stripe.com]]\": disallowed rune U+005B"},
-	{"https", "connect", "[[stripe.com]]", "host matched rule in global deny list"},
-	{"http", "http", "[[[stripe.com]]]", "invalid domain \"[[[stripe.com]]]\": disallowed rune U+005B"},
-	{"https", "connect", "[2001:Db8::]:443", "Destination host cannot be determined"},
-	// These somewhat confusing error messages originate from net.SplitHostPort().
-	{"https", "connect", "[[[stripe.com]]]", "address [[stripe.com]]:443: missing port in address"},
-	{"http", "http", "[[stripe.com]]:80", "address [[stripe.com]]:80: missing port in address"},
-}
-
-func TestHostSquareBrackets(t *testing.T) {
-	for _, testCase := range hostSquareBracketsCases {
-		t.Run(testCase.scheme+"://"+testCase.hostname, func(t *testing.T) {
-			r := require.New(t)
-
-			cfg, err := testConfig("test-open-srv")
-			require.NoError(t, err)
-			logHook := proxyLogHook(cfg)
-
-			proxySrv := proxyServer(cfg)
-			defer proxySrv.Close()
-
-			// Create a http.Client that uses our proxy
-			client, err := proxyClient(proxySrv.URL)
-			r.NoError(err)
-
-			resp, err := client.Get(fmt.Sprintf("%s://%s", testCase.scheme, testCase.hostname))
-			if err != nil {
-				r.Contains(err.Error(), "Request rejected by proxy")
-			} else {
-				r.Equal(http.StatusProxyAuthRequired, resp.StatusCode)
-			}
-
-			entry := findCanonicalProxyDecision(logHook.AllEntries())
-			r.NotNil(entry)
-
-			r.Equal(entry.Data["proxy_type"], testCase.proxyType)
-			if allowed, ok := entry.Data["allow"]; ok {
-				r.Equal(false, allowed)
-				r.Equal(testCase.msg, entry.Data["decision_reason"])
-			} else {
-				r.Equal(testCase.msg, entry.Data["error"])
-			}
-		})
-	}
-}
 
 func TestErrorHeader(t *testing.T) {
 	a := assert.New(t)