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

kubeadm: allow creating a cluster with ECDSA keys #86953

Merged
merged 1 commit into from Feb 24, 2020

Conversation

@rojkov
Copy link
Member

rojkov commented Jan 8, 2020

What type of PR is this?
/kind feature

What this PR does / why we need it:
Allow creating a cluster with ECDSA keys. The selected key type is defined in InitConfiguration's publicKeyAlgorithm field. The default value for the option is RSA still.
Upon renewal the new certs are generated with the same algo used for the corresponding old certs.

Which issue(s) this PR fixes:
Contributes to kubernetes/kubeadm#1535

Special notes for your reviewer:
I decided to leave a new command line option for specifying encryption key algo out of scope of this PR since the flag would be removed anyway. The option is set via InitConfiguration only.

Does this PR introduce a user-facing change?:

kubeadm: add the experimental feature gate PublicKeysECDSA that can be used to create a
cluster with ECDSA certificates from "kubeadm init". Renewal of existing ECDSA certificates is
also supported using "kubeadm alpha certs renew", but not switching between the RSA and
ECDSA algorithms on the fly or during upgrades.
}

out.PublicKeyAlgorithm = &PublicKeyAlgorithm{
PublicKeyAlgorithm: in.PublicKeyAlgorithm,

This comment has been minimized.

Copy link
@neolit123

neolit123 Jan 8, 2020

Member

v1beta1 and v1beta2 are locked at this point, which means we can only add this as part of v1beta3.
i'm marking kubernetes/kubeadm#1535 as kind/api-change so we need to batch-include it in the next iteration of the API.

this is a nice change to push the kubeadm config forward.

@@ -22,6 +22,7 @@ import (

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"

This comment has been minimized.

Copy link
@neolit123

neolit123 Jan 8, 2020

Member

the public types cannot include the internal types. this is breaking an api-machinery contract.

// RSA represents RSA algorithm
RSA PublicKeyAlgorithm = iota
// ECDSA represents ECDSA algorithm
ECDSA

This comment has been minimized.

Copy link
@neolit123

neolit123 Jan 8, 2020

Member

can we get away with using elements from the go enumeration directly?
https://golang.org/src/crypto/x509/x509.go

type PublicKeyAlgorithm int

const (
	UnknownPublicKeyAlgorithm PublicKeyAlgorithm = iota
	RSA
	DSA
	ECDSA
	Ed25519
)

e.g. x509.ECDSA

EDIT: i ended up proposing using "string" bellow.

This comment has been minimized.

Copy link
@rosti

rosti Jan 9, 2020

Member

I would rather use string consts here too and then convert them to the Go lang types shortly before use. This would allow for a practically No-Op conversion between versioned types, that have the field and the internal ones.

@@ -54,6 +79,9 @@ type InitConfiguration struct {
// CertificateKey sets the key with which certificates and keys are encrypted prior to being uploaded in
// a secret in the cluster during the uploadcerts init phase.
CertificateKey string

// PublicKeyAlgorithm sets the algorithm used for generating encryption keys: RSA or ECDSA.
PublicKeyAlgorithm PublicKeyAlgorithm

This comment has been minimized.

Copy link
@neolit123

neolit123 Jan 8, 2020

Member

InitConfiguration vs something else is an interesting topic.
do we gain anything if this is part of ClusterConfiguration?

i'm hoping that nobody would like to switch algorithms on a working cluster, but unfortunately people will want to do that on their existing RSA based clusters. this implies CA rotation, so it opens a whole can of worms.

i'm starting to think that this feature needed a KEP (enhancement proposal that clarifies all the details).

This comment has been minimized.

Copy link
@rosti

rosti Jan 9, 2020

Member

It makes more sense to me to have this field in the ClusterConfiguration. I am too concerned if certificate renewal will work as expected when switching from RSA to ECDSA.

@@ -31,6 +31,10 @@ func Convert_kubeadm_InitConfiguration_To_v1beta1_InitConfiguration(in *kubeadm.
return errors.New("certificateKey field is not supported by v1beta1 config format")
}

if in.PublicKeyAlgorithm != kubeadm.RSA {
return errors.New("publicKeyAlgorithm field is not supported by v1beta1 config format")

This comment has been minimized.

Copy link
@neolit123

neolit123 Jan 8, 2020

Member

as mentioned earlier, old config types should not know about this field and just error out at the unmarshall stage.

)

// PublicKeyAlgorithm is a wrapper around kubeadm.PublicKeyAlgorithm which supports
// correct marshaling to YAML and JSON. In particular, it marshals into strings.

This comment has been minimized.

Copy link
@neolit123

neolit123 Jan 8, 2020

Member

instead of using int in the internal config (also not use the go enumeration) we might as well use "string" in the internal configuration and this can avoid having this file.

it would still need to convert from the go enumeration to our string enumeration.

This comment has been minimized.

Copy link
@rosti

rosti Jan 9, 2020

Member

Indeed, we don't need this file. Let's use strings and string consts here.

@@ -101,7 +101,7 @@ func CreateCACertAndKeyFiles(certSpec *KubeadmCert, cfg *kubeadmapi.InitConfigur
if certSpec.CAName != "" {
return errors.Errorf("this function should only be used for CAs, but cert %s has CA %s", certSpec.Name, certSpec.CAName)
}
klog.V(1).Infof("creating a new certificate authority for %s", certSpec.Name)
klog.V(1).Infof("creating a new certificate authority for %s (public key algo: %s)", certSpec.Name, cfg.PublicKeyAlgorithm)

This comment has been minimized.

Copy link
@neolit123

neolit123 Jan 8, 2020

Member

algo -> algorithm

Copy link
Member

neolit123 left a comment

@rojkov
thanks for working on this.

/cc @rosti on the API change.
we need to batch for v1beta3.

Copy link
Member

rosti left a comment

Thanks @rojkov !
It's the config representation and possible upgrade issues that bother me here.
We should use simple string consts in the config and currently only in the internal types. The new public field should be added in v1beta3 when we decide to have it.
One possibility to enable it externally is to use an alpha feature gate. We probably need more time discussing it.

// RSA represents RSA algorithm
RSA PublicKeyAlgorithm = iota
// ECDSA represents ECDSA algorithm
ECDSA

This comment has been minimized.

Copy link
@rosti

rosti Jan 9, 2020

Member

I would rather use string consts here too and then convert them to the Go lang types shortly before use. This would allow for a practically No-Op conversion between versioned types, that have the field and the internal ones.

)

// PublicKeyAlgorithm is a wrapper around kubeadm.PublicKeyAlgorithm which supports
// correct marshaling to YAML and JSON. In particular, it marshals into strings.

This comment has been minimized.

Copy link
@rosti

rosti Jan 9, 2020

Member

Indeed, we don't need this file. Let's use strings and string consts here.

@@ -54,6 +79,9 @@ type InitConfiguration struct {
// CertificateKey sets the key with which certificates and keys are encrypted prior to being uploaded in
// a secret in the cluster during the uploadcerts init phase.
CertificateKey string

// PublicKeyAlgorithm sets the algorithm used for generating encryption keys: RSA or ECDSA.
PublicKeyAlgorithm PublicKeyAlgorithm

This comment has been minimized.

Copy link
@rosti

rosti Jan 9, 2020

Member

It makes more sense to me to have this field in the ClusterConfiguration. I am too concerned if certificate renewal will work as expected when switching from RSA to ECDSA.

@fedebongio

This comment has been minimized.

Copy link
Contributor

fedebongio commented Jan 9, 2020

/assign @yliaog to take a look at the client-go part

@yliaog

This comment has been minimized.

Copy link
Contributor

yliaog commented Jan 9, 2020

for client-go
/lgtm

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Jan 28, 2020

thanks for the update @rojkov
@kubernetes/sig-cluster-lifecycle-pr-reviews

the deadline for KEPs is today. if you think this is something we can enable in the API, support upgrades and switching between algorithm types, we'd need a KEP for a future release.

Does this PR introduce a user-facing change?:
One can choose ECDSA to generate cluster certificates in kubeadm by specifying the
publicKeyAlgoritm field of the InitConfiguration config.

i think we should add the following note instead:

kubeadm: add the experimental feature gate PublicKeysECDSA that can be used to create a cluster with ECDSA certificates from "kubeadm init". Renewal of existing ECDSA certificates is also supported using "kubeadm alpha certs renew", but not switching between the RSA and ECDSA algorithms on the fly or during upgrades.

making it slightly more detailed and "official". please amend the details if needed.

@rojkov

This comment has been minimized.

Copy link
Member Author

rojkov commented Jan 28, 2020

@neolit123 Thanks! I updated the notes.

There's no chance I can write the KEP today unfortunately, but the feature not urgent for us now.

@rojkov

This comment has been minimized.

Copy link
Member Author

rojkov commented Jan 28, 2020

/test pull-kubernetes-e2e-gce

1 similar comment
@rojkov

This comment has been minimized.

Copy link
Member Author

rojkov commented Jan 29, 2020

/test pull-kubernetes-e2e-gce

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Feb 19, 2020

/assign @neolit123

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Feb 19, 2020

/assign @rosti

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Feb 19, 2020

@rojkov sorry for the delays on reviews, everyone is busy again.

so i think merging this is fine.
/approve
/priority important-longterm

i will have another pass tomorrow and potentially apply the LGTM label too.

Organization []string
AltNames AltNames
Usages []x509.ExtKeyUsage
PublicKeyAlgorithm x509.PublicKeyAlgorithm

This comment has been minimized.

Copy link
@neolit123

neolit123 Feb 20, 2020

Member

@rojkov i pinged the #sig-api-machinery channel about this change and it is not approved.
https://kubernetes.slack.com/archives/C0EG7JC6T/p1582187786382200

i guess the alternative is to pass the algorithm type as an argument to functions that need it.

This comment has been minimized.

Copy link
@rojkov

rojkov Feb 20, 2020

Author Member

Ok! I'll follow @rosti's suggestion to introduce a wrapper struct.

Copy link
Member

rosti left a comment

Thanks @rojkov !
I've got a few comments left.


"k8s.io/apimachinery/pkg/runtime/schema"
)

// PublicKeyAlgorithm is a type of encryption keys used for certificates generated by kubeadm
type PublicKeyAlgorithm string

This comment has been minimized.

Copy link
@rosti

rosti Feb 20, 2020

Member

I don't think that we need this type. This is strictly an internal type and as such we can just use x509.PublicKeyAlgorithm.

@@ -158,6 +190,16 @@ const (
KubeDNS DNSAddOnType = "kube-dns"
)

// Version returns the current version of DNS type
func (dnsType DNSAddOnType) Version() string {

This comment has been minimized.

Copy link
@rosti

rosti Feb 20, 2020

Member

I don't think we need this change in this PR.

@@ -401,6 +443,15 @@ func (cfg *ClusterConfiguration) GetControlPlaneImageRepository() string {
return cfg.ImageRepository
}

// PublicKeyAlgorithm returns the type of encryption keys used in the cluster.
func (cfg *ClusterConfiguration) PublicKeyAlgorithm() PublicKeyAlgorithm {

This comment has been minimized.

Copy link
@rosti

rosti Feb 20, 2020

Member

Again, this should return x509.PublicKeyAlgorithm.

Organization []string
AltNames AltNames
Usages []x509.ExtKeyUsage
PublicKeyAlgorithm x509.PublicKeyAlgorithm

This comment has been minimized.

Copy link
@rosti

rosti Feb 20, 2020

Member

Do we really need this setting here? We can create a kubeadm wrapper struct that embeds this one and adds the PublicKeyAlgorithm field.

@rojkov rojkov force-pushed the rojkov:ecdsa branch from 10a34c3 to 1ab3e61 Feb 21, 2020
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 21, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, rojkov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rojkov

This comment has been minimized.

Copy link
Member Author

rojkov commented Feb 21, 2020

@neolit123 @rosti Thank you for review! I've updated the PR.

Still I had to touch seemingly unrelated features_test.go to break the import cycle. Let me know if it's better to submit a separate PR for that.

@@ -129,7 +129,7 @@ func TestNewFeatureGate(t *testing.T) {
func TestValidateVersion(t *testing.T) {
var someFeatures = FeatureList{
"feature1": {FeatureSpec: featuregate.FeatureSpec{Default: false, PreRelease: featuregate.Beta}},
"feature2": {FeatureSpec: featuregate.FeatureSpec{Default: true, PreRelease: featuregate.Alpha}, MinimumVersion: constants.MinimumControlPlaneVersion.WithPreRelease("alpha.1")},
"feature2": {FeatureSpec: featuregate.FeatureSpec{Default: true, PreRelease: featuregate.Alpha}, MinimumVersion: version.MustParseSemantic("v1.17.0").WithPreRelease("alpha.1")},

This comment has been minimized.

Copy link
@neolit123

neolit123 Feb 21, 2020

Member

i think this is fine, but wonder why it broke related to this change:

Still I had to touch seemingly unrelated features_test.go to break the import cycle. Let me know if it's better to submit a separate PR for that.

This comment has been minimized.

Copy link
@rojkov

rojkov Feb 21, 2020

Author Member

This features imports constants which imports kubeadmapi which is going to import features in this PR.

This comment has been minimized.

Copy link
@neolit123

neolit123 Feb 21, 2020

Member

ok, understood.

EDIT: P.S. once we nuke kube-dns from kubeadm.
constants will stop importing the api.

This comment has been minimized.

Copy link
@rosti

rosti Feb 24, 2020

Member

Actually GetDNSVersion should be in the DNS addon package, but that way we get circular dependency through the images package...

@@ -26,13 +26,16 @@ import (

certutil "k8s.io/client-go/util/cert"
"k8s.io/client-go/util/keyutil"
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"

This comment has been minimized.

Copy link
@neolit123

neolit123 Feb 21, 2020

Member

CertConfig should be defined here, so that the kubeadm API does not have to be imported in the utils.
ultimately it is a utility structure similarly to the wrapped one from client-go.

This comment has been minimized.

Copy link
@rojkov

rojkov Feb 21, 2020

Author Member

Thanks! Will do.
This should fix the failing typecheck test as well.

This comment has been minimized.

Copy link
@rojkov

rojkov Feb 24, 2020

Author Member

@neolit123 I had to move CertConfig to pkiutil, otherwise there would be another import loop (certs/util -> pkiutil -> certs/util).

Perhaps it makes sense to merge certs/util into pkiutil at some point.

This comment has been minimized.

Copy link
@neolit123

neolit123 Feb 24, 2020

Member

i think what i meant was "util/pkiutil" and not "util/certs".
it seems like the better location.

The selected key type is defined by kubeadm's --feature-gates option:
if it contains PublicKeysECDSA=true then ECDSA keys will be generated
and used.

By default RSA keys are used still.

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>
@rojkov rojkov force-pushed the rojkov:ecdsa branch from 1ab3e61 to 109f5db Feb 24, 2020
@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Feb 24, 2020

this looks fine to me.
@rosti any more comments?

@rosti
rosti approved these changes Feb 24, 2020
Copy link
Member

rosti left a comment

Thanks @rojkov !
/lgtm

@@ -129,7 +129,7 @@ func TestNewFeatureGate(t *testing.T) {
func TestValidateVersion(t *testing.T) {
var someFeatures = FeatureList{
"feature1": {FeatureSpec: featuregate.FeatureSpec{Default: false, PreRelease: featuregate.Beta}},
"feature2": {FeatureSpec: featuregate.FeatureSpec{Default: true, PreRelease: featuregate.Alpha}, MinimumVersion: constants.MinimumControlPlaneVersion.WithPreRelease("alpha.1")},
"feature2": {FeatureSpec: featuregate.FeatureSpec{Default: true, PreRelease: featuregate.Alpha}, MinimumVersion: version.MustParseSemantic("v1.17.0").WithPreRelease("alpha.1")},

This comment has been minimized.

Copy link
@rosti

rosti Feb 24, 2020

Member

Actually GetDNSVersion should be in the DNS addon package, but that way we get circular dependency through the images package...

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 24, 2020
@k8s-ci-robot k8s-ci-robot merged commit 116e27f into kubernetes:master Feb 24, 2020
16 of 17 checks passed
16 of 17 checks passed
tide Not mergeable. Retesting: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation rojkov authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-alpha-features Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-e2e-kind-ipv6 Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 24, 2020
@rojkov rojkov deleted the rojkov:ecdsa branch Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.