allow multi-cert pem bundles in CustomRootCATLSSecret (#1680)

Signed-off-by: Mayank Shah <mayank.shah@percona.com>

Author: mayankshah1607

internal/patroni/reconcile_test.go

@@ -6,6 +6,7 @@ package patroni
 
 import (
 	"context"
+	"strings"
 	"testing"
 
 	"gotest.tools/v3/assert"
@@ -92,6 +93,44 @@ func TestReconcileInstanceCertificates(t *testing.T) {
 	assert.DeepEqual(t, secret, before)
 }
 
+// TestReconcileInstanceCertificatesWithCABundle covers a custom root CA
+// secret whose certificate file is a bundle of an intermediate CA followed
+// by its root, as produced by cert-manager when leaf certificates are issued
+// through a sub-CA. Every certificate in the bundle must reach
+// patroni.ca-roots, or the leaf presented by peers (signed by the
+// intermediate) cannot be chained to a trusted root and TLS verification
+// fails at bootstrap.
+func TestReconcileInstanceCertificatesWithCABundle(t *testing.T) {
+	t.Parallel()
+
+	root, err := pki.NewRootCertificateAuthority()
+	assert.NilError(t, err, "bug in test")
+	intermediate, err := pki.NewRootCertificateAuthority()
+	assert.NilError(t, err, "bug in test")
+
+	rootText, err := root.Certificate.MarshalText()
+	assert.NilError(t, err, "bug in test")
+	intermediateText, err := intermediate.Certificate.MarshalText()
+	assert.NilError(t, err, "bug in test")
+
+	bundle := append(append([]byte{}, intermediateText...), rootText...)
+
+	var bundledCA pki.Certificate
+	assert.NilError(t, bundledCA.UnmarshalText(bundle))
+
+	leaf, err := intermediate.GenerateLeafCertificate("any", nil)
+	assert.NilError(t, err, "bug in test")
+
+	ctx := context.Background()
+	secret := new(corev1.Secret)
+	assert.NilError(t, InstanceCertificates(ctx,
+		bundledCA, leaf.Certificate, leaf.PrivateKey, secret))
+
+	assert.DeepEqual(t, secret.Data["patroni.ca-roots"], bundle)
+	assert.Equal(t,
+		strings.Count(string(secret.Data["patroni.ca-roots"]), "-----BEGIN CERTIFICATE-----"), 2)
+}
+
 func TestInstanceConfigMap(t *testing.T) {
 	t.Parallel()
 

internal/pki/encoding.go

@@ -29,32 +29,57 @@ var (
 	_ encoding.TextUnmarshaler = (*Certificate)(nil)
 )
 
-// MarshalText returns a PEM encoding of c that OpenSSL understands.
+// MarshalText returns a PEM encoding of c that OpenSSL understands. When c
+// was unmarshaled from a bundle containing more than one certificate, e.g. an
+// intermediate certificate authority followed by its root, the additional
+// certificates are included after the first so the full bundle round-trips.
 func (c Certificate) MarshalText() ([]byte, error) {
 	if c.x509 == nil || len(c.x509.Raw) == 0 {
 		_, err := x509.ParseCertificate(nil)
 		return nil, err
 	}
 
-	return pem.EncodeToMemory(&pem.Block{
+	out := pem.EncodeToMemory(&pem.Block{
 		Type:  pemLabelCertificate,
 		Bytes: c.x509.Raw,
-	}), nil
+	})
+
+	return append(out, c.chain...), nil
 }
 
-// UnmarshalText populates c from its PEM encoding.
+// UnmarshalText populates c from its PEM encoding. When data contains more
+// than one PEM-encoded certificate, e.g. a CA bundle made up of an
+// intermediate certificate authority followed by its root, the first is
+// parsed for cryptographic use and every certificate is kept so the full
+// bundle round-trips through MarshalText. Any other kind of PEM block
+// (a private key, for example) is ignored rather than carried along.
 func (c *Certificate) UnmarshalText(data []byte) error {
-	block, _ := pem.Decode(data)
+	block, rest := pem.Decode(data)
 
 	if block == nil || block.Type != pemLabelCertificate {
 		return errors.New("not a PEM-encoded certificate")
 	}
 
 	parsed, err := x509.ParseCertificate(block.Bytes)
-	if err == nil {
-		c.x509 = parsed
+	if err != nil {
+		return err
 	}
-	return err
+
+	c.x509 = parsed
+	c.chain = nil
+
+	for {
+		var next *pem.Block
+		next, rest = pem.Decode(rest)
+		if next == nil {
+			break
+		}
+		if next.Type == pemLabelCertificate {
+			c.chain = append(c.chain, pem.EncodeToMemory(next)...)
+		}
+	}
+
+	return nil
 }
 
 var (

internal/pki/encoding_test.go

@@ -51,10 +51,41 @@ func TestCertificateTextMarshaling(t *testing.T) {
 
 		bundle := bytes.Join([][]byte{txt, otherText}, nil)
 
-		// Only the first certificate of a bundle is parsed.
+		// Only the first certificate of a bundle is parsed for
+		// cryptographic use (e.g. signing, CommonName, DNSNames), but every
+		// certificate in the bundle is kept so the full chain — such as an
+		// intermediate CA followed by its root — round-trips through
+		// MarshalText. Otherwise a custom root CA bundle silently loses
+		// every certificate after the first when it is copied into
+		// per-instance trust stores.
 		var sink Certificate
 		assert.NilError(t, sink.UnmarshalText(bundle))
 		assert.DeepEqual(t, cert, sink)
+
+		sinkText, err := sink.MarshalText()
+		assert.NilError(t, err)
+		assert.DeepEqual(t, sinkText, bundle)
+	})
+
+	t.Run("BundleIgnoresNonCertificateBlocks", func(t *testing.T) {
+		// A non-certificate PEM block (e.g. a private key) appended after
+		// the first certificate must never be forwarded into a trust store,
+		// even though certificates that follow are kept.
+		keyText, err := root.PrivateKey.MarshalText()
+		assert.NilError(t, err)
+
+		other, _ := NewRootCertificateAuthority()
+		otherText, err := other.Certificate.MarshalText()
+		assert.NilError(t, err)
+
+		bundle := bytes.Join([][]byte{txt, keyText, otherText}, nil)
+
+		var sink Certificate
+		assert.NilError(t, sink.UnmarshalText(bundle))
+
+		sinkText, err := sink.MarshalText()
+		assert.NilError(t, err)
+		assert.DeepEqual(t, sinkText, bytes.Join([][]byte{txt, otherText}, nil))
 	})
 
 	t.Run("EncodedEmpty", func(t *testing.T) {

internal/pki/pki.go

@@ -15,7 +15,16 @@ const renewalRatio = 3
 
 // Certificate represents an X.509 certificate that conforms to the Internet
 // PKI Profile, RFC 5280.
-type Certificate struct{ x509 *x509.Certificate }
+type Certificate struct {
+	x509 *x509.Certificate
+
+	// chain holds the PEM encoding of any certificates that followed the
+	// first one when this Certificate was unmarshaled from a bundle, e.g. an
+	// intermediate certificate authority followed by its root. It is nil for
+	// certificates that were generated rather than unmarshaled. MarshalText
+	// appends it after the leading certificate so the full bundle round-trips.
+	chain []byte
+}
 
 // PrivateKey represents the private key of a Certificate.
 type PrivateKey struct{ ecdsa *ecdsa.PrivateKey }