-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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
net/http: custom net.Conn impls break http.Request.TLS #14221
Comments
Yes, we can do that for 1.7. |
CL https://golang.org/cl/19741 mentions this issue. |
Note that it's possible to do this with a mux today, so long as you're able to demux upstream of the |
Would asserting on a local/private interface of: type tlsHandshaker interface {
SetReadDeadline(time.Time)
SetWriteDeadline(time.Time)
Handshake() error
ConnectionState() tls.ConnectionState
} be sufficient for this? |
Probably; that's just about what was done in golang/net@6ccd669#diff-2bfffed39205fa6cd0bb2601af7251a4R427 |
(happy to do that if so) |
That almost works. What should be done about the |
The problem spot being here, where |
@dpiddy, yeah, that's the problem. Maybe we shouldn't even "fix" this. Generally people want this so they can use https://godoc.org/github.com/spacemonkeygo/openssl or something, but that's not something I really want to encourage. For a mux, can't you record the traffic seen at the beginning of the handshake during routing and then replay it, stitching together the captured traffic and the underlying net.Conn, feeding that into https://golang.org/pkg/crypto/tls/#Server? @tamird, can I can get an update on the motivation for this bug? |
That's only possible if you are willing to read "raw" data from a TLS connection. This case is pretty rare because your mux usually wants to direct traffic based on the decrypted data read from the connection. You want to be able to:
where |
Seems like a mux could make its own private *tls.Conn for routing but record the underlying net.Conn's raw traffic for replay later, letting the downstream net/http tls.Listener make its own new *tls.Conn later from the recorded traffic. |
How would that work? TLS handshakes necessarily involve sending data back and forth, so the downstream tls.Listener would have to know to not send its part of the handshake? |
No, you'd record the handshake on the underlying conn and return a new underlying conn which would eat those parts for the second *tls.Conn, so the net/http's use of the *tls.Listener thinks it's handshaking, but it's speaking to a script instead. You'd probably have to set the *tls.Config.Rand io.Reader to something deterministic between the two, using crypto/rand.Reader the first time, recording it too, and using the same randomness in the later one. I haven't actually implemented this, but it seems plausible. In any case, I'd prefer to see such hackery put into the code that is implementing a TLS mux, because it's already aware of TLS, rather than infecting APIs for everybody else. |
replacing *tls.Conn with an interface is "infecting" APIs for everybody else? The problem here is that public APIs expect a concrete type which is overly rigid and difficult to compose. I don't see how correcting that is "infecting" anything for anyone. I understand that this is a difficult change to make, but let's not throw red herrings around, mmkay? |
No, the ConnWrapper from https://go-review.googlesource.com/#/c/19741/1/src/crypto/tls/conn.go is an infection. I don't want more of that. It hurts composability if everybody has to worry about optional unwrapping interfaces. The *tls.Conn is already part of APIs and is not an interface. That ship has sailed, for better or worse. I'm going to close this for now, until a more compelling use case arises. I don't think performance or muxing is enough. Performance gets better each release, and muxing seems like it has workarounds which can live in the mux. |
The explicit use of tls.Conn makes it hard to use something like LimitListener while still having http.Request.TLS work properly, as described by this issue. That's what I'm dealing with currently. It would be workable if a request could grab its underlying net.Conn. Maybe that could be added to the request context? |
@dpiddy, wouldn't you just pass the LimitListener to https://golang.org/pkg/crypto/tls/#NewListener? |
Yes, of course. Thanks. |
I understand it's not encouraged, but is there a way to use https://godoc.org/github.com/spacemonkeygo/openssl while still serving http2? Due to the huge performance diff, currently we have to use that openssl pkg since the assembly isn't there for arm/arm64 AES like it is for amd64 in crypto/tls :( |
For questions about Go, see https://golang.org/wiki/Questions. |
Partial continuation of #12737 which made it possible to fix the problem for http2, but not http1.
As pointed out in the cmux readme's limitations section:
This is really due to the type assertion used to identify TLS connections, which sadly breaks the goodness of net.Conn and net.Listener being interfaces.
Relevant code: https://github.com/golang/go/blob/release-branch.go1.6/src/net/http/server.go#L1398:L1410
Can this type assertion to a concrete type be replaced with an assertion to an interface?
The text was updated successfully, but these errors were encountered: