Don't use server_name config for RPC connections#5394
Conversation
|
|
||
| if tlsConfig.ServerName == "" { | ||
| tlsConfig.ServerName = c.base.NodeName | ||
| InsecureSkipVerify: !c.base.VerifyServerHostname, |
There was a problem hiding this comment.
Since ServerName is not used here anymore, this only depends on VerifyServerHostname now.
| tlsConfig.ServerName = c.base.ServerName | ||
| if tlsConfig.ServerName == "" { | ||
| tlsConfig.ServerName = c.base.NodeName | ||
| } |
There was a problem hiding this comment.
OutgoingTLSConfigForChecks is the only place where we allow setting it now. Even that I find questionable... But I don't want to change to many things at once.
| require.Equal(t, tlsConf.ServerName, "consul.example.com") | ||
| require.False(t, tlsConf.InsecureSkipVerify) | ||
| require.Empty(t, tlsConf.ServerName) | ||
| require.True(t, tlsConf.InsecureSkipVerify) |
There was a problem hiding this comment.
I was thinking about this test, which I had to change to make it pass.
It is skipping the builtin check from golang tls, but we are still checking the cert chain. Since VerifyServerHostname is disabled, we do not check that. I think this is what we want.
mkeeler
left a comment
There was a problem hiding this comment.
This was a good find. You are absolutely correct that our usage of server names in outgoing TLS RPC connections was not good.
server_nameshouldn't be used for RPC connections. If it is hardcoded, it can never properly work in a multi dc setup, since for that it has to be dynamic to account for forwarding calls into other dcs. Fixes #5357.This PR contains a bunch of comment wrapping.