From 1ded025400bc48776e7941eee64cbe53be7b7892 Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Thu, 8 Sep 2022 00:26:34 -0700 Subject: [PATCH 1/2] connect/ca: Clarify behavior around IntermediateCertTTL in CA config --- agent/structs/connect_ca.go | 11 +++++++++-- .../partials/http_api_connect_ca_common_options.mdx | 9 +++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/agent/structs/connect_ca.go b/agent/structs/connect_ca.go index bc50b416e..b6be5c477 100644 --- a/agent/structs/connect_ca.go +++ b/agent/structs/connect_ca.go @@ -382,9 +382,12 @@ func (c *CAConfiguration) GetCommonConfig() (*CommonCAProviderConfig, error) { } type CommonCAProviderConfig struct { - LeafCertTTL time.Duration + LeafCertTTL time.Duration + RootCertTTL time.Duration + + // IntermediateCertTTL is only valid in the primary datacenter, and determines + // the duration that any signed intermediates are valid for. IntermediateCertTTL time.Duration - RootCertTTL time.Duration SkipValidate bool @@ -439,6 +442,10 @@ func (c CommonCAProviderConfig) Validate() error { return nil } + // todo(kyhavlov): should we output some kind of warning here (or in a Warnings() func) + // if the intermediate TTL is set in a secondary DC? allowing it to be set and do nothing + // seems bad. + // it's sufficient to check that the root cert ttl >= intermediate cert ttl // since intermediate cert ttl >= 3* leaf cert ttl; so root cert ttl >= 3 * leaf cert ttl > leaf cert ttl if c.RootCertTTL < c.IntermediateCertTTL { diff --git a/website/content/partials/http_api_connect_ca_common_options.mdx b/website/content/partials/http_api_connect_ca_common_options.mdx index 6ef48d5e5..b6d269bee 100644 --- a/website/content/partials/http_api_connect_ca_common_options.mdx +++ b/website/content/partials/http_api_connect_ca_common_options.mdx @@ -43,6 +43,15 @@ The following configuration options are supported by all CA providers: For the Vault provider, this value is only used if the backend is not initialized at first. +- `IntermediateCertTTL` / `intermediate_cert_ttl` (`duration: "8760h"`) The time to live (TTL) for + any intermediate certificates signed by root certificate of the primary datacenter. *This field is only + valid in the primary datacenter*. + Defaults to 1 year as `8760h`. + + This setting applies to all Consul CA providers. + + For the Vault provider, this value is only used if the backend is not initialized at first. + - `PrivateKeyType` / `private_key_type` (`string: "ec"`) - The type of key to generate for this CA. This is only used when the provider is generating a new key. If `private_key` is set for the Consul provider, or existing root or intermediate From 33e616987c497ddddb424c920f618a37b7ca09c7 Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Thu, 8 Sep 2022 01:11:19 -0700 Subject: [PATCH 2/2] Update intermediate pki mount/role when reconfiguring Vault provider --- .changelog/14516.txt | 3 + agent/connect/ca/provider_vault.go | 80 ++++++++++++------------- agent/connect/ca/provider_vault_test.go | 29 +++++++-- agent/structs/connect_ca.go | 4 -- 4 files changed, 68 insertions(+), 48 deletions(-) create mode 100644 .changelog/14516.txt diff --git a/.changelog/14516.txt b/.changelog/14516.txt new file mode 100644 index 000000000..9980ee20f --- /dev/null +++ b/.changelog/14516.txt @@ -0,0 +1,3 @@ +```release-note:bug +ca: Fixed a bug with the Vault CA provider where the intermediate PKI mount and leaf cert role were not being updated when the CA configuration was changed. +``` \ No newline at end of file diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index 270d53a01..fd57366bd 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -66,11 +66,10 @@ type VaultProvider struct { stopWatcher func() - isPrimary bool - clusterID string - spiffeID *connect.SpiffeIDSigning - setupIntermediatePKIPathDone bool - logger hclog.Logger + isPrimary bool + clusterID string + spiffeID *connect.SpiffeIDSigning + logger hclog.Logger } func NewVaultProvider(logger hclog.Logger) *VaultProvider { @@ -174,6 +173,11 @@ func (v *VaultProvider) Configure(cfg ProviderConfig) error { go v.renewToken(ctx, lifetimeWatcher) } + // Update the intermediate (managed) PKI mount and role + if err := v.setupIntermediatePKIPath(); err != nil { + return err + } + return nil } @@ -363,8 +367,8 @@ func (v *VaultProvider) GenerateIntermediateCSR() (string, error) { } func (v *VaultProvider) setupIntermediatePKIPath() error { - if v.setupIntermediatePKIPathDone { - return nil + mountConfig := vaultapi.MountConfigInput{ + MaxLeaseTTL: v.config.IntermediateCertTTL.String(), } _, err := v.getCA(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath) @@ -373,9 +377,7 @@ func (v *VaultProvider) setupIntermediatePKIPath() error { err := v.mountNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath, &vaultapi.MountInput{ Type: "pki", Description: "intermediate CA backend for Consul Connect", - Config: vaultapi.MountConfigInput{ - MaxLeaseTTL: v.config.IntermediateCertTTL.String(), - }, + Config: mountConfig, }) if err != nil { return err @@ -383,39 +385,28 @@ func (v *VaultProvider) setupIntermediatePKIPath() error { } else { return err } - } - - // Create the role for issuing leaf certs if it doesn't exist yet - rolePath := v.config.IntermediatePKIPath + "roles/" + VaultCALeafCertRole - role, err := v.readNamespaced(v.config.IntermediatePKINamespace, rolePath) - - if err != nil { - return err - } - if role == nil { - _, err := v.writeNamespaced(v.config.IntermediatePKINamespace, rolePath, map[string]interface{}{ - "allow_any_name": true, - "allowed_uri_sans": "spiffe://*", - "key_type": "any", - "max_ttl": v.config.LeafCertTTL.String(), - "no_store": true, - "require_cn": false, - }) - + } else { + err := v.tuneMountNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath, &mountConfig) if err != nil { return err } } - v.setupIntermediatePKIPathDone = true - return nil + + // Create the role for issuing leaf certs if it doesn't exist yet + rolePath := v.config.IntermediatePKIPath + "roles/" + VaultCALeafCertRole + _, err = v.writeNamespaced(v.config.IntermediatePKINamespace, rolePath, map[string]interface{}{ + "allow_any_name": true, + "allowed_uri_sans": "spiffe://*", + "key_type": "any", + "max_ttl": v.config.LeafCertTTL.String(), + "no_store": true, + "require_cn": false, + }) + + return err } func (v *VaultProvider) generateIntermediateCSR() (string, error) { - err := v.setupIntermediatePKIPath() - if err != nil { - return "", err - } - // Generate a new intermediate CSR for the root to sign. uid, err := connect.CompactUID() if err != nil { @@ -465,10 +456,6 @@ func (v *VaultProvider) SetIntermediate(intermediatePEM, rootPEM string) error { // ActiveIntermediate returns the current intermediate certificate. func (v *VaultProvider) ActiveIntermediate() (string, error) { - if err := v.setupIntermediatePKIPath(); err != nil { - return "", err - } - cert, err := v.getCA(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath) // This error is expected when calling initializeSecondaryCA for the @@ -737,6 +724,19 @@ func (v *VaultProvider) mountNamespaced(namespace, path string, mountInfo *vault return err } +func (v *VaultProvider) tuneMountNamespaced(namespace, path string, mountConfig *vaultapi.MountConfigInput) error { + defer v.setNamespace(namespace)() + r := v.client.NewRequest("POST", fmt.Sprintf("/v1/sys/mounts/%s/tune", path)) + if err := r.SetJSONBody(mountConfig); err != nil { + return err + } + resp, err := v.client.RawRequest(r) + if resp != nil { + defer resp.Body.Close() + } + return err +} + func (v *VaultProvider) unmountNamespaced(namespace, path string) error { defer v.setNamespace(namespace)() r := v.client.NewRequest("DELETE", fmt.Sprintf("/v1/sys/mounts/%s", path)) diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index 11689ae69..66b89ba9f 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -19,6 +19,16 @@ import ( "github.com/hashicorp/consul/sdk/testutil/retry" ) +const pkiTestPolicy = ` +path "sys/mounts/*" +{ + capabilities = ["create", "read", "update", "delete", "list", "sudo"] +} +path "pki-intermediate/*" +{ + capabilities = ["create", "read", "update", "delete", "list", "sudo"] +}` + func TestVaultCAProvider_ParseVaultCAConfig(t *testing.T) { cases := map[string]struct { rawConfig map[string]interface{} @@ -653,7 +663,7 @@ func TestVaultProvider_ConfigureWithAuthMethod(t *testing.T) { authMethodType: "userpass", configureAuthMethodFunc: func(t *testing.T, vaultClient *vaultapi.Client) map[string]interface{} { _, err := vaultClient.Logical().Write("/auth/userpass/users/test", - map[string]interface{}{"password": "foo", "policies": "admins"}) + map[string]interface{}{"password": "foo", "policies": "pki"}) require.NoError(t, err) return map[string]interface{}{ "Type": "userpass", @@ -667,7 +677,8 @@ func TestVaultProvider_ConfigureWithAuthMethod(t *testing.T) { { authMethodType: "approle", configureAuthMethodFunc: func(t *testing.T, vaultClient *vaultapi.Client) map[string]interface{} { - _, err := vaultClient.Logical().Write("auth/approle/role/my-role", nil) + _, err := vaultClient.Logical().Write("auth/approle/role/my-role", + map[string]interface{}{"token_policies": "pki"}) require.NoError(t, err) resp, err := vaultClient.Logical().Read("auth/approle/role/my-role/role-id") require.NoError(t, err) @@ -695,6 +706,9 @@ func TestVaultProvider_ConfigureWithAuthMethod(t *testing.T) { err := testVault.Client().Sys().EnableAuthWithOptions(c.authMethodType, &vaultapi.EnableAuthOptions{Type: c.authMethodType}) require.NoError(t, err) + err = testVault.Client().Sys().PutPolicy("pki", pkiTestPolicy) + require.NoError(t, err) + authMethodConf := c.configureAuthMethodFunc(t, testVault.Client()) conf := map[string]interface{}{ @@ -726,11 +740,18 @@ func TestVaultProvider_RotateAuthMethodToken(t *testing.T) { testVault := NewTestVaultServer(t) - err := testVault.Client().Sys().EnableAuthWithOptions("approle", &vaultapi.EnableAuthOptions{Type: "approle"}) + err := testVault.Client().Sys().PutPolicy("pki", pkiTestPolicy) + require.NoError(t, err) + + err = testVault.Client().Sys().EnableAuthWithOptions("approle", &vaultapi.EnableAuthOptions{Type: "approle"}) require.NoError(t, err) _, err = testVault.Client().Logical().Write("auth/approle/role/my-role", - map[string]interface{}{"token_ttl": "2s", "token_explicit_max_ttl": "2s"}) + map[string]interface{}{ + "token_ttl": "2s", + "token_explicit_max_ttl": "2s", + "token_policies": "pki", + }) require.NoError(t, err) resp, err := testVault.Client().Logical().Read("auth/approle/role/my-role/role-id") require.NoError(t, err) diff --git a/agent/structs/connect_ca.go b/agent/structs/connect_ca.go index b6be5c477..9f319ad09 100644 --- a/agent/structs/connect_ca.go +++ b/agent/structs/connect_ca.go @@ -442,10 +442,6 @@ func (c CommonCAProviderConfig) Validate() error { return nil } - // todo(kyhavlov): should we output some kind of warning here (or in a Warnings() func) - // if the intermediate TTL is set in a secondary DC? allowing it to be set and do nothing - // seems bad. - // it's sufficient to check that the root cert ttl >= intermediate cert ttl // since intermediate cert ttl >= 3* leaf cert ttl; so root cert ttl >= 3 * leaf cert ttl > leaf cert ttl if c.RootCertTTL < c.IntermediateCertTTL {