Go 1.20 TODO list

[francislavoie]

Did some reading of the upcoming Go 1.20 release notes, and I'm writing down thing things we should consider doing once we've bumped to 1.20 minimum (probably ~1 year from now) https://tip.golang.org/doc/go1.20

• https://tip.golang.org/doc/go1.20#http_responsecontroller would be useful to add timeouts to individual requests. I'm thinking this would be new options for the request_body directive.
• https://tip.golang.org/doc/go1.20#reverseproxy_rewrite Go stdlib's proxy implementation is getting some changes relating to how headers are handled via a new API for transforming the request before sending it upstream; the linked github issue in that section has some interesting discussion that should get reviewed to see if there's anything we need to action.
• https://tip.golang.org/doc/go1.20#context WithCancelCause is a really neat new feature, there's some places we could probably make use of this to smooth things over, for example a context cancellation due to a config reload could allow for some additional intelligence in some areas by allowing parts of the config to compare old vs new to see if the config significantly changed, and if it hasn't, ignore the cancellation maybe. For example, frequent config reloads right now cause in-flight TLS automation to get cancelled, which adds friction, can continually prevent TLS automation from completing if reloads are too frequent. It could prevent websocket connections from being closed maybe, if only unrelated config is changed. Obviously this is much easier said than done, but just some thinking 🧠
• https://tip.golang.org/doc/go1.20#crypto/tls Looks like we'll get some free optimizations, mostly for HTTPS proxy upstreams, by having certs shared in memory for all connections. Also, there's a new CertificateVerificationError; we should probably detect those and write out a DEBUG level log with the additional details we can get from that error, it would be helpful for diagnosing some issues.
• https://tip.golang.org/doc/go1.20#crypto/x509 We may want to consider using the new SetFallbackRoots to bundle a reasonable default set of root CAs, mainly for Let's Encrypt and ZeroSSL, so that TLS automation can still work without having a system root store. This would be mainly for FROM scratch Docker containers, although our official image is based on alpine which does ship a trust store via the ca-certificates package so it's only relevant for users making their own Docker images instead of using ours.
• https://tip.golang.org/doc/go1.20#net/http The new StreamError would probably be useful to detect and write out at DEBUG level for diagnosis.
• https://go.dev/doc/go1.20#path/filepath The new IsLocal function can be used to further vet the request path in SanitizedPathJoin. See the comment in path/filepath: mishandling of Windows UNC paths golang/go#56336 (comment).

[WeidiDeng]

Golang 1.20 is released today.

[francislavoie]

Yep, this list is for when we change our minimum version to Go 1.20, which will be like 6 months to 1 year from now.

Adding support for 1.20 here #5353

[stefanb]

quic-go 0.37.0 was released yesterday with Go 1.20 and 1.21 support: https://github.com/quic-go/quic-go/releases/tag/v0.37.0 fixed via #5644

Go 1.21 is scheduled to be released in August.

[mholt]

https://tip.golang.org/doc/go1.20#http_responsecontroller would be useful to add timeouts to individual requests. I'm thinking this would be new options for the request_body directive.

I like this idea. The "write timeout" for a handler called request_body feels a little weird, but at the same time, reading the request body is usually a prerequisite to writing a response (even if there is no request body), so maybe it works.

https://tip.golang.org/doc/go1.20#reverseproxy_rewrite Go stdlib's proxy implementation is getting some changes relating to how headers are handled via a new API for transforming the request before sending it upstream; the linked github issue in that section has some interesting discussion that should get reviewed to see if there's anything we need to action.

Looks like the changes to the headers handling is mostly related to the proxy hop headers like X-Forwarded-*, etc. And I believe our updated handling of trusted client IPs (thanks to you) kind of addresses who/what is allowed to set those headers. But further confirmation here that that's the case would be good.

Other changes to their reverse proxy involve a Rewrite hook and URL setter which basically change the outgoing request, which our proxy can already do.

I haven't gone through all the updated code yet, but one thing I did notice is that they now normalize query strings in URIs: https://cs.opensource.google/go/go/+/refs/tags/go1.22.2:src/net/http/httputil/reverseproxy.go;l=800-819 -- if a semicolon or invalid escape sequence is present, the query string is re-encoded.

I have mixed feelings about that, because while variations in query string interpretations can result in bugs including security bugs, our purpose as a proxy is to faithfully transmit the request to the backend... which may have a purpose behind the semicolons, for example. And we leave the security implications up to the backend (i.e. it's not the proxy's job to sanitize for arbitrary application semantics -- but I can see why a standard library would, because it's a standard library).

https://tip.golang.org/doc/go1.20#context WithCancelCause is a really neat new feature, there's some places we could probably make use of this to smooth things over, for example a context cancellation due to a config reload could allow for some additional intelligence in some areas by allowing parts of the config to compare old vs new to see if the config significantly changed, and if it hasn't, ignore the cancellation maybe. For example, frequent config reloads right now cause in-flight TLS automation to get cancelled, which adds friction, can continually prevent TLS automation from completing if reloads are too frequent. It could prevent websocket connections from being closed maybe, if only unrelated config is changed. Obviously this is much easier said than done, but just some thinking 🧠

So, since this issue, we've actually taken some steps to address the bigger remaining pain points of config reloads (which haven't been many): websockets, and reloading TLS certs into the cache. I don't think we've addressed in-flight TLS automation being cancelled, but that still can be a good thing -- we'd need to study it more, and I'd rather wait for a complaint/feature request before going there. This is a good thought, though. We can see where we might use WithCancelCause going forward (or looking back).

https://tip.golang.org/doc/go1.20#crypto/tls Looks like we'll get some free optimizations, mostly for HTTPS proxy upstreams, by having certs shared in memory for all connections. Also, there's a new CertificateVerificationError; we should probably detect those and write out a DEBUG level log with the additional details we can get from that error, it would be helpful for diagnosing some issues.

Yay free wins. As for the error, it's not clear to me exactly which external-facing methods return this error, but from searching the source it looks like it gets generated in TLS clients when verifying server certificates (makes sense -- we don't do this much except for in the reverse proxy if upstreams are HTTPS, but this is uncommon), and TLS servers when verifying client certificates (also makes sense -- but also uncommon, as it requires client auth enabled).

I'm not really sure where to put the logs, basically 🤔

https://tip.golang.org/doc/go1.20#crypto/x509 We may want to consider using the new SetFallbackRoots to bundle a reasonable default set of root CAs, mainly for Let's Encrypt and ZeroSSL, so that TLS automation can still work without having a system root store. This would be mainly for FROM scratch Docker containers, although our official image is based on alpine which does ship a trust store via the ca-certificates package so it's only relevant for users making their own Docker images instead of using ours.

Cool -- easy enough, one line import.

https://tip.golang.org/doc/go1.20#net/http The new StreamError would probably be useful to detect and write out at DEBUG level for diagnosis.

It looks like it combines a stream ID and error code with an error, but I'm trying to think of issues where the culprit was the actual HTTP/2 implementation such that this would be more helpful than the higher-level errors that server admins find useful, like "context canceled" or "aborted" or "connection reset" and things like that. I'm not opposed to this, but part of me is wondering if it makes more sense to wait until there's a need before we add this just by guessing the right place and info to log.

https://go.dev/doc/go1.20#path/filepath The new IsLocal function can be used to further vet the request path in SanitizedPathJoin. See the comment in golang/go#56336 (comment).

Interesting discussion in that issue -- I can't actually find the filepath.Localize() function that @neild mentions, and that the CL adds: https://go-review.googlesource.com/c/go/+/531677 -- but I did find an internal-only function FromFS() which seems like it was supposed to be renamed to Localize? https://cs.opensource.google/go/go/+/refs/tags/go1.22.2:src/internal/safefilepath/path.go

In any case, we can't use it.

About 14 months ago we already updated our SanitizedPathJoin() function to use the suggestions from that issue that were current at the time.

I did, however, find that I could use the second-to-last suggestion and vet the result with IsLocal().

I've pushed the discussed changes to #6252.

[mholt]

Re-reading the remaining items and I feel like most of them are going to be most effectively done in a "Oh hey, we need this because of a certain report/issue/question" fashion, rather than proactively. For instance, WithCancelCause does indeed seem useful, but I don't have a concrete use case for it at the moment.

@francislavoie When you have a chance, do you want to review #6252 and then maybe we'll close this and can refer back to it later?

[francislavoie]

Yep, I think that covers it. Thanks!