Allow empty sni extension #51
Conversation
|
Thanks for the PR! I did give 👍 to the empty SNI implementation in #38 (comment) , but in a different way, it seems. If I understand this correctly, the behavior of existing applications that use uTLS with empty SNIs will change with this PR, which is undesirable. We can implement omitting of SNI in a backward compatible way by adding a new |
|
Great ok that makes sense - thanks for the feedback! I'll get on that. |
|
Ok so I reverted the existing commits on this branch (I can either rebase this branch or you can just use github's "squash and merge" functionality if you'd like. It looked to me like it wouldn't work to have the |
u_conn.go
Outdated
| // RemoveSNIExtension will cause the UConn connection client hello to not include a SNI extension | ||
| // This only applies to non-HelloGolang ClientHelloIDs | ||
| func (uconn *UConn) RemoveSNIExtension() error { | ||
| if uconn.ClientHelloID == HelloGolang { | ||
| return fmt.Errorf("Cannot call RemoveSNIExtension on a UConn with a HelloGolang ClientHelloID") | ||
| } | ||
| uconn.omitSNIExtension = true | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This returns an error to prevent someone from accidentally trying to use it on a UConn that has a HelloGolang ClientHelloID
u_conn_test.go
Outdated
| test := *template | ||
| // clone the test template config so we can successfully re-run | ||
| templateConfig := *template.config // nolint | ||
| test.config = &templateConfig |
There was a problem hiding this comment.
Is this a reasonable way to clone the test config for re-running it?
There was a problem hiding this comment.
Those look like good changes, but I do have a couple of nitpicks.
It would be nicer if the test was in it's own TestRemoveSNIExtension function such that we could understand immediately if it broke. So, we should not clone the test config.
It also doesn't need to double the amount of test cases just for that feature, I think a single test (with say latest Chrome) is probably enough. My opinion on that is less strong on this one and I am happy to accept a PR that doubles test data, just does not seem necessary.
u_conn.go
Outdated
| @@ -162,6 +167,26 @@ func (uconn *UConn) SetSNI(sni string) { | |||
| } | |||
| } | |||
|
|
|||
| // RemoveSNIExtension will cause the UConn connection client hello to not include a SNI extension | |||
| // This only applies to non-HelloGolang ClientHelloIDs | |||
There was a problem hiding this comment.
I'd invert the negatives. Something like
"RemoveSNIExtension removes SNI from the list of extensions sent in ClientHello.
It returns an error when used with HelloGolang ClientHelloID."
Works for me - I agree that's a better approach. Let me know if I've missed anything else. Thanks! |
|
Great thanks so much!! |
This attempts to get merged just the empty SNI changes from #38
I used the changes from #50 to generate the
testdataIt looked to me like you had given the 👍 to the empty SNI changes in #38, but let me know if I misread that or if there's anything else you'd like incorporated here.
Thanks!