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

Switch to generating ECDSA keys #1535

Closed
2 of 3 tasks
rojkov opened this issue Apr 25, 2019 · 28 comments
Closed
2 of 3 tasks

Switch to generating ECDSA keys #1535

rojkov opened this issue Apr 25, 2019 · 28 comments
Assignees
Labels
area/security kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence.
Milestone

Comments

@rojkov
Copy link

rojkov commented Apr 25, 2019

edit(neolit123)

v1beta4 tasks

- [ ] remove the feature gate PublicKeysECDSA (same release as v1beta4 or one later max):
should be done with the removal of v1beta3 !


FEATURE REQUEST

To unify the type of certificates used across Kubernetes make kubeadm generate ECDSA keys and certificates instead of RSA ones during cluster initialization and key renewal.

Motivation

As explained in OpenSSL wiki the primary advantage of using Elliptic Curve based cryptography is reduced key size and hence speed.

In addition to that kubelet already generates self-signed server certificates of the ECDSA type and uses ECDSA client certificates when communicating with API servers. It would reduce complexity if the same defaults were used across Kubernetes components.

Goals

  • unconditionally generate ECDSA keys upon cluster initialization.

Proposal

Since people feel more comfortable when there is a transition period it makes sense to introduce a configuration option first for the type of keys used in new clusters or the clusters undergoing upgrades.

The selected key type should be defined in InitConfiguration or optionally overridden with a command line option applicable to the init command and to the certs related subcommands.

The function pkiutil.NewPrivateKey() should accept keyType parameter of two possible values ECDSA and RSA. The actual value is passed from InitConfiguration by callers. Currently the function unconditionally generates RSA keys only.

The default value for the option is RSA.

It is OK to have certificates of mixed types in the same certificate chain thus after two or three releases the default value can be switched to ECDSA.

After the transition period is over the option can be removed completely and all new keys are going to be of the ECDSA type.

@fabriziopandini
Copy link
Member

@rojkov is it possible to merge this issue with #984?

@rojkov
Copy link
Author

rojkov commented Apr 26, 2019

@fabriziopandini I don't think it's needed as they are different issues: #984 is about kubeadm accepting pre-generated ECDSA certs (without kubeadm) and this issue is about kubeadm generating ECDSA certs itself.

@timothysc timothysc added this to the v1.15 milestone Apr 26, 2019
@timothysc timothysc added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Apr 26, 2019
@timothysc
Copy link
Member

@rojkov - could you please attach your PRs to this issue and what else needs to get done here?

/cc @kad

@neolit123
Copy link
Member

xref kubernetes/kubernetes#76390

@neolit123
Copy link
Member

moving to melestone Next.

kubernetes/kubernetes#76390
added support for user generated ECDSA.

this ticket tracks the generation of ECDSA by kubeadm.

After the transition period is over the option can be removed completely and all new keys are going to be of the ECDSA type.

we probably should define when this transition is going to happen.

@neolit123 neolit123 modified the milestones: v1.16, Next Jun 10, 2019
@neolit123 neolit123 added area/security kind/feature Categorizes issue or PR as related to a new feature. labels Jun 10, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 8, 2019
@neolit123 neolit123 added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 8, 2019
@anguslees
Copy link
Member

ECDSA keys are significantly faster fwiw, particularly on arm32. I could not enable TLS peering certs on my rPi2 cluster and have the health checks (TLS handshakes) complete in less than the etcd hard-coded connect timeout with RSA keys - ECDSA keys work fine.

@fabriziopandini
Copy link
Member

@rojkov thanks for the feedback.
@neolit123 @timothysc we should start to consider if/how to make ECDSA became the kubeadm default

@neolit123
Copy link
Member

neolit123 commented Nov 11, 2019

@anguslees

ECDSA keys are significantly faster fwiw, particularly on arm32. I could not enable TLS peering certs on my rPi2 cluster and have the health checks (TLS handshakes) complete in less than the etcd hard-coded connect timeout with RSA keys - ECDSA keys work fine.

i think i've seen related issues before.
the workaround until now was patching the timeouts to be longer.

but isn't 15 seconds a bit too much even for rpi?

people were considering race conditions at some point on ARM...
also the kubeadm kubeconfig logic randomly panic-ed for a certain rpi2 user and that did not make any sense (possibly compiler bugs?).

rpi in general is not supported until we have e2e tests for it.

@fabriziopandini

we should start to consider if/how to make ECDSA became the kubeadm default

+1 if @rojkov can get back to this work.

@rojkov
Copy link
Author

rojkov commented Nov 11, 2019

I might get back to this. Will update tomorrow or on this Wednesday.

@rojkov
Copy link
Author

rojkov commented Nov 14, 2019

I need to finalize one my PR to Envoy's main repo and one more PR to the OpenSSL extension to Envoy first. They take all my time budget allowed for open source contributions. Then I can continue with ECDSA keys.

@neolit123
Copy link
Member

thanks @rojkov

@rojkov
Copy link
Author

rojkov commented Jan 8, 2020

Submitted kubernetes/kubernetes#86953

@neolit123 neolit123 added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jan 8, 2020
@neolit123
Copy link
Member

neolit123 commented Jan 8, 2020

@rojkov
during the kubeadm meeting today we discussed the alternative option to use feature gates instead of adding this as part of the API, but we didn't make a clear decision.

i'm personally in favor of adding this as part of the API, but it means it has to wait until we have enough new features for v1beta3. we might not be able to release v1beta3 this cycle because kubeadm is moving out of k/k and that's where most of the effort will be.

if you want to add this as part of kubeadm ASAP, then it has to be a future gate and only work on init. my problem with that is that the new cluster certs will be stuck to ECDSA.

ideally i wish we support renew paths that support transitioning old certs to ECDSA from RSA, or the other way around for people to opt-out of the feature gate.

@neolit123
Copy link
Member

@rojkov thank you!

@stealthybox
Copy link
Member

Thanks for working on this.

What's the rough order of operations to manually move a cluster from RSA to ECDSA.

I can guess that there are dependencies on certain flags in components and the communication between those components. (kube-apiserver / kubelet / etcd / ....)

Is downtime required in some or all upgrade techniques? (multiple controlplane nodes?)

If we can document this with the gated feature, it will serve users and maybe point us toward a more automated behavior.

@rojkov
Copy link
Author

rojkov commented Jan 30, 2020

@stealthybox It's safe to have keys of different types in a single certificate bundle or chain. I don't expect problems if to add the ECDSA feature gate to ClusterConfiguration of an existing cluster and then renew the keys. But it makes sense to test first :)

@neolit123 neolit123 added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Mar 31, 2021
@neolit123 neolit123 modified the milestones: Next, v1.22 Apr 14, 2021
@neolit123 neolit123 mentioned this issue Apr 14, 2021
16 tasks
@neolit123
Copy link
Member

we decided to not add this as a field in v1beta3, due to some uncertainties around the feature.
we should discuss this more.

@neolit123 neolit123 modified the milestones: v1.22, Next Jun 9, 2021
@maximmasiutin
Copy link

I have generated keys and certificates with the secp256k1, run rke version v1.2.8 today and got the following error:

FATA[0000] Failed to read certificates from dir [/home/max/cluster_certs]: failed to read certificate [kube-apiserver-requestheader-ca.pem]: x509: unsupported elliptic curve

kubectl version:

Client Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.1", GitCommit:"5e58841cce77d4bc13713ad2b91fa0d961e69192", GitTreeState:"clean", BuildDate:"2021-05-12T14:18:45Z", GoVersion:"go1.16.4", Compiler:"gc", Platform:"linux/amd64"}

Which of the curves are actually supported today?

@neolit123
Copy link
Member

neolit123 commented Jun 16, 2021 via email

@chendave chendave mentioned this issue Jun 9, 2023
26 tasks
@chendave
Copy link
Member

This feature is still alpha, the original plan for this is making the ECDSA as the defaults, and deprecated the RSA which I think is breaking change as well.

we decided to not add this as a field in v1beta3, due to some uncertainties around the feature.

not sure if we can add this to v1beta4, since I haven't done enough test around this, something we can do for v1beta4 is graduate this feature to beta with RSA still supported, properly, we need to create some e2e testcases for this to be beta.

@neolit123 @fabriziopandini @SataQiu @pacoxu and others, WDTY about this?

@neolit123
Copy link
Member

neolit123 commented Jun 15, 2023

my take:
i think we need to remove it from being a feature gate. FG implies ECDSA will be the default without opt out. some users cannot use ECDSA in product actually. instead it seems it can be a cluster level field (off by default). preferably without a CLI flag for kubeadm init/upgrade.

re tests: if we add it as a field we need to test it like that. currently it has a dedicted e2e as a FG, i believe.

edit: i can work on this if we have agreement.

@neolit123 neolit123 assigned neolit123 and unassigned rojkov Jun 15, 2023
@chendave
Copy link
Member

chendave commented Jun 15, 2023

thanks @neolit123 !

I am +1 for the idea of cluster level field instead of a FG, so that this won't be a breaking change.

@neolit123
Copy link
Member

neolit123 commented Jun 15, 2023

thanks @neolit123 !

I am +1 for the idea of cluster level field instead of a FG, so that this won't be a breaking change.

technically, it will be a breaking change as we will drop the alpha FG. but not breaking in the API, per se.

@neolit123
Copy link
Member

PR that adds add v1beta4.ClusterConfiguration.EncryptionAlgorithm is up:
kubernetes/kubernetes#120417

@neolit123 neolit123 modified the milestones: v1.29, v1.30 Nov 1, 2023
@neolit123 neolit123 modified the milestones: v1.30, v1.31 Apr 5, 2024
@neolit123
Copy link
Member

add kinder e2e test (for latest only)?:
TO-BE-DECIDED, maybe we are OK with just unit tests?

maybe so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

No branches or pull requests

10 participants