-
Notifications
You must be signed in to change notification settings - Fork 337
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
chore(*) better handling of SAN mismatch when DP connects to the CP #1205
Conversation
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
@@ -292,10 +294,11 @@ var _ = Describe("bootstrapGenerator", func() { | |||
|
|||
// given | |||
params := bootstrap_config.DefaultBootstrapParamsConfig() | |||
params.XdsHost = "127.0.0.1" | |||
params.XdsHost = "localhost" |
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.
Please note that localhost
does not resolve always to 127.0.0.1
. For example:
$ uname -a
Darwin ............ 19.6.0 Darwin Kernel Version 19.6.0: Thu Oct 29 22:56:45 PDT 2020; root:xnu-6153.141.2.2~1/RELEASE_X86_64 x86_64
$ curl -vv localhost:80
* Trying ::1...
* TCP_NODELAY set
* Connection failed
* connect to ::1 port 80 failed: Connection refused
* Trying 127.0.0.1...
* TCP_NODELAY set
* Connection failed
* connect to 127.0.0.1 port 80 failed: Connection refused
* Failed to connect to localhost port 80: Connection refused
* Closing connection 0
curl: (7) Failed to connect to localhost port 80: Connection refused
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.
Yup, that's true. The cert we were using for tests had localhost
not 127.0.0.1
therefore I thought it was easier to change it here. We are not resolving localhost to 127.0.0.1
in this test.
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.
A small remark on testing. I specifically had this problem running make test
on my Mac.
pkg/xds/bootstrap/generator.go
Outdated
@@ -176,6 +224,14 @@ func (b *bootstrapGenerator) generateFor(proxyId core_xds.ProxyId, dataplane *co | |||
return b.configForParameters(params) | |||
} | |||
|
|||
func (b *bootstrapGenerator) xdsHost(request types.BootstrapRequest) string { | |||
if b.config.XdsHost != "" { |
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.
Why is it possible to return value from config
if the idea is to validate request
?
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.
because config takes precedence over request in case you want to override the XDS Host
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.
But the fact that b.config.XdsHost
satisfies SANs should be validated somewhere in the config validation. You shouldn't allow kuma-cp
to start if certificates don't have SAN for XdsHost
, right? It's not a problem of a bootstrap client which sent the request
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.
Ok, changed validation to "constructor"
…smatch-on-cp-dp Signed-off-by: Jakub Dyszkiewicz <[email protected]>
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
…smatch-on-cp-dp Signed-off-by: Jakub Dyszkiewicz <[email protected]>
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
…1205) Signed-off-by: Jakub Dyszkiewicz <[email protected]> (cherry picked from commit a872b19)
…1205) (#1253) Signed-off-by: Jakub Dyszkiewicz <[email protected]> (cherry picked from commit a872b19) Co-authored-by: Jakub Dyszkiewicz <[email protected]>
Summary
We autogenerate certificates with SANs of all IPs and hostnames from the machine. When CP starts we were warning in logs that we are autogenerating certificates and you can connect with the following addresses. That was not enough, Envoy was not helping with error msgs of
gRPC error code 13
. This PR introduces more graceful handling of this problem. We can check the host and SAN certs when DP is asking for the bootstrap config. If there is mismatch, we are printing the following message:It works even if there is an initial mismatch between SAN and
--cp-address
because kuma-dp by default does not verify CP cert against CA (only on universal, we print warning msg) therefore this request will pass.Documentation