From 3a522d05c0ae591af9e4587dcd0791f963150a7f Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Mon, 21 Nov 2022 17:15:30 -0500 Subject: [PATCH 1/5] Fix Vault managed intermediate PKI bug --- agent/connect/ca/provider_vault.go | 51 +++++++++++++---- agent/connect/ca/provider_vault_test.go | 73 +++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 10 deletions(-) diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index 25eb07691815..ee9c20eb3804 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -70,6 +70,8 @@ type VaultProvider struct { clusterID string spiffeID *connect.SpiffeIDSigning logger hclog.Logger + + isConsulMountedIntermediate bool } func NewVaultProvider(logger hclog.Logger) *VaultProvider { @@ -310,9 +312,10 @@ func (v *VaultProvider) GenerateRoot() (RootResult, error) { }, }) if err != nil { - return RootResult{}, err + return RootResult{}, fmt.Errorf("failed to mount root CA backend") } + // We want to initialize afterwards fallthrough case ErrBackendNotInitialized: uid, err := connect.CompactUID() @@ -326,7 +329,7 @@ func (v *VaultProvider) GenerateRoot() (RootResult, error) { "key_bits": v.config.PrivateKeyBits, }) if err != nil { - return RootResult{}, err + return RootResult{}, fmt.Errorf("failed to initialize root CA: %w", err) } var ok bool rootPEM, ok = resp.Data["certificate"].(string) @@ -336,7 +339,7 @@ func (v *VaultProvider) GenerateRoot() (RootResult, error) { default: if err != nil { - return RootResult{}, err + return RootResult{}, fmt.Errorf("unexpected error while setting root PKI backend: %w", err) } } @@ -381,19 +384,47 @@ func (v *VaultProvider) setupIntermediatePKIPath() error { Config: mountConfig, }) if err != nil { - return err + return fmt.Errorf("failed to mount intermediate PKI backend: %w", err) } + // Required to determine if we should tune the mount + // if the VaultProvider is ever reconfigured. + v.isConsulMountedIntermediate = true + + } else if err == ErrBackendNotInitialized { + // If this is the first time calling setupIntermediatePKIPath, the backend + // will not have been initialized. Since the mount is ready we can suppress + // this error. } else { - return err + return fmt.Errorf("unexpected error while fetching intermediate CA: %w", err) } } else { - err := v.tuneMountNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath, &mountConfig) - if err != nil { - v.logger.Warn("Could not update intermediate PKI mount settings", "path", v.config.IntermediatePKIPath, "error", err) + // If Consul was responsible for mounting the intermediate PKI path + // we should update the mount with any new config. + if v.isConsulMountedIntermediate { + // This codepath requires the Vault policy: + // + // path "/sys/mounts//tune" { + // capabilities = [ "update" ] + // } + // + err := v.tuneMountNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath, &mountConfig) + if err != nil { + v.logger.Warn("Intermediate PKI path was mounted by Consul but could not be tuned", + "namespace", v.config.IntermediatePKINamespace, + "path", v.config.IntermediatePKIPath, + "error", err, + ) + } + + } else { + v.logger.Info("Found existing Intermediate PKI path mount", + "namespace", v.config.IntermediatePKINamespace, + "path", v.config.IntermediatePKIPath, + ) } } - // Create the role for issuing leaf certs if it doesn't exist yet + // Create the role for issuing leaf certs rolePath := v.config.IntermediatePKIPath + "roles/" + VaultCALeafCertRole _, err = v.writeNamespaced(v.config.IntermediatePKINamespace, rolePath, map[string]interface{}{ "allow_any_name": true, @@ -710,7 +741,7 @@ func (v *VaultProvider) SignIntermediate(csr *x509.CertificateRequest) (string, func (v *VaultProvider) CrossSignCA(cert *x509.Certificate) (string, error) { rootPEM, err := v.getCA(v.config.RootPKINamespace, v.config.RootPKIPath) if err != nil { - return "", err + return "", fmt.Errorf("failed to get root CA: %w", err) } rootCert, err := connect.ParseCert(rootPEM) if err != nil { diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index 797084c2e4f6..716524cfdf3e 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -924,6 +924,79 @@ func TestVaultCAProvider_GenerateIntermediate(t *testing.T) { require.NotEqual(t, orig, new) } +func TestVaultCAProvider_VaultManaged(t *testing.T) { + const vaultManagedPKIPolicy = ` +path "/pki-root/" { + capabilities = [ "read" ] +} + +path "/pki-root/root/sign-intermediate" { + capabilities = [ "update" ] +} + +path "/pki-intermediate/*" { + capabilities = [ "create", "read", "update", "delete", "list" ] +} + +path "auth/token/renew-self" { + capabilities = [ "update" ] +} + +path "auth/token/lookup-self" { + capabilities = [ "read" ] +} +` + + testVault, err := runTestVault(t) + if err != nil { + t.Fatalf("err: %v", err) + } + + testVault.WaitUntilReady(t) + + client := testVault.Client() + + client.SetToken("root") + + // Mount pki root externally + require.NoError(t, client.Sys().Mount("pki-root", &vaultapi.MountInput{ + Type: "pki", + Description: "root CA backend for Consul Connect", + Config: vaultapi.MountConfigInput{ + MaxLeaseTTL: "12m", + }, + })) + _, err = client.Logical().Write("pki-root/root/generate/internal", map[string]interface{}{ + "common_name": "testconsul", + }) + require.NoError(t, err) + + // Mount pki intermediate externally + require.NoError(t, client.Sys().Mount("pki-intermediate", &vaultapi.MountInput{ + Type: "pki", + Description: "intermediate CA backend for Consul Connect", + Config: vaultapi.MountConfigInput{ + MaxLeaseTTL: "6m", + }, + })) + + // Generate a policy and token for the VaultProvider to use + require.NoError(t, client.Sys().PutPolicy("consul-ca", vaultManagedPKIPolicy)) + tcr := &vaultapi.TokenCreateRequest{ + Policies: []string{"consul-ca"}, + } + secret, err := testVault.client.Auth().Token().Create(tcr) + require.NoError(t, err) + providerToken := secret.Auth.ClientToken + + // We want to test the provider.Configure() step + _, err = createVaultProvider(t, true, testVault.Addr, providerToken, nil) + if err != nil { + testVault.Stop() + t.Fatalf("err: %v", err) + } +} + func getIntermediateCertTTL(t *testing.T, caConf *structs.CAConfiguration) time.Duration { t.Helper() From 122cd9fd7cd1a84984641bedea68440b1d5ed14c Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Mon, 21 Nov 2022 17:15:30 -0500 Subject: [PATCH 2/5] Fix Vault managed intermediate PKI bug --- agent/connect/ca/provider_vault.go | 2 +- agent/connect/ca/provider_vault_test.go | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index ee9c20eb3804..6f4d2d57ce3f 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -312,7 +312,7 @@ func (v *VaultProvider) GenerateRoot() (RootResult, error) { }, }) if err != nil { - return RootResult{}, fmt.Errorf("failed to mount root CA backend") + return RootResult{}, fmt.Errorf("failed to mount root CA backend: %w", err) } // We want to initialize afterwards diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index 716524cfdf3e..86c6417b2811 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -991,10 +991,7 @@ path "auth/token/lookup-self" { // We want to test the provider.Configure() step _, err = createVaultProvider(t, true, testVault.Addr, providerToken, nil) - if err != nil { - testVault.Stop() - t.Fatalf("err: %v", err) - } + require.NoError(t, err) } func getIntermediateCertTTL(t *testing.T, caConf *structs.CAConfiguration) time.Duration { From dfca9caf946e6961688df57790531a1c43ffecd1 Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Tue, 22 Nov 2022 09:43:10 -0500 Subject: [PATCH 3/5] Add test for Consul-managed --- agent/connect/ca/provider_vault_test.go | 35 +++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index 86c6417b2811..9fbe9f337d19 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -925,6 +925,9 @@ func TestVaultCAProvider_GenerateIntermediate(t *testing.T) { } func TestVaultCAProvider_VaultManaged(t *testing.T) { + + SkipIfVaultNotPresent(t) + const vaultManagedPKIPolicy = ` path "/pki-root/" { capabilities = [ "read" ] @@ -994,6 +997,38 @@ path "auth/token/lookup-self" { require.NoError(t, err) } +func TestVaultCAProvider_ConsulManaged(t *testing.T) { + + SkipIfVaultNotPresent(t) + + testVault, err := runTestVault(t) + if err != nil { + t.Fatalf("err: %v", err) + } + + testVault.WaitUntilReady(t) + + client := testVault.Client() + + client.SetToken("root") + + // We do not configure any mounts and instead let Consul + // be responsible for mounting root and intermediate PKI + + // Generate a policy and token for the VaultProvider to use + require.NoError(t, client.Sys().PutPolicy("consul-ca", pkiTestPolicy)) + tcr := &vaultapi.TokenCreateRequest{ + Policies: []string{"consul-ca"}, + } + secret, err := testVault.client.Auth().Token().Create(tcr) + require.NoError(t, err) + providerToken := secret.Auth.ClientToken + + // We want to test the provider.Configure() step + _, err = createVaultProvider(t, true, testVault.Addr, providerToken, nil) + require.NoError(t, err) +} + func getIntermediateCertTTL(t *testing.T, caConf *structs.CAConfiguration) time.Duration { t.Helper() From bfe437464207ba6d47d36ed18b1d8092c2e73b6b Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Wed, 23 Nov 2022 17:25:10 -0500 Subject: [PATCH 4/5] Add changelog --- .changelog/15525.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/15525.txt diff --git a/.changelog/15525.txt b/.changelog/15525.txt new file mode 100644 index 000000000000..d920109e6493 --- /dev/null +++ b/.changelog/15525.txt @@ -0,0 +1,3 @@ +```release-note:bug +ca: Fixed issue where using Vault as Connect CA with Vault-managed policies would error on start-up if the intermediate PKI mount existed but was empty +``` \ No newline at end of file From 227762eb0a2669dab10b806ba0b03c758244de28 Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Mon, 28 Nov 2022 14:54:05 -0500 Subject: [PATCH 5/5] Tune always but log conditionally --- agent/connect/ca/provider_vault.go | 41 ++++++++++++++++++------------ 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index 6f4d2d57ce3f..48eacb9da3d7 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -71,6 +71,11 @@ type VaultProvider struct { spiffeID *connect.SpiffeIDSigning logger hclog.Logger + // isConsulMountedIntermediate is used to determine if we should tune the + // mount if the VaultProvider is ever reconfigured. This is at most a + // "best guess" to determine whether this instance of Consul created the + // intermediate mount but will not be able to tell if an existing mount + // was created by Consul (in a previous running instance) or was external. isConsulMountedIntermediate bool } @@ -398,29 +403,33 @@ func (v *VaultProvider) setupIntermediatePKIPath() error { return fmt.Errorf("unexpected error while fetching intermediate CA: %w", err) } } else { - // If Consul was responsible for mounting the intermediate PKI path - // we should update the mount with any new config. - if v.isConsulMountedIntermediate { - // This codepath requires the Vault policy: - // - // path "/sys/mounts//tune" { - // capabilities = [ "update" ] - // } - // - err := v.tuneMountNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath, &mountConfig) - if err != nil { + v.logger.Info("Found existing Intermediate PKI path mount", + "namespace", v.config.IntermediatePKINamespace, + "path", v.config.IntermediatePKIPath, + ) + + // This codepath requires the Vault policy: + // + // path "/sys/mounts//tune" { + // capabilities = [ "update" ] + // } + // + err := v.tuneMountNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath, &mountConfig) + if err != nil { + if v.isConsulMountedIntermediate { v.logger.Warn("Intermediate PKI path was mounted by Consul but could not be tuned", "namespace", v.config.IntermediatePKINamespace, "path", v.config.IntermediatePKIPath, "error", err, ) + } else { + v.logger.Debug("Failed to tune Intermediate PKI mount. 403 Forbidden is expected if Consul does not have tune capabilities for the Intermediate PKI mount (i.e. using Vault-managed policies)", + "namespace", v.config.IntermediatePKINamespace, + "path", v.config.IntermediatePKIPath, + "error", err, + ) } - } else { - v.logger.Info("Found existing Intermediate PKI path mount", - "namespace", v.config.IntermediatePKINamespace, - "path", v.config.IntermediatePKIPath, - ) } }