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

Accept https proxy #740

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Accept https proxy #740

wants to merge 11 commits into from

Conversation

philipatl
Copy link

@philipatl philipatl commented Dec 3, 2021

Fixes #739

Summary of Changes

  1. call proxy_RegisterDialerType for the https scheme to allow communication to https proxies
  2. some scheme-specific setup is necessary in Dialer's DialContext in order to use the TLSClientConfig
  3. add a UsesTLS method to the proxy_Dialer interface to ensure the https forwardDialer can inspect that https is supported, and to adjust if necessary

@ghost
Copy link

ghost commented Dec 3, 2021

I am not a maintainer of the project, but I do have some comments on the CL:

  • The CL modifies the generated file x_net_proxy.go. Avoid editing the file or regenerate the file if a new version of x/net/proxy has the desired functionality.
  • The CL is missing tests.

@philipatl philipatl changed the title Tls proxy Accept https proxy Dec 6, 2021
@philipatl
Copy link
Author

I am not a maintainer of the project, but I do have some comments on the CL:

  • The CL modifies the generated file x_net_proxy.go. Avoid editing the file or regenerate the file if a new version of x/net/proxy has the desired functionality.
  • The CL is missing tests.

Thanks, I've made the suggested changes.

@philipatl
Copy link
Author

Is there anything else needed here that prevents merging? Any comments? @garyburd

christianbagley-viz pushed a commit to christianbagley-viz/websocket that referenced this pull request May 26, 2022
christianbagley-viz pushed a commit to christianbagley-viz/websocket that referenced this pull request Jun 8, 2022
joemiller added a commit to planetscale/loki that referenced this pull request Jun 23, 2022
Add `--proxy-url` flag to `logcli` to support connecting to Loki
instances running behind either HTTP or HTTPS CONNECT-style proxies.

Examples:

http proxy:

```
logcli \
  --addr="http://loki-distributed-querier.loki.svc.cluster.local.:3100/" \
  --proxy-url "http://envoy.dom.tld" \
  query '{app"foo"}'
```

https proxy with mTLS auth:
```
logcli \
  --addr="https://loki-distributed-querier.loki.svc.cluster.local.:3100/" \
  --proxy-url "https://envoy.dom.tld" \
  --cert tls.crt \
  --key tls.key \
  --ca-cert ca.crt \
  query '{app"foo"}'
```

Note that tail (`-f`) support and https proxies only works with a fork of the
`gorilla/websocket` lib. There are open PRs that promise to add support
for https proxies such as: gorilla/websocket#740

Add the following to `go.mod` to use the fork with https support:

```
replace github.com/gorilla/websocket v1.4.2 => github.com/philipatl/websocket v1.4.3-0.20211206152948-d16969baa130
```
dannykopping pushed a commit to grafana/loki that referenced this pull request Jun 24, 2022
Add `--proxy-url` flag to `logcli` to support connecting to Loki
instances running behind either HTTP or HTTPS CONNECT-style proxies.

Examples:

http proxy:

```
logcli \
  --addr="http://loki-distributed-querier.loki.svc.cluster.local.:3100/" \
  --proxy-url "http://envoy.dom.tld" \
  query '{app"foo"}'
```

https proxy with mTLS auth:
```
logcli \
  --addr="https://loki-distributed-querier.loki.svc.cluster.local.:3100/" \
  --proxy-url "https://envoy.dom.tld" \
  --cert tls.crt \
  --key tls.key \
  --ca-cert ca.crt \
  query '{app"foo"}'
```

Note that tail (`-f`) support and https proxies only works with a fork of the
`gorilla/websocket` lib. There are open PRs that promise to add support
for https proxies such as: gorilla/websocket#740

Add the following to `go.mod` to use the fork with https support:

```
replace github.com/gorilla/websocket v1.4.2 => github.com/philipatl/websocket v1.4.3-0.20211206152948-d16969baa130
```
@joemiller
Copy link

It would be great to see https proxy support. Are there any open blockers or concerns that the community could help with this PR or an alternate implementation?

@kamikazechaser
Copy link

Are there any open blockers or concerns that the community could help with this PR or an alternate implementation?

Almost every PR is blocked by a lack of an active maintainer.

@tomerb-neosec
Copy link

Why this PR wasn't merged?

@AlexVulaj
Copy link
Member

@tomerb-neosec it looks like there are merge conflicts in this PR. If you get those taken care of we can go forward with reviewing and getting this merged in. Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

[feature] Support for proxying websocket through https proxy
7 participants