kubeadm: support updating certificate organization during 'kubeadm certs renew'

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,
+				Name:               cert.Name,
-				LongName:   cert.LongName,
+				LongName:           cert.LongName,
-				FileName:   cert.BaseName,
+				FileName:           cert.BaseName,
-				CAName:     ca.Name,
+				CAName:             ca.Name,
-				CABaseName: ca.BaseName, //Nb. this is a path for etcd certs (they are stored in a subfolder)
+				CABaseName:         ca.BaseName, // Nb. this is a path for etcd certs (they are stored in a subfolder)
-				readwriter: pkiReadWriter,
+				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
+	}
+}

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{
+	testCertOrganization = []string{"sig-cluster-lifecycle"}
-		Config: certutil.Config{
+
-			CommonName:   "test-common-name",
+	testCertCfg = makeTestCertConfig(testCertOrganization)
-			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},
-		},
-	}
 )
 
 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
+		name                 string
-		certName       string
+		certName             string
-		createCertFunc func() *x509.Certificate
+		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},
+		},
+	}
+}

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 {
+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, testCertCfg)
+	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")},

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 {

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 {