Handle tls #18
Handle tls #18
Conversation
|
Hmm. Well, this PR never touched any code affecting the test that failed. So it's something related to the http2 PR. Problem being right now is I am finding no possible path for the error that was thrown to exist. Might be a bit debugging this one. First time travis has failed on this error, and can't reproduce locally |
|
Ah. Looks like I was too aggressive on the timeouts for TLS negotiation and that was causing some issues. Got it on a test loop I'll run for a few hours just to be sure it doesn't crop-up again. And also did some heavier error handling |
We will always use TCPConns so be explicit in passing
Added insecure option for enabling insecure mode for handlers which support insecure transport
|
@mbyczkowski Bug patched. Back to you for final review |
net/utils.go
Outdated
| @@ -70,7 +70,7 @@ func GenericTLSWrap(conn *net.TCPConn, cfg *tls.Config, tFunc TLSWrapperFunc) (* | |||
| continue | |||
| } | |||
| } | |||
| return nil, err | |||
| continue | |||
There was a problem hiding this comment.
@doodles526 can you walk me thru this change of behavior for doing the TLS handshake? The way I understand it, we'll keep attempting a handshake until we get one or we weren't able to set the deadline again (because the conn is dead or similar). That suggest to me that the loop is going to be pretty tight. At the very top of the wormhole client we have a loop that that tries to connect, backs off and tries again. Would it make more sense to just error out here or at the very least limit the handshake to n attempts?
There was a problem hiding this comment.
Yeah, it would probably make sense to either err-out or have an exponential back-off with n attempts. In a near-future PR I think we can fix some of these negotiation issues with better messaging structures. But for now, just err out and let the client retry a new connection like you said
net/utils.go
Outdated
| @@ -107,7 +107,7 @@ func HTTP2ALPNTLSWrap(conn *net.TCPConn, cfg *tls.Config, tFunc TLSWrapperFunc) | |||
| continue | |||
| } | |||
| } | |||
| return nil, err | |||
| continue | |||
There was a problem hiding this comment.
same question as for TLS handshake
Pull Request Checklist
Is this in reference to an existing issue?
Yes, finishes the second half of #11
General
Update Changelog following the conventions laid out on Keep A Changelog
Update README with any necessary configuration snippets
Existing tests pass
Purpose