Add stricter verification of issuers PEM format

This ensures each issuer is only a single certificate entry (as
validated by count and parsing) without any trailing data.

We further ensure that each certificate PEM has leading and trailing
spaces removed with only a single trailing new line remaining.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

builtin/logical/pki/path_manage_issuers.go

@@ -130,6 +130,11 @@ func (b *backend) pathImportIssuers(ctx context.Context, req *logical.Request, d
 	var issuers []string
 	var keys []string
 
+	// By decoding and re-encoding PEM blobs, we can pass strict PEM blobs
+	// to the import functionality (importKeys, importIssuers). This allows
+	// them to validate no duplicate issuers exist (and place greater
+	// restrictions during parsing) but allows this code to accept OpenSSL
+	// parsed chains (with full textual output between PEM entries).
 	for len(bytes.TrimSpace(pemBytes)) > 0 {
 		pemBlock, pemBytes = pem.Decode(pemBytes)
 		if pemBlock == nil {

builtin/logical/pki/storage.go

@@ -250,10 +250,13 @@ func importKey(ctx context.Context, s logical.Storage, keyValue string, keyName
 }
 
 func (i issuer) GetCertificate() (*x509.Certificate, error) {
-	block, _ := pem.Decode([]byte(i.Certificate))
+	block, extra := pem.Decode([]byte(i.Certificate))
 	if block == nil {
 		return nil, errutil.InternalError{Err: fmt.Sprintf("unable to parse certificate from issuer: invalid PEM: %v", i.ID)}
 	}
+	if len(strings.TrimSpace(string(extra))) > 0 {
+		return nil, errutil.InternalError{Err: fmt.Sprintf("unable to parse certificate for issuer (%v): trailing PEM data: %v", i.ID, string(extra))}
+	}
 
 	return x509.ParseCertificate(block.Bytes)
 }
@@ -373,7 +376,21 @@ func importIssuer(ctx context.Context, s logical.Storage, certValue string, issu
 	// already existed during import (in which case, *issuer points to the
 	// existing issuer reference and identifier); the last return field is
 	// whether or not an error occurred.
+
+	// Before we begin, we need to ensure the PEM formatted certificate looks
+	// good. Restricting to "just" `CERTIFICATE` entries is a little
+	// restrictive, as it could be a `X509 CERTIFICATE` entry or a custom
+	// value wrapping an actual DER cert. So validating the contents of the
+	// PEM header is out of the question (and validating the contents of the
+	// PEM block is left to our GetCertificate call below).
+	//
+	// However, we should trim all leading and trailing spaces and add a
+	// single new line. This allows callers to blindly concatenate PEM
+	// blobs from the API and get roughly what they'd expect.
 	//
+	// Discussed further in #11960 and RFC 7468.
+	certValue = strings.TrimSpace(certValue) + "\n"
+
 	// Before we can import a known issuer, we first need to know if the issuer
 	// exists in storage already. This means iterating through all known
 	// issuers and comparing their private value against this value.
@@ -412,6 +429,12 @@ func importIssuer(ctx context.Context, s logical.Storage, certValue string, issu
 	result.Name = issuerName
 	result.Certificate = certValue
 
+	// We shouldn't add CSRs or multiple certificates in this
+	countCertificates := strings.Count(result.Certificate, "-BEGIN ")
+	if countCertificates != 1 {
+		return nil, false, fmt.Errorf("bad issuer: potentially multiple PEM blobs in one certificate storage entry:\n%v", result.Certificate)
+	}
+
 	// Extracting the certificate is necessary for two reasons: first, it lets
 	// us fetch the serial number; second, for the public key comparison with
 	// known keys.

builtin/logical/pki/storage_migrations_test.go

@@ -76,7 +76,7 @@ func Test_migrateStorageSimpleBundle(t *testing.T) {
 
 	require.Equal(t, issuerId, issuer.ID)
 	require.Equal(t, bundle.SerialNumber, issuer.SerialNumber)
-	require.Equal(t, bundle.Certificate, issuer.Certificate)
+	require.Equal(t, strings.TrimSpace(bundle.Certificate), strings.TrimSpace(issuer.Certificate))
 	require.Equal(t, keyId, issuer.KeyID)
 	// FIXME: Add tests for CAChain...
 

builtin/logical/pki/storage_test.go

@@ -3,6 +3,7 @@ package pki
 import (
 	"context"
 	"crypto/rand"
+	"strings"
 	"testing"
 
 	"github.com/hashicorp/vault/sdk/framework"
@@ -119,7 +120,7 @@ func Test_KeysIssuerImport(t *testing.T) {
 	issuer1_ref1, existing, err := importIssuer(ctx, s, issuer1.Certificate, "issuer1")
 	require.NoError(t, err)
 	require.False(t, existing)
-	require.Equal(t, issuer1.Certificate, issuer1_ref1.Certificate)
+	require.Equal(t, strings.TrimSpace(issuer1.Certificate), strings.TrimSpace(issuer1_ref1.Certificate))
 	require.Equal(t, key1_ref1.ID, issuer1_ref1.KeyID)
 	require.Equal(t, "issuer1", issuer1_ref1.Name)
 
@@ -128,7 +129,7 @@ func Test_KeysIssuerImport(t *testing.T) {
 	issuer1_ref2, existing, err := importIssuer(ctx, s, issuer1.Certificate, "ignore-me")
 	require.NoError(t, err)
 	require.True(t, existing)
-	require.Equal(t, issuer1.Certificate, issuer1_ref1.Certificate)
+	require.Equal(t, strings.TrimSpace(issuer1.Certificate), strings.TrimSpace(issuer1_ref1.Certificate))
 	require.Equal(t, issuer1_ref1.ID, issuer1_ref2.ID)
 	require.Equal(t, key1_ref1.ID, issuer1_ref2.KeyID)
 	require.Equal(t, issuer1_ref1.Name, issuer1_ref2.Name)
@@ -143,7 +144,7 @@ func Test_KeysIssuerImport(t *testing.T) {
 	issuer2_ref, existing, err := importIssuer(ctx, s, issuer2.Certificate, "ignore-me")
 	require.NoError(t, err)
 	require.True(t, existing)
-	require.Equal(t, issuer2.Certificate, issuer2_ref.Certificate)
+	require.Equal(t, strings.TrimSpace(issuer2.Certificate), strings.TrimSpace(issuer2_ref.Certificate))
 	require.Equal(t, issuer2.ID, issuer2_ref.ID)
 	require.Equal(t, "", issuer2_ref.Name)
 	require.Equal(t, issuer2.KeyID, issuer2_ref.KeyID)
@@ -173,7 +174,7 @@ func genIssuerAndKey(t *testing.T, b *backend) (issuer, key) {
 	pkiIssuer := issuer{
 		ID:           issuerId,
 		KeyID:        keyId,
-		Certificate:  certBundle.Certificate,
+		Certificate:  strings.TrimSpace(certBundle.Certificate) + "\n",
 		CAChain:      certBundle.CAChain,
 		SerialNumber: certBundle.SerialNumber,
 	}