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
Add logging for attempted and successful TLS connections to server #1486
Add logging for attempted and successful TLS connections to server #1486
Conversation
common/auth/tlsConfigHelper.go
Outdated
) *tls.Config { | ||
c := NewEmptyTLSConfig() | ||
c.ClientAuth = clientAuth | ||
c.Certificates = certificates | ||
c.ClientCAs = clientCAs | ||
c.VerifyConnection = func(state tls.ConnectionState) error { | ||
logger.Info("successfully established incoming TLS connection", tag.HostID(state.ServerName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we think this should be Debug level log to avoid spamming if there are a lot of incoming connections? Or is Info fine?
if perHostCertProviderMap != nil { | ||
perHostCertProvider, hostClientAuthRequired, err := perHostCertProviderMap.GetCertProvider(c.ServerName) | ||
if err != nil { | ||
logger.Warn("cannot find a per-host provider for attempted incoming TLS connection", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to log the error here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we are logging a bit too much in some places?
What changed?
Added logging of attempted and successfully established TLS connections to server
Why?
To have records in the log of all attempted TLS connections to server and remote IP addresses of parties trying to connect.
How did you test it?
Unit tests.
Potential risks
No risks.
Is hotfix candidate?
No.