diff --git a/.changelog/15253.txt b/.changelog/15253.txt new file mode 100644 index 0000000000..b0063ffb47 --- /dev/null +++ b/.changelog/15253.txt @@ -0,0 +1,3 @@ +```release-note:bug +connect: Fixed issue where using Vault 1.11+ as CA provider would eventually break Intermediate CAs [[GH-15217](https://github.com/hashicorp/consul/issues/15217)] +``` \ No newline at end of file diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index 03f8aaec3c..25eb076918 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -363,7 +363,8 @@ func (v *VaultProvider) GenerateIntermediateCSR() (string, error) { "cannot generate an intermediate CSR") } - return v.generateIntermediateCSR() + csr, _, err := v.generateIntermediateCSR() + return csr, err } func (v *VaultProvider) setupIntermediatePKIPath() error { @@ -406,11 +407,13 @@ func (v *VaultProvider) setupIntermediatePKIPath() error { return err } -func (v *VaultProvider) generateIntermediateCSR() (string, error) { +// generateIntermediateCSR returns the CSR and key_id (only present in +// Vault 1.11+) or any errors encountered. +func (v *VaultProvider) generateIntermediateCSR() (string, string, error) { // Generate a new intermediate CSR for the root to sign. uid, err := connect.CompactUID() if err != nil { - return "", err + return "", "", err } data, err := v.writeNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"intermediate/generate/internal", map[string]interface{}{ "common_name": connect.CACN("vault", uid, v.clusterID, v.isPrimary), @@ -419,17 +422,26 @@ func (v *VaultProvider) generateIntermediateCSR() (string, error) { "uri_sans": v.spiffeID.URI().String(), }) if err != nil { - return "", err + return "", "", err } if data == nil || data.Data["csr"] == "" { - return "", fmt.Errorf("got empty value when generating intermediate CSR") + return "", "", fmt.Errorf("got empty value when generating intermediate CSR") } csr, ok := data.Data["csr"].(string) if !ok { - return "", fmt.Errorf("csr result is not a string") + return "", "", fmt.Errorf("csr result is not a string") } - - return csr, nil + // Vault 1.11+ will return a "key_id" field which helps + // identify the correct issuer to set as default. + // https://github.com/hashicorp/vault/blob/e445c8b4f58dc20a0316a7fd1b5725b401c3b17a/builtin/logical/pki/path_intermediate.go#L154 + if rawkeyId, ok := data.Data["key_id"]; ok { + keyId, ok := rawkeyId.(string) + if !ok { + return "", "", fmt.Errorf("key_id is not a string") + } + return csr, keyId, nil + } + return csr, "", nil } // SetIntermediate writes the incoming intermediate and root certificates to the @@ -531,7 +543,7 @@ func (v *VaultProvider) getCAChain(namespace, path string) (string, error) { // necessary, then generates and signs a new CA CSR using the root PKI backend // and updates the intermediate backend to use that new certificate. func (v *VaultProvider) GenerateIntermediate() (string, error) { - csr, err := v.generateIntermediateCSR() + csr, keyId, err := v.generateIntermediateCSR() if err != nil { return "", err } @@ -551,16 +563,81 @@ func (v *VaultProvider) GenerateIntermediate() (string, error) { } // Set the intermediate backend to use the new certificate. - _, err = v.writeNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"intermediate/set-signed", map[string]interface{}{ + importResp, err := v.writeNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"intermediate/set-signed", map[string]interface{}{ "certificate": intermediate.Data["certificate"], }) if err != nil { return "", err } + // Vault 1.11+ will return a non-nil response from intermediate/set-signed + if importResp != nil { + err := v.setDefaultIntermediateIssuer(importResp, keyId) + if err != nil { + return "", fmt.Errorf("failed to update default intermediate issuer: %w", err) + } + } + return v.ActiveIntermediate() } +// setDefaultIntermediateIssuer updates the default issuer for +// intermediate CA since Vault, as part of its 1.11+ support for +// multiple issuers, no longer overwrites the default issuer when +// generateIntermediateCSR (intermediate/generate/internal) is called. +// +// The response we get from calling [/intermediate/set-signed] +// should contain a "mapping" data field we can use to cross-reference +// with the keyId returned when calling [/intermediate/generate/internal]. +// +// [/intermediate/set-signed]: https://developer.hashicorp.com/vault/api-docs/secret/pki#import-ca-certificates-and-keys +// [/intermediate/generate/internal]: https://developer.hashicorp.com/vault/api-docs/secret/pki#generate-intermediate-csr +func (v *VaultProvider) setDefaultIntermediateIssuer(vaultResp *vaultapi.Secret, keyId string) error { + if vaultResp.Data["mapping"] == nil { + return fmt.Errorf("expected Vault response data to have a 'mapping' key") + } + if keyId == "" { + return fmt.Errorf("expected non-empty keyId") + } + + mapping, ok := vaultResp.Data["mapping"].(map[string]any) + if !ok { + return fmt.Errorf("unexpected type for 'mapping' value in Vault response") + } + + var intermediateId string + // The value in this KV pair is called "key" + for issuer, key := range mapping { + if key == keyId { + // Expect to find the key_id we got from Vault when we + // generated the intermediate CSR. + intermediateId = issuer + break + } + } + if intermediateId == "" { + return fmt.Errorf("could not find key_id %q in response from vault", keyId) + } + + // For Vault 1.11+ it is important to GET then POST to avoid clobbering fields + // like `default_follows_latest_issuer`. + // https://developer.hashicorp.com/vault/api-docs/secret/pki#default_follows_latest_issuer + resp, err := v.readNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"config/issuers") + if err != nil { + return fmt.Errorf("could not read from /config/issuers: %w", err) + } + issuersConf := resp.Data + // Overwrite the default issuer + issuersConf["default"] = intermediateId + + _, err = v.writeNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"config/issuers", issuersConf) + if err != nil { + return fmt.Errorf("could not write default issuer to /config/issuers: %w", err) + } + + return nil +} + // Sign calls the configured role in the intermediate PKI backend to issue // a new leaf certificate based on the provided CSR, with the issuing // intermediate CA cert attached. diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index 6dbf671d4c..797084c2e4 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -902,6 +902,28 @@ func TestVaultProvider_ReconfigureIntermediateTTL(t *testing.T) { require.Equal(t, 333*3600, mountConfig.MaxLeaseTTL) } +func TestVaultCAProvider_GenerateIntermediate(t *testing.T) { + + SkipIfVaultNotPresent(t) + + provider, _ := testVaultProviderWithConfig(t, true, nil) + + orig, err := provider.ActiveIntermediate() + require.NoError(t, err) + + // This test was created to ensure that our calls to Vault + // returns a new Intermediate certificate and further calls + // to ActiveIntermediate return the same new cert. + new, err := provider.GenerateIntermediate() + require.NoError(t, err) + + newActive, err := provider.ActiveIntermediate() + require.NoError(t, err) + + require.Equal(t, new, newActive) + require.NotEqual(t, orig, new) +} + func getIntermediateCertTTL(t *testing.T, caConf *structs.CAConfiguration) time.Duration { t.Helper()