kuma-cp: validate certificates that users want to employ as a "provid…

…ed" CA

app/kumactl/cmd/manage/ca/provided_test.go

@@ -2,21 +2,26 @@ package ca_test
 
 import (
 	"bytes"
-	"github.com/Kong/kuma/app/kumactl/pkg/ca"
 	"io/ioutil"
 	"path/filepath"
 
+	"github.com/Kong/kuma/app/kumactl/pkg/ca"
+
 	"github.com/Kong/kuma/app/kumactl/cmd"
 	kumactl_cmd "github.com/Kong/kuma/app/kumactl/pkg/cmd"
 	"github.com/Kong/kuma/pkg/catalog"
 	catalog_client "github.com/Kong/kuma/pkg/catalog/client"
 	kumactl_config "github.com/Kong/kuma/pkg/config/app/kumactl/v1alpha1"
 	"github.com/Kong/kuma/pkg/core/ca/provided/rest/types"
+	error_types "github.com/Kong/kuma/pkg/core/rest/errors/types"
 	test_catalog "github.com/Kong/kuma/pkg/test/catalog"
 	"github.com/Kong/kuma/pkg/tls"
 	"github.com/spf13/cobra"
 
+	"github.com/ghodss/yaml"
+
 	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 )
 
@@ -25,6 +30,7 @@ var _ ca.ProvidedCaClient = &staticProvidedCaClient{}
 type staticProvidedCaClient struct {
 	addMesh string
 	addPair tls.KeyPair
+	addErr  error
 
 	deleteCertMesh string
 	deleteCertId   string
@@ -38,6 +44,9 @@ type staticProvidedCaClient struct {
 func (s *staticProvidedCaClient) AddSigningCertificate(mesh string, pair tls.KeyPair) (types.SigningCert, error) {
 	s.addMesh = mesh
 	s.addPair = pair
+	if s.addErr != nil {
+		return types.SigningCert{}, s.addErr
+	}
 	return types.SigningCert{
 		Id: "id-13456",
 	}, nil
@@ -92,7 +101,7 @@ var _ = Describe("kumactl manage provided ca", func() {
 		rootCmd.SetOut(buf)
 	})
 
-	It("should add certificate", func() {
+	It("should add proper CA certificate", func() {
 		// setup
 		certBytes, err := ioutil.ReadFile(filepath.Join("testdata", "cert.pem"))
 		Expect(err).ToNot(HaveOccurred())
@@ -122,6 +131,71 @@ var _ = Describe("kumactl manage provided ca", func() {
 		Expect(buf.String()).To(Equal(`added certificate "id-13456"`))
 	})
 
+	Describe("should not add improper CA certificate", func() {
+
+		type testCase struct {
+			addErr      string
+			expectedOut string
+		}
+
+		DescribeTable("should reject invalid cert",
+			func(given testCase) {
+				// setup
+
+				addErr := error_types.Error{}
+				// when
+				err := yaml.Unmarshal([]byte(given.addErr), &addErr)
+				// then
+				Expect(err).ToNot(HaveOccurred())
+				// and
+				client.addErr = &addErr
+
+				// given
+				rootCmd.SetArgs([]string{
+					"manage", "ca", "provided", "certificates", "add",
+					"--mesh", "demo",
+					"--key-file", filepath.Join("testdata", "cert.key"),
+					"--cert-file", filepath.Join("testdata", "cert.pem"),
+				})
+
+				// when
+				err = rootCmd.Execute()
+				// then
+				Expect(err).To(HaveOccurred())
+
+				// and
+				Expect(buf.String()).To(Equal(given.expectedOut))
+			},
+			Entry("1 violation", testCase{
+				addErr: `
+                title: 'Could not add signing cert'
+                details: 'Resource is not valid'
+                causes:
+                - field: cert
+                  message: "key usage extension 'keyCertSign' must be set (see X509-SVID: 4.3. Key Usage)"
+`,
+				expectedOut: `Error: Could not add signing cert (Resource is not valid)
+* cert: key usage extension 'keyCertSign' must be set (see X509-SVID: 4.3. Key Usage)
+`,
+			}),
+			Entry("N violations", testCase{
+				addErr: `
+                title: 'Could not add signing cert'
+                details: 'Resource is not valid'
+                causes:
+                - field: cert
+                  message: "basic constraint 'CA' must be set to 'true' (see X509-SVID: 4.1. Basic Constraints)"
+                - field: cert
+                  message: "key usage extension 'keyCertSign' must be set (see X509-SVID: 4.3. Key Usage)"
+`,
+				expectedOut: `Error: Could not add signing cert (Resource is not valid)
+* cert: basic constraint 'CA' must be set to 'true' (see X509-SVID: 4.1. Basic Constraints)
+* cert: key usage extension 'keyCertSign' must be set (see X509-SVID: 4.3. Key Usage)
+`,
+			}),
+		)
+	})
+
 	It("should delete certificate", func() {
 		// given
 		rootCmd.SetArgs([]string{

pkg/core/ca/builtin/issuer/issuer.go

@@ -1,14 +1,12 @@
 package issuer
 
 import (
-	"bytes"
 	"crypto"
 	"crypto/rand"
 	"crypto/rsa"
 	"crypto/tls"
 	"crypto/x509"
 	"crypto/x509/pkix"
-	"encoding/pem"
 	"math/big"
 	"net/url"
 	"time"
@@ -35,7 +33,7 @@ func NewRootCA(mesh string) (*util_tls.KeyPair, error) {
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to generate X509 certificate")
 	}
-	return keyPair(key, cert)
+	return util_tls.ToKeyPair(key, cert)
 }
 
 func newCACert(signer crypto.Signer, trustDomain string) ([]byte, error) {
@@ -73,7 +71,7 @@ func NewWorkloadCert(ca util_tls.KeyPair, mesh string, workload string) (*util_t
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to generate X509 certificate")
 	}
-	return keyPair(workloadKey, workloadCert)
+	return util_tls.ToKeyPair(workloadKey, workloadCert)
 }
 
 func newWorkloadCert(signer crypto.PrivateKey, parent *x509.Certificate, trustDomain string, workload string, publicKey crypto.PublicKey) ([]byte, error) {
@@ -111,41 +109,3 @@ func loadKeyPair(pair util_tls.KeyPair) (crypto.PrivateKey, *x509.Certificate, e
 	}
 	return root.PrivateKey, rootCert, nil
 }
-
-func keyPair(key interface{}, cert []byte) (*util_tls.KeyPair, error) {
-	keyPem, err := pemEncodeKey(key)
-	if err != nil {
-		return nil, errors.Wrap(err, "failed to PEM encode a private key")
-	}
-	certPem, err := pemEncodeCert(cert)
-	if err != nil {
-		return nil, errors.Wrap(err, "failed to PEM encode a certificate")
-	}
-	return &util_tls.KeyPair{
-		CertPEM: certPem,
-		KeyPEM:  keyPem,
-	}, nil
-}
-
-func pemEncodeKey(priv interface{}) ([]byte, error) {
-	var block *pem.Block
-	switch k := priv.(type) {
-	case *rsa.PrivateKey:
-		block = &pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(k)}
-	default:
-		return nil, errors.Errorf("unsupported private key type %T", priv)
-	}
-	var keyBuf bytes.Buffer
-	if err := pem.Encode(&keyBuf, block); err != nil {
-		return nil, err
-	}
-	return keyBuf.Bytes(), nil
-}
-
-func pemEncodeCert(derBytes []byte) ([]byte, error) {
-	var certBuf bytes.Buffer
-	if err := pem.Encode(&certBuf, &pem.Block{Type: "CERTIFICATE", Bytes: derBytes}); err != nil {
-		return nil, err
-	}
-	return certBuf.Bytes(), nil
-}

pkg/core/ca/provided/ca_cert_validator.go

@@ -0,0 +1,53 @@
+package provided
+
+import (
+	"crypto/tls"
+	"crypto/x509"
+	"fmt"
+
+	"github.com/Kong/kuma/pkg/core/validators"
+
+	util_tls "github.com/Kong/kuma/pkg/tls"
+)
+
+func ValidateCaCert(signingPair util_tls.KeyPair) error {
+	err := validateCaCert(signingPair)
+	return err.OrNil()
+}
+
+func validateCaCert(signingPair util_tls.KeyPair) (verr validators.ValidationError) {
+	tlsKeyPair, err := tls.X509KeyPair(signingPair.CertPEM, signingPair.KeyPEM)
+	if err != nil {
+		verr.AddViolation(".", fmt.Sprintf("not a valid TLS key pair: %s", err))
+		return
+	}
+	if len(tlsKeyPair.Certificate) != 1 {
+		verr.AddViolation("cert", "certificate must be a root CA (certificate chains are not allowed)") // Envoy constraint
+		return
+	}
+	cert, err := x509.ParseCertificate(tlsKeyPair.Certificate[0])
+	if err != nil {
+		verr.AddViolation("cert", fmt.Sprintf("not a valid x509 certificate: %s", err))
+		return
+	}
+	if cert.Issuer.String() != cert.Subject.String() {
+		verr.AddViolation("cert", "certificate must be self-signed (intermediate CAs are not allowed)") // Envoy constraint
+	}
+	if !cert.IsCA {
+		verr.AddViolation("cert", "basic constraint 'CA' must be set to 'true' (see X509-SVID: 4.1. Basic Constraints)")
+	}
+	if cert.KeyUsage&x509.KeyUsageCertSign == 0 {
+		verr.AddViolation("cert", "key usage extension 'keyCertSign' must be set (see X509-SVID: 4.3. Key Usage)")
+	}
+	if cert.KeyUsage&x509.KeyUsageDigitalSignature != 0 {
+		verr.AddViolation("cert", "key usage extension 'digitalSignature' must NOT be set (see X509-SVID: Appendix A. X.509 Field Reference)")
+	}
+	if cert.KeyUsage&x509.KeyUsageKeyAgreement != 0 {
+		verr.AddViolation("cert", "key usage extension 'keyAgreement' must NOT be set (see X509-SVID: Appendix A. X.509 Field Reference)")
+	}
+	if cert.KeyUsage&x509.KeyUsageKeyEncipherment != 0 {
+		verr.AddViolation("cert", "key usage extension 'keyEncipherment' must NOT be set (see X509-SVID: Appendix A. X.509 Field Reference)")
+	}
+
+	return
+}