Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for Smokescreen -> HTTPS CONNECT Proxy ACLs #213

Merged
merged 8 commits into from
Feb 16, 2024
Merged

Add support for Smokescreen -> HTTPS CONNECT Proxy ACLs #213

merged 8 commits into from
Feb 16, 2024

Conversation

pspieker-stripe
Copy link
Contributor

@pspieker-stripe pspieker-stripe commented Feb 12, 2024 •
edited
Loading

We want to be able to talk to an external-to-Stripe HTTPS CONNECT proxy. We'd also like to default block traffic going to any external-to-Stripe HTTPS CONNECT proxy, unless it's on a given allow list (basically a CONNECT proxy URL ACL).

This PR adds support for such an allow list in smokescreen.

Testing via:

cd pkg/smokescreen
go test -v -run ^TestCONNECTProxyACLs$
dlv test -- -test.run=^TestCONNECTProxyACLs$

@coveralls
Copy link

coveralls commented Feb 12, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 7925606600

Details

  • 9 of 19 (47.37%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 62.279%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/smokescreen/acl/v1/acl.go 2 12 16.67%
Totals Coverage Status
Change from base Build 7064362574: -0.2%
Covered Lines: 1306
Relevant Lines: 2097
💛 - Coveralls

@pspieker-stripe pspieker-stripe force-pushed the pspieker-add-proxy-acl-support branch 4 times, most recently from eac5e8e to d5c4b68 Compare February 14, 2024 22:09
@pspieker-stripe pspieker-stripe changed the title Add support for Smokescreen -> HTTPS CONNECT Proxy traffic routing Add support for Smokescreen -> HTTPS CONNECT Proxy ACLs Feb 14, 2024
cds2-stripe
cds2-stripe previously requested changes Feb 14, 2024
View reviewed changes
pkg/smokescreen/smokescreen.go Outdated Show resolved Hide resolved
pkg/smokescreen/smokescreen.go Outdated Show resolved Hide resolved
pkg/smokescreen/smokescreen.go Outdated Show resolved Hide resolved
pkg/smokescreen/testdata/acl.yaml
allowed_domains:
- 127.0.0.1
allowed_external_proxies:
- myproxy.com
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add multiple external proxies to make sure the glob gets constructed correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This role is just used for the non-allowed proxies list; do you mean adding localhost here? How are you thinking of testing glob construction? Is just adding some different possible URL shapes helpful here, or is there something in particular you'd like us to assert against?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the addresses here construct a glob which should match any allowed_external_proxies specific in the header. My thought was to add something like myproxy2.com to the end of the this and then verify it is still allowed as an external proxy in one of your tests.

pkg/smokescreen/smokescreen_test.go
@@ -1236,6 +1236,77 @@ func TestCustomRequestHandler(t *testing.T) {
})
}

func TestCONNECTProxyACLs(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this!

pkg/smokescreen/smokescreen.go Outdated Show resolved Hide resolved
@pspieker-stripe
Copy link
Contributor Author

pspieker-stripe commented Feb 15, 2024 •
edited
Loading

Not sure why Test / test (1.21) (pull_request) is failing now btw - seems flakey but not sure it's unrelated?

Edit: fixed now - lemurheavy/coveralls-public#1716

@pspieker-stripe
Introduce CONNECT Proxy URL ACL Support
6f1bd0b 
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
@pspieker-stripe
WIP
7c9175c
@pspieker-stripe
WIP
6bf0712
@pspieker-stripe
PR feedback 1
4363cdb
@pspieker-stripe
Fixed tests
1eceba7
@pspieker-stripe pspieker-stripe force-pushed the pspieker-add-proxy-acl-support branch from 31190ca to fff9e77 Compare February 16, 2024 03:36
@pspieker-stripe pspieker-stripe force-pushed the pspieker-add-proxy-acl-support branch from fff9e77 to 995a427 Compare February 16, 2024 03:59
@cds2-stripe cds2-stripe merged commit 5c3d435 into master Feb 16, 2024
5 checks passed
@cds2-stripe cds2-stripe deleted the pspieker-add-proxy-acl-support branch February 16, 2024 19:20
amber-higgins added a commit to intercom/smokescreen that referenced this pull request Jan 27, 2025
@amber-higgins @JulesDT
@jmcconnell26 @kevinv-stripe @sergeyrud-stripe @cmoresco-stripe @cds2-stripe @jjiang-stripe @ne-bknn @dependabot @xieyuxi-stripe @pspieker-stripe @gauthamw-stripe @harold-stripe @harold-s @saurabhbhatia-stripe @cuishuang @eastebry
Sync with upstream (#10)
5ef31cf 
* add a custom interface for the resolver instead of forcing *net.Resolver (stripe#187)

* feature/add prometheus metrics (stripe#179)

* STORY-25143 - Add prometheus metrics to smokescreen

* STORY-25143 - Cleanup

* STORY-25143 - Fix tests to compare new metric labels

* STORY-25143 - Host prometheus endpoint on separate port

* STORY-25143 - Use value provided via command line flag

* STORY-25143 - Add prometheus timing metrics

* STORY-25143 - Fix nil map assignment and prometheus metric name sanitisation

* STORY-25143 - Cleanup comments

* STORY-25143 - Remove some repetition + add further unit testing

* STORY-25143 - Document new prometheus features in README + add port flag to prometheus config

* STORY-25143 - Make PR requested changes:
* Don't export metrics list
* Follow project sytlistic choices

* STORY-25143 - Rename only one receiver

* STORY-25143 - Add new `--expose-prometheus-metrics` flag to CLI to toggle exposing prometheus metrics

* Small cleanup of timer metrics

* Fix go module vendoring

* Use ElementsMatch to ignore order

* Just use require

* Move the custom request handler call after the main acl check

* Use local server instead of httpbin (stripe#192)

* Do not return a denyError for DNS resolution failures (stripe#194)

* dont return denial errors for dns resolution failures

* fix test

* move DNSError check into net.Error assertion, extend test

* fix integration test

* add AcceptResponseHandler to modify accepted responses (stripe#196)

* add AcceptResponseHandler to modify accepted responses

* customer->custom

* Update docs to clarify global_deny_list (stripe#197)

* update docs to clarify global_deny_list behavior

* consistent example domain

* be more concise

* Use AcceptResponseHandler in goproxy https CONNECT hook (stripe#199)

* pipe AcceptResponseHandler into new goproxy hook

* update comment

* go mod vendor

* unit test

* use smokescreenctx in acceptresponsehandler

* fix unit tests

* Export SmokescreenContext type (stripe#200)

* export SmokescreenContext type

* also export AclDecision

* ResolvedAddr too

* consistent caps

* Update pkg/smokescreen/smokescreen.go

Co-authored-by: jjiang-stripe <55402658+jjiang-stripe@users.noreply.github.com>

* export Decision

---------

Co-authored-by: jjiang-stripe <55402658+jjiang-stripe@users.noreply.github.com>

* generate new test pki (stripe#206)

* allow listen address specification for prom (stripe#203)

* Bump golang.org/x/net from 0.7.0 to 0.17.0 (stripe#204)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.7.0 to 0.17.0.
- [Commits](golang/net@v0.7.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* bump go versions (stripe#207)

* update dependency

* configure addr in smokescreen and add unit test

* use fmt

* try this workaround

* variable name change

* Update docs to disambiguate ACL vs --deny-address behavior (stripe#210)

* update docs to clarify how IP filtering works

* fix fields bug

* remove extra field setting

* trigger build

* Add support for Smokescreen -> HTTPS CONNECT Proxy ACLs (stripe#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

* Bump goproxy version to incorporate CONNECT proxy header changes

* WIP

* Bump google.golang.org/protobuf from 1.28.1 to 1.33.0 (stripe#216)

Bumps google.golang.org/protobuf from 1.28.1 to 1.33.0.

---
updated-dependencies:
- dependency-name: google.golang.org/protobuf
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Add support for username / password auth in URLs to external CONNECT proxies (stripe#222)

* Add support for UN / PW Auth for External CONNECT Proxies

* Fixed naming of log line

* PR feedback

* Debug commit

* Removing modifications of vendor-ed code

* Removed debug

* Removed missed cruft

* Fixed bug with env var proxy arg

* Add failure kind

* update goproxy version to master commit

* Ensure proxy passed in X-Upstream-Https-Proxy is parsable

* Update Github build workflows (stripe#228)

Co-authored-by: Harold Simpson <harold@stripe.com>

* Use goveralls parallel build

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

* Add MITM support to Smokescreen

* Use MitmTLSConfig in the config instead of MitmCa

* PR feedback + remove CloseIdleConnections

* Refactor allowed_domains_mitm to mitm_domains

* Rename ValidateRule

* Add Support for Reject Handler with Context

* Update comment

* Block smokescreen init incase of invalid config

* fix: fix slice init length

* Remove duplicate validation

* Make SmokeScreen Fields Public

* Revert Role fixes

* Revert Role fixes

* Update goproxy version to v0.0.0-20241017101008-e12ef0653f22 (stripe#235)

* Adding [allow|deny]_addresses settings to yaml config file

* Update goproxy version to v0.0.0-20241022131412-58117846327a (stripe#238)

* Ignore goveralls

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: JulesD <JulesDT@users.noreply.github.com>
Co-authored-by: Josh McConnell <josh.mcconnell465@gmail.com>
Co-authored-by: Kevin Vincent <kevinv@stripe.com>
Co-authored-by: kevinv-stripe <102822342+kevinv-stripe@users.noreply.github.com>
Co-authored-by: Sergey Rud <sergeyrud@stripe.com>
Co-authored-by: cmoresco-stripe <106690468+cmoresco-stripe@users.noreply.github.com>
Co-authored-by: Craig Shannon <cds@stripe.com>
Co-authored-by: jjiang-stripe <55402658+jjiang-stripe@users.noreply.github.com>
Co-authored-by: Timofey Bakunin <36561672+ne-bknn@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Yuxi Xie <xieyuxi@stripe.com>
Co-authored-by: xieyuxi-stripe <141708814+xieyuxi-stripe@users.noreply.github.com>
Co-authored-by: Jessica Jiang <jjiang@stripe.com>
Co-authored-by: pspieker-stripe <40726826+pspieker-stripe@users.noreply.github.com>
Co-authored-by: Patrick Spieker <pspieker@stripe.com>
Co-authored-by: Gautham Warrier <gauthamw@stripe.com>
Co-authored-by: gauthamw-stripe <109298754+gauthamw-stripe@users.noreply.github.com>
Co-authored-by: harold-stripe <49162741+harold-stripe@users.noreply.github.com>
Co-authored-by: Harold Simpson <harold@stripe.com>
Co-authored-by: Saurabh Bhatia <saurabhbhatia@stripe.com>
Co-authored-by: cui fliter <imcusg@gmail.com>
Co-authored-by: Bryan Eastes <eastebry@users.noreply.github.com>
amber-higgins added a commit to intercom/smokescreen that referenced this pull request Jan 27, 2025
@amber-higgins @JulesDT
@jmcconnell26 @kevinv-stripe @sergeyrud-stripe @cmoresco-stripe @cds2-stripe @jjiang-stripe @ne-bknn @dependabot @xieyuxi-stripe @pspieker-stripe @gauthamw-stripe @harold-stripe @harold-s @saurabhbhatia-stripe @cuishuang @eastebry
Sync with master (again) [skip ci] (#14)
5b5b8b9 
* add a custom interface for the resolver instead of forcing *net.Resolver (stripe#187)

* feature/add prometheus metrics (stripe#179)

* STORY-25143 - Add prometheus metrics to smokescreen

* STORY-25143 - Cleanup

* STORY-25143 - Fix tests to compare new metric labels

* STORY-25143 - Host prometheus endpoint on separate port

* STORY-25143 - Use value provided via command line flag

* STORY-25143 - Add prometheus timing metrics

* STORY-25143 - Fix nil map assignment and prometheus metric name sanitisation

* STORY-25143 - Cleanup comments

* STORY-25143 - Remove some repetition + add further unit testing

* STORY-25143 - Document new prometheus features in README + add port flag to prometheus config

* STORY-25143 - Make PR requested changes:
* Don't export metrics list
* Follow project sytlistic choices

* STORY-25143 - Rename only one receiver

* STORY-25143 - Add new `--expose-prometheus-metrics` flag to CLI to toggle exposing prometheus metrics

* Small cleanup of timer metrics

* Fix go module vendoring

* Use ElementsMatch to ignore order

* Just use require

* Move the custom request handler call after the main acl check

* Use local server instead of httpbin (stripe#192)

* Do not return a denyError for DNS resolution failures (stripe#194)

* dont return denial errors for dns resolution failures

* fix test

* move DNSError check into net.Error assertion, extend test

* fix integration test

* add AcceptResponseHandler to modify accepted responses (stripe#196)

* add AcceptResponseHandler to modify accepted responses

* customer->custom

* Update docs to clarify global_deny_list (stripe#197)

* update docs to clarify global_deny_list behavior

* consistent example domain

* be more concise

* Use AcceptResponseHandler in goproxy https CONNECT hook (stripe#199)

* pipe AcceptResponseHandler into new goproxy hook

* update comment

* go mod vendor

* unit test

* use smokescreenctx in acceptresponsehandler

* fix unit tests

* Export SmokescreenContext type (stripe#200)

* export SmokescreenContext type

* also export AclDecision

* ResolvedAddr too

* consistent caps

* Update pkg/smokescreen/smokescreen.go

Co-authored-by: jjiang-stripe <55402658+jjiang-stripe@users.noreply.github.com>

* export Decision

---------

Co-authored-by: jjiang-stripe <55402658+jjiang-stripe@users.noreply.github.com>

* generate new test pki (stripe#206)

* allow listen address specification for prom (stripe#203)

* Bump golang.org/x/net from 0.7.0 to 0.17.0 (stripe#204)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.7.0 to 0.17.0.
- [Commits](golang/net@v0.7.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* bump go versions (stripe#207)

* update dependency

* configure addr in smokescreen and add unit test

* use fmt

* try this workaround

* variable name change

* Update docs to disambiguate ACL vs --deny-address behavior (stripe#210)

* update docs to clarify how IP filtering works

* fix fields bug

* remove extra field setting

* trigger build

* Add support for Smokescreen -> HTTPS CONNECT Proxy ACLs (stripe#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

* Bump goproxy version to incorporate CONNECT proxy header changes

* WIP

* Bump google.golang.org/protobuf from 1.28.1 to 1.33.0 (stripe#216)

Bumps google.golang.org/protobuf from 1.28.1 to 1.33.0.

---
updated-dependencies:
- dependency-name: google.golang.org/protobuf
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Add support for username / password auth in URLs to external CONNECT proxies (stripe#222)

* Add support for UN / PW Auth for External CONNECT Proxies

* Fixed naming of log line

* PR feedback

* Debug commit

* Removing modifications of vendor-ed code

* Removed debug

* Removed missed cruft

* Fixed bug with env var proxy arg

* Add failure kind

* update goproxy version to master commit

* Ensure proxy passed in X-Upstream-Https-Proxy is parsable

* Update Github build workflows (stripe#228)

Co-authored-by: Harold Simpson <harold@stripe.com>

* Use goveralls parallel build

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

* Add MITM support to Smokescreen

* Use MitmTLSConfig in the config instead of MitmCa

* PR feedback + remove CloseIdleConnections

* Refactor allowed_domains_mitm to mitm_domains

* Rename ValidateRule

* Add Support for Reject Handler with Context

* Update comment

* Block smokescreen init incase of invalid config

* fix: fix slice init length

* Remove duplicate validation

* Make SmokeScreen Fields Public

* Revert Role fixes

* Revert Role fixes

* Update goproxy version to v0.0.0-20241017101008-e12ef0653f22 (stripe#235)

* Adding [allow|deny]_addresses settings to yaml config file

* Update goproxy version to v0.0.0-20241022131412-58117846327a (stripe#238)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: JulesD <JulesDT@users.noreply.github.com>
Co-authored-by: Josh McConnell <josh.mcconnell465@gmail.com>
Co-authored-by: Kevin Vincent <kevinv@stripe.com>
Co-authored-by: kevinv-stripe <102822342+kevinv-stripe@users.noreply.github.com>
Co-authored-by: Sergey Rud <sergeyrud@stripe.com>
Co-authored-by: cmoresco-stripe <106690468+cmoresco-stripe@users.noreply.github.com>
Co-authored-by: Craig Shannon <cds@stripe.com>
Co-authored-by: jjiang-stripe <55402658+jjiang-stripe@users.noreply.github.com>
Co-authored-by: Timofey Bakunin <36561672+ne-bknn@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Yuxi Xie <xieyuxi@stripe.com>
Co-authored-by: xieyuxi-stripe <141708814+xieyuxi-stripe@users.noreply.github.com>
Co-authored-by: Jessica Jiang <jjiang@stripe.com>
Co-authored-by: pspieker-stripe <40726826+pspieker-stripe@users.noreply.github.com>
Co-authored-by: Patrick Spieker <pspieker@stripe.com>
Co-authored-by: Gautham Warrier <gauthamw@stripe.com>
Co-authored-by: gauthamw-stripe <109298754+gauthamw-stripe@users.noreply.github.com>
Co-authored-by: harold-stripe <49162741+harold-stripe@users.noreply.github.com>
Co-authored-by: Harold Simpson <harold@stripe.com>
Co-authored-by: Saurabh Bhatia <saurabhbhatia@stripe.com>
Co-authored-by: cui fliter <imcusg@gmail.com>
Co-authored-by: Bryan Eastes <eastebry@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cds2-stripe cds2-stripe cds2-stripe approved these changes

Assignees

@cds2-stripe cds2-stripe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pspieker-stripe @coveralls @cds2-stripe @jjiang-stripe