etcdctl: set TLS servername on discovery #6084
Conversation
|
lgtm. /cc @xiang90 |
|
I think the srv discovery has two layers of indirections. First we reach the srv domain, then resolve the services domain, and then resolve the actual IP. We should trust the actual service domain, not the srv domain, right? An example is here: |
|
@xiang90 I believe the problem is if there's a MITM on the dns server then a SRV record, when discovering on |
|
The user specifies that they want to connect to the SRV record, so that is the place where the checked servername needs to come from. For each level of indirection, you need to consider if you trust it or not. In this case, you do not trust the SRV lookup, and then you do not trust the lookup of the services domain. As such, you need to use the SRV domain as the servername. An alternate case might be the indirection of connecting to one etcd node and then discovering other nodes from there. As we trust the first etcd node, we trust our method of indirection, it is OK to simply verify the certs of the other nodes discovered in that way against their IPs. This is all a nice example of the end to end principle, fwiw. |
|
@bluepeppers can you confirm this patch works as intended? |
|
Yeah, let me just rebuild the cluster exhibiting this bug. I'll try and get back to you some time today. |
|
Yup, that works |
|
With this solution, we force users to use the same domain for both discovery service and etcd service when TLS is set. User cannot use a.com for discovery, but *.b.com for the actual clusters. We can make this better by not setting serverName if a cert is provided explicitly. |
|
Also we probably need to change the clustering doc accordingly. |
|
@xiang90 I think we need to fix the peer discovery path in etcdmain/embed too in case there's no explicit CA? |
|
@heyitsanthony Yes. Peer discovery, gateway discovery. Fix docs for them. |
heyitsanthony
force-pushed
the
srv-servername
branch
4 times, most recently
from
53fe7fb to
545e8da
Compare
| @@ -81,6 +85,37 @@ func startGateway(cmd *cobra.Command, args []string) { | |||
| os.Exit(1) | |||
| } | |||
| plog.Infof("discovered the cluster %s from %s", eps, gatewayDNSCluster) | |||
|
|
|||
| // confirm TLS connections are good | |||
| tlsInfo := transport.TLSInfo{ | |||
There was a problem hiding this comment.
would tlsutil be better? not sure about how to draw the lines between transport/tlsutil/netutil
There was a problem hiding this comment.
ok. probably tlsutil is better.
heyitsanthony
force-pushed
the
srv-servername
branch
from
545e8da to
ef218d8
Compare
| @@ -81,6 +86,21 @@ func startGateway(cmd *cobra.Command, args []string) { | |||
| os.Exit(1) | |||
| } | |||
| plog.Infof("discovered the cluster %s from %s", eps, gatewayDNSCluster) | |||
| // confirm TLS connections are good | |||
| if !gatewayInsecureDiscovery { | |||
| tlsInfo := transport.TLSInfo{ | |||
There was a problem hiding this comment.
probably log that we are verifying discovered endpoints a,b,c here
|
LGTM after adding logging informations when we skip any endpoints. |
heyitsanthony
force-pushed
the
srv-servername
branch
from
ef218d8 to
a752338
Compare
ServerName prevents accepting forged SRV records with cross-domain credentials. ValidateSecureEndpoints prevents downgrade attacks from SRV records.
/cc @bluepeppers