Fix bug with Vault CA provider where updating

RootPKIPath but not IntermediatePKIPath would not update leaf signing certs with the new root.
hashicorp · Jul 13, 2023 · 57bb6f3 · 57bb6f3
1 parent efe9816
commit 57bb6f3
Show file tree

Hide file tree

Showing 12 changed files with 223 additions and 138 deletions.
diff --git a/agent/connect/ca/mock_Provider.go b/agent/connect/ca/mock_Provider.go
diff --git a/agent/connect/ca/provider.go b/agent/connect/ca/provider.go
@@ -137,7 +137,7 @@ type PrimaryProvider interface {
 	// 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)
+	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
@@ -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
@@ -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
@@ -155,25 +155,25 @@ 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
 	newState := *providerState
 	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
@@ -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
@@ -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,15 +302,15 @@ 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
 		fallthrough
 	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) {