From 6b5c266a20e09616a7c4a4a0f905e470ac6b883e Mon Sep 17 00:00:00 2001 From: hc-github-team-consul-core Date: Mon, 17 Jul 2023 13:46:33 -0500 Subject: [PATCH] Backport of Fix bug with Vault CA provider into release/1.16.x (#18161) * backport of commit 57bb6f3d729e4d76d1043efa2fa6a46137398d32 * backport of commit b2dad880653285a975795e89b0d77a6ea2fa60f1 * backport of commit 753d3c0d3f4797b6cf2d3490df996dffa8e885de --------- Co-authored-by: Chris S. Kim --- .changelog/18112.txt | 3 + agent/connect/ca/mock_Provider.go | 48 +++++----- agent/connect/ca/provider.go | 9 +- agent/connect/ca/provider_aws.go | 10 +-- agent/connect/ca/provider_aws_test.go | 18 ++-- agent/connect/ca/provider_consul.go | 18 ++-- agent/connect/ca/provider_consul_test.go | 16 ++-- agent/connect/ca/provider_vault.go | 31 +++---- agent/connect/ca/provider_vault_test.go | 25 +++--- agent/consul/leader_connect_ca.go | 70 +++++++++++---- agent/consul/leader_connect_ca_test.go | 109 ++++++++++++++++++----- agent/consul/leader_connect_test.go | 10 ++- 12 files changed, 229 insertions(+), 138 deletions(-) create mode 100644 .changelog/18112.txt diff --git a/.changelog/18112.txt b/.changelog/18112.txt new file mode 100644 index 000000000..ddd37786f --- /dev/null +++ b/.changelog/18112.txt @@ -0,0 +1,3 @@ +```release-note:bug +ca: Fixes a Vault CA provider bug where updating RootPKIPath but not IntermediatePKIPath would not renew leaf signing certificates +``` diff --git a/agent/connect/ca/mock_Provider.go b/agent/connect/ca/mock_Provider.go index 0c9725f5e..0a745ad9b 100644 --- a/agent/connect/ca/mock_Provider.go +++ b/agent/connect/ca/mock_Provider.go @@ -89,6 +89,30 @@ func (_m *MockProvider) CrossSignCA(_a0 *x509.Certificate) (string, error) { return r0, r1 } +// GenerateCAChain provides a mock function with given fields: +func (_m *MockProvider) GenerateCAChain() (string, error) { + ret := _m.Called() + + var r0 string + var r1 error + if rf, ok := ret.Get(0).(func() (string, error)); ok { + return rf() + } + if rf, ok := ret.Get(0).(func() string); ok { + r0 = rf() + } else { + r0 = ret.Get(0).(string) + } + + if rf, ok := ret.Get(1).(func() error); ok { + r1 = rf() + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // GenerateIntermediateCSR provides a mock function with given fields: func (_m *MockProvider) GenerateIntermediateCSR() (string, string, error) { ret := _m.Called() @@ -120,30 +144,6 @@ func (_m *MockProvider) GenerateIntermediateCSR() (string, string, error) { return r0, r1, r2 } -// GenerateCAChain provides a mock function with given fields: -func (_m *MockProvider) GenerateCAChain() (CAChainResult, error) { - ret := _m.Called() - - var r0 CAChainResult - var r1 error - if rf, ok := ret.Get(0).(func() (CAChainResult, error)); ok { - return rf() - } - if rf, ok := ret.Get(0).(func() CAChainResult); ok { - r0 = rf() - } else { - r0 = ret.Get(0).(CAChainResult) - } - - if rf, ok := ret.Get(1).(func() error); ok { - r1 = rf() - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - // SetIntermediate provides a mock function with given fields: intermediatePEM, rootPEM, opaque func (_m *MockProvider) SetIntermediate(intermediatePEM string, rootPEM string, opaque string) error { ret := _m.Called(intermediatePEM, rootPEM, opaque) diff --git a/agent/connect/ca/provider.go b/agent/connect/ca/provider.go index 25d0deae3..2ef34228b 100644 --- a/agent/connect/ca/provider.go +++ b/agent/connect/ca/provider.go @@ -135,9 +135,12 @@ type PrimaryProvider interface { // provider. // // Depending on the provider and its configuration, GenerateCAChain may return - // a single root certificate or a chain of certs. The provider should return an - // existing CA chain if one exists or generate a new one and return it. - GenerateCAChain() (CAChainResult, error) + // a single root certificate or a chain of certs. + // The first certificate must be the primary CA used to sign intermediates for + // secondary datacenters, and the last certificate must be the trusted CA. + // The provider should return an existing CA chain if one exists or generate a + // new one and return it. + GenerateCAChain() (string, error) // SignIntermediate will validate the CSR to ensure the trust domain in the // URI SAN matches the local one and that basic constraints for a CA diff --git a/agent/connect/ca/provider_aws.go b/agent/connect/ca/provider_aws.go index b7808edf9..d45f3295a 100644 --- a/agent/connect/ca/provider_aws.go +++ b/agent/connect/ca/provider_aws.go @@ -140,19 +140,19 @@ func (a *AWSProvider) State() (map[string]string, error) { } // GenerateCAChain implements Provider -func (a *AWSProvider) GenerateCAChain() (CAChainResult, error) { +func (a *AWSProvider) GenerateCAChain() (string, error) { if !a.isPrimary { - return CAChainResult{}, fmt.Errorf("provider is not the root certificate authority") + return "", fmt.Errorf("provider is not the root certificate authority") } if err := a.ensureCA(); err != nil { - return CAChainResult{}, err + return "", err } if a.rootPEM == "" { - return CAChainResult{}, fmt.Errorf("AWS CA provider not fully Initialized") + return "", fmt.Errorf("AWS CA provider not fully Initialized") } - return CAChainResult{PEM: a.rootPEM}, nil + return a.rootPEM, nil } // ensureCA loads the CA resource to check it exists if configured by User or in diff --git a/agent/connect/ca/provider_aws_test.go b/agent/connect/ca/provider_aws_test.go index 8c6a1f679..cba2897fa 100644 --- a/agent/connect/ca/provider_aws_test.go +++ b/agent/connect/ca/provider_aws_test.go @@ -49,9 +49,8 @@ func TestAWSBootstrapAndSignPrimary(t *testing.T) { provider := testAWSProvider(t, testProviderConfigPrimary(t, cfg)) defer provider.Cleanup(true, nil) - root, err := provider.GenerateCAChain() + rootPEM, err := provider.GenerateCAChain() require.NoError(t, err) - rootPEM := root.PEM // Ensure they use the right key type rootCert, err := connect.ParseCert(rootPEM) @@ -76,9 +75,8 @@ func TestAWSBootstrapAndSignPrimary(t *testing.T) { provider := testAWSProvider(t, testProviderConfigPrimary(t, nil)) defer provider.Cleanup(true, nil) - root, err := provider.GenerateCAChain() + rootPEM, err := provider.GenerateCAChain() require.NoError(t, err) - rootPEM := root.PEM // Ensure they use the right key type rootCert, err := connect.ParseCert(rootPEM) @@ -111,9 +109,8 @@ func TestAWSBootstrapAndSignSecondary(t *testing.T) { p1 := testAWSProvider(t, testProviderConfigPrimary(t, nil)) defer p1.Cleanup(true, nil) - root, err := p1.GenerateCAChain() + rootPEM, err := p1.GenerateCAChain() require.NoError(t, err) - rootPEM := root.PEM p2 := testAWSProvider(t, testProviderConfigSecondary(t, nil)) defer p2.Cleanup(true, nil) @@ -140,9 +137,8 @@ func TestAWSBootstrapAndSignSecondary(t *testing.T) { cfg1 := testProviderConfigPrimary(t, nil) cfg1.State = p1State p1 = testAWSProvider(t, cfg1) - root, err := p1.GenerateCAChain() + newRootPEM, err := p1.GenerateCAChain() require.NoError(t, err) - newRootPEM := root.PEM cfg2 := testProviderConfigPrimary(t, nil) cfg2.State = p2State @@ -174,9 +170,8 @@ func TestAWSBootstrapAndSignSecondary(t *testing.T) { "ExistingARN": p1State[AWSStateCAARNKey], }) p1 = testAWSProvider(t, cfg1) - root, err := p1.GenerateCAChain() + newRootPEM, err := p1.GenerateCAChain() require.NoError(t, err) - newRootPEM := root.PEM cfg2 := testProviderConfigPrimary(t, map[string]interface{}{ "ExistingARN": p2State[AWSStateCAARNKey], @@ -213,9 +208,8 @@ func TestAWSBootstrapAndSignSecondary(t *testing.T) { p2 = testAWSProvider(t, cfg2) require.NoError(t, p2.SetIntermediate(newIntPEM, newRootPEM, "")) - root, err = p1.GenerateCAChain() + newRootPEM, err = p1.GenerateCAChain() require.NoError(t, err) - newRootPEM = root.PEM newIntPEM, err = p2.ActiveLeafSigningCert() require.NoError(t, err) diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go index b35800c6d..01c4987e0 100644 --- a/agent/connect/ca/provider_consul.go +++ b/agent/connect/ca/provider_consul.go @@ -155,17 +155,17 @@ func (c *ConsulProvider) State() (map[string]string, error) { } // GenerateCAChain initializes a new root certificate and private key if needed. -func (c *ConsulProvider) GenerateCAChain() (CAChainResult, error) { +func (c *ConsulProvider) GenerateCAChain() (string, error) { providerState, err := c.getState() if err != nil { - return CAChainResult{}, err + return "", err } if !c.isPrimary { - return CAChainResult{}, fmt.Errorf("provider is not the root certificate authority") + return "", fmt.Errorf("provider is not the root certificate authority") } if providerState.RootCert != "" { - return CAChainResult{PEM: providerState.RootCert}, nil + return providerState.RootCert, nil } // Generate a private key if needed @@ -173,7 +173,7 @@ func (c *ConsulProvider) GenerateCAChain() (CAChainResult, error) { if c.config.PrivateKey == "" { _, pk, err := connect.GeneratePrivateKeyWithConfig(c.config.PrivateKeyType, c.config.PrivateKeyBits) if err != nil { - return CAChainResult{}, err + return "", err } newState.PrivateKey = pk } else { @@ -184,12 +184,12 @@ func (c *ConsulProvider) GenerateCAChain() (CAChainResult, error) { if c.config.RootCert == "" { nextSerial, err := c.incrementAndGetNextSerialNumber() if err != nil { - return CAChainResult{}, fmt.Errorf("error computing next serial number: %v", err) + return "", fmt.Errorf("error computing next serial number: %v", err) } ca, err := c.generateCA(newState.PrivateKey, nextSerial, c.config.RootCertTTL) if err != nil { - return CAChainResult{}, fmt.Errorf("error generating CA: %v", err) + return "", fmt.Errorf("error generating CA: %v", err) } newState.RootCert = ca } else { @@ -202,10 +202,10 @@ func (c *ConsulProvider) GenerateCAChain() (CAChainResult, error) { ProviderState: &newState, } if _, err := c.Delegate.ApplyCARequest(args); err != nil { - return CAChainResult{}, err + return "", err } - return CAChainResult{PEM: newState.RootCert}, nil + return newState.RootCert, nil } // GenerateIntermediateCSR creates a private key and generates a CSR diff --git a/agent/connect/ca/provider_consul_test.go b/agent/connect/ca/provider_consul_test.go index fd521a5bb..0c6959c7f 100644 --- a/agent/connect/ca/provider_consul_test.go +++ b/agent/connect/ca/provider_consul_test.go @@ -93,10 +93,10 @@ func TestConsulCAProvider_Bootstrap(t *testing.T) { // Intermediate should be the same cert. inter, err := provider.ActiveLeafSigningCert() require.NoError(t, err) - require.Equal(t, root.PEM, inter) + require.Equal(t, root, inter) // Should be a valid cert - parsed, err := connect.ParseCert(root.PEM) + parsed, err := connect.ParseCert(root) require.NoError(t, err) require.Equal(t, parsed.URIs[0].String(), fmt.Sprintf("spiffe://%s.consul", conf.ClusterID)) requireNotEncoded(t, parsed.SubjectKeyId) @@ -128,10 +128,10 @@ func TestConsulCAProvider_Bootstrap_WithCert(t *testing.T) { root, err := provider.GenerateCAChain() require.NoError(t, err) - require.Equal(t, root.PEM, rootCA.RootCert) + require.Equal(t, root, rootCA.RootCert) // Should be a valid cert - parsed, err := connect.ParseCert(root.PEM) + parsed, err := connect.ParseCert(root) require.NoError(t, err) // test that the default root cert ttl was not applied to the provided cert @@ -298,7 +298,7 @@ func testCrossSignProviders(t *testing.T, provider1, provider2 Provider) { root, err := provider2.GenerateCAChain() require.NoError(t, err) - newRoot, err := connect.ParseCert(root.PEM) + newRoot, err := connect.ParseCert(root) require.NoError(t, err) oldSubject := newRoot.Subject.CommonName requireNotEncoded(t, newRoot.SubjectKeyId) @@ -321,7 +321,7 @@ func testCrossSignProviders(t *testing.T, provider1, provider2 Provider) { p1Root, err := provider1.GenerateCAChain() require.NoError(t, err) - oldRoot, err := connect.ParseCert(p1Root.PEM) + oldRoot, err := connect.ParseCert(p1Root) require.NoError(t, err) requireNotEncoded(t, oldRoot.SubjectKeyId) requireNotEncoded(t, oldRoot.AuthorityKeyId) @@ -385,7 +385,7 @@ func testCrossSignProvidersShouldFail(t *testing.T, provider1, provider2 Provide root, err := provider2.GenerateCAChain() require.NoError(t, err) - newRoot, err := connect.ParseCert(root.PEM) + newRoot, err := connect.ParseCert(root) require.NoError(t, err) requireNotEncoded(t, newRoot.SubjectKeyId) requireNotEncoded(t, newRoot.AuthorityKeyId) @@ -454,7 +454,7 @@ func testSignIntermediateCrossDC(t *testing.T, provider1, provider2 Provider) { require.NoError(t, err) root, err := provider1.GenerateCAChain() require.NoError(t, err) - rootPEM := root.PEM + rootPEM := root // Give the new intermediate to provider2 to use. require.NoError(t, provider2.SetIntermediate(intermediatePEM, rootPEM, opaque)) diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index 00a598d92..d48a38ce3 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -280,9 +280,9 @@ func (v *VaultProvider) State() (map[string]string, error) { } // GenerateCAChain mounts and initializes a new root PKI backend if needed. -func (v *VaultProvider) GenerateCAChain() (CAChainResult, error) { +func (v *VaultProvider) GenerateCAChain() (string, error) { if !v.isPrimary { - return CAChainResult{}, fmt.Errorf("provider is not the root certificate authority") + return "", fmt.Errorf("provider is not the root certificate authority") } // Set up the root PKI backend if necessary. @@ -302,7 +302,7 @@ func (v *VaultProvider) GenerateCAChain() (CAChainResult, error) { }, }) if err != nil { - return CAChainResult{}, fmt.Errorf("failed to mount root CA backend: %w", err) + return "", fmt.Errorf("failed to mount root CA backend: %w", err) } // We want to initialize afterwards @@ -310,7 +310,7 @@ func (v *VaultProvider) GenerateCAChain() (CAChainResult, error) { case ErrBackendNotInitialized: uid, err := connect.CompactUID() if err != nil { - return CAChainResult{}, err + return "", err } resp, err := v.writeNamespaced(v.config.RootPKINamespace, v.config.RootPKIPath+"root/generate/internal", map[string]interface{}{ "common_name": connect.CACN("vault", uid, v.clusterID, v.isPrimary), @@ -319,23 +319,23 @@ func (v *VaultProvider) GenerateCAChain() (CAChainResult, error) { "key_bits": v.config.PrivateKeyBits, }) if err != nil { - return CAChainResult{}, fmt.Errorf("failed to initialize root CA: %w", err) + return "", fmt.Errorf("failed to initialize root CA: %w", err) } var ok bool rootPEM, ok = resp.Data["certificate"].(string) if !ok { - return CAChainResult{}, fmt.Errorf("unexpected response from Vault: %v", resp.Data["certificate"]) + return "", fmt.Errorf("unexpected response from Vault: %v", resp.Data["certificate"]) } default: if err != nil { - return CAChainResult{}, fmt.Errorf("unexpected error while setting root PKI backend: %w", err) + return "", fmt.Errorf("unexpected error while setting root PKI backend: %w", err) } } rootChain, err := v.getCAChain(v.config.RootPKINamespace, v.config.RootPKIPath) if err != nil { - return CAChainResult{}, err + return "", err } // Workaround for a bug in the Vault PKI API. @@ -344,18 +344,7 @@ func (v *VaultProvider) GenerateCAChain() (CAChainResult, error) { rootChain = rootPEM } - intermediate, err := v.ActiveLeafSigningCert() - if err != nil { - return CAChainResult{}, fmt.Errorf("error fetching active intermediate: %w", err) - } - if intermediate == "" { - intermediate, err = v.GenerateLeafSigningCert() - if err != nil { - return CAChainResult{}, fmt.Errorf("error generating intermediate: %w", err) - } - } - - return CAChainResult{PEM: rootChain, IntermediatePEM: intermediate}, nil + return rootChain, nil } // GenerateIntermediateCSR creates a private key and generates a CSR @@ -582,7 +571,7 @@ func (v *VaultProvider) getCAChain(namespace, path string) (string, error) { return root, nil } -// GenerateIntermediate mounts the configured intermediate PKI backend if +// GenerateLeafSigningCert mounts the configured intermediate PKI backend if // 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) GenerateLeafSigningCert() (string, error) { diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index b0e341fe9..e75d02984 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -408,7 +408,7 @@ func TestVaultCAProvider_Bootstrap(t *testing.T) { }, certFunc: func(provider *VaultProvider) (string, error) { root, err := provider.GenerateCAChain() - return root.PEM, err + return root, err }, backendPath: "pki-root/", rootCaCreation: true, @@ -485,9 +485,8 @@ func TestVaultCAProvider_SignLeaf(t *testing.T) { Service: "foo", } - root, err := provider.GenerateCAChain() + rootPEM, err := provider.GenerateCAChain() require.NoError(t, err) - rootPEM := root.PEM assertCorrectKeyType(t, tc.KeyType, rootPEM) intPEM, err := provider.ActiveLeafSigningCert() @@ -588,9 +587,9 @@ func TestVaultCAProvider_CrossSignCA(t *testing.T) { }) testutil.RunStep(t, "init", func(t *testing.T) { - root, err := provider1.GenerateCAChain() + rootPEM, err := provider1.GenerateCAChain() require.NoError(t, err) - assertCorrectKeyType(t, tc.SigningKeyType, root.PEM) + assertCorrectKeyType(t, tc.SigningKeyType, rootPEM) intPEM, err := provider1.ActiveLeafSigningCert() require.NoError(t, err) @@ -616,9 +615,9 @@ func TestVaultCAProvider_CrossSignCA(t *testing.T) { }) testutil.RunStep(t, "swap", func(t *testing.T) { - root, err := provider2.GenerateCAChain() + rootPEM, err := provider2.GenerateCAChain() require.NoError(t, err) - assertCorrectKeyType(t, tc.CSRKeyType, root.PEM) + assertCorrectKeyType(t, tc.CSRKeyType, rootPEM) intPEM, err := provider2.ActiveLeafSigningCert() require.NoError(t, err) @@ -1135,14 +1134,14 @@ func TestVaultCAProvider_GenerateIntermediate(t *testing.T) { // This test was created to ensure that our calls to Vault // returns a new Intermediate certificate and further calls // to ActiveLeafSigningCert return the same new cert. - new, err := provider.GenerateLeafSigningCert() + newLeaf, err := provider.GenerateLeafSigningCert() require.NoError(t, err) newActive, err := provider.ActiveLeafSigningCert() require.NoError(t, err) - require.Equal(t, new, newActive) - require.NotEqual(t, orig, new) + require.Equal(t, newLeaf, newActive) + require.NotEqual(t, orig, newLeaf) } func TestVaultCAProvider_AutoTidyExpiredIssuers(t *testing.T) { @@ -1228,9 +1227,8 @@ func TestVaultCAProvider_GenerateIntermediate_inSecondary(t *testing.T) { // Sign the CSR with primaryProvider. intermediatePEM, err := primaryProvider.SignIntermediate(csr) require.NoError(t, err) - root, err := primaryProvider.GenerateCAChain() + rootPEM, err := primaryProvider.GenerateCAChain() require.NoError(t, err) - rootPEM := root.PEM // Give the new intermediate to provider to use. require.NoError(t, provider.SetIntermediate(intermediatePEM, rootPEM, issuerID)) @@ -1249,9 +1247,8 @@ func TestVaultCAProvider_GenerateIntermediate_inSecondary(t *testing.T) { // Sign the CSR with primaryProvider. intermediatePEM, err := primaryProvider.SignIntermediate(csr) require.NoError(t, err) - root, err := primaryProvider.GenerateCAChain() + rootPEM, err := primaryProvider.GenerateCAChain() require.NoError(t, err) - rootPEM := root.PEM // Give the new intermediate to provider to use. require.NoError(t, provider.SetIntermediate(intermediatePEM, rootPEM, issuerID)) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index c8c63c887..717c9ff0b 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -258,8 +258,8 @@ func (c *CAManager) initializeCAConfig() (*structs.CAConfiguration, error) { } // newCARoot returns a filled-in structs.CARoot from a raw PEM value. -func newCARoot(rootResult ca.CAChainResult, provider, clusterID string) (*structs.CARoot, error) { - primaryCert, err := connect.ParseCert(rootResult.PEM) +func newCARoot(caPem, provider, clusterID string) (*structs.CARoot, error) { + primaryCert, err := connect.ParseCert(caPem) if err != nil { return nil, err } @@ -275,17 +275,12 @@ func newCARoot(rootResult ca.CAChainResult, provider, clusterID string) (*struct ExternalTrustDomain: clusterID, NotBefore: primaryCert.NotBefore, NotAfter: primaryCert.NotAfter, - RootCert: lib.EnsureTrailingNewline(rootResult.PEM), + RootCert: lib.EnsureTrailingNewline(caPem), PrivateKeyType: keyType, PrivateKeyBits: keyBits, Active: true, } - if rootResult.IntermediatePEM == "" { - return caRoot, nil - } - if err := setLeafSigningCert(caRoot, rootResult.IntermediatePEM); err != nil { - return nil, fmt.Errorf("error setting leaf signing cert: %w", err) - } + return caRoot, nil } @@ -518,6 +513,19 @@ func (c *CAManager) primaryInitialize(provider ca.Provider, conf *structs.CAConf return err } + // provider may use intermediates for leaf signing in which case + // we need to generate a leaf signing CA. + if usesIntermediate, ok := provider.(ca.PrimaryUsesIntermediate); ok { + leafPem, err := usesIntermediate.GenerateLeafSigningCert() + if err != nil { + return fmt.Errorf("error generating new leaf signing cert: %w", err) + } + + if err := setLeafSigningCert(rootCA, leafPem); err != nil { + return fmt.Errorf("error setting leaf signing cert: %w", err) + } + } + var rootUpdateRequired bool if len(rootCA.IntermediateCerts) > 0 { rootUpdateRequired = true @@ -764,7 +772,6 @@ func (c *CAManager) UpdateConfiguration(args *structs.CARequest) (reterr error) return err } - // Exit early if it's a no-op change state := c.delegate.State() _, config, err := state.CAConfig(nil) if err != nil { @@ -780,6 +787,8 @@ func (c *CAManager) UpdateConfiguration(args *structs.CARequest) (reterr error) // Don't allow users to change the ClusterID. args.Config.ClusterID = config.ClusterID + + // Exit early if it's a no-op change if args.Config.Provider == config.Provider && reflect.DeepEqual(args.Config.Config, config.Config) { return nil } @@ -866,26 +875,53 @@ func (c *CAManager) primaryUpdateRootCA(newProvider ca.Provider, args *structs.C } args.Config.State = pState - providerRoot, err := newProvider.GenerateCAChain() + caPEM, err := newProvider.GenerateCAChain() if err != nil { return fmt.Errorf("error generating CA root certificate: %v", err) } - newRootPEM := providerRoot.PEM - newActiveRoot, err := newCARoot(providerRoot, args.Config.Provider, args.Config.ClusterID) + newActiveRoot, err := newCARoot(caPEM, args.Config.Provider, args.Config.ClusterID) if err != nil { return err } + // Fetch the existing root CA to compare with the current one. state := c.delegate.State() - // Compare the new provider's root CA ID to the current one. If they - // match, just update the existing provider with the new config. - // If they don't match, begin the root rotation process. _, root, err := state.CARootActive(nil) if err != nil { return err } + // provider may use intermediates for leaf signing in which case + // we may need to generate a leaf signing CA if the root has changed. + if usesIntermediate, ok := newProvider.(ca.PrimaryUsesIntermediate); ok { + var leafPemFunc func() (string, error) + if root != nil && root.ID == newActiveRoot.ID { + // If Root ID is the same, we can reuse the existing leaf signing cert + leafPemFunc = newProvider.ActiveLeafSigningCert + } else { + // If Root ID is different, we need to generate a new leaf signing cert + // else the trust chain will break when the old root expires. + leafPemFunc = usesIntermediate.GenerateLeafSigningCert + } + leafPem, err := leafPemFunc() + if err != nil { + return fmt.Errorf("error fetching leaf signing cert: %w", err) + } + // newProvider.ActiveLeafSigningCert may return a blank leafPem so we + // fall back to generating a new one just in case. + if leafPem == "" { + leafPem, err = usesIntermediate.GenerateLeafSigningCert() + if err != nil { + return fmt.Errorf("error generating new leaf signing cert: %w", err) + } + } + + if err := setLeafSigningCert(newActiveRoot, leafPem); err != nil { + return fmt.Errorf("error setting leaf signing cert: %w", err) + } + } + // If the root didn't change, just update the config and return. if root != nil && root.ID == newActiveRoot.ID { args.Op = structs.CAOpSetConfig @@ -919,7 +955,7 @@ func (c *CAManager) primaryUpdateRootCA(newProvider ca.Provider, args *structs.C // 3. Take the active root for the new provider and append the intermediate from step 2 // to its list of intermediates. // TODO: this cert is already parsed once in newCARoot, could we remove the second parse? - newRoot, err := connect.ParseCert(newRootPEM) + newRoot, err := connect.ParseCert(caPEM) if err != nil { return err } diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index a8ca93101..e1c2cf850 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -259,8 +259,8 @@ type mockCAProvider struct { func (m *mockCAProvider) Configure(cfg ca.ProviderConfig) error { return nil } func (m *mockCAProvider) State() (map[string]string, error) { return nil, nil } -func (m *mockCAProvider) GenerateCAChain() (ca.CAChainResult, error) { - return ca.CAChainResult{PEM: m.rootPEM}, nil +func (m *mockCAProvider) GenerateCAChain() (string, error) { + return m.rootPEM, nil } func (m *mockCAProvider) GenerateIntermediateCSR() (string, string, error) { m.callbackCh <- "provider/GenerateIntermediateCSR" @@ -624,7 +624,7 @@ func TestCAManager_UpdateConfiguration_Vault_Primary(t *testing.T) { require.Equal(t, connect.HexString(cert.SubjectKeyId), origRoot.SigningKeyID) t.Run("update config without changing root", func(t *testing.T) { - err = s1.caManager.UpdateConfiguration(&structs.CARequest{ + require.NoError(t, s1.caManager.UpdateConfiguration(&structs.CARequest{ Config: &structs.CAConfiguration{ Provider: "vault", Config: map[string]interface{}{ @@ -635,26 +635,81 @@ func TestCAManager_UpdateConfiguration_Vault_Primary(t *testing.T) { "CSRMaxPerSecond": 100, }, }, - }) - require.NoError(t, err) - _, sameRoot, err := s1.fsm.State().CARootActive(nil) - require.NoError(t, err) - require.Len(t, sameRoot.IntermediateCerts, 1) - sameRoot.CreateIndex = s1.caManager.providerRoot.CreateIndex - sameRoot.ModifyIndex = s1.caManager.providerRoot.ModifyIndex + })) - cert, err := connect.ParseCert(s1.caManager.getLeafSigningCertFromRoot(sameRoot)) + _, newRoot, err := s1.fsm.State().CARootActive(nil) require.NoError(t, err) - require.Equal(t, connect.HexString(cert.SubjectKeyId), sameRoot.SigningKeyID) + require.Len(t, newRoot.IntermediateCerts, 1) + newRoot.CreateIndex = s1.caManager.providerRoot.CreateIndex + newRoot.ModifyIndex = s1.caManager.providerRoot.ModifyIndex - require.Equal(t, origRoot, sameRoot) - require.Equal(t, sameRoot, s1.caManager.providerRoot) + orig, err := connect.ParseCert(s1.caManager.getLeafSigningCertFromRoot(newRoot)) + require.NoError(t, err) + require.Equal(t, connect.HexString(orig.SubjectKeyId), newRoot.SigningKeyID) + + require.Equal(t, origRoot, newRoot) + require.Equal(t, newRoot, s1.caManager.providerRoot) }) - t.Run("update config and change root", func(t *testing.T) { + t.Run("update config and change root only", func(t *testing.T) { + // Read the active leaf CA + provider, _ := s1.caManager.getCAProvider() + + before, err := provider.ActiveLeafSigningCert() + require.NoError(t, err) + vaultToken2 := ca.CreateVaultTokenWithAttrs(t, vault.Client(), &ca.VaultTokenAttributes{ RootPath: "pki-root-2", - IntermediatePath: "pki-intermediate-2", + IntermediatePath: "pki-intermediate", + ConsulManaged: true, + WithSudo: true, + }) + + require.NoError(t, s1.caManager.UpdateConfiguration(&structs.CARequest{ + Config: &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vaultToken2, + "RootPKIPath": "pki-root-2/", + "IntermediatePKIPath": "pki-intermediate/", + }, + }, + })) + + // fetch the new root from the state store to check that + // raft apply has occurred. + _, newRoot, err := s1.fsm.State().CARootActive(nil) + require.NoError(t, err) + require.Len(t, newRoot.IntermediateCerts, 2, + "expected one cross-sign cert and one local leaf sign cert") + + // Refresh provider + provider, _ = s1.caManager.getCAProvider() + + // Leaf signing cert should have been updated + after, err := provider.ActiveLeafSigningCert() + require.NoError(t, err) + + require.NotEqual(t, before, after, + "expected leaf signing cert to be changed after RootPKIPath was changed") + + cert, err = connect.ParseCert(after) + require.NoError(t, err) + + require.Equal(t, connect.HexString(cert.SubjectKeyId), newRoot.SigningKeyID) + }) + + t.Run("update config, change root and intermediate", func(t *testing.T) { + // Read the active leaf CA + provider, _ := s1.caManager.getCAProvider() + + before, err := provider.ActiveLeafSigningCert() + require.NoError(t, err) + + vaultToken3 := ca.CreateVaultTokenWithAttrs(t, vault.Client(), &ca.VaultTokenAttributes{ + RootPath: "pki-root-3", + IntermediatePath: "pki-intermediate-3", ConsulManaged: true, }) @@ -663,22 +718,34 @@ func TestCAManager_UpdateConfiguration_Vault_Primary(t *testing.T) { Provider: "vault", Config: map[string]interface{}{ "Address": vault.Addr, - "Token": vaultToken2, - "RootPKIPath": "pki-root-2/", - "IntermediatePKIPath": "pki-intermediate-2/", + "Token": vaultToken3, + "RootPKIPath": "pki-root-3/", + "IntermediatePKIPath": "pki-intermediate-3/", }, }, }) require.NoError(t, err) + // fetch the new root from the state store to check that + // raft apply has occurred. _, newRoot, err := s1.fsm.State().CARootActive(nil) require.NoError(t, err) require.Len(t, newRoot.IntermediateCerts, 2, "expected one cross-sign cert and one local leaf sign cert") - require.NotEqual(t, origRoot.ID, newRoot.ID) - cert, err = connect.ParseCert(s1.caManager.getLeafSigningCertFromRoot(newRoot)) + // Refresh provider + provider, _ = s1.caManager.getCAProvider() + + // Leaf signing cert should have been updated + after, err := provider.ActiveLeafSigningCert() require.NoError(t, err) + + require.NotEqual(t, before, after, + "expected leaf signing cert to be changed after RootPKIPath and IntermediatePKIPath were changed") + + cert, err = connect.ParseCert(after) + require.NoError(t, err) + require.Equal(t, connect.HexString(cert.SubjectKeyId), newRoot.SigningKeyID) }) } diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index 105bf317b..539226b29 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -1362,10 +1362,12 @@ func TestNewCARoot(t *testing.T) { } run := func(t *testing.T, tc testCase) { - root, err := newCARoot(ca.CAChainResult{ - PEM: tc.pem, - IntermediatePEM: tc.intermediatePem, - }, "provider-name", "cluster-id") + root, err := newCARoot( + tc.pem, + "provider-name", "cluster-id") + if tc.intermediatePem != "" { + setLeafSigningCert(root, tc.intermediatePem) + } if tc.expectedErr != "" { testutil.RequireErrorContains(t, err, tc.expectedErr) return