chore(tls): cleanup sample yaml + auto cert for client auth#430
Conversation
| // transport.SelfCert generates a self-signed cert whose ExtKeyUsage | ||
| // defaults to ServerAuth only. The operator reuses the server Secret | ||
| // as its own mTLS client identity against the etcd client listener. |
There was a problem hiding this comment.
| // transport.SelfCert generates a self-signed cert whose ExtKeyUsage | |
| // defaults to ServerAuth only. The operator reuses the server Secret | |
| // as its own mTLS client identity against the etcd client listener. | |
| // transport.SelfCert generates a self-signed cert whose ExtKeyUsage | |
| // defaults to ServerAuth only. The operator reuses the server Secret | |
| // as its own mTLS client identity talking with the etcd cluster. |
| // defaults to ServerAuth only. The operator reuses the server Secret | ||
| // as its own mTLS client identity against the etcd client listener. | ||
| // The cert must also carry ClientAuth. | ||
| tlsInfo, selfCertErr := transport.SelfCert(zap.NewNop(), tmpDir, hosts, validityYears, x509.ExtKeyUsageClientAuth) |
There was a problem hiding this comment.
I am curious why we did not see any issue previously without x509.ExtKeyUsageClientAuth? cc @ArkaSaha30
There was a problem hiding this comment.
What I see is that etcdserver pod has never enabled TLS args.
There was a problem hiding this comment.
What I see is that etcdserver pod has never enabled TLS args.
Just double checked the source code on main branch, this is true. We mounted the secret into POD, but actually do not use it at all. Peers talk to each other use http always, and etcd-operator talks to etcd via http always as well.
We missed such an elephant!
Signed-off-by: shenmu.wy <shenmu.wy@antfin.com>
I think this is a good catch. But why we did not see any issue previously with Statefulset? The etcd-operator needs to perform health check, it should fail right? @ArkaSaha30 did you see any issue previously when you were testing the feature? |
| autoCfg: | ||
| commonName: "etcd-operator-system" | ||
| validityDuration: 365d | ||
| validityDuration: 8760h No newline at end of file |
There was a problem hiding this comment.
We need to consider an alternative utility to replace the time.ParseDuration, we should support duration like "365d"
There was a problem hiding this comment.
OK, there gona be some research work. Do you mind that I handle it in a follow-up PR?
There was a problem hiding this comment.
Do you mind that I handle it in a follow-up PR?
sure
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, silentred The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ff197c3 to
e9a33e9
Compare
2 participants
sub-task 1 of #428
Questions might be asked:
Why change sample yaml?
time.ParseDurationcannot parse strings like365d. v0.2.0 produces the same error. If support for parsing the d suffix is desired, it can be addressed in a follow-up PR.Why add
ClientAuthusage to cert?The controller is supposed use the server TLS certificate as the etcd client certificate to talk to the etcd server; it does not use client TLS.
Because the client ca.crt is issued by an independent CA and is not included in the etcd server's --trusted-ca-file, client TLS cannot complete a handshake with the etcd server today.
Future work: bundle the client CA together with the server CA and add the bundle to the etcd server's trust chain (--trusted-ca-file). Once that is in place, client TLS will be able to communicate with the etcd server normally.
/cc @ahrtr @nwnt