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

proposal: net/http: allow use of external TLS stacks #21753

Closed
FiloSottile opened this issue Sep 4, 2017 · 11 comments
Closed

proposal: net/http: allow use of external TLS stacks #21753

FiloSottile opened this issue Sep 4, 2017 · 11 comments
Labels
FrozenDueToAge Proposal Thinking v2 A language change or incompatible library change
Milestone

Comments

@FiloSottile
Copy link
Contributor

net/http is hardcoded to work with crypto/tls, and it's currently impossible to use any other TLS stack. That meant that for example to develop the tls-tris fork of crypto/tls with TLS 1.3 support I had to do horrible things which made life pretty hard for other developers on the project.

@rsc mentioned he'd be interested in making it possible to use non-crypto/tls stacks with net/http. While I don't think we should necessarily encourage it (it's good that "what TLS stack do I want?" is not a question a Go developer has to ask themselves), I agree it would be nice to at least make it possible.

Here's a survey of the coupling points, and proposals on how to fix (most) of them.

/cc @agl @bradfitz @rsc @mholt

Server side

  • Request.TLS has type *tls.ConnectionState
    • I think it's acceptable to require external stacks to use the crypto/tls ConnectionState type
  • serve type-asserts *tls.Conn to detect a TLS connection and call Handshake() and ConnectionState()
    • Here we can type-assert to an (probably unexported) interface with those two methods
  • Server.TLSNextProto has type map[string]func(*Server, *tls.Conn, Handler)
    • This is a problem: the function here will have to work with the concrete *tls.Conn, and there is code out there written with this assumption, including the standard http2 library
  • ServeTLS and ListenAndServeTLS
    • These are helpers, no reason they shouldn't use crypto/tls
  • Server.TLSConfig has type *tls.Config
    • We can mostly ignore this, as it's only used for ServeTLS/ListenAndServeTLS and for some auto-HTTP/2 logic

Client side

  • Client asserts tls.RecordHeaderError to detect a mistaken plain HTTP response
    • External stacks can just use the crypto/tls error type
  • Response.TLS has type *tls.ConnectionState
    • As above, it's acceptable to require external stacks to use the crypto/tls ConnectionState type
  • Transport.TLSClientConfig has type *tls.Config
    • We can ignore this, as it can be overridden with DialTLS, so it's basically part of a helper
  • Auto-HTTP/2 is disabled by DialTLS being set
    • I think the client logic should be more like the server one, where the HTTP stack is configured if the TLS state is unknown (t.DialTLS != nil) or the TLS Config is available (t.TLSClientConfig != nil) and h2 is enabled (this might be a separate issue)
  • Transport.TLSNextProto has type map[string]func(authority string, c *tls.Conn) RoundTripper
    • Like Server.TLSNextProto, this is a problem
  • dialConn type-asserts *tls.Conn
    • Like above, we can instead assert an interface

Conclusion

The main problem are the two TLSNextProto, for which I have no good solution. The auto-HTTP/2 logic might also need attention. Other parts can be either ignored as helpers, or discreetly fixed.

@gopherbot gopherbot added this to the Proposal milestone Sep 4, 2017
@mholt
Copy link

mholt commented Sep 4, 2017

I'll add that I'm also quite interested in this. It would make rolling out TLS 1.3 at scale much easier without having to endanger the reputation of the Go standard library with an experimental TLS version.

Re: The TLSNextProto map's reliance on concrete *tls.Conn types: you may know I'm kind of a fan of "plugin" kinds of designs for certain needs in Go (like how you register a value for gob or a flag name in flag). It can get a little messy when used a lot, but this kind of design is why Caddy is so easily extensible.

Following that line of thought, is it feasible to add an interface field or "setter" method for an unexported interface field on the tls.Conn struct so that its methods defer to the methods implemented by that value? Essentially you "plug in" your implementation of a TLS connection, and the tls.Conn defers to those methods rather than its own?

type Conn struct {
        Impl ConnInterface

        // contains filtered or unexported fields
}

or maybe:

func (c *Conn) Use(ci ConnInterface)

Where ConnInterface is an interface describing the exported methods on Conn. This is obviously not a pleasant solution as the interface isn't particularly small, and adding methods/fields to structs for niche uses is not ideal, but it's just an idea to stir the pot. (Also, I'm not sure if allowing "overriding" the exported methods will give sufficient coverage to essentially replace the concrete tls.Conn with another type...)

@golang golang deleted a comment Sep 4, 2017
@Scratch-net
Copy link

Would also be nice to run http2 over something different than the default TLS, ex NoiseSocket. The ordinary HTTP runs fine, but h2 uses *tls.Conn explicitly

@jtolio
Copy link
Contributor

jtolio commented Sep 4, 2017

Just wanted to chime in with support for this. It would make our usage of https://github.com/spacemonkeygo/openssl much easier. Initially we forked net/http but now we use a lot of unsafe stuff to get to the underlying *openssl.Conn
(edit: the main lingering reason for using openssl bindings is esoteric hardware accelerated encryption)

@tombergan
Copy link
Contributor

Auto-HTTP/2 is disabled by DialTLS being set
I think the client logic should be more like the server one, where the HTTP stack is configured if the TLS state is unknown (t.DialTLS != nil) or the TLS Config is available (t.TLSClientConfig != nil) and h2 is enabled (this might be a separate issue)

This looks like #21336 and I came to the same conclusion you did ("client logic should be more like the server").

@rsc rsc changed the title proposal: make net/http work with external TLS stacks proposal: net/http: allow use of external TLS stacks Oct 16, 2017
@bradfitz
Copy link
Contributor

I think that if external TLS stacks need to be supported, they should plug into the crypto/tls package and all users of crypto/tls should be unaffected. That is, I think that the concrete types in crypto/tls being used by other packages is a feature, not a bug.

@bradfitz
Copy link
Contributor

@FiloSottile, is this proposal purely about net/http? If so, I would like to decline this. I don't want to deal with any additional complexity in net/http for this.

The BoringCrypto branch (https://go.googlesource.com/go/+/dev.boringcrypto) also seems to suggest this abstraction isn't necessarily needed. BoringCrypto seems to be patched in elsewhere such that each package using crypto/tls doesn't need to change.

@FiloSottile
Copy link
Contributor Author

It is about net/http, as it seems to be the most tightly coupled. What other packages depend on crypto/tls types?

I think BoringCrypto is an example of why this is needed, not the opposite. That effort required maintaining a fork of the standard library, just like tls-tris, which is not an option for most users. (Also, BoringCrypto changes most primitives, but leaves most of the TLS logic alone, so it's different from tls-tris.)

@bradfitz
Copy link
Contributor

I think modifying the standard library is an acceptable cost for both of those examples.

BoringCrypto does not exist for technical reasons, and tls-tris seems to be just a staging ground to get TLS 1.3 into the standard library, so that one will go away.

I do not want extra complexity in net/http (at least in Go 1.x) if the only benefit is to fracture the ecosystem and push the decision of what TLS stack to use to the users. We should make our TLS stack good instead.

I'll tag this for Go 2 instead, for if/when there's an net/http/v2 package.

@bradfitz bradfitz added Thinking v2 A language change or incompatible library change labels Jul 16, 2018
@FiloSottile
Copy link
Contributor Author

crypto/tls has a strict complexity budget, so it does not work for certain advanced users. Forking the entire standard library is a very high cost alternative, while having the opportunity to fork only the package would reduce pressure to add features to crypto/tls.

Agreed it probably makes sense for Go 2, though.

@bradfitz
Copy link
Contributor

I'm not necessarily saying it makes sense for Go 2, FWIW. But it definitely doesn't for Go 1.

@ianlancetaylor
Copy link
Contributor

Folding this into #5465.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Thinking v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

8 participants