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

set a net.Conn with the correct addresses on the tls.ClientHelloInfo #4001

Merged
merged 1 commit into from Jul 31, 2023

Conversation

marten-seemann
Copy link
Member

Fixes #4000.

@mholt, would you mind taking a look at this PR?

@@ -140,6 +140,30 @@ var _ = Describe("Handshake tests", func() {
Expect(err).ToNot(HaveOccurred())
})

It("has the right local and remote address on the ClientHelloInfo.Conn", func() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Now including an end-to-end test to make sure this API never accidentally breaks again.

@codecov
Copy link

codecov bot commented Jul 29, 2023

Codecov Report

Merging #4001 (d5a4a70) into master (469a615) will decrease coverage by 0.10%.
Report is 1 commits behind head on master.
The diff coverage is 12.50%.

@@            Coverage Diff             @@
##           master    #4001      +/-   ##
==========================================
- Coverage   82.86%   82.76%   -0.10%     
==========================================
  Files         146      147       +1     
  Lines       14729    14744      +15     
==========================================
- Hits        12204    12202       -2     
- Misses       2025     2040      +15     
- Partials      500      502       +2     
Files Changed Coverage Δ
internal/handshake/conn.go 0.00% <0.00%> (ø)
internal/handshake/crypto_setup.go 53.62% <0.00%> (-0.84%) ⬇️
connection.go 72.22% <100.00%> (+0.03%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Contributor

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Sorry it took me over the weekend. At a glance this looks good! If we can get the remote/local addr at time of handshake, that's good enough for our use case for now, I think.

Thank you :)

I can probably get this out in Caddy 2.7. When are you planning the next release?

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.

regression in v0.37.0: ClientHelloInfo.Conn is nil
2 participants