From 207692c75602d05aded5a94d1aac439e6bb178c7 Mon Sep 17 00:00:00 2001
From: Markus Rudy <mr@edgeless.systems>
Date: Fri, 28 Jun 2024 18:31:31 +0200
Subject: [PATCH] ca: verify certificates strictly

(cherry picked from commit 1d10e91072d9628b27d7b4e5d509b8d7990fb29c)
---
 e2e/openssl/openssl_test.go |  4 +--
 internal/ca/ca.go           | 53 ++++++++++++++++++-------------------
 internal/ca/ca_test.go      | 41 ++++++++++++++++++++++++----
 3 files changed, 64 insertions(+), 34 deletions(-)

diff --git a/e2e/openssl/openssl_test.go b/e2e/openssl/openssl_test.go
index 1721cdeb57..7fff46ab81 100644
--- a/e2e/openssl/openssl_test.go
+++ b/e2e/openssl/openssl_test.go
@@ -152,7 +152,7 @@ func TestOpenSSL(t *testing.T) {
 			stdout, stderr, err := c.ExecDeployment(ctx, ct.Namespace, opensslFrontend, []string{"/bin/bash", "-c", opensslConnectCmd("openssl-backend:443", "mesh-ca.pem")})
 			t.Log("openssl with wrong certificates:", stdout)
 			require.Error(err)
-			require.Contains(stderr, "certificate signature failure")
+			require.Contains(stderr, "self-signed certificate in certificate chain")
 
 			// Connect from backend to fronted, because the frontend does not require client certs.
 			// This should succeed because the root cert did not change.
@@ -186,6 +186,6 @@ func TestMain(m *testing.M) {
 
 func opensslConnectCmd(addr, caCert string) string {
 	return fmt.Sprintf(
-		`openssl s_client -connect %s -verify_return_error -CAfile /tls-config/%s -cert /tls-config/certChain.pem -key /tls-config/key.pem </dev/null`,
+		`openssl s_client -connect %s -verify_return_error -x509_strict -CAfile /tls-config/%s -cert /tls-config/certChain.pem -key /tls-config/key.pem </dev/null`,
 		addr, caCert)
 }
diff --git a/internal/ca/ca.go b/internal/ca/ca.go
index b34d0be2b9..4ff91fc6fc 100644
--- a/internal/ca/ca.go
+++ b/internal/ca/ca.go
@@ -4,7 +4,6 @@
 package ca
 
 import (
-	"bytes"
 	"crypto/ecdsa"
 	"crypto/rand"
 	"crypto/x509"
@@ -29,13 +28,11 @@ import (
 // https://docs.edgeless.systems/marblerun/architecture/security#public-key-infrastructure-and-certificate-authority
 type CA struct {
 	rootCAPrivKey *ecdsa.PrivateKey
-	rootCACert    *x509.Certificate
 	rootCAPEM     []byte
 
 	intermPrivKey *ecdsa.PrivateKey
 
-	intermCACert *x509.Certificate
-	intermCAPEM  []byte
+	intermCAPEM []byte
 
 	meshCACert *x509.Certificate
 	meshCAPEM  []byte
@@ -47,7 +44,7 @@ func New(rootPrivKey, intermPrivKey *ecdsa.PrivateKey) (*CA, error) {
 	notBefore := now.Add(-time.Hour)
 	notAfter := now.AddDate(10, 0, 0)
 
-	root := &x509.Certificate{
+	rootTemplate := &x509.Certificate{
 		Subject:               pkix.Name{CommonName: "system:coordinator:root"},
 		NotBefore:             notBefore,
 		NotAfter:              notAfter,
@@ -55,13 +52,13 @@ func New(rootPrivKey, intermPrivKey *ecdsa.PrivateKey) (*CA, error) {
 		KeyUsage:              x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
 		BasicConstraintsValid: true,
 	}
-	rootPEM, err := createCert(root, root, &rootPrivKey.PublicKey, rootPrivKey)
+	rootCert, rootPEM, err := createCert(rootTemplate, rootTemplate, &rootPrivKey.PublicKey, rootPrivKey)
 	if err != nil {
 		return nil, fmt.Errorf("creating root certificate: %w", err)
 	}
 
 	notAfter = now.AddDate(10, 0, 0)
-	intermCACert := &x509.Certificate{
+	intermCACertTemplate := &x509.Certificate{
 		Subject:               pkix.Name{CommonName: "system:coordinator:intermediate"},
 		NotBefore:             notBefore,
 		NotAfter:              notAfter,
@@ -69,12 +66,12 @@ func New(rootPrivKey, intermPrivKey *ecdsa.PrivateKey) (*CA, error) {
 		KeyUsage:              x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
 		BasicConstraintsValid: true,
 	}
-	intermCAPEM, err := createCert(intermCACert, root, &intermPrivKey.PublicKey, rootPrivKey)
+	_, intermCAPEM, err := createCert(intermCACertTemplate, rootCert, &intermPrivKey.PublicKey, rootPrivKey)
 	if err != nil {
 		return nil, fmt.Errorf("creating intermediate certificate: %w", err)
 	}
 
-	meshCACert := &x509.Certificate{
+	meshCACertTemplate := &x509.Certificate{
 		Subject:               pkix.Name{CommonName: "system:coordinator:intermediate"},
 		NotBefore:             notBefore,
 		NotAfter:              notAfter,
@@ -82,17 +79,15 @@ func New(rootPrivKey, intermPrivKey *ecdsa.PrivateKey) (*CA, error) {
 		KeyUsage:              x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
 		BasicConstraintsValid: true,
 	}
-	meshCAPEM, err := createCert(meshCACert, meshCACert, &intermPrivKey.PublicKey, intermPrivKey)
+	meshCACert, meshCAPEM, err := createCert(meshCACertTemplate, meshCACertTemplate, &intermPrivKey.PublicKey, intermPrivKey)
 	if err != nil {
 		return nil, fmt.Errorf("creating mesh certificate: %w", err)
 	}
 
 	ca := CA{
 		rootCAPrivKey: rootPrivKey,
-		rootCACert:    root,
 		rootCAPEM:     rootPEM,
 		intermPrivKey: intermPrivKey,
-		intermCACert:  intermCACert,
 		intermCAPEM:   intermCAPEM,
 		meshCACert:    meshCACert,
 		meshCAPEM:     meshCAPEM,
@@ -118,7 +113,6 @@ func (c *CA) NewAttestedMeshCert(names []string, extensions []pkix.Extension, su
 	now := time.Now()
 	certTemplate := &x509.Certificate{
 		Subject:               pkix.Name{CommonName: dnsNames[0]},
-		Issuer:                pkix.Name{CommonName: "system:coordinator:intermediate"},
 		NotBefore:             now.Add(-time.Hour),
 		NotAfter:              now.AddDate(1, 0, 0),
 		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth},
@@ -129,7 +123,7 @@ func (c *CA) NewAttestedMeshCert(names []string, extensions []pkix.Extension, su
 		IPAddresses:           ips,
 	}
 
-	certPEM, err := createCert(certTemplate, c.meshCACert, subjectPublicKey, c.intermPrivKey)
+	_, certPEM, err := createCert(certTemplate, c.meshCACert, subjectPublicKey, c.intermPrivKey)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create certificate: %w", err)
 	}
@@ -152,35 +146,40 @@ func (c *CA) GetMeshCACert() []byte {
 	return c.meshCAPEM
 }
 
-func createCert(template, parent *x509.Certificate, pub, priv any) ([]byte, error) {
+// createCert issues a new certificate for pub, based on template, signed by parent with priv.
+//
+// It returns the certificate both in PEM encoding and as an x509 struct.
+func createCert(template, parent *x509.Certificate, pub, priv any) (*x509.Certificate, []byte, error) {
 	if parent == nil {
-		return nil, errors.New("parent cannot be nil")
+		return nil, nil, errors.New("parent cannot be nil")
 	}
 	if template == nil {
-		return nil, errors.New("cert cannot be nil")
+		return nil, nil, errors.New("cert cannot be nil")
 	}
 	if template.SerialNumber != nil {
-		return nil, errors.New("cert serial number must be nil")
+		return nil, nil, errors.New("cert serial number must be nil")
 	}
 
 	serialNum, err := crypto.GenerateCertificateSerialNumber()
 	if err != nil {
-		return nil, fmt.Errorf("generating serial number: %w", err)
+		return nil, nil, fmt.Errorf("generating serial number: %w", err)
 	}
 	template.SerialNumber = serialNum
 
-	certBytes, err := x509.CreateCertificate(rand.Reader, template, parent, pub, priv)
+	certDER, err := x509.CreateCertificate(rand.Reader, template, parent, pub, priv)
 	if err != nil {
-		return nil, fmt.Errorf("creating certificate: %w", err)
+		return nil, nil, fmt.Errorf("creating certificate: %w", err)
 	}
 
-	certPEM := new(bytes.Buffer)
-	if err := pem.Encode(certPEM, &pem.Block{
+	certPem := pem.EncodeToMemory(&pem.Block{
 		Type:  "CERTIFICATE",
-		Bytes: certBytes,
-	}); err != nil {
-		return nil, fmt.Errorf("encoding certificate: %w", err)
+		Bytes: certDER,
+	})
+
+	cert, err := x509.ParseCertificate(certDER)
+	if err != nil {
+		return nil, nil, fmt.Errorf("parsing the created certificate: %w", err)
 	}
 
-	return certPEM.Bytes(), nil
+	return cert, certPem, nil
 }
diff --git a/internal/ca/ca_test.go b/internal/ca/ca_test.go
index d3714c2cf8..8afbb68cd3 100644
--- a/internal/ca/ca_test.go
+++ b/internal/ca/ca_test.go
@@ -29,10 +29,8 @@ func TestNewCA(t *testing.T) {
 	require.NoError(err)
 	assert.NotNil(ca)
 	assert.NotNil(ca.rootCAPrivKey)
-	assert.NotNil(ca.rootCACert)
 	assert.NotNil(ca.rootCAPEM)
 	assert.NotNil(ca.intermPrivKey)
-	assert.NotNil(ca.intermCACert)
 	assert.NotNil(ca.intermCAPEM)
 
 	root := pool(t, ca.rootCAPEM)
@@ -145,14 +143,19 @@ func TestCreateCert(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			assert := assert.New(t)
 
-			pem, err := createCert(tc.template, tc.parent, tc.pub, tc.priv)
+			cert, pem, err := createCert(tc.template, tc.parent, tc.pub, tc.priv)
 			if tc.wantErr {
 				assert.Error(err)
 				return
 			}
 
-			assert.NoError(err)
-			parsePEMCertificate(t, pem)
+			require.NoError(t, err)
+			parsedCert := parsePEMCertificate(t, pem)
+			assert.Equal(*parsedCert.SerialNumber, *cert.SerialNumber)
+			assert.Equal(parsedCert.Subject, cert.Subject)
+			assert.Equal(parsedCert.SubjectKeyId, cert.SubjectKeyId)
+			assert.Equal(parsedCert.Issuer, cert.Issuer)
+			assert.Equal(parsedCert.AuthorityKeyId, cert.AuthorityKeyId)
 		})
 	}
 }
@@ -213,6 +216,34 @@ func TestCAConcurrent(t *testing.T) {
 	wg.Wait()
 }
 
+func TestCertValidity(t *testing.T) {
+	require := require.New(t)
+	rootCAKey := newKey(require)
+	meshCAKey := newKey(require)
+	key := newKey(require)
+
+	ca, err := New(rootCAKey, meshCAKey)
+	require.NoError(err)
+	crt, err := ca.NewAttestedMeshCert([]string{"localhost"}, nil, key.Public())
+	require.NoError(err)
+
+	assertValidPEMCert(t, ca.GetRootCACert())
+	assertValidPEMCert(t, ca.GetMeshCACert())
+	assertValidPEMCert(t, ca.GetIntermCACert())
+	assertValidPEMCert(t, crt)
+}
+
+func assertValidPEMCert(t *testing.T, pem []byte) {
+	crt := parsePEMCertificate(t, pem)
+	if crt.IsCA {
+		assert.NotEmpty(t, crt.SubjectKeyId, "TLSv3 requires a Subject Key ID for CA certificates")
+	}
+	if crt.Issuer.CommonName != crt.Subject.CommonName {
+		assert.NotEmpty(t, crt.AuthorityKeyId, "TLSv3 requires an Authority Key ID for non-root certificates")
+	}
+	assert.Equal(t, 3, crt.Version, "certificate should be TLSv3")
+}
+
 // TestCARecovery asserts that certificates issued by a CA verify correctly under a new CA using the same keys.
 func TestCARecovery(t *testing.T) {
 	require := require.New(t)