Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix bug with Vault CA provider #18112

Merged
merged 3 commits into from
Jul 14, 2023
Merged

Fix bug with Vault CA provider #18112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
57bb6f3
Fix bug with Vault CA provider where updating
Jul 12, 2023
b2dad88
Fix bug with Vault CA provider where updating
Jul 12, 2023
753d3c0
Add changelog
Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/18112.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
```
48 changes: 24 additions & 24 deletions agent/connect/ca/mock_Provider.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 6 additions & 3 deletions agent/connect/ca/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the implementations it looks like this might always just be the root. Is this comment still correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While not common, users could have a trust chain of certs in the root like:

root CA -> [any number of intermediate certs] -> primary CA


// 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
Expand Down
10 changes: 5 additions & 5 deletions agent/connect/ca/provider_aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 6 additions & 12 deletions agent/connect/ca/provider_aws_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -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)

Expand Down
18 changes: 9 additions & 9 deletions agent/connect/ca/provider_consul.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
Expand Down
16 changes: 8 additions & 8 deletions agent/connect/ca/provider_consul_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
Expand Down
Loading