-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
etcdserver: config flag to skip verification of peer client address #10524
etcdserver: config flag to skip verification of peer client address #10524
Conversation
21a6d36
to
ec4196b
Compare
Codecov Report
@@ Coverage Diff @@
## master #10524 +/- ##
==========================================
+ Coverage 63.64% 63.77% +0.13%
==========================================
Files 401 401
Lines 37470 37473 +3
==========================================
+ Hits 23846 23899 +53
+ Misses 12016 11938 -78
- Partials 1608 1636 +28
Continue to review full report at Codecov.
|
cc @gyuho |
cc @wenjiaswe |
Any chance this gets merged anytime soon? |
Ping. Any chances for this mini pull request consisting of just 6 new lines? |
@MartinWeindel Thanks for the PR! Sorry for the delay, would you please add a test to it? Otherwise lgtm @hexfusion would you please also take a look to make sure? |
kubernetes/kubernetes#21283 (comment)
/cc @liggitt |
That’s exactly the purpose of this PR: to trust the certificates issued by the *private* CA. As I tried to explain is it not possible to check the subject/sans on the client side if the ETCD server instances are living in different networks with NAT.
And just to repeat it. This is only an advanced option for non-standard scenarios, which has to be activated explicitly.
The only possible alternative I can think of in our scenario is to set up a VPN, which is quite an effort, if you only need it for the internal communication of a distributed ETCD cluster.
|
bd2cd6e
to
296183f
Compare
@wenjiaswe As there was no test coverage for checking the client address from its certificate, the added test checks the correct behaviour for three cases:
|
Friendly ping @gyuho @liggitt for your opinion, cc @hexfusion as well. Thanks! |
etcdmain/config.go
Outdated
@@ -210,6 +210,7 @@ func newConfig() *config { | |||
fs.StringVar(&cfg.ec.PeerTLSInfo.CRLFile, "peer-crl-file", "", "Path to the peer certificate revocation list file.") | |||
fs.StringVar(&cfg.ec.PeerTLSInfo.AllowedCN, "peer-cert-allowed-cn", "", "Allowed CN for inter peer authentication.") | |||
fs.Var(flags.NewStringsValue(""), "cipher-suites", "Comma-separated list of supported TLS cipher suites between client/server and peers (empty will be auto-populated by Go).") | |||
fs.BoolVar(&cfg.ec.PeerTLSInfo.SkipClientVerify, "peer-skip-client-verify", false, "Skip client IP verification for peer connections.") |
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.
Skip client SAN field verification for peer connections
?
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 think the skip client verify is a very vague description. can we make it clear to include what exactly is skipped?
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.
Changed description to Skip verification of SAN field in client certificate for peer connections.
Any more ideas?
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.
Skip verification of SAN field in client certificate for peer connections
This is clearer.
I will check with SIG-auth https://github.com/kubernetes/community/tree/master/sig-auth. Overall, the use case is reasonable. |
@MartinWeindel can you update commit title to |
7694a69
to
6a28ae2
Compare
etcdmain/config.go
Outdated
@@ -210,6 +210,7 @@ func newConfig() *config { | |||
fs.StringVar(&cfg.ec.PeerTLSInfo.CRLFile, "peer-crl-file", "", "Path to the peer certificate revocation list file.") | |||
fs.StringVar(&cfg.ec.PeerTLSInfo.AllowedCN, "peer-cert-allowed-cn", "", "Allowed CN for inter peer authentication.") | |||
fs.Var(flags.NewStringsValue(""), "cipher-suites", "Comma-separated list of supported TLS cipher suites between client/server and peers (empty will be auto-populated by Go).") | |||
fs.BoolVar(&cfg.ec.PeerTLSInfo.SkipClientVerify, "peer-skip-client-verify", false, "Skip verification of SAN field in client certificate for peer connections.") |
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.
@MartinWeindel can we also change the flag naming to peer-skip-client-san-veirfy? not a fan of long flag, but we need to be clear on what is skipped on flag naming too.
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.
Changed flag to peer-skip-client-san-verification
Can ETCD not be configured with different certs for client and server TLS auth? It seems like checkSAN should be OR'd with the AllowedCN check. This is allowed by the spec for both client and server auth, but is especially useful for client auth. |
I would like some time to test this before we merge. In general this feels like a flag that will promote poor usage and thus reduce overall security. While I understand the reporters networking challenge I still am not convinced this is the only path forward. |
f3ef5bc
to
aeaec51
Compare
defer to @hexfusion |
@hexfusion Have you already found some time to test this? |
@MartinWeindel we are going to talk about this today at the community meeting |
@MartinWeindel it would be nice if we had a solid reproducible use case to test alternative solutions against. Your description has general networking detail but not enough to validate the need for this change. Can you provide a test ENV or steps to reproduce the ENV which we could clearly see the failure case? Thanks! |
@hexfusion You can find a test ENV using Kubernetes services in the GitHub project MartinWeindel/etcd-peer-skip-client-verify. Feel free to contact me if anything is unclear. |
@MartinWeindel after talking with the other maintainers we have decided to move this forward for v3.4 as an experimental flag. Can you please append [1] https://github.com/etcd-io/etcd/blob/master/Documentation/op-guide/configuration.md#experimental-flags |
aeaec51
to
ff3ded7
Compare
@hexfusion Thanks for your endurance |
ff3ded7
to
865beb4
Compare
865beb4
to
718e9c1
Compare
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.
/lgtm
@MartinWeindel thank you for your patience.
Hi @xiang90 is it possible we cherry-pick this change to the etcd 3.2/3.3 too? |
On initialising the peer-to-peer communication, ETCD checks the CN/SAN for the client side. This is not feasible, if the client IP address (of the HTTPS connection) is not stable or unknown at the time of creation of the certificate.
For example, if the peers of a distributed ETCD cluster with TLS enabled run in multiple clouds (or behind NAT with some kind of DHCP),
the IP addresses of the peers are not stable. Another example for such an environment is a Kubernetes cluster using Calico CNI. Here incoming requests from outside the cluster get the client address of the accepting K8s node.
Therefore a new command line option
--peer-skip-client-verify
is introduced to disable the client address check optionally. Basically it should be enough to rely on the knowledge of a valid certificate.This partially fixes #8912 "Make it possible to ignore CN/SAN mismatches" (if certificates are created for DNS names and DNS records are updated according to the current proxy IP addresses).