Fix the bug that cert-chain.pem file does not have intermediate certs (…

…istio#4640)

Automatic merge from submit-queue.

Fix the bug that cert-chain.pem file does not have intermediate certs

security/pkg/pki/ca/controller/secret.go

@@ -45,12 +45,12 @@ const (
 	PrivateKeyID = "key.pem"
 	// The ID/name for the CA root certificate file.
 	RootCertID = "root-cert.pem"
+	// The key to specify corresponding service account in the annotation of K8s secrets.
+	ServiceAccountNameAnnotationKey = "istio.io/service-account.name"
 
 	secretNamePrefix   = "istio."
 	secretResyncPeriod = time.Minute
 
-	serviceAccountNameAnnotationKey = "istio.io/service-account.name"
-
 	recommendedMinGracePeriodRatio = 0.2
 	recommendedMaxGracePeriodRatio = 0.8
 
@@ -150,6 +150,11 @@ func (sc *SecretController) Run(stopCh chan struct{}) {
 	go sc.saController.Run(stopCh)
 }
 
+// GetSecretName returns the secret name for a given service account name.
+func GetSecretName(saName string) string {
+	return secretNamePrefix + saName
+}
+
 // Handles the event where a service account is added.
 func (sc *SecretController) saAdded(obj interface{}) {
 	acct := obj.(*v1.ServiceAccount)
@@ -189,8 +194,8 @@ func (sc *SecretController) saUpdated(oldObj, curObj interface{}) {
 func (sc *SecretController) upsertSecret(saName, saNamespace string) {
 	secret := &v1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Annotations: map[string]string{serviceAccountNameAnnotationKey: saName},
-			Name:        getSecretName(saName),
+			Annotations: map[string]string{ServiceAccountNameAnnotationKey: saName},
+			Name:        GetSecretName(saName),
 			Namespace:   saNamespace,
 		},
 		Type: IstioSecretType,
@@ -242,7 +247,7 @@ func (sc *SecretController) upsertSecret(saName, saNamespace string) {
 }
 
 func (sc *SecretController) deleteSecret(saName, saNamespace string) {
-	err := sc.core.Secrets(saNamespace).Delete(getSecretName(saName), nil)
+	err := sc.core.Secrets(saNamespace).Delete(GetSecretName(saName), nil)
 	// kube-apiserver returns NotFound error when the secret is successfully deleted.
 	if err == nil || errors.IsNotFound(err) {
 		log.Infof("Istio secret for service account \"%s\" in namespace \"%s\" has been deleted", saName, saNamespace)
@@ -260,7 +265,7 @@ func (sc *SecretController) scrtDeleted(obj interface{}) {
 		return
 	}
 
-	saName := scrt.Annotations[serviceAccountNameAnnotationKey]
+	saName := scrt.Annotations[ServiceAccountNameAnnotationKey]
 	if sa, _ := sc.core.ServiceAccounts(scrt.GetNamespace()).Get(saName, metav1.GetOptions{}); sa != nil {
 		log.Errorf("Re-create deleted Istio secret for existing service account.")
 		sc.upsertSecret(saName, scrt.GetNamespace())
@@ -286,10 +291,12 @@ func (sc *SecretController) generateKeyAndCert(saName string, saNamespace string
 		return nil, nil, err
 	}
 
+	_, _, certChainPEM, _ := sc.ca.GetCAKeyCertBundle().GetAll()
 	certPEM, err := sc.ca.Sign(csrPEM, sc.certTTL, false)
 	if err != nil {
 		return nil, nil, err
 	}
+	certPEM = append(certPEM, certChainPEM...)
 
 	return certPEM, keyPEM, nil
 }
@@ -333,7 +340,7 @@ func (sc *SecretController) scrtUpdated(oldObj, newObj interface{}) {
 		log.Infof("Refreshing secret %s/%s, either the leaf certificate is about to expire "+
 			"or the root certificate is outdated", namespace, name)
 
-		saName := scrt.Annotations[serviceAccountNameAnnotationKey]
+		saName := scrt.Annotations[ServiceAccountNameAnnotationKey]
 
 		chain, key, err := sc.generateKeyAndCert(saName, namespace)
 		if err != nil {
@@ -352,7 +359,3 @@ func (sc *SecretController) scrtUpdated(oldObj, newObj interface{}) {
 		}
 	}
 }
-
-func getSecretName(saName string) string {
-	return secretNamePrefix + saName
-}

security/pkg/pki/ca/controller/secret_test.go

@@ -15,6 +15,7 @@
 package controller
 
 import (
+	"bytes"
 	"fmt"
 	"testing"
 	"time"
@@ -40,6 +41,14 @@ const (
 	sidecarInjectorSvc        = "istio-sidecar-injector"
 )
 
+var (
+	caCert     = []byte("fake CA cert")
+	caKey      = []byte("fake private key")
+	certChain  = []byte("fake cert chain")
+	rootCert   = []byte("fake root cert")
+	signedCert = []byte("fake signed cert")
+)
+
 func TestSecretController(t *testing.T) {
 	gvr := schema.GroupVersionResource{
 		Resource: "secrets",
@@ -154,6 +163,36 @@ func TestSecretController(t *testing.T) {
 	}
 }
 
+func TestSecretContent(t *testing.T) {
+	saName := "test-serviceaccount"
+	saNamespace := "test-namespace"
+	client := fake.NewSimpleClientset()
+	controller, err := NewSecretController(createFakeCA(), defaultTTL, defaultGracePeriodRatio, defaultMinGracePeriod,
+		client.CoreV1(), metav1.NamespaceAll, map[string]DNSNameEntry{})
+	if err != nil {
+		t.Errorf("Failed to create secret controller: %v", err)
+	}
+	controller.saAdded(createServiceAccount(saName, saNamespace))
+
+	secret := &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{ServiceAccountNameAnnotationKey: saName},
+			Name:        GetSecretName(saName),
+			Namespace:   saNamespace,
+		},
+		Type: IstioSecretType,
+	}
+	secret, err = client.CoreV1().Secrets(saNamespace).Get(GetSecretName(saName), metav1.GetOptions{})
+	if err != nil {
+		t.Errorf("Failed to retrieve secret: %v", err)
+	}
+	if !bytes.Equal(rootCert, secret.Data[RootCertID]) {
+		t.Errorf("Root cert verification error: expected %v but got %v", rootCert, secret.Data[RootCertID])
+	}
+	if !bytes.Equal(append(signedCert, certChain...), secret.Data[CertChainID]) {
+		t.Errorf("Cert chain verification error: expected %v but got %v", certChain, secret.Data[CertChainID])
+	}
+}
 func TestDeletedIstioSecret(t *testing.T) {
 	client := fake.NewSimpleClientset()
 	controller, err := NewSecretController(createFakeCA(), defaultTTL, defaultGracePeriodRatio, defaultMinGracePeriod,
@@ -313,13 +352,13 @@ func checkActions(actual, expected []ktesting.Action) error {
 
 func createFakeCA() *mockca.FakeCA {
 	return &mockca.FakeCA{
-		SignedCert: []byte("fake signed cert"),
+		SignedCert: signedCert,
 		SignErr:    nil,
 		KeyCertBundle: &mockutil.FakeKeyCertBundle{
-			CertBytes:      []byte("fake CA cert"),
-			PrivKeyBytes:   []byte("fake private key"),
-			CertChainBytes: []byte("fake cert chain"),
-			RootCertBytes:  []byte("fake root cert"),
+			CertBytes:      caCert,
+			PrivKeyBytes:   caKey,
+			CertChainBytes: certChain,
+			RootCertBytes:  rootCert,
 		},
 	}
 }