plugin/tls: Add the keylog option to configure TLSConfig.KeyLogWriter#7537
Conversation
|
No interest in this one? |
thevilledev
left a comment
There was a problem hiding this comment.
Hey @Kentzo, sorry for the delay. I'm catching up with open PRs now. Some comments below.
e77ada3 to
ed8be22
Compare
|
The CircleCI test failure is weird. Can you try to rebase this branch against the latest in |
Signed-off-by: Ilya Kulakov <kulakov.ilya@gmail.com>
Signed-off-by: Ilya Kulakov <kulakov.ilya@gmail.com>
| return c.Errf("unable to write TLS Key Log to %q: %s", absKeyLog, err) | ||
| } | ||
| c.OnShutdown(func() error { | ||
| f.Close() |
There was a problem hiding this comment.
Closing the file will cause TLS handshake to fail on connections that still has this config: https://cs.opensource.google/go/go/+/refs/tags/go1.26.1:src/crypto/tls/handshake_server_tls13.go (search for writerKeyLog).
This is fine if we expect OnShutdown to get called after underlying dns.Server is closed and doesn't accept new connections.
There was a problem hiding this comment.
On restart/reload, Caddy starts the new instance first (taking over the listeners), then calls Stop() on the old instance to drain, and only then calls OnShutdown.
For SIGTERM/SIGINT, the server is dying anyway. Probably fine for the handshake to fail in a debug tool?
There was a problem hiding this comment.
IIRC dns.Server should not accept new connections by the time OnShutdown is called.
OTOH one might say it's fine for an file descriptor to wait for GC event in a debug tool :)
I don't have CircleCI as an authorized OAuth App on my GitHub account. |
1. Why is this pull request needed and what does it do?
In order to debug DNS-over-TLS it's helpful to export TLS master secrets so tools like Wireshark could decrypt traffic.
This PR adds the
keylogoption to the tls plugin which the user can set. It behaves similarly to theSSLKEYLOGFILEenvironment variable in other applications.2. Which issues (if any) are related?
None
3. Which documentation changes (if any) need to be made?
The keylog option should be added to the tls plugin description.
4. Does this introduce a backward incompatible change or deprecation?
No