Skip to content

Fix Vault managed intermediate PKI bug #15525

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

Merged
merged 5 commits into from
Nov 28, 2022
Merged

Fix Vault managed intermediate PKI bug #15525

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3a522d0
Fix Vault managed intermediate PKI bug
Nov 21, 2022
122cd9f
Fix Vault managed intermediate PKI bug
Nov 21, 2022
dfca9ca
Add test for Consul-managed
Nov 22, 2022
bfe4374
Add changelog
Nov 23, 2022
227762e
Tune always but log conditionally
Nov 28, 2022
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
51 changes: 41 additions & 10 deletions agent/connect/ca/provider_vault.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ type VaultProvider struct {
clusterID string
spiffeID *connect.SpiffeIDSigning
logger hclog.Logger

isConsulMountedIntermediate bool
}

func NewVaultProvider(logger hclog.Logger) *VaultProvider {
Expand Down Expand Up @@ -310,9 +312,10 @@ func (v *VaultProvider) GenerateRoot() (RootResult, error) {
},
})
if err != nil {
return RootResult{}, err
return RootResult{}, fmt.Errorf("failed to mount root CA backend: %w", err)
}

// We want to initialize afterwards
fallthrough
case ErrBackendNotInitialized:
uid, err := connect.CompactUID()
Expand All @@ -326,7 +329,7 @@ func (v *VaultProvider) GenerateRoot() (RootResult, error) {
"key_bits": v.config.PrivateKeyBits,
})
if err != nil {
return RootResult{}, err
return RootResult{}, fmt.Errorf("failed to initialize root CA: %w", err)
}
var ok bool
rootPEM, ok = resp.Data["certificate"].(string)
Expand All @@ -336,7 +339,7 @@ func (v *VaultProvider) GenerateRoot() (RootResult, error) {

default:
if err != nil {
return RootResult{}, err
return RootResult{}, fmt.Errorf("unexpected error while setting root PKI backend: %w", err)
}
}

Expand Down Expand Up @@ -381,19 +384,47 @@ func (v *VaultProvider) setupIntermediatePKIPath() error {
Config: mountConfig,
})
if err != nil {
return err
return fmt.Errorf("failed to mount intermediate PKI backend: %w", err)
}
// Required to determine if we should tune the mount
// if the VaultProvider is ever reconfigured.
v.isConsulMountedIntermediate = true
Copy link
Member

@rboyer rboyer Nov 28, 2022

Choose a reason for hiding this comment

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

Note: this would only get set to true the FIRST time a consul instance ran through this code. After the mount existed it wouldn't set this if it matters.

Copy link
Contributor Author

@kisunji kisunji Nov 28, 2022

Choose a reason for hiding this comment

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

Yeah I intended it to be a "best guess" field that only sets if the VaultProvider EVER creates the intermediate mount. If the mount ever existed then we'll never attempt to tune the mount but output a log to help operators diagnose it if they're ever confused by the behavior.


Copy link
Member

@rboyer rboyer Nov 22, 2022

Choose a reason for hiding this comment

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

On an earlier meeting did we decide that this needed a fallthrough like the root-generate switch had?

Copy link
Contributor Author

@kisunji kisunji Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

When we talked I thought I should be initializing the mount from within setupIntermediatePKIPath but realized it wasn't necessary to do that here. So all we need to do is prevent error-ing out on ErrBackendNotInitialized; other codepaths like generateIntermediateCSR below will initialize the mount as necessary.

Copy link
Contributor Author

@kisunji kisunji Nov 22, 2022

Choose a reason for hiding this comment

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

So if we do end up mounting the intermediate PKI, there's no other action to be done (unlike GenerateRoot which is actually responsible for mounting+initializing+generating).

I thought about making the code paths for root and intermediate more parallel to each other but deferred it to future Chris

} else if err == ErrBackendNotInitialized {
// If this is the first time calling setupIntermediatePKIPath, the backend
// will not have been initialized. Since the mount is ready we can suppress
// this error.
Comment on lines +399 to +401
Copy link
Contributor Author

@kisunji kisunji Nov 21, 2022

Choose a reason for hiding this comment

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

This else if block is the bugfix

} else {
return err
return fmt.Errorf("unexpected error while fetching intermediate CA: %w", err)
}
} else {
err := v.tuneMountNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath, &mountConfig)
if err != nil {
v.logger.Warn("Could not update intermediate PKI mount settings", "path", v.config.IntermediatePKIPath, "error", err)
// If Consul was responsible for mounting the intermediate PKI path
// we should update the mount with any new config.
if v.isConsulMountedIntermediate {
// This codepath requires the Vault policy:
//
// path "/sys/mounts/<intermediate_pki_path>/tune" {
// capabilities = [ "update" ]
// }
//
err := v.tuneMountNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath, &mountConfig)
if err != nil {
v.logger.Warn("Intermediate PKI path was mounted by Consul but could not be tuned",
"namespace", v.config.IntermediatePKINamespace,
"path", v.config.IntermediatePKIPath,
"error", err,
)
}

} else {
v.logger.Info("Found existing Intermediate PKI path mount",
"namespace", v.config.IntermediatePKINamespace,
"path", v.config.IntermediatePKIPath,
)
}
Copy link
Contributor Author

@kisunji kisunji Nov 21, 2022

Choose a reason for hiding this comment

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

Since tuning the mount is only relevant for Consul-managed, I added a conditional to skip since otherwise you'd see an ominous WARN in the logs on startup.

This isn't the cleanest way to determine if the provider is "Consul-managed" or "Vault-managed" but introducing explicit configuration requires some more consideration around default/missing values.

Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp Nov 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If MaxLeaseTTL is set by the operator, tuning this mount really only matters if the mount's MaxLeaseTTL is lower than the IntermediateCertTTL. Otherwise, tuning the config has no effect, as the intermediate CA cert will still be issued with the requested TTL.

I think I heard, though haven't tested, that if you ask Vault to generate a cert (i.e., intermediate CA cert) with a TTL greater than MaxLeaseTTL, the operation will still succeed, the TTL will just be truncated to MaxLeaseTTL. So everything will still work, it's just that the intermediate CA cert will expire sooner than configured. (Unless there's something about our "attempt to renew at 50% of lifetime" math that breaks because it depends on IntermediateCertTTL rather than the TTL of the actual issuer cert.)

I don't know what the default behavior of MaxLeaseTTL is if not configured by the Vault operator.

Copy link
Contributor

@cipherboy cipherboy Nov 22, 2022

Choose a reason for hiding this comment

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

I think its somewhat short: https://developer.hashicorp.com/vault/docs/concepts/tokens#the-general-case -- 32 days.

You'll definitely want to read the cert's actual validity period rather than assuming the TTL is OK, not sure if that's useful in this PR or a follow-up though.

Also, I think in 1.11+, we've started adding a warning in this event, but obviously that doesn't help Vault <= 1.10...

Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp Nov 23, 2022

Choose a reason for hiding this comment

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

I confirmed that we do work based on the cert's actual validity period:

consul/agent/consul/leader_connect_ca.go

Line 1187 in 6b477ce

if lessThanHalfTimePassed(c.timeNow(), intermediateCert.NotBefore, intermediateCert.NotAfter) {

}

// Create the role for issuing leaf certs if it doesn't exist yet
// Create the role for issuing leaf certs
Copy link
Contributor Author

@kisunji kisunji Nov 21, 2022

Choose a reason for hiding this comment

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

Updating comment to match code, since it no longer checks for existence

rolePath := v.config.IntermediatePKIPath + "roles/" + VaultCALeafCertRole
_, err = v.writeNamespaced(v.config.IntermediatePKINamespace, rolePath, map[string]interface{}{
"allow_any_name": true,
Expand Down Expand Up @@ -710,7 +741,7 @@ func (v *VaultProvider) SignIntermediate(csr *x509.CertificateRequest) (string,
func (v *VaultProvider) CrossSignCA(cert *x509.Certificate) (string, error) {
rootPEM, err := v.getCA(v.config.RootPKINamespace, v.config.RootPKIPath)
if err != nil {
return "", err
return "", fmt.Errorf("failed to get root CA: %w", err)
}
rootCert, err := connect.ParseCert(rootPEM)
if err != nil {
Expand Down
105 changes: 105 additions & 0 deletions agent/connect/ca/provider_vault_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,6 +924,111 @@ func TestVaultCAProvider_GenerateIntermediate(t *testing.T) {
require.NotEqual(t, orig, new)
}

func TestVaultCAProvider_VaultManaged(t *testing.T) {

SkipIfVaultNotPresent(t)

const vaultManagedPKIPolicy = `
path "/pki-root/" {
capabilities = [ "read" ]
}

path "/pki-root/root/sign-intermediate" {
capabilities = [ "update" ]
}

path "/pki-intermediate/*" {
capabilities = [ "create", "read", "update", "delete", "list" ]
}

path "auth/token/renew-self" {
capabilities = [ "update" ]
}

path "auth/token/lookup-self" {
capabilities = [ "read" ]
}
`

testVault, err := runTestVault(t)
if err != nil {
t.Fatalf("err: %v", err)
}

testVault.WaitUntilReady(t)

client := testVault.Client()

client.SetToken("root")

// Mount pki root externally
require.NoError(t, client.Sys().Mount("pki-root", &vaultapi.MountInput{
Type: "pki",
Description: "root CA backend for Consul Connect",
Config: vaultapi.MountConfigInput{
MaxLeaseTTL: "12m",
},
}))
_, err = client.Logical().Write("pki-root/root/generate/internal", map[string]interface{}{
"common_name": "testconsul",
})
require.NoError(t, err)

// Mount pki intermediate externally
require.NoError(t, client.Sys().Mount("pki-intermediate", &vaultapi.MountInput{
Type: "pki",
Description: "intermediate CA backend for Consul Connect",
Config: vaultapi.MountConfigInput{
MaxLeaseTTL: "6m",
},
}))

// Generate a policy and token for the VaultProvider to use
require.NoError(t, client.Sys().PutPolicy("consul-ca", vaultManagedPKIPolicy))
tcr := &vaultapi.TokenCreateRequest{
Policies: []string{"consul-ca"},
}
secret, err := testVault.client.Auth().Token().Create(tcr)
require.NoError(t, err)
providerToken := secret.Auth.ClientToken

// We want to test the provider.Configure() step
_, err = createVaultProvider(t, true, testVault.Addr, providerToken, nil)
require.NoError(t, err)
}

func TestVaultCAProvider_ConsulManaged(t *testing.T) {

SkipIfVaultNotPresent(t)

testVault, err := runTestVault(t)
if err != nil {
t.Fatalf("err: %v", err)
}

testVault.WaitUntilReady(t)

client := testVault.Client()

client.SetToken("root")

// We do not configure any mounts and instead let Consul
// be responsible for mounting root and intermediate PKI

// Generate a policy and token for the VaultProvider to use
require.NoError(t, client.Sys().PutPolicy("consul-ca", pkiTestPolicy))
tcr := &vaultapi.TokenCreateRequest{
Policies: []string{"consul-ca"},
}
secret, err := testVault.client.Auth().Token().Create(tcr)
require.NoError(t, err)
providerToken := secret.Auth.ClientToken

// We want to test the provider.Configure() step
_, err = createVaultProvider(t, true, testVault.Addr, providerToken, nil)
require.NoError(t, err)
}

func getIntermediateCertTTL(t *testing.T, caConf *structs.CAConfiguration) time.Duration {
t.Helper()

Expand Down