From bda722bb6845f3828eb60a581b65680a77a1300f Mon Sep 17 00:00:00 2001 From: SataQiu Date: Sat, 11 Nov 2023 13:29:00 +0800 Subject: [PATCH] kubeadm: support updating certificate organization during 'kubeadm certs renew' --- .../app/phases/certs/renewal/manager.go | 84 +++++++++++++++++-- .../app/phases/certs/renewal/manager_test.go | 69 ++++++++++----- .../phases/certs/renewal/readwriter_test.go | 8 +- .../app/phases/kubeconfig/kubeconfig_test.go | 2 +- cmd/kubeadm/app/util/certs/util.go | 5 +- 5 files changed, 135 insertions(+), 33 deletions(-) diff --git a/cmd/kubeadm/app/phases/certs/renewal/manager.go b/cmd/kubeadm/app/phases/certs/renewal/manager.go index 27b350694b1..61c2ccdb714 100644 --- a/cmd/kubeadm/app/phases/certs/renewal/manager.go +++ b/cmd/kubeadm/app/phases/certs/renewal/manager.go @@ -46,6 +46,8 @@ type Manager struct { cas map[string]*CAExpirationHandler } +type certConfigMutatorFunc func(*certutil.Config) error + // CertificateRenewHandler defines required info for renewing a certificate type CertificateRenewHandler struct { // Name of the certificate to be used for UX. @@ -66,6 +68,10 @@ type CertificateRenewHandler struct { // readwriter defines a CertificateReadWriter to be used for certificate renewal readwriter certificateReadWriter + + // certConfigMutators holds the mutator functions that can be applied to the input cert config object + // These functions will be run in series. + certConfigMutators []certConfigMutatorFunc } // CAExpirationHandler defines required info for CA expiration check @@ -109,17 +115,19 @@ func NewManager(cfg *kubeadmapi.ClusterConfiguration, kubernetesDir string) (*Ma for _, cert := range certs { // create a ReadWriter for certificates stored in the K8s local PKI pkiReadWriter := newPKICertificateReadWriter(rm.cfg.CertificatesDir, cert.BaseName) + certConfigMutators := loadCertConfigMutators(cert.BaseName) // adds the certificateRenewHandler. // PKI certificates are indexed by name, that is a well know constant defined // in the certsphase package and that can be reused across all the kubeadm codebase rm.certificates[cert.Name] = &CertificateRenewHandler{ - Name: cert.Name, - LongName: cert.LongName, - FileName: cert.BaseName, - CAName: ca.Name, - CABaseName: ca.BaseName, //Nb. this is a path for etcd certs (they are stored in a subfolder) - readwriter: pkiReadWriter, + Name: cert.Name, + LongName: cert.LongName, + FileName: cert.BaseName, + CAName: ca.Name, + CABaseName: ca.BaseName, // Nb. this is a path for etcd certs (they are stored in a subfolder) + readwriter: pkiReadWriter, + certConfigMutators: certConfigMutators, } } @@ -230,8 +238,15 @@ func (rm *Manager) RenewUsingLocalCA(name string) (bool, error) { } // extract the certificate config + certConfig := certToConfig(cert) + for _, f := range handler.certConfigMutators { + if err := f(&certConfig); err != nil { + return false, err + } + } + cfg := &pkiutil.CertConfig{ - Config: certToConfig(cert), + Config: certConfig, EncryptionAlgorithm: rm.cfg.EncryptionAlgorithmType(), } @@ -273,8 +288,14 @@ func (rm *Manager) CreateRenewCSR(name, outdir string) error { } // extracts the certificate config + certConfig := certToConfig(cert) + for _, f := range handler.certConfigMutators { + if err := f(&certConfig); err != nil { + return err + } + } cfg := &pkiutil.CertConfig{ - Config: certToConfig(cert), + Config: certConfig, EncryptionAlgorithm: rm.cfg.EncryptionAlgorithmType(), } @@ -400,3 +421,50 @@ func certToConfig(cert *x509.Certificate) certutil.Config { Usages: cert.ExtKeyUsage, } } + +func loadCertConfigMutators(certBaseName string) []certConfigMutatorFunc { + // TODO: Remove these mutators after the organization migration is complete in a future release + // https://github.com/kubernetes/kubeadm/issues/2414 + switch certBaseName { + case kubeadmconstants.EtcdHealthcheckClientCertAndKeyBaseName, + kubeadmconstants.APIServerEtcdClientCertAndKeyBaseName: + return []certConfigMutatorFunc{ + removeSystemPrivilegedGroupMutator(), + } + case kubeadmconstants.APIServerKubeletClientCertAndKeyBaseName: + return []certConfigMutatorFunc{ + removeSystemPrivilegedGroupMutator(), + addClusterAdminsGroupMutator(), + } + } + return nil +} + +func removeSystemPrivilegedGroupMutator() certConfigMutatorFunc { + return func(c *certutil.Config) error { + organizations := make([]string, 0, len(c.Organization)) + for _, org := range c.Organization { + if org != kubeadmconstants.SystemPrivilegedGroup { + organizations = append(organizations, org) + } + } + c.Organization = organizations + return nil + } +} + +func addClusterAdminsGroupMutator() certConfigMutatorFunc { + return func(c *certutil.Config) error { + found := false + for _, org := range c.Organization { + if org == kubeadmconstants.ClusterAdminsGroupAndClusterRoleBinding { + found = true + break + } + } + if !found { + c.Organization = append(c.Organization, kubeadmconstants.ClusterAdminsGroupAndClusterRoleBinding) + } + return nil + } +} diff --git a/cmd/kubeadm/app/phases/certs/renewal/manager_test.go b/cmd/kubeadm/app/phases/certs/renewal/manager_test.go index 9163d2d1517..b06df5fe7cf 100644 --- a/cmd/kubeadm/app/phases/certs/renewal/manager_test.go +++ b/cmd/kubeadm/app/phases/certs/renewal/manager_test.go @@ -30,6 +30,7 @@ import ( netutils "k8s.io/utils/net" kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm" + kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants" certtestutil "k8s.io/kubernetes/cmd/kubeadm/app/util/certs" "k8s.io/kubernetes/cmd/kubeadm/app/util/pkiutil" testutil "k8s.io/kubernetes/cmd/kubeadm/test" @@ -42,17 +43,9 @@ var ( testCACert, testCAKey, _ = pkiutil.NewCertificateAuthority(testCACertCfg) - testCertCfg = &pkiutil.CertConfig{ - Config: certutil.Config{ - CommonName: "test-common-name", - Organization: []string{"sig-cluster-lifecycle"}, - AltNames: certutil.AltNames{ - IPs: []net.IP{netutils.ParseIPSloppy("10.100.0.1")}, - DNSNames: []string{"test-domain.space"}, - }, - Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, - }, - } + testCertOrganization = []string{"sig-cluster-lifecycle"} + + testCertCfg = makeTestCertConfig(testCertOrganization) ) func TestNewManager(t *testing.T) { @@ -99,6 +92,11 @@ func TestRenewUsingLocalCA(t *testing.T) { t.Fatalf("couldn't write out CA certificate to %s", dir) } + etcdDir := filepath.Join(dir, "etcd") + if err := pkiutil.WriteCertAndKey(etcdDir, "ca", testCACert, testCAKey); err != nil { + t.Fatalf("couldn't write out CA certificate to %s", etcdDir) + } + cfg := &kubeadmapi.ClusterConfiguration{ CertificatesDir: dir, } @@ -108,16 +106,18 @@ func TestRenewUsingLocalCA(t *testing.T) { } tests := []struct { - name string - certName string - createCertFunc func() *x509.Certificate + name string + certName string + createCertFunc func() *x509.Certificate + expectedOrganization []string }{ { name: "Certificate renewal for a PKI certificate", certName: "apiserver", createCertFunc: func() *x509.Certificate { - return writeTestCertificate(t, dir, "apiserver", testCACert, testCAKey) + return writeTestCertificate(t, dir, "apiserver", testCACert, testCAKey, testCertOrganization) }, + expectedOrganization: testCertOrganization, }, { name: "Certificate renewal for a certificate embedded in a kubeconfig file", @@ -125,6 +125,23 @@ func TestRenewUsingLocalCA(t *testing.T) { createCertFunc: func() *x509.Certificate { return writeTestKubeconfig(t, dir, "admin.conf", testCACert, testCAKey) }, + expectedOrganization: testCertOrganization, + }, + { + name: "apiserver-etcd-client cert should not contain SystemPrivilegedGroup after renewal", + certName: "apiserver-etcd-client", + createCertFunc: func() *x509.Certificate { + return writeTestCertificate(t, dir, "apiserver-etcd-client", testCACert, testCAKey, []string{kubeadmconstants.SystemPrivilegedGroup}) + }, + expectedOrganization: []string{}, + }, + { + name: "apiserver-kubelet-client cert should replace SystemPrivilegedGroup with ClusterAdminsGroup after renewal", + certName: "apiserver-kubelet-client", + createCertFunc: func() *x509.Certificate { + return writeTestCertificate(t, dir, "apiserver-kubelet-client", testCACert, testCAKey, []string{kubeadmconstants.SystemPrivilegedGroup}) + }, + expectedOrganization: []string{kubeadmconstants.ClusterAdminsGroupAndClusterRoleBinding}, }, } @@ -154,7 +171,7 @@ func TestRenewUsingLocalCA(t *testing.T) { certtestutil.AssertCertificateIsSignedByCa(t, newCert, testCACert) certtestutil.AssertCertificateHasClientAuthUsage(t, newCert) - certtestutil.AssertCertificateHasOrganizations(t, newCert, testCertCfg.Organization...) + certtestutil.AssertCertificateHasOrganizations(t, newCert, test.expectedOrganization...) certtestutil.AssertCertificateHasCommonName(t, newCert, testCertCfg.CommonName) certtestutil.AssertCertificateHasDNSNames(t, newCert, testCertCfg.AltNames.DNSNames...) certtestutil.AssertCertificateHasIPAddresses(t, newCert, testCertCfg.AltNames.IPs...) @@ -193,7 +210,7 @@ func TestCreateRenewCSR(t *testing.T) { name: "Creation of a CSR request for renewal of a PKI certificate", certName: "apiserver", createCertFunc: func() *x509.Certificate { - return writeTestCertificate(t, dir, "apiserver", testCACert, testCAKey) + return writeTestCertificate(t, dir, "apiserver", testCACert, testCAKey, testCertOrganization) }, }, { @@ -233,7 +250,7 @@ func TestCreateRenewCSR(t *testing.T) { func TestCertToConfig(t *testing.T) { expectedConfig := &certutil.Config{ CommonName: "test-common-name", - Organization: []string{"sig-cluster-lifecycle"}, + Organization: testCertOrganization, AltNames: certutil.AltNames{ IPs: []net.IP{netutils.ParseIPSloppy("10.100.0.1")}, DNSNames: []string{"test-domain.space"}, @@ -244,7 +261,7 @@ func TestCertToConfig(t *testing.T) { cert := &x509.Certificate{ Subject: pkix.Name{ CommonName: "test-common-name", - Organization: []string{"sig-cluster-lifecycle"}, + Organization: testCertOrganization, }, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, DNSNames: []string{"test-domain.space"}, @@ -274,3 +291,17 @@ func TestCertToConfig(t *testing.T) { t.Errorf("expected SAN DNSNames %v, got %v", expectedConfig.AltNames.DNSNames, cfg.AltNames.DNSNames) } } + +func makeTestCertConfig(organization []string) *pkiutil.CertConfig { + return &pkiutil.CertConfig{ + Config: certutil.Config{ + CommonName: "test-common-name", + Organization: organization, + AltNames: certutil.AltNames{ + IPs: []net.IP{netutils.ParseIPSloppy("10.100.0.1")}, + DNSNames: []string{"test-domain.space"}, + }, + Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + }, + } +} diff --git a/cmd/kubeadm/app/phases/certs/renewal/readwriter_test.go b/cmd/kubeadm/app/phases/certs/renewal/readwriter_test.go index 9627531e9d8..da851952553 100644 --- a/cmd/kubeadm/app/phases/certs/renewal/readwriter_test.go +++ b/cmd/kubeadm/app/phases/certs/renewal/readwriter_test.go @@ -41,7 +41,7 @@ func TestPKICertificateReadWriter(t *testing.T) { defer os.RemoveAll(dir) // creates a certificate - cert := writeTestCertificate(t, dir, "test", testCACert, testCAKey) + cert := writeTestCertificate(t, dir, "test", testCACert, testCAKey, testCertOrganization) // Creates a pkiCertificateReadWriter pkiReadWriter := newPKICertificateReadWriter(dir, "test") @@ -145,8 +145,8 @@ func TestKubeconfigReadWriter(t *testing.T) { } // writeTestCertificate is a utility for creating a test certificate -func writeTestCertificate(t *testing.T, dir, name string, caCert *x509.Certificate, caKey crypto.Signer) *x509.Certificate { - cert, key, err := pkiutil.NewCertAndKey(caCert, caKey, testCertCfg) +func writeTestCertificate(t *testing.T, dir, name string, caCert *x509.Certificate, caKey crypto.Signer, organization []string) *x509.Certificate { + cert, key, err := pkiutil.NewCertAndKey(caCert, caKey, makeTestCertConfig(organization)) if err != nil { t.Fatalf("couldn't generate certificate: %v", err) } @@ -164,7 +164,7 @@ func writeTestKubeconfig(t *testing.T, dir, name string, caCert *x509.Certificat cfg := &pkiutil.CertConfig{ Config: certutil.Config{ CommonName: "test-common-name", - Organization: []string{"sig-cluster-lifecycle"}, + Organization: testCertOrganization, Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, AltNames: certutil.AltNames{ IPs: []net.IP{netutils.ParseIPSloppy("10.100.0.1")}, diff --git a/cmd/kubeadm/app/phases/kubeconfig/kubeconfig_test.go b/cmd/kubeadm/app/phases/kubeconfig/kubeconfig_test.go index 8b0d49f8e98..cb6bfd928d3 100644 --- a/cmd/kubeadm/app/phases/kubeconfig/kubeconfig_test.go +++ b/cmd/kubeadm/app/phases/kubeconfig/kubeconfig_test.go @@ -480,7 +480,7 @@ func TestWriteKubeConfig(t *testing.T) { if test.withClientCert { // checks that kubeconfig files have expected client cert - kubeconfigtestutil.AssertKubeConfigCurrentAuthInfoWithClientCert(t, config, caCert, "myUser") + kubeconfigtestutil.AssertKubeConfigCurrentAuthInfoWithClientCert(t, config, caCert, "myUser", "myOrg") } if test.withToken { diff --git a/cmd/kubeadm/app/util/certs/util.go b/cmd/kubeadm/app/util/certs/util.go index 531edec4c7d..8d6fff4ffc7 100644 --- a/cmd/kubeadm/app/util/certs/util.go +++ b/cmd/kubeadm/app/util/certs/util.go @@ -60,8 +60,11 @@ func AssertCertificateHasCommonName(t *testing.T, cert *x509.Certificate, common } // AssertCertificateHasOrganizations is a utility function for kubeadm testing that asserts if a given certificate has -// the expected Subject.Organization +// and only has the expected Subject.Organization func AssertCertificateHasOrganizations(t *testing.T, cert *x509.Certificate, organizations ...string) { + if len(cert.Subject.Organization) != len(organizations) { + t.Fatalf("cert contains a different number of Subject.Organization, expected %v, got %v", organizations, cert.Subject.Organization) + } for _, organization := range organizations { found := false for i := range cert.Subject.Organization {