From bc38d13f0de5a7c1bdbfc557d9a739f36cded5f2 Mon Sep 17 00:00:00 2001 From: Saurabh Bhatia Date: Thu, 10 Oct 2024 19:52:51 +0530 Subject: [PATCH 1/3] Make SmokeScreen Fields Public --- pkg/smokescreen/role_test.go | 8 +-- pkg/smokescreen/smokescreen.go | 105 ++++++++++++++-------------- pkg/smokescreen/smokescreen_test.go | 6 +- 3 files changed, 60 insertions(+), 59 deletions(-) diff --git a/pkg/smokescreen/role_test.go b/pkg/smokescreen/role_test.go index 872e4905..328cb42f 100644 --- a/pkg/smokescreen/role_test.go +++ b/pkg/smokescreen/role_test.go @@ -25,14 +25,14 @@ func _testGetRole(t *testing.T, rfr_s string, rfr_e error, allow_missing bool, e t.Fatalf("expected err %v got %v\n", expect_e, e) } if s != expect_s { - t.Fatalf("expected role %v got %v\n", expect_s, s) + t.Fatalf("expected Role %v got %v\n", expect_s, s) } } func TestGetRole(t *testing.T) { - role := "some role" + role := "some Role" genErr := errors.New("general error") - mre := MissingRoleError("missing role") + mre := MissingRoleError("missing Role") t.Run("good", func(t *testing.T) { _testGetRole(t, role, nil, false, role, nil) @@ -44,7 +44,7 @@ func TestGetRole(t *testing.T) { t.Run("missing not allowed -> err", func(t *testing.T) { _testGetRole(t, "", mre, false, "", mre) }) - t.Run("missing allowed -> empty role", func(t *testing.T) { + t.Run("missing allowed -> empty Role", func(t *testing.T) { _testGetRole(t, "", mre, true, "", nil) }) } diff --git a/pkg/smokescreen/smokescreen.go b/pkg/smokescreen/smokescreen.go index 14295123..6f3e996d 100644 --- a/pkg/smokescreen/smokescreen.go +++ b/pkg/smokescreen/smokescreen.go @@ -50,8 +50,8 @@ const ( LogFieldTraceID = "trace_id" LogFieldInRemoteX509CN = "inbound_remote_x509_cn" LogFieldInRemoteX509OU = "inbound_remote_x509_ou" - LogFieldRole = "role" - LogFieldProject = "project" + LogFieldRole = "Role" + LogFieldProject = "Project" LogFieldContentLength = "content_length" LogFieldDecisionReason = "decision_reason" LogFieldEnforceWouldDeny = "enforce_would_deny" @@ -68,7 +68,7 @@ const ( type ipType int type ACLDecision struct { - reason, role, project, outboundHost string + Reason, Role, Project, OutboundHost string ResolvedAddr *net.TCPAddr allow bool enforceWouldDeny bool @@ -79,9 +79,9 @@ type SmokescreenContext struct { cfg *Config start time.Time Decision *ACLDecision - proxyType string - logger *logrus.Entry - requestedHost string + ProxyType string + Logger *logrus.Entry + RequestedHost string // Time spent resolving the requested hostname lookupTime time.Duration @@ -257,11 +257,11 @@ func dialContext(ctx context.Context, network, addr string) (net.Conn, error) { } d := sctx.Decision - // If an address hasn't been resolved, does not match the original outboundHost, + // If an address hasn't been resolved, does not match the original OutboundHost, // or is not tcp we must re-resolve it before establishing the connection. - if d.ResolvedAddr == nil || d.outboundHost != addr || network != "tcp" { + if d.ResolvedAddr == nil || d.OutboundHost != addr || network != "tcp" { var err error - d.ResolvedAddr, d.reason, err = safeResolve(sctx.cfg, network, addr) + d.ResolvedAddr, d.Reason, err = safeResolve(sctx.cfg, network, addr) if err != nil { if _, ok := err.(denyError); ok { sctx.cfg.Log.WithFields( @@ -289,25 +289,25 @@ func dialContext(ctx context.Context, network, addr string) (net.Conn, error) { conn, err = sctx.cfg.ProxyDialTimeout(ctx, network, d.ResolvedAddr.String(), sctx.cfg.ConnectTimeout) } connTime := time.Since(start) - sctx.logger = sctx.logger.WithFields(dialContextLoggerFields(pctx, sctx, conn, connTime)) + sctx.Logger = sctx.Logger.WithFields(dialContextLoggerFields(pctx, sctx, conn, connTime)) if sctx.cfg.TimeConnect { - sctx.cfg.MetricsClient.TimingWithTags("cn.atpt.connect.time", connTime, map[string]string{"domain": sctx.requestedHost}, 1) + sctx.cfg.MetricsClient.TimingWithTags("cn.atpt.connect.time", connTime, map[string]string{"domain": sctx.RequestedHost}, 1) } if err != nil { sctx.cfg.MetricsClient.IncrWithTags("cn.atpt.total", map[string]string{"success": "false"}, 1) - sctx.cfg.ConnTracker.RecordAttempt(sctx.requestedHost, false) + sctx.cfg.ConnTracker.RecordAttempt(sctx.RequestedHost, false) metrics.ReportConnError(sctx.cfg.MetricsClient, err) return nil, err } sctx.cfg.MetricsClient.IncrWithTags("cn.atpt.total", map[string]string{"success": "true"}, 1) - sctx.cfg.ConnTracker.RecordAttempt(sctx.requestedHost, true) + sctx.cfg.ConnTracker.RecordAttempt(sctx.RequestedHost, true) // Only wrap CONNECT conns with an InstrumentedConn. Connections used for traditional HTTP proxy // requests are pooled and reused by net.Transport. - if sctx.proxyType == connectProxy { - ic := sctx.cfg.ConnTracker.NewInstrumentedConnWithTimeout(conn, sctx.cfg.IdleTimeout, sctx.logger, d.role, d.outboundHost, sctx.proxyType) + if sctx.ProxyType == connectProxy { + ic := sctx.cfg.ConnTracker.NewInstrumentedConnWithTimeout(conn, sctx.cfg.IdleTimeout, sctx.Logger, d.Role, d.OutboundHost, sctx.ProxyType) pctx.ConnErrorHandler = ic.Error conn = ic } else { @@ -346,11 +346,11 @@ func HTTPErrorHandler(w io.WriteCloser, pctx *goproxy.ProxyCtx, err error) { resp := rejectResponse(pctx, err) if err := resp.Write(w); err != nil { - sctx.logger.Errorf("Failed to write HTTP error response: %s", err) + sctx.Logger.Errorf("Failed to write HTTP error response: %s", err) } if err := w.Close(); err != nil { - sctx.logger.Errorf("Failed to close proxy client connection: %s", err) + sctx.Logger.Errorf("Failed to close proxy client connection: %s", err) } } @@ -384,12 +384,12 @@ func rejectResponse(pctx *goproxy.ProxyCtx, err error) *http.Response { status = "Internal server error" code = http.StatusInternalServerError msg = "An unexpected error occurred: " + err.Error() - sctx.logger.WithField("error", err.Error()).Warn("rejectResponse called with unexpected error") + sctx.Logger.WithField("error", err.Error()).Warn("rejectResponse called with unexpected error") } // Do not double log deny errors, they are logged in a previous call to logProxy. if _, ok := err.(denyError); !ok { - sctx.logger.Error(msg) + sctx.Logger.Error(msg) } if sctx.cfg.AdditionalErrorMessageOnDeny != "" { @@ -438,10 +438,10 @@ func newContext(cfg *Config, proxyType string, req *http.Request) *SmokescreenCo return &SmokescreenContext{ cfg: cfg, - logger: logger, - proxyType: proxyType, + Logger: logger, + ProxyType: proxyType, start: start, - requestedHost: req.Host, + RequestedHost: req.Host, } } @@ -493,7 +493,7 @@ func BuildProxy(config *Config) *goproxy.ProxyHttpServer { req.Header.Del(traceHeader) }() - sctx.logger.WithField("url", req.RequestURI).Debug("received HTTP proxy request") + sctx.Logger.WithField("url", req.RequestURI).Debug("received HTTP proxy request") // Build an address parsable by net.ResolveTCPAddr destination, err := hostport.NewWithScheme(req.Host, req.URL.Scheme, false) if err != nil { @@ -510,7 +510,7 @@ func BuildProxy(config *Config) *goproxy.ProxyHttpServer { return req, rejectResponse(pctx, pctx.Error) } if !sctx.Decision.allow { - return req, rejectResponse(pctx, denyError{errors.New(sctx.Decision.reason)}) + return req, rejectResponse(pctx, denyError{errors.New(sctx.Decision.Reason)}) } // Call the custom request handler if it exists @@ -576,7 +576,7 @@ func BuildProxy(config *Config) *goproxy.ProxyHttpServer { // We don't want to log if the connection is a MITM as it will be done in HandleConnectFunc if pctx.ConnectAction != goproxy.ConnectMitm { // In case of an error, this function is called a second time to filter the - // response we generate so this logger will be called once. + // response we generate so this Logger will be called once. logProxy(pctx) } return resp @@ -586,6 +586,7 @@ func BuildProxy(config *Config) *goproxy.ProxyHttpServer { // The goproxy OnResponse() function above is only called for non-https responses. if config.AcceptResponseHandler != nil { proxy.ConnectRespHandler = func(pctx *goproxy.ProxyCtx, resp *http.Response) error { + sctx, ok := pctx.UserData.(*SmokescreenContext) if !ok { return fmt.Errorf("goproxy ProxyContext missing required UserData *SmokescreenContext") @@ -611,7 +612,7 @@ func logProxy(pctx *goproxy.ProxyCtx) { } if sctx.Decision != nil { - fields[LogFieldDecisionReason] = decision.reason + fields[LogFieldDecisionReason] = decision.Reason fields[LogFieldEnforceWouldDeny] = decision.enforceWouldDeny fields[LogFieldAllow] = decision.allow } @@ -621,7 +622,7 @@ func logProxy(pctx *goproxy.ProxyCtx) { fields[LogFieldError] = err.Error() } - entry := sctx.logger.WithFields(fields) + entry := sctx.Logger.WithFields(fields) var logMethod func(...interface{}) if _, ok := err.(denyError); !ok && err != nil { logMethod = entry.Error @@ -648,8 +649,8 @@ func extractContextLogFields(pctx *goproxy.ProxyCtx, sctx *SmokescreenContext) l // Retrieve information from the ACL decision decision := sctx.Decision if sctx.Decision != nil { - fields[LogFieldRole] = decision.role - fields[LogFieldProject] = decision.project + fields[LogFieldRole] = decision.Role + fields[LogFieldProject] = decision.Project } return fields } @@ -657,7 +658,7 @@ func extractContextLogFields(pctx *goproxy.ProxyCtx, sctx *SmokescreenContext) l func handleConnect(config *Config, pctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string, error) { sctx := pctx.UserData.(*SmokescreenContext) - // Check if requesting role is allowed to talk to remote + // Check if requesting Role is allowed to talk to remote destination, err := hostport.New(pctx.Req.Host, false) if err != nil { pctx.Error = denyError{err} @@ -673,11 +674,11 @@ func handleConnect(config *Config, pctx *goproxy.ProxyCtx) (*goproxy.ConnectActi return nil, "", pctx.Error } - // add context fields to all future log messages sent using this smokescreen context's logger - sctx.logger = sctx.logger.WithFields(extractContextLogFields(pctx, sctx)) + // add context fields to all future log messages sent using this smokescreen context's Logger + sctx.Logger = sctx.Logger.WithFields(extractContextLogFields(pctx, sctx)) if !sctx.Decision.allow { - return nil, "", denyError{errors.New(sctx.Decision.reason)} + return nil, "", denyError{errors.New(sctx.Decision.Reason)} } // Call the custom request handler if it exists @@ -696,7 +697,7 @@ func handleConnect(config *Config, pctx *goproxy.ProxyCtx) (*goproxy.ConnectActi deny := denyError{errors.New("ACLDecision specified MITM but Smokescreen doesn't have MITM enabled")} sctx.Decision.allow = false sctx.Decision.MitmConfig = nil - sctx.Decision.reason = deny.Error() + sctx.Decision.Reason = deny.Error() return nil, "", deny } mitm := sctx.Decision.MitmConfig @@ -912,10 +913,10 @@ func runServer(config *Config, server *http.Server, listener net.Listener, quit } } -// Extract the client's ACL role from the HTTP request, using the configured -// RoleFromRequest function. Returns the role, or an error if the role cannot +// Extract the client's ACL Role from the HTTP request, using the configured +// RoleFromRequest function. Returns the Role, or an error if the Role cannot // be determined (including no RoleFromRequest configured), unless -// AllowMissingRole is configured, in which case an empty role and no error is +// AllowMissingRole is configured, in which case an empty Role and no error is // returned. func getRole(config *Config, req *http.Request) (string, error) { var role string @@ -937,7 +938,7 @@ func getRole(config *Config, req *http.Request) (string, error) { "error": err, "is_missing_role": IsMissingRoleError(err), "allow_missing_role": config.AllowMissingRole, - }).Error("Unable to get role for request") + }).Error("Unable to get Role for request") return "", err } } @@ -955,7 +956,7 @@ func checkIfRequestShouldBeProxied(config *Config, req *http.Request, destinatio if _, ok := err.(denyError); !ok { return decision, lookupTime, err } - decision.reason = fmt.Sprintf("%s. %s", err.Error(), reason) + decision.Reason = fmt.Sprintf("%s. %s", err.Error(), reason) decision.allow = false decision.enforceWouldDeny = true } else { @@ -968,28 +969,28 @@ func checkIfRequestShouldBeProxied(config *Config, req *http.Request, destinatio func checkACLsForRequest(config *Config, req *http.Request, destination hostport.HostPort) *ACLDecision { decision := &ACLDecision{ - outboundHost: destination.String(), + OutboundHost: destination.String(), } if config.EgressACL == nil { decision.allow = true - decision.reason = "Egress ACL is not configured" + decision.Reason = "Egress ACL is not configured" return decision } role, roleErr := getRole(config, req) if roleErr != nil { config.MetricsClient.Incr("acl.role_not_determined", 1) - decision.reason = "Client role cannot be determined" + decision.Reason = "Client Role cannot be determined" return decision } - decision.role = role + decision.Role = role // This host validation prevents IPv6 addresses from being used as destinations. // Added for backwards compatibility. if strings.ContainsAny(destination.Host, ":") { - decision.reason = "Destination host cannot be determined" + decision.Reason = "Destination host cannot be determined" return decision } @@ -1010,7 +1011,7 @@ func checkACLsForRequest(config *Config, req *http.Request, destination hostport if err != nil { config.Log.WithFields(logrus.Fields{ "error": err, - "role": role, + "Role": role, "upstream_proxy_name": req.Header.Get("X-Upstream-Https-Proxy"), "destination_host": destination.Host, "kind": "parse_failure", @@ -1024,13 +1025,13 @@ func checkACLsForRequest(config *Config, req *http.Request, destination hostport } ACLDecision, err := config.EgressACL.Decide(role, destination.Host, connectProxyHost) - decision.project = ACLDecision.Project - decision.reason = ACLDecision.Reason + decision.Project = ACLDecision.Project + decision.Reason = ACLDecision.Reason decision.MitmConfig = ACLDecision.MitmConfig if err != nil { config.Log.WithFields(logrus.Fields{ "error": err, - "role": role, + "Role": role, }).Warn("EgressAcl.Decide returned an error.") config.MetricsClient.Incr("acl.decide_error", 1) @@ -1038,9 +1039,9 @@ func checkACLsForRequest(config *Config, req *http.Request, destination hostport } tags := map[string]string{ - "role": decision.role, + "Role": decision.Role, "def_rule": fmt.Sprintf("%t", ACLDecision.Default), - "project": ACLDecision.Project, + "Project": ACLDecision.Project, } switch ACLDecision.Result { @@ -1060,11 +1061,11 @@ func checkACLsForRequest(config *Config, req *http.Request, destination hostport config.MetricsClient.IncrWithTags("acl.allow", tags, 1) default: config.Log.WithFields(logrus.Fields{ - "role": role, + "Role": role, "destination": destination.Host, "action": ACLDecision.Result.String(), }).Warn("Unknown ACL action") - decision.reason = "Internal error" + decision.Reason = "Internal error" config.MetricsClient.IncrWithTags("acl.unknown_error", tags, 1) } diff --git a/pkg/smokescreen/smokescreen_test.go b/pkg/smokescreen/smokescreen_test.go index 89718145..a2d81f8e 100644 --- a/pkg/smokescreen/smokescreen_test.go +++ b/pkg/smokescreen/smokescreen_test.go @@ -710,7 +710,7 @@ func TestProxyTimeouts(t *testing.T) { // for an EOF returned from HTTP client to indicate a connection interruption // which in our case represents the timeout. // - // To correctly hook into this, we'd need to pass a logger from Smokescreen to Goproxy + // To correctly hook into this, we'd need to pass a Logger from Smokescreen to Goproxy // which we have hooks into. This would be able to verify the timeout as errors from // each end of the connection pair are logged by Goproxy. t.Run("CONNECT proxy timeouts", func(t *testing.T) { @@ -1310,7 +1310,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("test-external-connect-proxy-blocked-srv", entry.Data["role"]) + r.Equal("test-external-connect-proxy-blocked-srv", entry.Data["Role"]) r.Equal(false, entry.Data["allow"]) }) @@ -1380,7 +1380,7 @@ func TestCONNECTProxyACLs(t *testing.T) { 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("test-external-connect-proxy-allowed-srv", entry.Data["Role"]) r.Equal(true, entry.Data["allow"]) }) From a6b1a3442305a0c5a814136123ab8833b5f1d05d Mon Sep 17 00:00:00 2001 From: Saurabh Bhatia Date: Thu, 10 Oct 2024 20:21:59 +0530 Subject: [PATCH 2/3] Revert Role fixes --- pkg/smokescreen/role_test.go | 8 ++++---- pkg/smokescreen/smokescreen.go | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/smokescreen/role_test.go b/pkg/smokescreen/role_test.go index 328cb42f..872e4905 100644 --- a/pkg/smokescreen/role_test.go +++ b/pkg/smokescreen/role_test.go @@ -25,14 +25,14 @@ func _testGetRole(t *testing.T, rfr_s string, rfr_e error, allow_missing bool, e t.Fatalf("expected err %v got %v\n", expect_e, e) } if s != expect_s { - t.Fatalf("expected Role %v got %v\n", expect_s, s) + t.Fatalf("expected role %v got %v\n", expect_s, s) } } func TestGetRole(t *testing.T) { - role := "some Role" + role := "some role" genErr := errors.New("general error") - mre := MissingRoleError("missing Role") + mre := MissingRoleError("missing role") t.Run("good", func(t *testing.T) { _testGetRole(t, role, nil, false, role, nil) @@ -44,7 +44,7 @@ func TestGetRole(t *testing.T) { t.Run("missing not allowed -> err", func(t *testing.T) { _testGetRole(t, "", mre, false, "", mre) }) - t.Run("missing allowed -> empty Role", func(t *testing.T) { + t.Run("missing allowed -> empty role", func(t *testing.T) { _testGetRole(t, "", mre, true, "", nil) }) } diff --git a/pkg/smokescreen/smokescreen.go b/pkg/smokescreen/smokescreen.go index 6f3e996d..67d8fe01 100644 --- a/pkg/smokescreen/smokescreen.go +++ b/pkg/smokescreen/smokescreen.go @@ -50,8 +50,8 @@ const ( LogFieldTraceID = "trace_id" LogFieldInRemoteX509CN = "inbound_remote_x509_cn" LogFieldInRemoteX509OU = "inbound_remote_x509_ou" - LogFieldRole = "Role" - LogFieldProject = "Project" + LogFieldRole = "role" + LogFieldProject = "project" LogFieldContentLength = "content_length" LogFieldDecisionReason = "decision_reason" LogFieldEnforceWouldDeny = "enforce_would_deny" @@ -938,7 +938,7 @@ func getRole(config *Config, req *http.Request) (string, error) { "error": err, "is_missing_role": IsMissingRoleError(err), "allow_missing_role": config.AllowMissingRole, - }).Error("Unable to get Role for request") + }).Error("Unable to get role for request") return "", err } } @@ -981,7 +981,7 @@ func checkACLsForRequest(config *Config, req *http.Request, destination hostport role, roleErr := getRole(config, req) if roleErr != nil { config.MetricsClient.Incr("acl.role_not_determined", 1) - decision.Reason = "Client Role cannot be determined" + decision.Reason = "Client role cannot be determined" return decision } @@ -1011,7 +1011,7 @@ func checkACLsForRequest(config *Config, req *http.Request, destination hostport if err != nil { config.Log.WithFields(logrus.Fields{ "error": err, - "Role": role, + "role": role, "upstream_proxy_name": req.Header.Get("X-Upstream-Https-Proxy"), "destination_host": destination.Host, "kind": "parse_failure", @@ -1039,9 +1039,9 @@ func checkACLsForRequest(config *Config, req *http.Request, destination hostport } tags := map[string]string{ - "Role": decision.Role, + "role": decision.Role, "def_rule": fmt.Sprintf("%t", ACLDecision.Default), - "Project": ACLDecision.Project, + "project": ACLDecision.Project, } switch ACLDecision.Result { @@ -1061,7 +1061,7 @@ func checkACLsForRequest(config *Config, req *http.Request, destination hostport config.MetricsClient.IncrWithTags("acl.allow", tags, 1) default: config.Log.WithFields(logrus.Fields{ - "Role": role, + "role": role, "destination": destination.Host, "action": ACLDecision.Result.String(), }).Warn("Unknown ACL action") From c75cffbae9c2ad5b33217e52536e506b090fc8e1 Mon Sep 17 00:00:00 2001 From: Saurabh Bhatia Date: Thu, 10 Oct 2024 20:26:04 +0530 Subject: [PATCH 3/3] Revert Role fixes --- pkg/smokescreen/smokescreen.go | 2 +- pkg/smokescreen/smokescreen_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/smokescreen/smokescreen.go b/pkg/smokescreen/smokescreen.go index 67d8fe01..6c49af9b 100644 --- a/pkg/smokescreen/smokescreen.go +++ b/pkg/smokescreen/smokescreen.go @@ -1031,7 +1031,7 @@ func checkACLsForRequest(config *Config, req *http.Request, destination hostport if err != nil { config.Log.WithFields(logrus.Fields{ "error": err, - "Role": role, + "role": role, }).Warn("EgressAcl.Decide returned an error.") config.MetricsClient.Incr("acl.decide_error", 1) diff --git a/pkg/smokescreen/smokescreen_test.go b/pkg/smokescreen/smokescreen_test.go index a2d81f8e..22532981 100644 --- a/pkg/smokescreen/smokescreen_test.go +++ b/pkg/smokescreen/smokescreen_test.go @@ -1310,7 +1310,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("test-external-connect-proxy-blocked-srv", entry.Data["Role"]) + r.Equal("test-external-connect-proxy-blocked-srv", entry.Data["role"]) r.Equal(false, entry.Data["allow"]) }) @@ -1380,7 +1380,7 @@ func TestCONNECTProxyACLs(t *testing.T) { 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("test-external-connect-proxy-allowed-srv", entry.Data["role"]) r.Equal(true, entry.Data["allow"]) })