From f05bad4a1d6cfcb2cca0ff78eb6aa19ffc9df76c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 1 Dec 2021 15:11:20 -0500 Subject: [PATCH 1/3] ca: update GenerateRoot godoc --- agent/connect/ca/provider.go | 14 +++++++++++--- agent/connect/ca/provider_consul.go | 3 +-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/agent/connect/ca/provider.go b/agent/connect/ca/provider.go index 4bab873aa9..6eab92f96e 100644 --- a/agent/connect/ca/provider.go +++ b/agent/connect/ca/provider.go @@ -118,9 +118,17 @@ type Provider interface { } type PrimaryProvider interface { - // GenerateRoot causes the creation of a new root certificate for this provider. - // This can also be a no-op if a root certificate already exists for the given - // config. If IsPrimary is false, calling this method is an error. + // GenerateRoot is called: + // * to initialize the CA system when a server is elected as a raft leader + // * when the CA configuration is updated in a way that might require + // generating a new root certificate. + // + // In both cases GenerateRoot is always called on a newly created provider + // after calling Provider.Configure, and before any other calls to the + // provider. + // + // The provider should return an existing root certificate if one exists, + // otherwise it should generate a new root certificate and return it. GenerateRoot() error // ActiveRoot returns the currently active root CA for this diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go index dea91e5d7d..d0e746803c 100644 --- a/agent/connect/ca/provider_consul.go +++ b/agent/connect/ca/provider_consul.go @@ -159,8 +159,7 @@ func (c *ConsulProvider) ActiveRoot() (string, error) { return providerState.RootCert, nil } -// GenerateRoot initializes a new root certificate and private key -// if needed. +// GenerateRoot initializes a new root certificate and private key if needed. func (c *ConsulProvider) GenerateRoot() error { providerState, err := c.getState() if err != nil { From c2b9c81a5569f989a93f65e4b3b974192c9798fc Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 1 Dec 2021 15:40:27 -0500 Subject: [PATCH 2/3] ca: update MockProvider for new interface --- agent/connect/ca/mock_Provider.go | 46 +++++++++++-------------------- 1 file changed, 16 insertions(+), 30 deletions(-) diff --git a/agent/connect/ca/mock_Provider.go b/agent/connect/ca/mock_Provider.go index 59f5374fef..ec79ea5c70 100644 --- a/agent/connect/ca/mock_Provider.go +++ b/agent/connect/ca/mock_Provider.go @@ -34,34 +34,13 @@ func (_m *MockProvider) ActiveIntermediate() (string, error) { return r0, r1 } -// ActiveRoot provides a mock function with given fields: -func (_m *MockProvider) ActiveRoot() (string, error) { - ret := _m.Called() - - var r0 string - if rf, ok := ret.Get(0).(func() string); ok { - r0 = rf() - } else { - r0 = ret.Get(0).(string) - } - - var r1 error - if rf, ok := ret.Get(1).(func() error); ok { - r1 = rf() - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - -// Cleanup provides a mock function with given fields: providerTypeChange, config -func (_m *MockProvider) Cleanup(providerTypeChange bool, config map[string]interface{}) error { - ret := _m.Called(providerTypeChange, config) +// Cleanup provides a mock function with given fields: providerTypeChange, otherConfig +func (_m *MockProvider) Cleanup(providerTypeChange bool, otherConfig map[string]interface{}) error { + ret := _m.Called(providerTypeChange, otherConfig) var r0 error if rf, ok := ret.Get(0).(func(bool, map[string]interface{}) error); ok { - r0 = rf(providerTypeChange, config) + r0 = rf(providerTypeChange, otherConfig) } else { r0 = ret.Error(0) } @@ -147,17 +126,24 @@ func (_m *MockProvider) GenerateIntermediateCSR() (string, error) { } // GenerateRoot provides a mock function with given fields: -func (_m *MockProvider) GenerateRoot() error { +func (_m *MockProvider) GenerateRoot() (RootResult, error) { ret := _m.Called() - var r0 error - if rf, ok := ret.Get(0).(func() error); ok { + var r0 RootResult + if rf, ok := ret.Get(0).(func() RootResult); ok { r0 = rf() } else { - r0 = ret.Error(0) + r0 = ret.Get(0).(RootResult) } - return r0 + var r1 error + 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 From 9b7468f99e503177aa08f01a4fdb3bea22609641 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 1 Dec 2021 15:39:20 -0500 Subject: [PATCH 3/3] ca/provider: remove ActiveRoot from Provider --- agent/connect/ca/provider.go | 17 +++++----- agent/connect/ca/provider_aws.go | 26 ++++++--------- agent/connect/ca/provider_aws_test.go | 36 ++++++++++---------- agent/connect/ca/provider_consul.go | 37 +++++++-------------- agent/connect/ca/provider_consul_test.go | 41 +++++++++++++---------- agent/connect/ca/provider_vault.go | 42 +++++++++++++----------- agent/connect/ca/provider_vault_test.go | 24 +++++++++----- agent/consul/leader_connect_ca.go | 20 ++++------- agent/consul/leader_connect_ca_test.go | 5 ++- 9 files changed, 120 insertions(+), 128 deletions(-) diff --git a/agent/connect/ca/provider.go b/agent/connect/ca/provider.go index 6eab92f96e..59431fdbd7 100644 --- a/agent/connect/ca/provider.go +++ b/agent/connect/ca/provider.go @@ -129,14 +129,7 @@ type PrimaryProvider interface { // // The provider should return an existing root certificate if one exists, // otherwise it should generate a new root certificate and return it. - GenerateRoot() error - - // ActiveRoot returns the currently active root CA for this - // provider. This should be a parent of the certificate returned by - // ActiveIntermediate() - // - // TODO: currently called from secondaries, but shouldn't be so is on PrimaryProvider - ActiveRoot() (string, error) + GenerateRoot() (RootResult, error) // GenerateIntermediate returns a new intermediate signing cert and sets it to // the active intermediate. If multiple intermediates are needed to complete @@ -189,6 +182,14 @@ type SecondaryProvider interface { SetIntermediate(intermediatePEM, rootPEM string) error } +// RootResult is the result returned by PrimaryProvider.GenerateRoot. +// +// TODO: rename this struct +type RootResult struct { + // PEM encoded certificate that will be used as the primary CA. + PEM string +} + // NeedsStop is an optional interface that allows a CA to define a function // to be called when the CA instance is no longer in use. This is different // from Cleanup(), as only the local provider instance is being shut down diff --git a/agent/connect/ca/provider_aws.go b/agent/connect/ca/provider_aws.go index b813ef5072..25786ab409 100644 --- a/agent/connect/ca/provider_aws.go +++ b/agent/connect/ca/provider_aws.go @@ -134,12 +134,19 @@ func (a *AWSProvider) State() (map[string]string, error) { } // GenerateRoot implements Provider -func (a *AWSProvider) GenerateRoot() error { +func (a *AWSProvider) GenerateRoot() (RootResult, error) { if !a.isPrimary { - return fmt.Errorf("provider is not the root certificate authority") + return RootResult{}, fmt.Errorf("provider is not the root certificate authority") } - return a.ensureCA() + if err := a.ensureCA(); err != nil { + return RootResult{}, err + } + + if a.rootPEM == "" { + return RootResult{}, fmt.Errorf("AWS CA provider not fully Initialized") + } + return RootResult{PEM: a.rootPEM}, nil } // ensureCA loads the CA resource to check it exists if configured by User or in @@ -489,19 +496,6 @@ func (a *AWSProvider) signCSR(csrPEM string, templateARN string, ttl time.Durati }) } -// ActiveRoot implements Provider -func (a *AWSProvider) ActiveRoot() (string, error) { - err := a.ensureCA() - if err != nil { - return "", err - } - - if a.rootPEM == "" { - return "", fmt.Errorf("Secondary AWS CA provider not fully Initialized") - } - return a.rootPEM, nil -} - // GenerateIntermediateCSR implements Provider func (a *AWSProvider) GenerateIntermediateCSR() (string, error) { if a.isPrimary { diff --git a/agent/connect/ca/provider_aws_test.go b/agent/connect/ca/provider_aws_test.go index a090df7858..b73d4a95d5 100644 --- a/agent/connect/ca/provider_aws_test.go +++ b/agent/connect/ca/provider_aws_test.go @@ -46,12 +46,9 @@ func TestAWSBootstrapAndSignPrimary(t *testing.T) { provider := testAWSProvider(t, testProviderConfigPrimary(t, cfg)) defer provider.Cleanup(true, nil) - // Generate the root - require.NoError(t, provider.GenerateRoot()) - - // Fetch Active Root - rootPEM, err := provider.ActiveRoot() + root, err := provider.GenerateRoot() require.NoError(t, err) + rootPEM := root.PEM // Generate Intermediate (not actually needed for this provider for now // but this simulates the calls in Server.initializeRoot). @@ -81,16 +78,12 @@ func TestAWSBootstrapAndSignPrimary(t *testing.T) { } t.Run("Test default root ttl for aws ca provider", func(t *testing.T) { - provider := testAWSProvider(t, testProviderConfigPrimary(t, nil)) defer provider.Cleanup(true, nil) - // Generate the root - require.NoError(t, provider.GenerateRoot()) - - // Fetch Active Root - rootPEM, err := provider.ActiveRoot() + root, err := provider.GenerateRoot() require.NoError(t, err) + rootPEM := root.PEM // Ensure they use the right key type rootCert, err := connect.ParseCert(rootPEM) @@ -123,8 +116,9 @@ func TestAWSBootstrapAndSignSecondary(t *testing.T) { p1 := testAWSProvider(t, testProviderConfigPrimary(t, nil)) defer p1.Cleanup(true, nil) - rootPEM, err := p1.ActiveRoot() + root, err := p1.GenerateRoot() require.NoError(t, err) + rootPEM := root.PEM p2 := testAWSProvider(t, testProviderConfigSecondary(t, nil)) defer p2.Cleanup(true, nil) @@ -151,8 +145,9 @@ func TestAWSBootstrapAndSignSecondary(t *testing.T) { cfg1 := testProviderConfigPrimary(t, nil) cfg1.State = p1State p1 = testAWSProvider(t, cfg1) - newRootPEM, err := p1.ActiveRoot() + root, err := p1.GenerateRoot() require.NoError(t, err) + newRootPEM := root.PEM cfg2 := testProviderConfigPrimary(t, nil) cfg2.State = p2State @@ -184,8 +179,9 @@ func TestAWSBootstrapAndSignSecondary(t *testing.T) { "ExistingARN": p1State[AWSStateCAARNKey], }) p1 = testAWSProvider(t, cfg1) - newRootPEM, err := p1.ActiveRoot() + root, err := p1.GenerateRoot() require.NoError(t, err) + newRootPEM := root.PEM cfg2 := testProviderConfigPrimary(t, map[string]interface{}{ "ExistingARN": p2State[AWSStateCAARNKey], @@ -222,8 +218,9 @@ func TestAWSBootstrapAndSignSecondary(t *testing.T) { p2 = testAWSProvider(t, cfg2) require.NoError(t, p2.SetIntermediate(newIntPEM, newRootPEM)) - newRootPEM, err = p1.ActiveRoot() + root, err = p1.GenerateRoot() require.NoError(t, err) + newRootPEM = root.PEM newIntPEM, err = p2.ActiveIntermediate() require.NoError(t, err) @@ -243,7 +240,8 @@ func TestAWSBootstrapAndSignSecondaryConsul(t *testing.T) { p1 := TestConsulProvider(t, delegate) cfg := testProviderConfig(conf) require.NoError(t, p1.Configure(cfg)) - require.NoError(t, p1.GenerateRoot()) + _, err := p1.GenerateRoot() + require.NoError(t, err) p2 := testAWSProvider(t, testProviderConfigSecondary(t, nil)) defer p2.Cleanup(true, nil) @@ -254,7 +252,9 @@ func TestAWSBootstrapAndSignSecondaryConsul(t *testing.T) { t.Run("pri=aws,sec=consul", func(t *testing.T) { p1 := testAWSProvider(t, testProviderConfigPrimary(t, nil)) defer p1.Cleanup(true, nil) - require.NoError(t, p1.GenerateRoot()) + + _, err := p1.GenerateRoot() + require.NoError(t, err) conf := testConsulCAConfig() delegate := newMockDelegate(t, conf) @@ -315,11 +315,13 @@ func TestAWSProvider_Cleanup(t *testing.T) { } requirePCADeleted := func(t *testing.T, provider *AWSProvider) { + t.Helper() deleted, err := describeCA(t, provider) require.True(t, err != nil || deleted, "The AWS PCA instance has not been deleted") } requirePCANotDeleted := func(t *testing.T, provider *AWSProvider) { + t.Helper() deleted, err := describeCA(t, provider) require.NoError(t, err) require.False(t, deleted, "The AWS PCA instance should not have been deleted") diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go index d0e746803c..a12f70a1a0 100644 --- a/agent/connect/ca/provider_consul.go +++ b/agent/connect/ca/provider_consul.go @@ -149,28 +149,18 @@ func (c *ConsulProvider) State() (map[string]string, error) { return c.testState, nil } -// ActiveRoot returns the active root CA certificate. -func (c *ConsulProvider) ActiveRoot() (string, error) { - providerState, err := c.getState() - if err != nil { - return "", err - } - - return providerState.RootCert, nil -} - // GenerateRoot initializes a new root certificate and private key if needed. -func (c *ConsulProvider) GenerateRoot() error { +func (c *ConsulProvider) GenerateRoot() (RootResult, error) { providerState, err := c.getState() if err != nil { - return err + return RootResult{}, err } if !c.isPrimary { - return fmt.Errorf("provider is not the root certificate authority") + return RootResult{}, fmt.Errorf("provider is not the root certificate authority") } if providerState.RootCert != "" { - return nil + return RootResult{PEM: providerState.RootCert}, nil } // Generate a private key if needed @@ -178,7 +168,7 @@ func (c *ConsulProvider) GenerateRoot() error { if c.config.PrivateKey == "" { _, pk, err := connect.GeneratePrivateKeyWithConfig(c.config.PrivateKeyType, c.config.PrivateKeyBits) if err != nil { - return err + return RootResult{}, err } newState.PrivateKey = pk } else { @@ -189,12 +179,12 @@ func (c *ConsulProvider) GenerateRoot() error { if c.config.RootCert == "" { nextSerial, err := c.incrementAndGetNextSerialNumber() if err != nil { - return fmt.Errorf("error computing next serial number: %v", err) + return RootResult{}, fmt.Errorf("error computing next serial number: %v", err) } ca, err := c.generateCA(newState.PrivateKey, nextSerial, c.config.RootCertTTL) if err != nil { - return fmt.Errorf("error generating CA: %v", err) + return RootResult{}, fmt.Errorf("error generating CA: %v", err) } newState.RootCert = ca } else { @@ -207,10 +197,10 @@ func (c *ConsulProvider) GenerateRoot() error { ProviderState: &newState, } if _, err := c.Delegate.ApplyCARequest(args); err != nil { - return err + return RootResult{}, err } - return nil + return RootResult{PEM: newState.RootCert}, nil } // GenerateIntermediateCSR creates a private key and generates a CSR @@ -287,18 +277,15 @@ func (c *ConsulProvider) SetIntermediate(intermediatePEM, rootPEM string) error return nil } -// We aren't maintaining separate root/intermediate CAs for the builtin -// provider, so just return the root. func (c *ConsulProvider) ActiveIntermediate() (string, error) { - if c.isPrimary { - return c.ActiveRoot() - } - providerState, err := c.getState() if err != nil { return "", err } + if c.isPrimary { + return providerState.RootCert, nil + } return providerState.IntermediateCert, nil } diff --git a/agent/connect/ca/provider_consul_test.go b/agent/connect/ca/provider_consul_test.go index dd4fd12a76..a4df05b767 100644 --- a/agent/connect/ca/provider_consul_test.go +++ b/agent/connect/ca/provider_consul_test.go @@ -83,18 +83,17 @@ func TestConsulCAProvider_Bootstrap(t *testing.T) { provider := TestConsulProvider(t, delegate) require.NoError(t, provider.Configure(testProviderConfig(conf))) - require.NoError(t, provider.GenerateRoot()) - root, err := provider.ActiveRoot() + root, err := provider.GenerateRoot() require.NoError(t, err) // Intermediate should be the same cert. inter, err := provider.ActiveIntermediate() require.NoError(t, err) - require.Equal(t, root, inter) + require.Equal(t, root.PEM, inter) // Should be a valid cert - parsed, err := connect.ParseCert(root) + parsed, err := connect.ParseCert(root.PEM) require.NoError(t, err) require.Equal(t, parsed.URIs[0].String(), fmt.Sprintf("spiffe://%s.consul", conf.ClusterID)) requireNotEncoded(t, parsed.SubjectKeyId) @@ -123,14 +122,13 @@ func TestConsulCAProvider_Bootstrap_WithCert(t *testing.T) { provider := TestConsulProvider(t, delegate) require.NoError(t, provider.Configure(testProviderConfig(conf))) - require.NoError(t, provider.GenerateRoot()) - root, err := provider.ActiveRoot() + root, err := provider.GenerateRoot() require.NoError(t, err) - require.Equal(t, root, rootCA.RootCert) + require.Equal(t, root.PEM, rootCA.RootCert) // Should be a valid cert - parsed, err := connect.ParseCert(root) + parsed, err := connect.ParseCert(root.PEM) require.NoError(t, err) // test that the default root cert ttl was not applied to the provided cert @@ -160,7 +158,8 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { provider := TestConsulProvider(t, delegate) require.NoError(t, provider.Configure(testProviderConfig(conf))) - require.NoError(t, provider.GenerateRoot()) + _, err := provider.GenerateRoot() + require.NoError(t, err) spiffeService := &connect.SpiffeIDService{ Host: connect.TestClusterID + ".consul", @@ -272,7 +271,8 @@ func TestConsulCAProvider_CrossSignCA(t *testing.T) { conf1.Config["PrivateKeyType"] = tc.SigningKeyType conf1.Config["PrivateKeyBits"] = tc.SigningKeyBits require.NoError(t, provider1.Configure(testProviderConfig(conf1))) - require.NoError(t, provider1.GenerateRoot()) + _, err := provider1.GenerateRoot() + require.NoError(t, err) conf2 := testConsulCAConfig() conf2.CreateIndex = 10 @@ -281,7 +281,8 @@ func TestConsulCAProvider_CrossSignCA(t *testing.T) { conf2.Config["PrivateKeyType"] = tc.CSRKeyType conf2.Config["PrivateKeyBits"] = tc.CSRKeyBits require.NoError(t, provider2.Configure(testProviderConfig(conf2))) - require.NoError(t, provider2.GenerateRoot()) + _, err = provider2.GenerateRoot() + require.NoError(t, err) testCrossSignProviders(t, provider1, provider2) }) @@ -291,9 +292,10 @@ func TestConsulCAProvider_CrossSignCA(t *testing.T) { func testCrossSignProviders(t *testing.T, provider1, provider2 Provider) { // Get the root from the new provider to be cross-signed. - newRootPEM, err := provider2.ActiveRoot() + root, err := provider2.GenerateRoot() require.NoError(t, err) - newRoot, err := connect.ParseCert(newRootPEM) + + newRoot, err := connect.ParseCert(root.PEM) require.NoError(t, err) oldSubject := newRoot.Subject.CommonName requireNotEncoded(t, newRoot.SubjectKeyId) @@ -314,9 +316,9 @@ func testCrossSignProviders(t *testing.T, provider1, provider2 Provider) { requireNotEncoded(t, xc.SubjectKeyId) requireNotEncoded(t, xc.AuthorityKeyId) - oldRootPEM, err := provider1.ActiveRoot() + p1Root, err := provider1.GenerateRoot() require.NoError(t, err) - oldRoot, err := connect.ParseCert(oldRootPEM) + oldRoot, err := connect.ParseCert(p1Root.PEM) require.NoError(t, err) requireNotEncoded(t, oldRoot.SubjectKeyId) requireNotEncoded(t, oldRoot.AuthorityKeyId) @@ -392,7 +394,8 @@ func TestConsulProvider_SignIntermediate(t *testing.T) { conf1.Config["PrivateKeyType"] = tc.SigningKeyType conf1.Config["PrivateKeyBits"] = tc.SigningKeyBits require.NoError(t, provider1.Configure(testProviderConfig(conf1))) - require.NoError(t, provider1.GenerateRoot()) + _, err := provider1.GenerateRoot() + require.NoError(t, err) conf2 := testConsulCAConfig() conf2.CreateIndex = 10 @@ -422,8 +425,9 @@ func testSignIntermediateCrossDC(t *testing.T, provider1, provider2 Provider) { // Sign the CSR with provider1. intermediatePEM, err := provider1.SignIntermediate(csr) require.NoError(t, err) - rootPEM, err := provider1.ActiveRoot() + root, err := provider1.GenerateRoot() require.NoError(t, err) + rootPEM := root.PEM // Give the new intermediate to provider2 to use. require.NoError(t, provider2.SetIntermediate(intermediatePEM, rootPEM)) @@ -496,7 +500,8 @@ func TestConsulCAProvider_MigrateOldID(t *testing.T) { provider := TestConsulProvider(t, delegate) require.NoError(t, provider.Configure(testProviderConfig(conf))) - require.NoError(t, provider.GenerateRoot()) + _, err = provider.GenerateRoot() + require.NoError(t, err) // After running Configure, the old ID entry should be gone. _, providerState, err = delegate.state.CAProviderState(tc.oldID) diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index ef1920d7af..3a1d5128e7 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -12,11 +12,12 @@ import ( "strings" "time" - "github.com/hashicorp/consul/lib/decode" "github.com/hashicorp/go-hclog" vaultapi "github.com/hashicorp/vault/api" "github.com/mitchellh/mapstructure" + "github.com/hashicorp/consul/lib/decode" + "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/structs" ) @@ -220,19 +221,14 @@ func (v *VaultProvider) State() (map[string]string, error) { return nil, nil } -// ActiveRoot returns the active root CA certificate. -func (v *VaultProvider) ActiveRoot() (string, error) { - return v.getCA(v.config.RootPKIPath) -} - // GenerateRoot mounts and initializes a new root PKI backend if needed. -func (v *VaultProvider) GenerateRoot() error { +func (v *VaultProvider) GenerateRoot() (RootResult, error) { if !v.isPrimary { - return fmt.Errorf("provider is not the root certificate authority") + return RootResult{}, fmt.Errorf("provider is not the root certificate authority") } // Set up the root PKI backend if necessary. - rootPEM, err := v.ActiveRoot() + rootPEM, err := v.getCA(v.config.RootPKIPath) switch err { case ErrBackendNotMounted: err := v.client.Sys().Mount(v.config.RootPKIPath, &vaultapi.MountInput{ @@ -247,14 +243,14 @@ func (v *VaultProvider) GenerateRoot() error { }, }) if err != nil { - return err + return RootResult{}, err } fallthrough case ErrBackendNotInitialized: uid, err := connect.CompactUID() if err != nil { - return err + return RootResult{}, err } _, err = v.client.Logical().Write(v.config.RootPKIPath+"root/generate/internal", map[string]interface{}{ "common_name": connect.CACN("vault", uid, v.clusterID, v.isPrimary), @@ -263,17 +259,25 @@ func (v *VaultProvider) GenerateRoot() error { "key_bits": v.config.PrivateKeyBits, }) if err != nil { - return err + return RootResult{}, err } + + // retrieve the newly generated cert so that we can return it + // TODO: is this already available from the Local().Write() above? + rootPEM, err = v.getCA(v.config.RootPKIPath) + if err != nil { + return RootResult{}, err + } + default: if err != nil { - return err + return RootResult{}, err } if rootPEM != "" { rootCert, err := connect.ParseCert(rootPEM) if err != nil { - return err + return RootResult{}, err } // Vault PKI doesn't allow in-place cert/key regeneration. That @@ -285,18 +289,18 @@ func (v *VaultProvider) GenerateRoot() error { // ForceWithoutCrossSigning option when changing key types. foundKeyType, foundKeyBits, err := connect.KeyInfoFromCert(rootCert) if err != nil { - return err + return RootResult{}, err } if v.config.PrivateKeyType != foundKeyType { - return fmt.Errorf("cannot update the PrivateKeyType field without choosing a new PKI mount for the root CA") + return RootResult{}, fmt.Errorf("cannot update the PrivateKeyType field without choosing a new PKI mount for the root CA") } if v.config.PrivateKeyBits != foundKeyBits { - return fmt.Errorf("cannot update the PrivateKeyBits field without choosing a new PKI mount for the root CA") + return RootResult{}, fmt.Errorf("cannot update the PrivateKeyBits field without choosing a new PKI mount for the root CA") } } } - return nil + return RootResult{PEM: rootPEM}, nil } // GenerateIntermediateCSR creates a private key and generates a CSR @@ -574,7 +578,7 @@ func (v *VaultProvider) SignIntermediate(csr *x509.CertificateRequest) (string, // CrossSignCA takes a CA certificate and cross-signs it to form a trust chain // back to our active root. func (v *VaultProvider) CrossSignCA(cert *x509.Certificate) (string, error) { - rootPEM, err := v.ActiveRoot() + rootPEM, err := v.getCA(v.config.RootPKIPath) if err != nil { return "", err } diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index bba2cd04ad..4605073831 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -238,7 +238,10 @@ func TestVaultCAProvider_Bootstrap(t *testing.T) { expectedRootCertTTL string }{ { - certFunc: providerWDefaultRootCertTtl.ActiveRoot, + certFunc: func() (string, error) { + root, err := providerWDefaultRootCertTtl.GenerateRoot() + return root.PEM, err + }, backendPath: "pki-root/", rootCaCreation: true, client: client1, @@ -323,8 +326,9 @@ func TestVaultCAProvider_SignLeaf(t *testing.T) { Service: "foo", } - rootPEM, err := provider.ActiveRoot() + root, err := provider.GenerateRoot() require.NoError(t, err) + rootPEM := root.PEM assertCorrectKeyType(t, tc.KeyType, rootPEM) intPEM, err := provider.ActiveIntermediate() @@ -407,9 +411,9 @@ func TestVaultCAProvider_CrossSignCA(t *testing.T) { defer testVault1.Stop() { - rootPEM, err := provider1.ActiveRoot() + root, err := provider1.GenerateRoot() require.NoError(t, err) - assertCorrectKeyType(t, tc.SigningKeyType, rootPEM) + assertCorrectKeyType(t, tc.SigningKeyType, root.PEM) intPEM, err := provider1.ActiveIntermediate() require.NoError(t, err) @@ -424,9 +428,9 @@ func TestVaultCAProvider_CrossSignCA(t *testing.T) { defer testVault2.Stop() { - rootPEM, err := provider2.ActiveRoot() + root, err := provider2.GenerateRoot() require.NoError(t, err) - assertCorrectKeyType(t, tc.CSRKeyType, rootPEM) + assertCorrectKeyType(t, tc.CSRKeyType, root.PEM) intPEM, err := provider2.ActiveIntermediate() require.NoError(t, err) @@ -492,7 +496,8 @@ func TestVaultProvider_SignIntermediateConsul(t *testing.T) { delegate := newMockDelegate(t, conf) provider1 := TestConsulProvider(t, delegate) require.NoError(t, provider1.Configure(testProviderConfig(conf))) - require.NoError(t, provider1.GenerateRoot()) + _, err := provider1.GenerateRoot() + require.NoError(t, err) // Ensure that we don't configure vault to try and mint leafs that // outlive their CA during the test (which hard fails in vault). @@ -786,8 +791,9 @@ func createVaultProvider(t *testing.T, isPrimary bool, addr, token string, rawCo t.Cleanup(provider.Stop) require.NoError(t, provider.Configure(cfg)) if isPrimary { - require.NoError(t, provider.GenerateRoot()) - _, err := provider.GenerateIntermediate() + _, err := provider.GenerateRoot() + require.NoError(t, err) + _, err = provider.GenerateIntermediate() require.NoError(t, err) } diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 51525f507e..203d3f2a40 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -482,16 +482,12 @@ func (c *CAManager) primaryInitialize(provider ca.Provider, conf *structs.CAConf if err := provider.Configure(pCfg); err != nil { return fmt.Errorf("error configuring provider: %v", err) } - if err := provider.GenerateRoot(); err != nil { + root, err := provider.GenerateRoot() + if err != nil { return fmt.Errorf("error generating CA root certificate: %v", err) } - // Get the active root cert from the CA - rootPEM, err := provider.ActiveRoot() - if err != nil { - return fmt.Errorf("error getting root cert: %v", err) - } - rootCA, err := parseCARoot(rootPEM, conf.Provider, conf.ClusterID) + rootCA, err := parseCARoot(root.PEM, conf.Provider, conf.ClusterID) if err != nil { return err } @@ -863,15 +859,12 @@ func (c *CAManager) UpdateConfiguration(args *structs.CARequest) (reterr error) } func (c *CAManager) primaryUpdateRootCA(newProvider ca.Provider, args *structs.CARequest, config *structs.CAConfiguration) error { - if err := newProvider.GenerateRoot(); err != nil { + providerRoot, err := newProvider.GenerateRoot() + if err != nil { return fmt.Errorf("error generating CA root certificate: %v", err) } - newRootPEM, err := newProvider.ActiveRoot() - if err != nil { - return err - } - + newRootPEM := providerRoot.PEM newActiveRoot, err := parseCARoot(newRootPEM, args.Config.Provider, args.Config.ClusterID) if err != nil { return err @@ -925,6 +918,7 @@ func (c *CAManager) primaryUpdateRootCA(newProvider ca.Provider, args *structs.C // get a cross-signed certificate. // 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 parseCARoot, could we remove the second parse? newRoot, err := connect.ParseCert(newRootPEM) if err != nil { return err diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 5e5e1de59d..17b310cb6a 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -227,9 +227,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) GenerateRoot() error { return nil } -func (m *mockCAProvider) ActiveRoot() (string, error) { - return m.rootPEM, nil +func (m *mockCAProvider) GenerateRoot() (ca.RootResult, error) { + return ca.RootResult{PEM: m.rootPEM}, nil } func (m *mockCAProvider) GenerateIntermediateCSR() (string, error) { m.callbackCh <- "provider/GenerateIntermediateCSR"