Enhancements to setupTLSConfig
#3284
Conversation
The function `setupTLSConfig` is now implemented in terms of the function `SetupTLSConfig` in `consul/api`. The function also respects two additional configuration properties. - `tls_server_name` (optional) which sets the server name to use as the SNI host when connecting via TLS (for when it differs from the server name in `address`). Has the same behavior as the environment variable `CONSUL_TLS_SERVER_NAME`. - `tls_ca_path` (optional) which sets the path to a directory of CA certificates to use for Consul communication. Has the same behavior as the environment variable `CONSUL_CAPATH`. The function also now logs: - a debug message containing the values used to create the `Config` from the `crypto/tls` package. - a warning message when only one of `tls_key_file` or `tls_cert_file` are provided in the `conf` map.
| insecureSkipVerify := false | ||
| if _, ok := conf["tls_skip_verify"]; ok { | ||
| insecureSkipVerify = true | ||
| serverName, ok := conf["tls_server_name"] |
There was a problem hiding this comment.
The default behavior still needs to be the net.SplitHostPort behavior, if not overridden with tls_server_name.
There was a problem hiding this comment.
SetupTLSConfig extracts the host portion from the TLSConfig.Address using net.SplitHostPort which is why I did not repeat the effort here.
if tlsConfig.Address != "" {
server := tlsConfig.Address
hasPort := strings.LastIndex(server, ":") > strings.LastIndex(server, "]")
if hasPort {
var err error
server, _, err = net.SplitHostPort(server)
if err != nil {
return nil, err
}
}
tlsClientConfig.ServerName = server
}
| @@ -100,6 +100,13 @@ connection. You can read more about encrypting Consul connections on the | |||
| [`ca_file`](https://www.consul.io/docs/agent/options.html#ca_file) setting in | |||
| Consul. | |||
|
|
|||
| - `tls_ca_path` `(string: "")` – Specifies the path to a directory of | |||
| certificate authority files for Consul communication. Is an alternative to | |||
| `tls_ca_file`. This defaults to the system bundle if not specified. | |||
There was a problem hiding this comment.
This is not a duplicate - i was trying to remark on the fact thattls_ca_path is used when tls_ca_file is not supplied (see rootcerts.LoadCACerts). Any suggestions for improved wording?
| data, err := ioutil.ReadFile(tlsCaFile) | ||
| insecureSkipVerifyStr, ok := conf["tls_skip_verify"] | ||
| if ok { | ||
| insecureSkipVerify, err := strconv.ParseBool(insecureSkipVerifyStr) |
There was a problem hiding this comment.
I don't have an issue with you updating how this works, but please use parseutil.ParseBool instead.
There was a problem hiding this comment.
having re-read the documentation i'm going to revert to the original implementation
| return nil, fmt.Errorf("invalid 'tls_min_version'") | ||
| } | ||
|
|
||
| tlsClientConfig.MinVersion = tlsMinVersion |
There was a problem hiding this comment.
You did a whole lot of reordering which is making the diff hard to follow. Can you please change the ordering of parsing the various values back to how they were so that it's easier to make sure nothing is being left out?
There was a problem hiding this comment.
okay, i'll rework the PR to minimize the diff
|
Closing as OP seems gone. |
The function
setupTLSConfigis now implemented in terms of thefunction
SetupTLSConfiginconsul/api.The function also respects two additional configuration properties.
tls_server_name(optional) which sets the server name to use asthe SNI host when connecting via TLS (for when it differs from the
server name in
address). Has the same behavior as the environmentvariable
CONSUL_TLS_SERVER_NAME.tls_ca_path(optional) which sets the path to a directory of CAcertificates to use for Consul communication. Has the same behavior
as the environment variable
CONSUL_CAPATH.The function also now logs:
Configfrom the
crypto/tlspackage.tls_key_fileortls_cert_fileare provided in the
confmap.