cloudfoundry / gorouter

CF Router
Apache License 2.0
441 stars 224 forks source link

feat: log TLS handshake failures #404

Closed maxmoehl closed 5 months ago

maxmoehl commented 6 months ago

Currently any failure during a TLS handshake is emitted using the log package directly from the net/http library. These logs are not in the same format as the other gorouter logs which makes them hard to parse and they also lack a lot of the metadata we'd like to log to troubleshoot such issues.

This commit replaces the crypto/tls.Listener with a custom version. This custom version does the TLS handshake as part of the Accept which allows us to log all details from the tls.Conn and its tls.ConnectionState.

See above.

maxmoehl commented 6 months ago

We can't merge this as-is since it will be doing the TLS handshake in the main accept loop. A slow TLS handshake could therefore block gorouter from accepting other connections resulting in a DoS attack vector.

One option we could consider is performing the handshake in a dedicated go routine to not block the http.Server routine.

maxmoehl commented 6 months ago

I've been prototyping locally to avoid the extra goroutine that would be spawned for each TLS handshake. While it shouldn't do any harm in theory (TLS handshakes are relatively expensive anyways and goroutines are lightweight) it does increase the resource consumption on server side.

One idea was to wrap the tls.Conn at some place to let the handshake occur naturally but capture the result using the wrapper. Unfortunately that is not possible because net/http has a very intimate relationship with crypto/tls and explicitly relies on the concrete types matching.

I guess the proper way would be to have the stdlib expose the data somehow. After reading through some of the related open issues I think that golang/go#38270 is probably the right way to go.

I'll open up this PR for review as I think the overall tradeoff is reasonable (maybe put the change behind a feature flag?), looking forward to other opinions!

PS: If we decide that the overall change is something we want I will add some tests.

geofffranks commented 6 months ago

I think having additional tls handshake failure logging could be helpful, but I'm pretty nervious about replacing core golang functionality with custom implementations. This should definitely have an opt-in feature flag.

maxmoehl commented 5 months ago

I won't be able to continue on this for now but we intend to pick this up in the near future. To-dos:

I'll put this PR to draft for now.

plowin commented 5 months ago

As we currently do not have concrete reasons to apply such a workaround that brings some risk, we are closing the PR and are monitoring the progress on a proper solution via golang (https://github.com/golang/go/issues/38270). Thanks for the contribution and feedback!