Merge #71548

71548: security: prevent PEM decoding errors by careful newline insertation r=catj-cockroach a=catj-cockroach

Fixes #71588

> Previously, we assumed that the Tenant CA certificate and the CA
> certificate would not be the same, and that they would be saved as
> files with an end of file newline character.
> 
> This change adds support for not loading the Tenant CA Certificate
> if it will just be a second copy of the CA certificate.
> 
> This change also adds a helper function to concatenate certificates
> with a newline separating each, to ensure that the easiest way to
> make a certificate bundle is the correct way.
> 
> Release note: bugfix for certificate loading logic

The bug in question was introduced in #71248 (and back ported in #71402).

Co-authored-by: Cat J <87989074+catj-cockroach@users.noreply.github.com>

pkg/security/certificate_manager.go

@@ -1007,10 +1007,12 @@ func (cm *CertificateManager) GetTenantTLSConfig() (*tls.Config, error) {
 
 	caBlob := ca.FileContents
 
-	// CA used to validate certs provided by other SQL servers.
-	tenantCA, err := cm.getTenantCACertLocked()
-	if err == nil {
-		caBlob = append(caBlob, tenantCA.FileContents...)
+	if cm.tenantCACert != nil {
+		// If it's available, we also include the tenant CA.
+		tenantCA, err := cm.getTenantCACertLocked()
+		if err == nil {
+			caBlob = AppendCertificatesToBlob(caBlob, tenantCA.FileContents)
+		}
 	}
 
 	// Client cert presented to KV nodes.
@@ -1113,10 +1115,12 @@ func (cm *CertificateManager) GetUIClientTLSConfig() (*tls.Config, error) {
 
 	caBlob := uiCA.FileContents
 
-	// If it's available, we also include the tenant CA.
-	tenantCA, err := cm.getTenantCACertLocked()
-	if err == nil {
-		caBlob = append(caBlob, tenantCA.FileContents...)
+	if cm.tenantCACert != nil {
+		// If it's available, we also include the tenant CA.
+		tenantCA, err := cm.getTenantCACertLocked()
+		if err == nil {
+			caBlob = AppendCertificatesToBlob(caBlob, tenantCA.FileContents)
+		}
 	}
 
 	cfg, err := newUIClientTLSConfig(cm.tlsSettings, caBlob)

pkg/security/certs.go

@@ -11,6 +11,7 @@
 package security
 
 import (
+	"bytes"
 	"context"
 	"crypto"
 	"crypto/rand"
@@ -561,3 +562,16 @@ func PEMContentsToX509(contents []byte) ([]*x509.Certificate, error) {
 
 	return certs, nil
 }
+
+// AppendCertificatesToBlob adds the passed PEM encoded certificates to the existing
+// byte slice containing PEM encoded certificates, ensuring that there is a newline
+// separating the original byte slice and each subsequent certificate byte slices.
+func AppendCertificatesToBlob(certBlob []byte, newCerts ...[]byte) []byte {
+	return bytes.Join(
+		append(
+			[][]byte{certBlob},
+			newCerts...,
+		),
+		[]byte{'\n'},
+	)
+}

pkg/security/certs_test.go

@@ -11,7 +11,9 @@
 package security_test
 
 import (
+	"bytes"
 	"context"
+	"crypto/x509"
 	gosql "database/sql"
 	"fmt"
 	"io/ioutil"
@@ -25,6 +27,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/rpc"
 	"github.com/cockroachdb/cockroach/pkg/security"
+	"github.com/cockroachdb/cockroach/pkg/security/securitytest"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -635,3 +638,43 @@ func TestUseWrongSplitCACerts(t *testing.T) {
 		}
 	}
 }
+
+func TestAppendCertificateToBlob(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	caBlob, err := securitytest.Asset(filepath.Join(security.EmbeddedCertsDir, security.EmbeddedCACert))
+	if err != nil {
+		t.Fatal(err)
+	}
+	testCertPool := x509.NewCertPool()
+
+	certsToAdd := make([][]byte, 0, 3)
+
+	for _, certFilename := range []string{
+		//		security.EmbeddedClientCACert,
+		//		security.EmbeddedUICACert,
+		security.EmbeddedTenantCACert,
+	} {
+		newCertBlob, err := securitytest.Asset(filepath.Join(security.EmbeddedCertsDir, certFilename))
+		if err != nil {
+			t.Errorf("failed to read certificate \"%s\": %s", certFilename, err)
+			continue
+		}
+
+		certsToAdd = append(certsToAdd, bytes.TrimRight(newCertBlob, "\n"))
+
+	}
+
+	caBlob = security.AppendCertificatesToBlob(
+		bytes.TrimRight(caBlob, "\n"),
+		certsToAdd...,
+	)
+
+	if !testCertPool.AppendCertsFromPEM(caBlob) {
+		if testing.Verbose() {
+			t.Fatalf("appendCertificatesToBlob failed to properly concatenate the test certificates together:\n===\n%s===\n", caBlob)
+		} else {
+			t.Fatal("appendCertificatesToBlob failed to properly concatenate the test certificates together. Run with the verbose flag set to see the output.")
+		}
+	}
+}