From 49ecdad1ada3969fc3d44c5f7b51291ed3ed72b2 Mon Sep 17 00:00:00 2001 From: Kit Haines Date: Mon, 10 Feb 2025 15:24:52 -0500 Subject: [PATCH] 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 | 36 +++++- .../logical/pki/path_fetch_issuers_test.go | 110 ++++++++++++++++++ builtin/logical/pki/path_issue_sign.go | 48 +++++++- changelog/29473.txt | 3 + 4 files changed, 190 insertions(+), 7 deletions(-) create mode 100644 builtin/logical/pki/path_fetch_issuers_test.go create mode 100644 changelog/29473.txt diff --git a/builtin/logical/pki/path_fetch_issuers.go b/builtin/logical/pki/path_fetch_issuers.go index fa21443a59..4e8846524d 100644 --- a/builtin/logical/pki/path_fetch_issuers.go +++ b/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 + } + } + } } diff --git a/builtin/logical/pki/path_fetch_issuers_test.go b/builtin/logical/pki/path_fetch_issuers_test.go new file mode 100644 index 0000000000..8ceff25a49 --- /dev/null +++ b/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") +} diff --git a/builtin/logical/pki/path_issue_sign.go b/builtin/logical/pki/path_issue_sign.go index 7d12e39154..627b721fda 100644 --- a/builtin/logical/pki/path_issue_sign.go +++ b/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 +} diff --git a/changelog/29473.txt b/changelog/29473.txt new file mode 100644 index 0000000000..89fe47bd01 --- /dev/null +++ b/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. +```