Add MITM support to smokescreen #225

harold-stripe · 2024-09-10T06:58:26Z

Description

This PR adds MITM support:

New DSL for config and ACL which:
- allows adding headers on the fly
- detailed HTTP logging (URL, method and headers with values redacted)
- allowlist of headers that should not be redacted
YAML support for this DSL

I've also:

Updated the stripe/goproxy dependency as this PR depends on it
Created a Development.md which now includes instructions on how to run locally for multiple scenarios:
- HTTP Proxy
- HTTP CONNECT Proxy
- Monitor metrics Smokescreen emits
- HTTP CONNECT Proxy over TL
- MITM (Man in the middle) Proxy
- MITM (Man in the middle) Proxy over TLS
Added .vscode/launch.json to allow debugging easily in Vscode
Removed config *Config argument from logProxy as it was un-used
Changes the way extractContextLogFields are added to accommodate the MITM code

pctx.RoundTripper which is normally used for http proxy request is now also used by the MITM outbound request. By default it pools requests and keep them idle for a short period of time to potentially re-use. (even when Response.Body.Close() is run) This doesn't work well with InstrumentedConn that logs CANONICAL-PROXY-CN-CLOSE once the connection is closed.

There are multiple ways to go around this but I chose to run proxy.Tr.CloseIdleConnections as this closes any connections which were previously connected from previous requests but are now sitting idle in a "keep-alive" state. This proxy is not primarily intended to support browser traffic and the performance gain of keeping this connection is negligible for our use-case.

Alternatives considered:

req.Header.Set("Connection", "close") would work but the header is wiped in goproxy which calls removeProxyHeaders which deletes the Connection header.
Create a custom dialer and close the connection manually but this added unnecessary complexity.

Testing

I have added automated tests for ACL and the whole MITM flow.

HTTP Proxy (happy path) ✅

See set-up from Development.md HTTP Proxy)

go run . --config-file config.yaml --egress-acl-file acl.yaml
{"level":"info","msg":"Loading egress ACL from acl.yaml","time":"2024-09-09T17:16:34.234136+02:00"}
{"level":"info","msg":"starting","time":"2024-09-09T17:16:34+02:00"}
{"allow":true,"conn_establish_time_ms":98,"content_length":-1,"decision_reason":"host matched allowed domain in rule","dns_lookup_time_ms":1,"enforce_would_deny":false,"id":"crfn7lut29fjr92l4g70","inbound_remote_addr":"[::1]:52470","level":"info","msg":"CANONICAL-PROXY-DECISION","outbound_local_addr":"[2a02:8428:5ae6:a601:854:83b0:cc2a:fc61]:52471","outbound_remote_addr":"[2606:2800:21f:cb07:6820:80da:af6b:8b2c]:80","proxy_type":"http","requested_host":"example.com","start_time":"2024-09-09T15:16:55.303862Z","time":"2024-09-09T17:16:55+02:00","trace_id":""}

HTTP CONNECT Proxy (happy path) ✅

See set-up from Development.md HTTP CONNECT Proxy)

go run . --config-file config.yaml --egress-acl-file acl.yaml
{"level":"info","msg":"Loading egress ACL from acl.yaml","time":"2024-09-09T17:42:55.710681+02:00"}
{"level":"info","msg":"starting","time":"2024-09-09T17:42:55+02:00"}
{"allow":true,"decision_reason":"host matched allowed domain in rule","dns_lookup_time_ms":6,"enforce_would_deny":false,"id":"crflrnut29fufb87e880","inbound_remote_addr":"[::1]:64026","level":"info","msg":"CANONICAL-PROXY-DECISION","project":"security","proxy_type":"connect","requested_host":"api.github.com:443","role":"","start_time":"2024-09-09T15:43:11.728056Z","time":"2024-09-09T17:43:11+02:00","trace_id":""}
{"bytes_in":4706,"bytes_out":594,"conn_establish_time_ms":92,"duration":0.468236125,"end_time":"2024-09-09T15:43:12.295369Z","error":"","id":"crflrnut29fufb87e880","inbound_remote_addr":"[::1]:64026","last_activity":"2024-09-09T15:43:12.294314Z","level":"info","msg":"CANONICAL-PROXY-CN-CLOSE","outbound_local_addr":"192.168.1.65:64027","outbound_remote_addr":"140.82.121.5:443","project":"security","proxy_type":"connect","requested_host":"api.github.com:443","role":"","start_time":"2024-09-09T15:43:11.728056Z","time":"2024-09-09T17:43:12+02:00","trace_id":""}

HTTP CONNECT Proxy over TLS (happy path) ✅

See set-up from Development.md HTTP CONNECT Proxy over TLS)

go run . --config-file config.yaml --egress-acl-file acl.yaml
warn: no statsd addr provided, using noop client
info: Loaded CA with Authority ID 'ffdb815e7ba5132cbd786c176175c6107d26809b'
warn: no CRL loaded for Authority ID 'ffdb815e7ba5132cbd786c176175c6107d26809b'
{"level":"info","msg":"Loading egress ACL from acl.yaml","time":"2024-09-09T17:54:56.824782+02:00"}
{"level":"info","msg":"starting","time":"2024-09-09T17:54:56+02:00"}
{"allow":true,"decision_reason":"host matched allowed domain in rule","dns_lookup_time_ms":593,"enforce_would_deny":false,"id":"crfm1d6t29fgv2nj56n0","inbound_remote_addr":"[::1]:52641","inbound_remote_x509_cn":"localhost","inbound_remote_x509_ou":"Writer","level":"info","msg":"CANONICAL-PROXY-DECISION","project":"github","proxy_type":"connect","requested_host":"api.github.com:443","role":"localhost","start_time":"2024-09-09T15:55:16.019972Z","time":"2024-09-09T17:55:16+02:00","trace_id":""}
{"bytes_in":4680,"bytes_out":594,"conn_establish_time_ms":219,"duration":0.893843,"end_time":"2024-09-09T15:55:17.727861Z","error":"","id":"crfm1d6t29fgv2nj56n0","inbound_remote_addr":"[::1]:52641","inbound_remote_x509_cn":"localhost","inbound_remote_x509_ou":"Writer","last_activity":"2024-09-09T15:55:17.726988Z","level":"info","msg":"CANONICAL-PROXY-CN-CLOSE","outbound_local_addr":"192.168.1.65:52648","outbound_remote_addr":"140.82.121.6:443","project":"github","proxy_type":"connect","requested_host":"api.github.com:443","role":"localhost","start_time":"2024-09-09T15:55:16.019972Z","time":"2024-09-09T17:55:17+02:00","trace_id":""}

MITM (Man in the middle) Proxy (happy path) ✅

See set-up from Development.md MITM (Man in the middle) Proxy)

go run . --config-file config.yaml --egress-acl-file acl.yaml
warn: no statsd addr provided, using noop client
{"level":"info","msg":"Loading egress ACL from acl.yaml","time":"2024-09-09T18:11:30.835473+02:00"}
{"level":"info","msg":"starting","time":"2024-09-09T18:11:30+02:00"}
{"allow":true,"decision_reason":"host matched allowed domain in rule","dns_lookup_time_ms":87,"enforce_would_deny":false,"id":"crfm93mt29fikttq21bg","inbound_remote_addr":"[::1]:59008","level":"info","msg":"CANONICAL-PROXY-DECISION","project":"security","proxy_type":"connect","requested_host":"wttr.in:443","role":"","start_time":"2024-09-09T16:11:42.641312Z","time":"2024-09-09T18:11:42+02:00","trace_id":""}
{"bytes_in":12109,"bytes_out":479,"conn_establish_time_ms":25,"duration":0.497213875,"end_time":"2024-09-09T16:11:43.461775Z","error":"","id":"crfm93mt29fikttq21bg","inbound_remote_addr":"[::1]:59008","last_activity":"2024-09-09T16:11:43.461714Z","level":"info","mitm_req_headers":{"Accept":["[REDACTED]"],"Accept-Language":["[REDACTED]"],"User-Agent":["curl/8.7.1"]},"mitm_req_method":"GET","mitm_req_url":"https://wttr.in:443/","msg":"CANONICAL-PROXY-CN-CLOSE","outbound_local_addr":"192.168.1.65:59009","outbound_remote_addr":"5.9.243.187:443","project":"security","proxy_type":"connect","requested_host":"wttr.in:443","role":"","start_time":"2024-09-09T16:11:42.641312Z","time":"2024-09-09T18:11:43+02:00","trace_id":""}

Accept-Language: el was correctly sent

Notice the mitm_req_headers, mitm_req_method and mitm_req_url fields

MITM (Man in the middle) Proxy over TLS (happy path) ✅

See set-up from Development.md MITM (Man in the middle) Proxy over TLS)
Accept-Language: el was correctly sent (weather is in Greek)

go run . --config-file config.yaml --egress-acl-file acl.yaml
warn: no statsd addr provided, using noop client
info: Loaded CA with Authority ID 'ffdb815e7ba5132cbd786c176175c6107d26809b'
warn: no CRL loaded for Authority ID 'ffdb815e7ba5132cbd786c176175c6107d26809b'
{"level":"info","msg":"Loading egress ACL from acl.yaml","time":"2024-09-09T18:17:01.764258+02:00"}
{"level":"info","msg":"starting","time":"2024-09-09T18:17:01+02:00"}
{"allow":true,"decision_reason":"host matched allowed domain in rule","dns_lookup_time_ms":15,"enforce_would_deny":false,"id":"crfmbket29fj5e1mvrag","inbound_remote_addr":"[::1]:61252","inbound_remote_x509_cn":"localhost","inbound_remote_x509_ou":"Writer","level":"info","msg":"CANONICAL-PROXY-DECISION","project":"github","proxy_type":"connect","requested_host":"wttr.in:443","role":"localhost","start_time":"2024-09-09T16:17:05.96784Z","time":"2024-09-09T18:17:05+02:00","trace_id":""}
{"bytes_in":12109,"bytes_out":479,"conn_establish_time_ms":36,"duration":0.065963916,"end_time":"2024-09-09T16:17:06.243684Z","error":"","id":"crfmbket29fj5e1mvrag","inbound_remote_addr":"[::1]:61252","inbound_remote_x509_cn":"localhost","inbound_remote_x509_ou":"Writer","last_activity":"2024-09-09T16:17:06.243663Z","level":"info","mitm_req_headers":{"Accept":["[REDACTED]"],"Accept-Language":["[REDACTED]"],"User-Agent":["curl/8.7.1"]},"mitm_req_method":"GET","mitm_req_url":"https://wttr.in:443/","msg":"CANONICAL-PROXY-CN-CLOSE","outbound_local_addr":"192.168.1.44:61255","outbound_remote_addr":"5.9.243.187:443","project":"github","proxy_type":"connect","requested_host":"wttr.in:443","role":"localhost","start_time":"2024-09-09T16:17:05.96784Z","time":"2024-09-09T18:17:06+02:00","trace_id":""}

Notice the mitm_req_headers, mitm_req_method, mitm_req_url (MITM), inbound_remote_x509_cn and inbound_remote_x509_ou (TLS) fields.

MITM config not configured with ACL configured ✅

# config.yaml
---
tls:
  cert_file: "mtls_setup/server.crt"
  key_file: "mtls_setup/server.key"
  client_ca_files:
    - "mtls_setup/client-ca.crt"

# acl.yaml
---
version: v1
services:
  - name: localhost
    project: github
    action: enforce
    allowed_domains: []
    allowed_domains_mitm:
      - domain: wttr.in
        add_headers:
          Accept-Language: el
        detailed_http_logs: true
        detailed_http_logs_full_headers:
          - User-Agent
default:
  name: default
  project: security
  action: enforce
  allowed_domains: []

go run . --config-file config.yaml --egress-acl-file acl.yaml

warn: no statsd addr provided, using noop client
info: Loaded CA with Authority ID 'ffdb815e7ba5132cbd786c176175c6107d26809b'
warn: no CRL loaded for Authority ID 'ffdb815e7ba5132cbd786c176175c6107d26809b'
{"level":"info","msg":"Loading egress ACL from acl.yaml","time":"2024-09-09T18:58:32.027806+02:00"}
{"level":"info","msg":"starting","time":"2024-09-09T18:58:32+02:00"}
{"allow":false,"content_length":119,"decision_reason":"ACLDecision specified MITM but Smokescreen doesn't have MITM enabled","dns_lookup_time_ms":18,"enforce_would_deny":false,"id":"crfmv56t29fhhhmhr5i0","inbound_remote_addr":"[::1]:61235","inbound_remote_x509_cn":"localhost","inbound_remote_x509_ou":"Writer","level":"warning","msg":"CANONICAL-PROXY-DECISION","project":"github","proxy_type":"connect","requested_host":"wttr.in:443","role":"localhost","start_time":"2024-09-09T16:58:44.319077Z","time":"2024-09-09T18:58:44+02:00","trace_id":""}

Miss-configuration fails gracefully

coveralls · 2024-09-10T07:00:47Z

Pull Request Test Coverage Report for Build 10911714189

Details

165 of 206 (80.1%) changed or added relevant lines in 4 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage increased (+1.3%) to 54.407%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/smokescreen/acl/v1/yaml_loader.go	19	23	82.61%
pkg/smokescreen/acl/v1/acl.go	49	57	85.96%
pkg/smokescreen/smokescreen.go	97	108	89.81%
pkg/smokescreen/config_loader.go	0	18	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/smokescreen/config_loader.go	1	0.0%
pkg/smokescreen/acl/v1/acl.go	1	88.72%

Totals
Change from base Build 10911699761:	1.3%
Covered Lines:	1426
Relevant Lines:	2621

💛 - Coveralls

cds2-stripe

Left a first round of comments!

cds2-stripe · 2024-09-12T19:47:29Z

README.md

-```shellsession
-$ go test ./...
-```
+See [Development.md](Development.md)

 # Contributors


You should add yourself!

pkg/smokescreen/acl/v1/acl.go

cds2-stripe · 2024-09-12T19:53:31Z

pkg/smokescreen/acl/v1/acl.go

+	// if the host matches any of the rule's allowed domains with MITM config, allow
+	for _, dg := range rule.DomainMitmGlobs {
+		if HostMatchesGlob(host, dg.Domain) {
+			d.Result, d.Reason = Allow, "host matched allowed domain in rule"


Can we update reason to reference that this matched an allowed domain in the MITM globs?

Also, do we need this check? Why would a proxy request need to be MITMd but not be in the original ACL rule?

Good point.
On the check, are you talking about HostMatchesGlob? host is the current host being checked for a decision. So this loop is trying to find if there's a MITM rules corresponding to the current host

I believe I meant why are we updating the decision reason if we're only trying to determine if the domain needs to be MITMd. Shouldn't the ACL checking behavior be separate?

pkg/smokescreen/acl/v1/acl.go

pkg/smokescreen/acl/v1/yaml_loader.go

pkg/smokescreen/config_loader.go

pkg/smokescreen/smokescreen.go

cds2-stripe · 2024-09-12T20:14:10Z

pkg/smokescreen/smokescreen.go

-	if sctx.proxyType == connectProxy {
+	if sctx.proxyType == connectProxy || pctx.ConnectAction == goproxy.ConnectMitm {
+		// If we have a MITM and option is enabled, we can add detailed Request log fields
+		if pctx.ConnectAction == goproxy.ConnectMitm && sctx.Decision.MitmConfig != nil && sctx.Decision.MitmConfig.DetailedHttpLogs {


Does the MitmConfig reference need to live with the decision? Could we not move this directly into the context?

Decision.MitmConfig comes from ACLDecision.MitmConfig because the config is in the rule which comes from rule.DomainMitmGlobs
Al these happen in the decision functions (checkIfRequestShouldBeProxied, checkACLsForRequest, config.EgressACL.Decide and none of these have any context.
Do you think we should pass pctx down to all these methods to set MitmConfig there?

Mmmm no I don't feel too strongly about this, and it's fine leaving if the context isn't already available where we need it (I thought it was).

pkg/smokescreen/smokescreen.go

harold-stripe · 2024-09-13T09:55:24Z

Thanks for this great review! I've actioned all your comments except Decision.MitmConfig where I have replied to the comment.

I have re-tested ALL manual tests listed in the description of this PR.
Changes are also covered by automated tests.

harold-stripe · 2024-09-17T07:47:39Z

We talked on Slack and agreed it would be best for allowed_domains to be the single source of truth for the decision and have mitm_domains be an optional config which can be populated per domain

cds2-stripe

Thanks for addressing all of the feedback! Left one comment about a function name but otherwise LGTM

cds2-stripe · 2024-09-16T15:37:11Z

pkg/smokescreen/config_loader.go

-			return fmt.Errorf("could not load mitmCa: %v", err)
+			return fmt.Errorf("mitm_ca_key_file error tls.LoadX509KeyPair: %w", err)
+		}
+		// set the leaf certificat to reduce per-handshake processing


nit: certificate?

cds2-stripe · 2024-09-16T15:44:06Z

pkg/smokescreen/acl/v1/acl.go

+	// if the host matches any of the rule's allowed domains with MITM config, allow
+	for _, dg := range rule.DomainMitmGlobs {
+		if HostMatchesGlob(host, dg.Domain) {
+			d.Result, d.Reason = Allow, "host matched allowed domain in rule"


I believe I meant why are we updating the decision reason if we're only trying to determine if the domain needs to be MITMd. Shouldn't the ACL checking behavior be separate?

cds2-stripe · 2024-09-16T15:47:52Z

pkg/smokescreen/smokescreen.go

-	if sctx.proxyType == connectProxy {
+	if sctx.proxyType == connectProxy || pctx.ConnectAction == goproxy.ConnectMitm {
+		// If we have a MITM and option is enabled, we can add detailed Request log fields
+		if pctx.ConnectAction == goproxy.ConnectMitm && sctx.Decision.MitmConfig != nil && sctx.Decision.MitmConfig.DetailedHttpLogs {


Mmmm no I don't feel too strongly about this, and it's fine leaving if the context isn't already available where we need it (I thought it was).

cds2-stripe · 2024-09-17T19:50:26Z

pkg/smokescreen/acl/v1/acl.go

@@ -83,7 +83,7 @@ func (acl *ACL) Add(svc string, r Rule) error {
 		return err
 	}

-	err = acl.ValidateRuleDomainsGlobs(svc, r)
+	err = acl.ValidateRuleDomainsGlobsAndMitm(svc, r)


Should this actually just be ValidateRule since we're passing in the whole rule now?

harold-stripe assigned cds2-stripe Sep 10, 2024

harold-stripe changed the title ~~Harold/mitm support~~ Add MITM support to smokescreen Sep 10, 2024

harold-stripe force-pushed the harold/mitm_support branch from 7520500 to f4ef674 Compare September 12, 2024 14:25

cds2-stripe requested changes Sep 12, 2024

View reviewed changes

harold-stripe force-pushed the harold/mitm_support branch 3 times, most recently from 85d2bc1 to 85fea80 Compare September 13, 2024 09:52

harold-stripe force-pushed the harold/mitm_support branch from 85fea80 to 8fcae44 Compare September 13, 2024 09:56

harold-stripe force-pushed the harold/mitm_support branch 11 times, most recently from 6c30765 to b319203 Compare September 17, 2024 19:12

harold-stripe closed this Sep 17, 2024

This was referenced Sep 17, 2024

Harold/mitm support #226

Closed

Add MITM support to smokescreen #227

Closed

harold-stripe reopened this Sep 17, 2024

harold-stripe mentioned this pull request Sep 17, 2024

Update Github build workflows #228

Merged

harold-stripe force-pushed the harold/mitm_support branch from b319203 to c905517 Compare September 17, 2024 19:41

cds2-stripe previously approved these changes Sep 17, 2024

View reviewed changes

harold-stripe dismissed cds2-stripe’s stale review via b936c70 September 17, 2024 20:06

harold-stripe force-pushed the harold/mitm_support branch 4 times, most recently from b936c70 to 7311611 Compare September 17, 2024 20:41

harold-s added 6 commits September 17, 2024 23:55

go get -d github.com/stripe/goproxy@latest && go mod vendor

506e362

Add MITM support to Smokescreen

734c343

Use MitmTLSConfig in the config instead of MitmCa

4e1b3e2

PR feedback + remove CloseIdleConnections

ddde90f

Refactor allowed_domains_mitm to mitm_domains

92537ef

Rename ValidateRule

4cf6e0b

harold-stripe force-pushed the harold/mitm_support branch from 7311611 to 4cf6e0b Compare September 17, 2024 21:55

cds2-stripe approved these changes Sep 17, 2024

View reviewed changes

harold-stripe merged commit dab4bde into master Sep 17, 2024
9 checks passed

harold-stripe deleted the harold/mitm_support branch September 17, 2024 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MITM support to smokescreen #225

Add MITM support to smokescreen #225

harold-stripe commented Sep 10, 2024 •

edited

Loading

coveralls commented Sep 10, 2024 •

edited

Loading

cds2-stripe left a comment

cds2-stripe Sep 12, 2024

cds2-stripe Sep 12, 2024

harold-stripe Sep 13, 2024

cds2-stripe Sep 16, 2024

cds2-stripe Sep 12, 2024

harold-stripe Sep 13, 2024

cds2-stripe Sep 16, 2024

harold-stripe commented Sep 13, 2024 •

edited

Loading

harold-stripe commented Sep 17, 2024

cds2-stripe left a comment

cds2-stripe Sep 16, 2024

cds2-stripe Sep 16, 2024

cds2-stripe Sep 16, 2024

cds2-stripe Sep 17, 2024

harold-stripe Sep 17, 2024

Add MITM support to smokescreen #225

Add MITM support to smokescreen #225

Conversation

harold-stripe commented Sep 10, 2024 • edited Loading

Description

Testing

HTTP Proxy (happy path) ✅

HTTP CONNECT Proxy (happy path) ✅

HTTP CONNECT Proxy over TLS (happy path) ✅

MITM (Man in the middle) Proxy (happy path) ✅

MITM (Man in the middle) Proxy over TLS (happy path) ✅

MITM config not configured with ACL configured ✅

coveralls commented Sep 10, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10911714189

Details

💛 - Coveralls

cds2-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harold-stripe commented Sep 13, 2024 • edited Loading

harold-stripe commented Sep 17, 2024

cds2-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harold-stripe commented Sep 10, 2024 •

edited

Loading

coveralls commented Sep 10, 2024 •

edited

Loading

harold-stripe commented Sep 13, 2024 •

edited

Loading