Avoid relying on internals of crypto/tls

[FiloSottile]

I've noticed on #2614 that you're relying on unsafe casting to crypto/tls internal structures. I'm writing this to ask to please not do that.

Of course, that's not covered by any compatibility promise, but you know that. What you might not be considering is that we might and eventually will change internals for a security release, and then your users will be forced to choose between a security vulnerability and dropping a dependency in a hurry. Even more worrying, is that we might replace a field with a different one of the same type, and you might end up violating security-critical invariants without noticing.

Instead, please fork crypto/tls, and apply our changes on your own schedule, integrating them with the rest of the module. I hear git subtree is nice for that. You can even try to expose everything you need as clear, separate APIs, so you can more easily audit the contact points, and maybe eventually even get them upstreamed. It might require some more work upfront, but the only way to safely do what you are doing is anyway to be aware of every single change in crypto/tls.

[marten-seemann]

Hello @FiloSottile,

Thank you for opening this issue. I'd really really love to do that, the dependency on crypto/tls has been a huge pain point in the past, and I've spent a fair amount of time thinking about solutions for this problem.

Conceptually, forking crypto/tls indeed seems like the cleanest solution.
The problem with that approach is that this would have API implications for quic-go: Currently, we expose a Dial(..., *tls.Config) and Listen(..., *tls.Config) functions. Furthermore, in the http3 package we use the tls.Config of the http.Server.

This kind of API makes it trivial to use the same tls.Config to run services over TCP / QUIC (or HTTP2 / HTTP3) at the same time. Due to the occasional blackholing of UDP traffic, it is expected that most applications will have to do this for the foreseeable future.
Building a tls.Config can involve a fair amount of complexity. For example, Caddy uses the GetConfigForClient callback to generate a config per SNI, and runs the ACME protocol to obtain a certificate (@mholt, is that correct?). All of this code would have to be duplicated if we used a fork of crypto/tls, to use a qtls.Certificate and a qtls.ClientHelloInfo then.

I've tried to mitigate the security impact of the unsafe operations as far as possible by building a function that compares the structs: Adding an (exported or unexported) field to a struct will cause that check to fail, as will a reordering and a renaming. This code is run on init() (I didn't find a way to run it at compile time).
Of course, you're right to point out that there might be a delay between the release of a Go security update and the corresponding update of quic-go. All I can say is that I'm actively monitoring all releases and will upstream any changes as quickly as possible (the way we handled this for the Go 1.15 update will serve as an example of how to not do this in the future). Obviously, this is a suboptimal way to handle updates.

I'd be very happy about suggestions how to solve this problem.

[FiloSottile]

Do you need access to any of the unexported fields? Because otherwise something that might work is to make a simple NewConfigFromTLSConfig function that copies all the fields you know about, converting them as needed. It can also wrap most callbacks by copying the fields in the other direction on the way in, and again on the way out. It would not work for SetSessionTicketKeys, but really I think you'd be better off detaching your session ticket logic from ours.

[marten-seemann]

SetSessionTicketKeys indeed is a problem. Users would have to remember to call it on both the original and the converted config, which is easy to miss.

Another problem is the GetCertificate and the GetConfigForClient callback, as the ClientHelloInfo contains the config as an unexported field.

The ClientSessionCache is problematic as well, since the ClientSessionState only has unexported fields. One option would be to exclude this field from NewConfigFromTLSConfig, and require users to initialize a new cache, but again, that's easy to miss. Implementations of that interface would then have to be duplicated.

[mholt]

Caddy uses the GetConfigForClient callback to generate a config per SNI, and runs the ACME protocol to obtain a certificate (@mholt, is that correct?).

Yeah more or less. We pre generate the configs at startup and then use our own config selection logic so the user can customize which config is chosen at each handshake. Our configs also handle GetCertificate callback so we can present an ACME cert during challenges and even perform cert maintenance during handshakes if necessary.

[nqv]

I also maintain a custom implementation based on crypto/tls for another QUIC implementation. I'm trying to reuse tls.Config as much as I can but still haven't found a proper way to get/reuse the session tickets in tls.Config. It'd be much easier if tls.Config didn't contain unexported fields, but had a Session property or something