#292 Re-enable ReflectToSite and upgrade utls to v1.6.7 #633
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I just wanted to document your fantastic testing process here (copy/pasted from the linked Google Doc): [Testing Steps]
|
hwh33
approved these changes
Nov 20, 2024
There was a problem hiding this comment.
Just one minor thing and it was due to me changing my mind 🙂 . Once that's changed, this is good to go! Excellent investigation and work on this fix.
Let's be careful about merging this. Once this is merged, the proxies will pick up this change. Your test indicates that this should be fine since clients in the field should not need an update. However, we should watch traffic to reflect-to-site proxies just in case.
So, let's do the following:
- Please make the suggested change to
helloError. - I will merge this PR one day in the morning my time.
- I will monitor the metrics to ensure this change does not have negative effects for our users. If I see any negative effects, I will revert the merge and let you know.
- I will document here the metrics that I watch for your future reference.
tlslistener/clienthelloconn.go
Outdated
| @@ -205,16 +205,16 @@ func (rrc *clientHelloRecordingConn) processHello(info *tls.ClientHelloInfo) (*t | |||
| // pre-defined tickets. If it doesn't we should again return some sort of error or just | |||
| // close the connection. | |||
| if !helloMsg.TicketSupported { | |||
| return rrc.helloError("ClientHello does not support session tickets", true) | |||
| return rrc.helloError("ClientHello does not support session tickets", false) | |||
There was a problem hiding this comment.
Since continueOnError is always set to false now, let's just remove that parameter from clientHelloRecordingConn.helloError. Sorry, I think that's different than what I originally told you!
|
@hwh33 Removed |
|
Awesome @Jovis7, thank you! I will merge this tomorrow morning my time and update this PR with the results. |
|
Merging this now! As noted above, this should trigger an automatic update of our production proxies. I'll verify that this update re-enables reflect-to-site by trying a TLS connection to one of our TLS tracks (note that currently all TLS tracks run reflect-to-site). This should get "reflected" to the masquerade site and I should receive the certificate from that site. Currently, I see the proxy's certificate instead of the masquerade's certificate (as expected since reflect-to-site is currently disabled): Route Details(this is just a random HTTPs route I grabbed) Test TLS ConnectionI will monitor performance of our reflect-to-site tracks by watching our track dashboard in Signoz (shout out to @Crosse for setting that dashboard up!) and filtering for |
|
I am now receiving the masquerade certificate, indicating that reflect-to-site is re-enabled as intended. No change in the metrics so far, but I think it's a little early to say for sure. I'll keep watching! Test TLS Connection |
|
I was able to successfully connect to the TLS proxy I tested previously with a Lantern Client, running the following in lantern-client: I verified (via the logs) that my client was using the configured proxy and I was able to browse YouTube and other sites. |
Crosse
deleted the
jovis-issue292-upgrade-utls-reenable-reflect-to-site
branch
|
Metrics are all still looking hunky dory. I think you've done it @Jovis7! Excellent work on this 🙂 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We had Record client hello errors, but allow connections to proceed #585 to disable ReflectToSite, that is a workaround to this bug.
https://go.dev/doc/go1.22 : By default, cipher suites without ECDHE support are no longer offered by either clients or servers during pre-TLS 1.3 handshakes. This change can be reverted with the tlsrsakex=1 GODEBUG setting.