Simplify the PKI implementation

The original implementation dynamically assigns functions that return
errors so we can swap them under test. Errors from these calls are
wrapped in sentinels so they can be identified at runtime. In practice,
however, these errors are never examined.

- Sentinel errors are removed. The "encoding/pem.Decode" function does
  not return errors, so we still generate our own in two places.

- All "Parse" functions are removed and replaced by their "Unmarshal"
  equivalents.

- Most "New" functions are removed. One remains to generate a fresh root
  CA certificate and private key pair.

- IP addresses are removed.

Fields on the "Certificate" and "PrivateKey" types are not exported,
making them opaque to consumers except for the PEM marshaling methods.
This provides a few benefits:

- The algorithms for keys and signatures can change without affecting
  callers.

- Certificates are parsed as they are generated and unmarshaled. Their
  values are always either zero or fully parsed.

- The root CA is parsed once per reconcile loop rather than once per
  leaf.

- Getter methods return copies so that certificate fields cannot change.

Author: cbandy

internal/controller/postgrescluster/instance.go

@@ -1363,7 +1363,7 @@ func (r *Reconciler) reconcileInstanceCertificates(
 	}
 	if err == nil {
 		err = pgbackrest.InstanceCertificates(ctx, cluster,
-			*root.Certificate, *leafCert.Certificate, *leafCert.PrivateKey,
+			root.Certificate, leafCert.Certificate, leafCert.PrivateKey,
 			instanceCerts)
 	}
 	if err == nil {

internal/controller/postgrescluster/patroni.go

@@ -352,22 +352,17 @@ func (r *Reconciler) reconcileReplicationSecret(
 	err := errors.WithStack(client.IgnoreNotFound(
 		r.Client.Get(ctx, client.ObjectKeyFromObject(existing), existing)))
 
-	clientLeaf := pki.NewLeafCertificate("", nil, nil)
-	clientLeaf.DNSNames = []string{postgres.ReplicationUser}
-	clientLeaf.CommonName = clientLeaf.DNSNames[0]
+	clientLeaf := &pki.LeafCertificate{}
+	_ = clientLeaf.Certificate.UnmarshalText(existing.Data[naming.ReplicationCert])
+	_ = clientLeaf.PrivateKey.UnmarshalText(existing.Data[naming.ReplicationPrivateKey])
 
-	if data, ok := existing.Data[naming.ReplicationCert]; err == nil && ok {
-		clientLeaf.Certificate, err = pki.ParseCertificate(data)
-		err = errors.WithStack(err)
-	}
-	if data, ok := existing.Data[naming.ReplicationPrivateKey]; err == nil && ok {
-		clientLeaf.PrivateKey, err = pki.ParsePrivateKey(data)
-		err = errors.WithStack(err)
-	}
+	commonName := postgres.ReplicationUser
+	dnsNames := []string{commonName}
 
 	// if there is an error or the client leaf certificate is bad, generate a new one
 	if err != nil || pki.LeafCertIsBad(ctx, clientLeaf, rootCACert, cluster.Namespace) {
-		err = errors.WithStack(clientLeaf.Generate(rootCACert))
+		clientLeaf, err = rootCACert.GenerateLeafCertificate(commonName, dnsNames)
+		err = errors.WithStack(err)
 	}
 
 	intent := &corev1.Secret{ObjectMeta: naming.ReplicationClientCertSecret(cluster)}

internal/controller/postgrescluster/pgbackrest_test.go

@@ -244,8 +244,8 @@ func TestReconcilePGBackRest(t *testing.T) {
 			Type: condition, Reason: "testing", Status: status})
 	}
 
-	rootCA := pki.NewRootCertificateAuthority()
-	assert.NilError(t, rootCA.Generate())
+	rootCA, err := pki.NewRootCertificateAuthority()
+	assert.NilError(t, err)
 
 	result, err := r.reconcilePGBackRest(ctx, postgresCluster, instances, rootCA)
 	if err != nil || result != (reconcile.Result{}) {
@@ -1780,8 +1780,8 @@ func TestReconcilePostgresClusterDataSource(t *testing.T) {
 	t.Cleanup(func() { teardownManager(cancel, t) })
 
 	namespace := setupNamespace(t, tClient).Name
-	rootCA := pki.NewRootCertificateAuthority()
-	assert.NilError(t, rootCA.Generate())
+	rootCA, err := pki.NewRootCertificateAuthority()
+	assert.NilError(t, err)
 
 	type testResult struct {
 		configCount, jobCount, pvcCount                         int

internal/controller/postgrescluster/pki.go

@@ -55,20 +55,14 @@ func (r *Reconciler) reconcileRootCertificate(
 	err := errors.WithStack(client.IgnoreNotFound(
 		r.Client.Get(ctx, client.ObjectKeyFromObject(existing), existing)))
 
-	root := pki.NewRootCertificateAuthority()
-
-	if data, ok := existing.Data[keyCertificate]; err == nil && ok {
-		root.Certificate, err = pki.ParseCertificate(data)
-		err = errors.WithStack(err)
-	}
-	if data, ok := existing.Data[keyPrivateKey]; err == nil && ok {
-		root.PrivateKey, err = pki.ParsePrivateKey(data)
-		err = errors.WithStack(err)
-	}
+	root := &pki.RootCertificateAuthority{}
+	_ = root.Certificate.UnmarshalText(existing.Data[keyCertificate])
+	_ = root.PrivateKey.UnmarshalText(existing.Data[keyPrivateKey])
 
 	// if there is an error or the root CA is bad, generate a new one
 	if err != nil || pki.RootCAIsBad(root) {
-		err = errors.WithStack(root.Generate())
+		root, err = pki.NewRootCertificateAuthority()
+		err = errors.WithStack(err)
 	}
 
 	intent := &corev1.Secret{}
@@ -133,22 +127,17 @@ func (r *Reconciler) reconcileClusterCertificate(
 	err := errors.WithStack(client.IgnoreNotFound(
 		r.Client.Get(ctx, client.ObjectKeyFromObject(existing), existing)))
 
-	leaf := pki.NewLeafCertificate("", nil, nil)
-	leaf.DNSNames = naming.ServiceDNSNames(ctx, primaryService)
-	leaf.CommonName = leaf.DNSNames[0] // FQDN
+	leaf := &pki.LeafCertificate{}
+	_ = leaf.Certificate.UnmarshalText(existing.Data[keyCertificate])
+	_ = leaf.PrivateKey.UnmarshalText(existing.Data[keyPrivateKey])
 
-	if data, ok := existing.Data[keyCertificate]; err == nil && ok {
-		leaf.Certificate, err = pki.ParseCertificate(data)
-		err = errors.WithStack(err)
-	}
-	if data, ok := existing.Data[keyPrivateKey]; err == nil && ok {
-		leaf.PrivateKey, err = pki.ParsePrivateKey(data)
-		err = errors.WithStack(err)
-	}
+	dnsNames := naming.ServiceDNSNames(ctx, primaryService)
+	dnsFQDN := dnsNames[0]
 
 	// if there is an error or the leaf certificate is bad, generate a new one
 	if err != nil || pki.LeafCertIsBad(ctx, leaf, rootCACert, cluster.Namespace) {
-		err = errors.WithStack(leaf.Generate(rootCACert))
+		leaf, err = rootCACert.GenerateLeafCertificate(dnsFQDN, dnsNames)
+		err = errors.WithStack(err)
 	}
 
 	intent := &corev1.Secret{ObjectMeta: naming.PostgresTLSSecret(cluster)}
@@ -212,24 +201,19 @@ func (*Reconciler) instanceCertificate(
 	var err error
 	const keyCertificate, keyPrivateKey = "dns.crt", "dns.key"
 
+	leaf := &pki.LeafCertificate{}
+	_ = leaf.Certificate.UnmarshalText(existing.Data[keyCertificate])
+	_ = leaf.PrivateKey.UnmarshalText(existing.Data[keyPrivateKey])
+
 	// RFC 2818 states that the certificate DNS names must be used to verify
 	// HTTPS identity.
-	leaf := pki.NewLeafCertificate("", nil, nil)
-	leaf.DNSNames = naming.InstancePodDNSNames(ctx, instance)
-	leaf.CommonName = leaf.DNSNames[0] // FQDN
-
-	if data, ok := existing.Data[keyCertificate]; err == nil && ok {
-		leaf.Certificate, err = pki.ParseCertificate(data)
-		err = errors.WithStack(err)
-	}
-	if data, ok := existing.Data[keyPrivateKey]; err == nil && ok {
-		leaf.PrivateKey, err = pki.ParsePrivateKey(data)
-		err = errors.WithStack(err)
-	}
+	dnsNames := naming.InstancePodDNSNames(ctx, instance)
+	dnsFQDN := dnsNames[0]
 
 	// if there is an error or the leaf certificate is bad, generate a new one
 	if err != nil || pki.LeafCertIsBad(ctx, leaf, rootCACert, instance.Namespace) {
-		err = errors.WithStack(leaf.Generate(rootCACert))
+		leaf, err = rootCACert.GenerateLeafCertificate(dnsFQDN, dnsNames)
+		err = errors.WithStack(err)
 	}
 
 	if err == nil {

internal/controller/postgrescluster/pki_test.go

@@ -19,7 +19,6 @@ package postgrescluster
 
 import (
 	"context"
-	"crypto/x509"
 	"fmt"
 	"os"
 	"reflect"
@@ -145,7 +144,7 @@ func TestReconcileCerts(t *testing.T) {
 			assert.NilError(t, err)
 
 			// assert returned certificate matches the one created earlier
-			assert.DeepEqual(t, fromSecret, initialRoot.Certificate)
+			assert.DeepEqual(t, *fromSecret, initialRoot.Certificate)
 		})
 
 		t.Run("root certificate changes", func(t *testing.T) {
@@ -166,10 +165,10 @@ func TestReconcileCerts(t *testing.T) {
 			assert.NilError(t, err)
 
 			// check that the cert from the secret does not equal the initial certificate
-			assert.Assert(t, !fromSecret.Equal(*initialRoot.Certificate))
+			assert.Assert(t, !fromSecret.Equal(initialRoot.Certificate))
 
 			// check that the returned cert matches the cert from the secret
-			assert.DeepEqual(t, fromSecret, returnedRoot.Certificate)
+			assert.DeepEqual(t, *fromSecret, returnedRoot.Certificate)
 		})
 
 	})
@@ -201,13 +200,11 @@ func TestReconcileCerts(t *testing.T) {
 			initialLeafCert, err := r.instanceCertificate(ctx, instance, existing, intent, initialRoot)
 			assert.NilError(t, err)
 
-			certificate, err := pki.ParseCertificate(intent.Data["dns.crt"])
-			assert.NilError(t, err)
-			privateKey, err := pki.ParsePrivateKey(intent.Data["dns.key"])
-			assert.NilError(t, err)
+			fromSecret := &pki.LeafCertificate{}
+			assert.NilError(t, fromSecret.Certificate.UnmarshalText(intent.Data["dns.crt"]))
+			assert.NilError(t, fromSecret.PrivateKey.UnmarshalText(intent.Data["dns.key"]))
 
-			assert.DeepEqual(t, certificate, initialLeafCert.Certificate)
-			assert.DeepEqual(t, privateKey, initialLeafCert.PrivateKey)
+			assert.DeepEqual(t, fromSecret, initialLeafCert)
 		})
 
 		t.Run("check that the leaf certs update when root changes", func(t *testing.T) {
@@ -235,14 +232,14 @@ func TestReconcileCerts(t *testing.T) {
 			assert.NilError(t, err)
 
 			// assert old leaf cert does not match the newly reconciled one
-			assert.Assert(t, !initialLeaf.Certificate.Equal(*newLeaf.Certificate))
+			assert.Assert(t, !initialLeaf.Certificate.Equal(newLeaf.Certificate))
 
 			// 'reconcile' the certificate when the secret does not change. The returned leaf certificate should not change
 			newLeaf2, err := r.instanceCertificate(ctx, instance, intent, intent, newRootCert)
 			assert.NilError(t, err)
 
 			// check that the leaf cert did not change after another reconciliation
-			assert.DeepEqual(t, newLeaf2.Certificate, newLeaf.Certificate)
+			assert.DeepEqual(t, newLeaf2, newLeaf)
 
 		})
 
@@ -364,17 +361,16 @@ func TestReconcileCerts(t *testing.T) {
 
 			assert.Assert(t, !reflect.DeepEqual(initialClusterCertSecret, newClusterCertSecret))
 
-			pkiCert, err := pki.ParseCertificate(newClusterCertSecret.Data["tls.crt"])
-			assert.NilError(t, err)
+			leaf := &pki.LeafCertificate{}
+			assert.NilError(t, leaf.Certificate.UnmarshalText(newClusterCertSecret.Data["tls.crt"]))
+			assert.NilError(t, leaf.PrivateKey.UnmarshalText(newClusterCertSecret.Data["tls.key"]))
 
-			x509Cert, err := x509.ParseCertificate(pkiCert.Certificate)
-			assert.NilError(t, err)
 			assert.Assert(t,
-				strings.HasPrefix(x509Cert.Subject.CommonName, "the-primary."+namespace+".svc."),
-				"got %q", x509Cert.Subject.CommonName)
+				strings.HasPrefix(leaf.Certificate.CommonName(), "the-primary."+namespace+".svc."),
+				"got %q", leaf.Certificate.CommonName())
 
-			if assert.Check(t, len(x509Cert.DNSNames) > 1) {
-				assert.DeepEqual(t, x509Cert.DNSNames[1:], []string{
+			if dnsNames := leaf.Certificate.DNSNames(); assert.Check(t, len(dnsNames) > 1) {
+				assert.DeepEqual(t, dnsNames[1:], []string{
 					"the-primary." + namespace + ".svc",
 					"the-primary." + namespace,
 					"the-primary",
@@ -404,9 +400,6 @@ func getCertFromSecret(
 	}
 
 	// parse the cert from binary encoded data
-	if fromSecret, err := pki.ParseCertificate(secretCRT); fromSecret == nil || err != nil {
-		return nil, fmt.Errorf("error parsing %s", dataKey)
-	} else {
-		return fromSecret, nil
-	}
+	fromSecret := &pki.Certificate{}
+	return fromSecret, fromSecret.UnmarshalText(secretCRT)
 }

internal/patroni/certificates.go

@@ -30,7 +30,7 @@ const (
 )
 
 // certAuthorities encodes roots in a format suitable for Patroni's TLS verification.
-func certAuthorities(roots ...*pki.Certificate) ([]byte, error) {
+func certAuthorities(roots ...pki.Certificate) ([]byte, error) {
 	var out []byte
 
 	for i := range roots {
@@ -46,7 +46,7 @@ func certAuthorities(roots ...*pki.Certificate) ([]byte, error) {
 
 // certFile encodes cert and key as a combination suitable for
 // Patroni's TLS identification. It can be used by both the client and the server.
-func certFile(key *pki.PrivateKey, cert *pki.Certificate) ([]byte, error) {
+func certFile(key pki.PrivateKey, cert pki.Certificate) ([]byte, error) {
 	var out []byte
 
 	if b, err := key.MarshalText(); err == nil {

internal/patroni/certificates_test.go

@@ -38,8 +38,8 @@ BY8V4Ag=
 -----END CERTIFICATE-----`
 
 func TestCertAuthorities(t *testing.T) {
-	root, err := pki.ParseCertificate([]byte(rootPEM))
-	assert.NilError(t, err)
+	root := pki.Certificate{}
+	assert.NilError(t, root.UnmarshalText([]byte(rootPEM)))
 
 	data, err := certAuthorities(root)
 	assert.NilError(t, err)
@@ -49,11 +49,11 @@ func TestCertAuthorities(t *testing.T) {
 }
 
 func TestCertFile(t *testing.T) {
-	root := pki.NewRootCertificateAuthority()
-	assert.NilError(t, root.Generate())
+	root, err := pki.NewRootCertificateAuthority()
+	assert.NilError(t, err)
 
-	instance := pki.NewLeafCertificate("instance.pod-dns", nil, nil)
-	assert.NilError(t, instance.Generate(root))
+	instance, err := root.GenerateLeafCertificate("instance.pod-dns", nil)
+	assert.NilError(t, err)
 
 	data, err := certFile(instance.PrivateKey, instance.Certificate)
 	assert.NilError(t, err)

internal/patroni/reconcile.go

@@ -74,8 +74,8 @@ func InstanceConfigMap(ctx context.Context,
 
 // InstanceCertificates populates the shared Secret with certificates needed to run Patroni.
 func InstanceCertificates(ctx context.Context,
-	inRoot *pki.Certificate, inDNS *pki.Certificate,
-	inDNSKey *pki.PrivateKey, outInstanceCertificates *corev1.Secret,
+	inRoot pki.Certificate, inDNS pki.Certificate,
+	inDNSKey pki.PrivateKey, outInstanceCertificates *corev1.Secret,
 ) error {
 	initialize.ByteMap(&outInstanceCertificates.Data)
 

internal/patroni/reconcile_test.go

@@ -55,11 +55,11 @@ func TestClusterConfigMap(t *testing.T) {
 func TestReconcileInstanceCertificates(t *testing.T) {
 	t.Parallel()
 
-	root := pki.NewRootCertificateAuthority()
-	assert.NilError(t, root.Generate(), "bug in test")
+	root, err := pki.NewRootCertificateAuthority()
+	assert.NilError(t, err, "bug in test")
 
-	leaf := pki.NewLeafCertificate("any", nil, nil)
-	assert.NilError(t, leaf.Generate(root), "bug in test")
+	leaf, err := root.GenerateLeafCertificate("any", nil)
+	assert.NilError(t, err, "bug in test")
 
 	ctx := context.Background()
 	secret := new(corev1.Secret)