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

Serve webhook server on https #627

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

Serve webhook server on https #627

wants to merge 3 commits into from

Conversation

AGMETEOR
Copy link
Contributor

@AGMETEOR AGMETEOR commented Sep 11, 2024

This is some rough PR I was using to investigate why /ws endpoint was not showing it's cert to the client.
I started a local http-proxy using a custom broflake config.ini
Before doing openssl s_client -connect 127.0.0.1:1443 -showcerts would not show any certs
Add the change in this PR and doing openssl s_client -connect 127.0.0.1:1443 -showcerts reveals the cert details

I also created a custom self-signed cert and key and added them to my key chain (I am on a mac). I was able to connect to the proxy as egress server using the widget. So websocket connection was established.

Only issue I see is the desktop fails to connect due
2024/09/11 19:10:30 QUIC dial failed (CRYPTO_ERROR 0x146 (remote): tls: protocol version not supported), retrying...

I think I can get to the bottom of this soon.

@AGMETEOR AGMETEOR self-assigned this Sep 11, 2024
Copy link
Contributor

@WendelHime WendelHime left a comment

Choose a reason for hiding this comment

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

Hi AG, left one minor suggestion for verifying errors

http_proxy.go Outdated
Comment on lines 927 to 929
certPEM, _ := os.ReadFile(p.CertFile)
keyPEM, _ := os.ReadFile(p.KeyFile)
cert, _ := tls.X509KeyPair(certPEM, keyPEM)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some error verifications? If the cert file or Key file are empty we probably need to decide what to do.

  1. We can return an error informing about the missing parameters for this proxy
  2. We could ignore these errors and not build a TLS config (but we probably don't want that)

Copy link
Contributor Author

@AGMETEOR AGMETEOR Sep 11, 2024

Choose a reason for hiding this comment

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

I am going to take care of those. I just wanted to confirm that indeed this is the right path to the fix

@hwh33
Copy link
Contributor

hwh33 commented Sep 11, 2024

So this works, at least for establishing WSS connections to the server. However, there is clearly something wrong as you are creating a TLS listener and also passing the TLS keypair to egress.NewListener. I think we need to get to the bottom of the issue rather than apply a fix we don't understand on top of the existing code.

@AGMETEOR
Copy link
Contributor Author

So this works, at least for establishing WSS connections to the server. However, there is clearly something wrong as you are creating a TLS listener and also passing the TLS keypair to egress.NewListener. I think we need to get to the bottom of the issue rather than apply a fix we don't understand on top of the existing code.

What I observed was we were passing a listener not configured for TLS to broflake.Wrap and this is why https would fail. But I can continue digging and see if I can find anything else.

@AGMETEOR
Copy link
Contributor Author

Ok I think the fix is better done in broflake library, see getlantern/browsersunbounded#259

@AGMETEOR AGMETEOR marked this pull request as ready for review September 12, 2024 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants