Sign empty to cert on manual-chain update. (#29473)

* Sign empty to cert on manual-chain update.

* Add role defaults.

* Add changelog.

* More useful error message.

* Suggestions from PR Review.

* Fixes to update as well as write; test that still fails; revert code.

* Unit Test fix.

* Add go doc to TestManualChainValidation

builtin/logical/pki/path_fetch_issuers.go

@@ -714,19 +714,34 @@ func (b *backend) pathUpdateIssuer(ctx context.Context, req *logical.Request, da
 	}
 
 	if updateChain {
+		oldChain := issuer.ManualChain
 		issuer.ManualChain = constructedChain
 
 		// Building the chain will write the issuer to disk; no need to do it
 		// twice.
 		modified = false
-		err := sc.rebuildIssuersChains(issuer)
+		err = sc.rebuildIssuersChains(issuer)
 		if err != nil {
 			return nil, err
 		}
+
+		if issuer.Usage.HasUsage(issuing.IssuanceUsage) {
+			// Issuer has been saved by building the chain above
+			err = b.issueSignEmptyCert(ctx, req, issuer.ID.String())
+			if err != nil {
+				issuer.ManualChain = oldChain
+				newErr := sc.rebuildIssuersChains(issuer)
+				if newErr != nil {
+					return logical.ErrorResponse("error reverting bad chain update, state unknown: %v, \ninitial error: %v", newErr.Error(), err.Error()), nil
+				}
+				return logical.ErrorResponse("other changes to issuer may be persisted.  Error setting manual chain, issuer would be unusuable with this chain: %v", err), nil
+			}
+		}
+
 	}
 
 	if modified {
-		err := sc.writeIssuer(issuer)
+		err = sc.writeIssuer(issuer)
 		if err != nil {
 			return nil, err
 		}
@@ -976,15 +991,30 @@ func (b *backend) pathPatchIssuer(ctx context.Context, req *logical.Request, dat
 		}
 
 		if updateChain {
+			oldChain := issuer.ManualChain
 			issuer.ManualChain = constructedChain
 
 			// Building the chain will write the issuer to disk; no need to do it
 			// twice.
 			modified = false
-			err := sc.rebuildIssuersChains(issuer)
+			err = sc.rebuildIssuersChains(issuer)
 			if err != nil {
 				return nil, err
 			}
+			// If this issuer is supposed to be issuing certificates, test that will work
+			if issuer.Usage.HasUsage(issuing.IssuanceUsage) {
+				// Issuer has been saved by building the chain above
+				err = b.issueSignEmptyCert(ctx, req, issuer.Name)
+				if err != nil {
+					issuer.ManualChain = oldChain
+					newErr := sc.rebuildIssuersChains(issuer)
+					if newErr != nil {
+						return logical.ErrorResponse("error reverting bad chain update, state unknown: %v, \ninitial error: %v", newErr.Error(), err.Error()), nil
+					}
+					return logical.ErrorResponse("other changes to issuer may be persisted.  Error setting manual chain, issuer would be unusuable with this chain: %v", err), nil
+				}
+			}
+
 		}
 	}
 

builtin/logical/pki/path_fetch_issuers_test.go

@@ -0,0 +1,110 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: BUSL-1.1
+
+package pki
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestManualChainValidation creates a series of issuers, and then tries to set the manual_chain on the final issuer
+// to something completely incorrect (missing an intermediate issuer).  In this case, the attempt to write the manual
+// chain throws an error, and the manual chain is not updated.
+func TestManualChainValidation(t *testing.T) {
+	// Set Up a Cluster
+	cluster, client := setupTestPkiCluster(t)
+	defer cluster.Cleanup()
+
+	// Set Up Root-A
+	mount := "pki"
+
+	resp, err := client.Logical().WriteWithContext(context.Background(), mount+"/issuers/generate/root/internal",
+		map[string]interface{}{
+			"issuer_name": "rootCa",
+			"key_name":    "root-key",
+			"key_type":    "ec",
+			"common_name": "Test Root",
+			"ttl":         "7200h",
+		})
+	require.NoError(t, err, "failed creating root CA")
+
+	// Set Up Int-A
+	resp, err = client.Logical().WriteWithContext(context.Background(), mount+"/issuers/generate/intermediate/internal",
+		map[string]interface{}{
+			"key_name":    "int-a-key",
+			"key_type":    "ec",
+			"common_name": "Test Int A",
+		})
+	require.NoError(t, err, "failed creating intermediary CSR")
+	intermediateCSR := resp.Data["csr"].(string)
+
+	// Sign the intermediate CSR
+	resp, err = client.Logical().Write(mount+"/issuer/rootCa/sign-intermediate", map[string]interface{}{
+		"csr":         intermediateCSR,
+		"ttl":         "7100h",
+		"common_name": "Test Int A",
+	})
+	require.NoError(t, err, "failed signing intermediary CSR")
+	intermediateCertPEM := resp.Data["certificate"].(string)
+
+	// Import the intermediate cert
+	resp, err = client.Logical().Write(mount+"/issuers/import/cert", map[string]interface{}{
+		"pem_bundle": intermediateCertPEM,
+	})
+	require.NoError(t, err, "failed importing intermediary cert")
+	importedIssuersRaw := resp.Data["imported_issuers"].([]interface{})
+	require.Len(t, importedIssuersRaw, 1)
+	intCaUuid := importedIssuersRaw[0].(string)
+
+	_, err = client.Logical().Write(mount+"/issuer/"+intCaUuid, map[string]interface{}{
+		"issuer_name": "intA",
+	})
+	require.NoError(t, err, "failed updating issuer name")
+
+	// Set Up Int-B (Child of Int-A)
+	resp, err = client.Logical().WriteWithContext(context.Background(), mount+"/issuers/generate/intermediate/internal",
+		map[string]interface{}{
+			"key_name":    "int-b-key",
+			"key_type":    "ec",
+			"common_name": "Test Int B",
+		})
+	require.NoError(t, err, "failed creating intermediary CSR")
+	subIntermediateCSR := resp.Data["csr"].(string)
+
+	// Sign the intermediate CSR
+	resp, err = client.Logical().Write(mount+"/issuer/intA/sign-intermediate", map[string]interface{}{
+		"csr":         subIntermediateCSR,
+		"ttl":         "7100h",
+		"common_name": "Test Int B",
+	})
+	require.NoError(t, err, "failed signing intermediary CSR")
+	subIntermediateCertPEM := resp.Data["certificate"].(string)
+
+	// Import the intermediate cert
+	resp, err = client.Logical().Write(mount+"/issuers/import/cert", map[string]interface{}{
+		"pem_bundle": subIntermediateCertPEM,
+	})
+	require.NoError(t, err, "failed importing intermediary cert")
+	subImportedIssuersRaw := resp.Data["imported_issuers"].([]interface{})
+	require.Len(t, subImportedIssuersRaw, 1)
+	subIntCaUuid := subImportedIssuersRaw[0].(string)
+
+	resp, err = client.Logical().Write(mount+"/issuer/"+subIntCaUuid, map[string]interface{}{
+		"issuer_name": "intB",
+	})
+	require.NoError(t, err, "failed updating issuer name")
+
+	// Try to Set Int-B Manual Chain to Just Be Root-A; Expect An Error
+	resp, err = client.Logical().Write(mount+"/issuer/intB", map[string]interface{}{
+		"issuer_name":  "intB",
+		"manual_chain": []string{"intB", "rootCa"}, // Misses "intA" which issued "intB"
+	})
+	require.Error(t, err, "failed updating intermediary cert")
+
+	resp, err = client.Logical().Read(mount + "/issuer/intB")
+	require.NoError(t, err, "failed reading intermediary cert")
+	require.Nil(t, resp.Data["manual_chain"], "error reverting manual chain, got non-nil manual chain")
+}

builtin/logical/pki/path_issue_sign.go

@@ -339,6 +339,9 @@ func (b *backend) pathSignVerbatim(ctx context.Context, req *logical.Request, da
 	return b.pathIssueSignCert(ctx, req, data, entry, true, true)
 }
 
+// pathIssueSignCert is called by issueSignEmptyCert (to validate an issuer) in which case it is not handling the
+// request, only serving to provide useful (error) information to a request
+// pathIssueSignCert is also the handler for issuing and signing endpoints, in which case it serves requests entirely
 func (b *backend) pathIssueSignCert(ctx context.Context, req *logical.Request, data *framework.FieldData, role *issuing.RoleEntry, useCSR, useCSRValues bool) (*logical.Response, error) {
 	// Error out early if incompatible fields set:
 	certMetadata, metadataInRequest := data.GetOk("cert_metadata")
@@ -432,10 +435,6 @@ func (b *backend) pathIssueSignCert(ctx context.Context, req *logical.Request, d
 		}
 	}
 
-	if err := issuing.VerifyCertificate(sc.GetContext(), sc.GetStorage(), issuerId, parsedBundle); err != nil {
-		return nil, err
-	}
-
 	generateLease := false
 	if role.GenerateLease != nil && *role.GenerateLease {
 		generateLease = true
@@ -446,6 +445,10 @@ func (b *backend) pathIssueSignCert(ctx context.Context, req *logical.Request, d
 		return nil, err
 	}
 
+	if err = issuing.VerifyCertificate(sc.GetContext(), sc.GetStorage(), issuerId, parsedBundle); err != nil {
+		return nil, err
+	}
+
 	if !role.NoStore {
 		err = issuing.StoreCertificate(ctx, req.Storage, b.GetCertificateCounter(), parsedBundle)
 		if err != nil {
@@ -637,3 +640,40 @@ requested common name is allowed by the role policy.
 This path requires a CSR; if you want Vault to generate a private key
 for you, use the issue path instead.
 `
+
+func (b *backend) issueSignEmptyCert(ctx context.Context, req *logical.Request, issuerName string) error {
+	emptyRole := &issuing.RoleEntry{
+		AllowLocalhost:    true,
+		AllowedBaseDomain: "*",
+		AllowAnyName:      true,
+		AllowedDomains:    []string{"*"},
+		AllowBaseDomain:   true,
+		AllowBareDomains:  true,
+		AllowGlobDomains:  true,
+		AllowSubdomains:   true,
+		AllowIPSANs:       true,
+		NoStore:           true,
+		NoStoreMetadata:   true,
+		Issuer:            issuerName,
+		RequireCN:         false,
+		KeyBits:           256,  // Any stored role will have some value here;
+		KeyType:           "ec", // We need more tests with "ec"
+	}
+	schema := map[string]*framework.FieldSchema{}
+	schema = addNonCACommonFields(addIssueAndSignCommonFields(schema))
+	emptyData := &framework.FieldData{
+		Raw: map[string]interface{}{
+			"ttl":        "300s",
+			"issuer_ref": issuerName,
+		},
+		Schema: schema,
+	}
+	resp, err := b.pathIssueSignCert(ctx, req, emptyData, emptyRole, false, false)
+	if err != nil {
+		return fmt.Errorf("certificate path set on issuer %v is not functional: %v", issuerName, err)
+	}
+	if resp.IsError() {
+		return fmt.Errorf("certificate path set on issuer %v is not functional: %v", issuerName, resp.Error())
+	}
+	return nil
+}

changelog/29473.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+secrets(pki): Error if attempt to set a manual chain on an issuer that can't issue any certificate.
+```