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.

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

Implements missing HTTPS proxy functionality #978

Merged
merged 1 commit into from
Mar 19, 2025

Conversation

seans3
Copy link
Contributor

@seans3 seans3 commented Feb 27, 2025

What type of PR is this? (check all applicable)

  • Feature

Description

  • Implements missing HTTPS Proxy functionality.
  • Adds client/proxy/server unit tests.

Related Tickets & Documents

Added/updated tests?

  • Yes

Run verifications and test

  • Previous Unit Test Coverage: 83.6%
  • Current Unit Test Coverage: 84.2%
$ go test -race -cover
PASS
coverage: 84.2% of statements
ok  	github.com/gorilla/websocket	4.103s

@seans3
Copy link
Contributor Author

seans3 commented Feb 27, 2025

/cc @adrianosela

@seans3
Copy link
Contributor Author

seans3 commented Feb 27, 2025

/assign @liggitt
/assign @aojea

Copy link

@adrianosela adrianosela left a comment

Choose a reason for hiding this comment

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

Thanks for this @seans3 🚀

@aojea
Copy link

aojea commented Feb 27, 2025

IIUIC the same certificate is used for both the proxy and the backend, don't we need to handle the case when both are different? what happens if the proxy requires a different certificate than the backend?

EDIT

I see, the TLSConfig can be used for that, also @seans3 sent me this https://github.com/adrianosela/https-proxy/tree/main?tab=readme-ov-file#https-proxy

@seans3
Copy link
Contributor Author

seans3 commented Feb 27, 2025

IIUIC the same certificate is used for both the proxy and the backend, don't we need to handle the case when both are different? what happens if the proxy requires a different certificate than the backend?

The client is responsible for configuring the root CA certificates. The client TLS config contains a cert pool, which can contain multiple root CA certificates. So for TLS to the proxy and the upstream, the TLS config root CA cert pool must contain CA's which have signed both the proxy and the upstream.

@134130
Copy link

134130 commented Mar 3, 2025

This PR also closes #950

@seans3 seans3 force-pushed the https-proxy branch 7 times, most recently from 2ab73bd to 0b0f26a Compare March 5, 2025 00:17
@134130 134130 mentioned this pull request Mar 5, 2025
12 tasks
@seans3 seans3 force-pushed the https-proxy branch 2 times, most recently from d066f96 to efcac3b Compare March 11, 2025 21:05
@seans3 seans3 requested review from adrianosela and aojea March 11, 2025 22:12
@AlexVulaj
Copy link
Member

Thanks for the PR @seans3 , this looks good to me!

@AlexVulaj AlexVulaj merged commit e064f32 into gorilla:main Mar 19, 2025
1 check passed
@seans3
Copy link
Contributor Author

seans3 commented Mar 20, 2025

@AlexVulaj Thanks for the prompt review. Would it be appropriate to cut the next v1.5.4 release? Again, thanks :)

@liggitt liggitt mentioned this pull request Mar 20, 2025
2 tasks
@liggitt
Copy link

liggitt commented Mar 20, 2025

probably want to merge the follow-up #982 to deflake the unit test before cutting a tag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is https not supported? [feature] Support for proxying websocket through https proxy
6 participants