Skip to content
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

Separate etcd certificates #3777

Merged
merged 1 commit into from
Jun 23, 2021
Merged

Conversation

sergelogvinov
Copy link
Contributor

@sergelogvinov sergelogvinov commented Jun 17, 2021

Pull Request

Fixes #3092
Fixes #3091

Acceptance

Please use the following checklist:

  • you linked an issue (if applicable)
  • you included tests (if applicable)
  • you ran conformance (make conformance)
  • you formatted your code (make fmt)
  • you linted your code (make lint)
  • you generated documentation (make docs)
  • you ran unit-tests (make unit-tests)

See make help for a description of the available targets.


This change is Reviewable

@sergelogvinov
Copy link
Contributor Author

/rebase

1 similar comment
@smira
Copy link
Member

smira commented Jun 17, 2021

/rebase

@@ -26,7 +26,9 @@ type Etcd struct {

// EtcdCertsSpec describes etcd certs secrets.
type EtcdCertsSpec struct {
EtcdPeer *x509.PEMEncodedCertificateAndKey `yaml:"etcdPeer"`
EtcdPeer *x509.PEMEncodedCertificateAndKey `yaml:"etcdPeer"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need EtcdPeer and EtcdClient here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get etcdsecrets.secrets.talos.dev -oyaml -- looks nice
And you can check all certs.

https://github.com/talos-systems/talos/pull/3777/files#diff-f028d821b1f248fa75f380f9a91477df94583726a996ead5f16b089ec24baf04R130 here updateSecrets func recreate the certs

@sergelogvinov sergelogvinov force-pushed the etcd-certs branch 3 times, most recently from 37864fb to f67c7a0 Compare June 18, 2021 13:46
@@ -147,6 +147,8 @@ func (e *Etcd) Runner(r runtime.Runtime) (runner.Runner, error) {
env = append(env, "ETCD_UNSUPPORTED_ARCH=arm64")
}

env = append(env, "ETCD_CIPHER_SUITES=TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305") //nolint:lll
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the effect of this line on etcd security/performance?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref (one of them): etcd-io/etcd#10304

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • AES 128/256 with RSA/ECDSA for CPU with AES-IN
  • chacha-poly with RSA/ECDSA for ARM

https://blog.cloudflare.com/do-the-chacha-better-mobile-performance-with-cryptography/

etcd cluster works inside the tales cluster, and it does not support to connect external etcd nodes. We can set less choice but with better performance.

go.sum Outdated Show resolved Hide resolved
pkg/machinery/go.sum Outdated Show resolved Hide resolved

opts = append(opts,
x509.ExtKeyUsage([]stdlibx509.ExtKeyUsage{
stdlibx509.ExtKeyUsageServerAuth,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we have both client and server cert to be same cert? or should we split client and server certs?

not sure what is the best practice for etcd

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely yes.

But etcd uses this cert to check itself.

I've add talos etcd client cert, now we have 4 certs for etcd cluster.

@smira
Copy link
Member

smira commented Jun 23, 2021

👍 this looks good to me, I want to do a little bit of optimization to skip generating etcd certs twice in the controller and service code.

I will push to this PR resolving conflicts.

Changes:
* Etcd peer port key usage: ServerAuth,ClientAuth
* Etcd client port key usage: ServerAuth,ClientAuth
* Talos etcd client key usage: ClientAuth
* KubeAPI etcd client key usage: ClientAuth
* List of etcd allowed ciphers

Signed-off-by: Serge Logvinov <[email protected]>
Signed-off-by: Andrey Smirnov <[email protected]>
@smira
Copy link
Member

smira commented Jun 23, 2021

/approve

@smira
Copy link
Member

smira commented Jun 23, 2021

/lgtm

@talos-bot talos-bot merged commit b52b206 into siderolabs:master Jun 23, 2021
@sergelogvinov sergelogvinov deleted the etcd-certs branch October 18, 2021 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

etcd client certs etcd cipher suites
3 participants