Add support for TLS and mTLS to tctl #812
Conversation
tools/cli/factory.go
Outdated
| caCertPool, err := fetchCACert(caPath) | ||
| if err != nil { | ||
| b.logger.Fatal("Failed to load server CA certificate", zap.Error(err)) | ||
| return nil |
There was a problem hiding this comment.
i know the above link is log.Fatal, but should this line be return err?
There was a problem hiding this comment.
This basically the same thing. If this func returns error caller will do log anyway. I think it is ok to log from here. Actually I would also add info level logging in case of success. Like certs are loaded successfully, using TLS mode or something like this.
There was a problem hiding this comment.
This basically the same thing.
yeah, just a little bit allergic about this:
if err != nil {
return nil
}There was a problem hiding this comment.
i would expect
if err != nil {
return err
}There was a problem hiding this comment.
Actually I would also add info level logging in case of success. Like certs are loaded successfully, using TLS mode or something like this.
Since this is a command line tool, I think it'd be too noisy to log a "connected successfully" every time it is executed.
tools/cli/factory.go
Outdated
| // If we are given arguments to verify either server or client, configure TLS | ||
| if caPool != nil || cert != nil { | ||
| tlsConfig := &tls.Config{ | ||
| ServerName: host, |
There was a problem hiding this comment.
Is it required to set ServerName on tls.Config? Most of the examples don't set it.
There was a problem hiding this comment.
This is optional, but we should set ServerName as it allows for hostname verification of the servers certs and SNI (server name indication) in TLS which enables the server to support virtual hosts on the same IP and pass decryption through to the appropriate service.
But this should match the name of the server we are trying to connect to rather than the host we are trying to connect from.
There was a problem hiding this comment.
But this should match the name of the server we are trying to connect to rather than the host we are trying to connect from.
It takes address argument from the command line, but defaults to localHostPort for development scenarios if
There was a problem hiding this comment.
Sounds good - just saw localHostPort and got confused there.
A replacement for #805 that does use server TLS code
What changed?
Added support for TLS and mutual TLS to tctl.
Why?
For secure usage of tctl.
How did you test it?
Manually tested that tctl can connect to a cluster secured with TLS.
Potential risks
No risk as the TLS related flags are optional.