-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
This was a good find. You are absolutely correct that our usage of server names in outgoing TLS RPC connections was not good.
server_name
shouldn'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.