Validate Citadel certificate can be used for a CA (istio#12449)

* change tests to use canonical string comparison (i.e., ==, !=) instead of testing len(str)

* validate Citadel's root cert is CA capable

security/pkg/pki/ca/ca.go

@@ -16,9 +16,11 @@ package ca
 
 import (
 	"context"
+	"crypto/x509"
 	"encoding/base64"
 	"encoding/pem"
 	"fmt"
+	"io/ioutil"
 	"time"
 
 	v1 "k8s.io/api/core/v1"
@@ -179,6 +181,26 @@ func NewPluggedCertIstioCAOptions(certChainFile, signingCertFile, signingKeyFile
 		signingCertFile, signingKeyFile, certChainFile, rootCertFile); err != nil {
 		return nil, fmt.Errorf("failed to create CA KeyCertBundle (%v)", err)
 	}
+
+	// Validate that the passed in signing cert can be used as CA.
+	// The check can't be done inside `KeyCertBundle`, since bundle could also be used to
+	// validate workload certificates (i.e., where the leaf certificate is not a CA).
+	b, err := ioutil.ReadFile(signingCertFile)
+	if err != nil {
+		return nil, err
+	}
+	block, _ := pem.Decode(b)
+	if block == nil {
+		return nil, fmt.Errorf("invalid PEM encoded certificate")
+	}
+	cert, err := x509.ParseCertificate(block.Bytes)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse X.509 certificate")
+	}
+	if !cert.IsCA {
+		return nil, fmt.Errorf("certificate is not authorized to sign other certificates")
+	}
+
 	if err = updateCertInConfigmap(namespace, client, caOpts.KeyCertBundle); err != nil {
 		log.Errorf("Failed to write Citadel cert to configmap (%v). Node agents will not be able to connect.", err)
 	}

security/pkg/pki/util/keycertbundle_test.go

@@ -57,12 +57,12 @@ func TestKeyCertBundleWithRootCertFromFile(t *testing.T) {
 	for id, tc := range testCases {
 		bundle, err := NewKeyCertBundleWithRootCertFromFile(tc.rootCertFile)
 		if err != nil {
-			if len(tc.expectedErr) == 0 {
+			if tc.expectedErr == "" {
 				t.Errorf("%s: Unexpected error: %v", id, err)
 			} else if strings.Compare(err.Error(), tc.expectedErr) != 0 {
 				t.Errorf("%s: Unexpected error: %v VS (expected) %s", id, err, tc.expectedErr)
 			}
-		} else if len(tc.expectedErr) != 0 {
+		} else if tc.expectedErr != "" {
 			t.Errorf("%s: Expected error %s but succeeded", id, tc.expectedErr)
 		} else if bundle == nil {
 			t.Errorf("%s: the bundle should not be empty", id)
@@ -154,12 +154,12 @@ func TestCertOptionsAndRetrieveID(t *testing.T) {
 		}
 		opts, err := k.CertOptions()
 		if err != nil {
-			if len(tc.expectedErr) == 0 {
+			if tc.expectedErr == "" {
 				t.Errorf("%s: Unexpected error: %v", id, err)
 			} else if strings.Compare(err.Error(), tc.expectedErr) != 0 {
 				t.Errorf("%s: Unexpected error: %v VS (expected) %s", id, err, tc.expectedErr)
 			}
-		} else if len(tc.expectedErr) != 0 {
+		} else if tc.expectedErr != "" {
 			t.Errorf("%s: expected error %s but have error %v", id, tc.expectedErr, err)
 		} else {
 			compareCertOptions(opts, tc.certOptions, t)
@@ -290,12 +290,12 @@ func TestNewVerifiedKeyCertBundleFromFile(t *testing.T) {
 		_, err := NewVerifiedKeyCertBundleFromFile(
 			tc.caCertFile, tc.caKeyFile, tc.certChainFile, tc.rootCertFile)
 		if err != nil {
-			if len(tc.expectedErr) == 0 {
+			if tc.expectedErr == "" {
 				t.Errorf("%s: Unexpected error: %v", id, err)
 			} else if strings.Compare(err.Error(), tc.expectedErr) != 0 {
 				t.Errorf("%s: Unexpected error: %v VS (expected) %s", id, err, tc.expectedErr)
 			}
-		} else if len(tc.expectedErr) != 0 {
+		} else if tc.expectedErr != "" {
 			t.Errorf("%s: Expected error %s but succeeded", id, tc.expectedErr)
 		}
 	}