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.

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

Detect Vault 1.11+ import, update default issuer #15253

Merged
Merged
97 changes: 87 additions & 10 deletions agent/connect/ca/provider_vault.go
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,8 @@ func (v *VaultProvider) GenerateIntermediateCSR() (string, error) {
"cannot generate an intermediate CSR")
}

return v.generateIntermediateCSR()
csr, _, err := v.generateIntermediateCSR()
return csr, err
}

func (v *VaultProvider) setupIntermediatePKIPath() error {
Expand Down Expand Up @@ -406,11 +407,13 @@ func (v *VaultProvider) setupIntermediatePKIPath() error {
return err
}

func (v *VaultProvider) generateIntermediateCSR() (string, error) {
// generateIntermediateCSR returns the CSR and key_id (only present in
// Vault 1.11+) or any errors encountered.
func (v *VaultProvider) generateIntermediateCSR() (string, string, error) {
// Generate a new intermediate CSR for the root to sign.
uid, err := connect.CompactUID()
if err != nil {
return "", err
return "", "", err
}
data, err := v.writeNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"intermediate/generate/internal", map[string]interface{}{
"common_name": connect.CACN("vault", uid, v.clusterID, v.isPrimary),
Expand All @@ -419,17 +422,26 @@ func (v *VaultProvider) generateIntermediateCSR() (string, error) {
"uri_sans": v.spiffeID.URI().String(),
})
if err != nil {
return "", err
return "", "", err
}
if data == nil || data.Data["csr"] == "" {
return "", fmt.Errorf("got empty value when generating intermediate CSR")
return "", "", fmt.Errorf("got empty value when generating intermediate CSR")
}
csr, ok := data.Data["csr"].(string)
if !ok {
return "", fmt.Errorf("csr result is not a string")
return "", "", fmt.Errorf("csr result is not a string")
}

return csr, nil
// Vault 1.11+ will return a "key_id" field which helps
// identify the correct issuer to set as default.
// https://github.com/hashicorp/vault/blob/e445c8b4f58dc20a0316a7fd1b5725b401c3b17a/builtin/logical/pki/path_intermediate.go#L154
if rawkeyId, ok := data.Data["key_id"]; ok {
keyId, ok := rawkeyId.(string)
if !ok {
return "", "", fmt.Errorf("key_id is not a string")
}
return csr, keyId, nil
}
return csr, "", nil
}

// SetIntermediate writes the incoming intermediate and root certificates to the
Expand Down Expand Up @@ -531,7 +543,7 @@ func (v *VaultProvider) getCAChain(namespace, path string) (string, error) {
// 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) GenerateIntermediate() (string, error) {
csr, err := v.generateIntermediateCSR()
csr, keyId, err := v.generateIntermediateCSR()
if err != nil {
return "", err
}
Expand All @@ -551,16 +563,81 @@ func (v *VaultProvider) GenerateIntermediate() (string, error) {
}

// Set the intermediate backend to use the new certificate.
_, err = v.writeNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"intermediate/set-signed", map[string]interface{}{
importResp, err := v.writeNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"intermediate/set-signed", map[string]interface{}{
"certificate": intermediate.Data["certificate"],
})
if err != nil {
return "", err
}

// Vault 1.11+ will return a non-nil response from intermediate/set-signed
if importResp != nil {
cipherboy marked this conversation as resolved.
Show resolved Hide resolved
err := v.setDefaultIntermediateIssuer(importResp, keyId)
if err != nil {
return "", fmt.Errorf("failed to update default intermediate issuer: %w", err)
}
}

return v.ActiveIntermediate()
}

// setDefaultIntermediateIssuer updates the default issuer for
// intermediate CA since Vault, as part of its 1.11+ support for
// multiple issuers, no longer overwrites the default issuer when
// generateIntermediateCSR (intermediate/generate/internal) is called.
//
// The response we get from calling [/intermediate/set-signed]
// should contain a "mapping" data field we can use to cross-reference
// with the keyId returned when calling [/intermediate/generate/internal].
//
// [/intermediate/set-signed]: https://developer.hashicorp.com/vault/api-docs/secret/pki#import-ca-certificates-and-keys
// [/intermediate/generate/internal]: https://developer.hashicorp.com/vault/api-docs/secret/pki#generate-intermediate-csr
func (v *VaultProvider) setDefaultIntermediateIssuer(vaultResp *vaultapi.Secret, keyId string) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking good, like this refactor!

if vaultResp.Data["mapping"] == nil {
return fmt.Errorf("expected Vault response data to have a 'mapping' key")
}
if keyId == "" {
return fmt.Errorf("expected non-empty keyId")
}

mapping, ok := vaultResp.Data["mapping"].(map[string]any)
if !ok {
return fmt.Errorf("unexpected type for 'mapping' value in Vault response")
}

var intermediateId string
// The value in this KV pair is called "key"
for issuer, key := range mapping {
if key == keyId {
cipherboy marked this conversation as resolved.
Show resolved Hide resolved
// Expect to find the key_id we got from Vault when we
// generated the intermediate CSR.
intermediateId = issuer
break
}
}
if intermediateId == "" {
return fmt.Errorf("could not find key_id %q in response from vault", keyId)
}

// For Vault 1.11+ it is important to GET then POST to avoid clobbering fields
// like `default_follows_latest_issuer`.
// https://developer.hashicorp.com/vault/api-docs/secret/pki#default_follows_latest_issuer
resp, err := v.readNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"config/issuers")
if err != nil {
return fmt.Errorf("could not read from /config/issuers: %w", err)
}
issuersConf := resp.Data
// Overwrite the default issuer
issuersConf["default"] = intermediateId

_, err = v.writeNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath+"config/issuers", issuersConf)
if err != nil {
return fmt.Errorf("could not write default issuer to /config/issuers: %w", err)
}

return nil
}

// Sign calls the configured role in the intermediate PKI backend to issue
// a new leaf certificate based on the provided CSR, with the issuing
// intermediate CA cert attached.
Expand Down
23 changes: 23 additions & 0 deletions agent/connect/ca/provider_vault_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,29 @@ func TestVaultProvider_ReconfigureIntermediateTTL(t *testing.T) {
require.Equal(t, 333*3600, mountConfig.MaxLeaseTTL)
}

func TestVaultCAProvider_GenerateIntermediate(t *testing.T) {
Copy link
Contributor

@kisunji kisunji Nov 17, 2022

Choose a reason for hiding this comment

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

Unsure if Vault tests will run so I'm pasting local findings here:

Vault 1.10.8 without changes (expected to pass in Vault <1.11)

=== RUN   TestVaultCAProvider_RenewIntermediate
[INFO] freeport: detected ephemeral port range of [49152, 65535]
[INFO] freeport: reducing max blocks from 30 to 26 to avoid the ephemeral port range
[DEBUG] freeport: Test "TestVaultCAProvider_RenewIntermediate" took ports [25001 25002]
[INFO] agent/connect/ca: testing with vault server version: 1.10.8
[DEBUG] freeport: Test "TestVaultCAProvider_RenewIntermediate" returned ports [25001 25002]
--- PASS: TestVaultCAProvider_RenewIntermediate (3.33s)
PASS
ok      github.com/hashicorp/consul/agent/connect/ca    3.681s

Vault 1.10.8 with changes (regression test)

=== RUN   TestVaultCAProvider_RenewIntermediate
[INFO] freeport: detected ephemeral port range of [49152, 65535]
[INFO] freeport: reducing max blocks from 30 to 26 to avoid the ephemeral port range
[DEBUG] freeport: Test "TestVaultCAProvider_RenewIntermediate" took ports [20501 20502]
[INFO] agent/connect/ca: testing with vault server version: 1.10.8
[DEBUG] freeport: Test "TestVaultCAProvider_RenewIntermediate" returned ports [20501 20502]
--- PASS: TestVaultCAProvider_RenewIntermediate (1.33s)
PASS
ok      github.com/hashicorp/consul/agent/connect/ca    1.687s

Vault 1.11.0 without changes

=== RUN   TestVaultCAProvider_RenewIntermediate
[INFO] freeport: detected ephemeral port range of [49152, 65535]
[INFO] freeport: reducing max blocks from 30 to 26 to avoid the ephemeral port range
[DEBUG] freeport: Test "TestVaultCAProvider_RenewIntermediate" took ports [32501 32502]
[INFO] agent/connect/ca: testing with vault server version: 1.11.0
    provider_vault_test.go:918: 
                Error Trace:    /Users/chriskim/code/consul/agent/connect/ca/provider_vault_test.go:918
                Error:          Should not be: "-----BEGIN CERTIFICATE-----\nMIICMDCCAdWgAwIBAgIUE/pOjHz3fDoy9Fgwbw4DYt+8CwAwCgYIKoZIzj0EAwIw\nMDEuMCwGA1UEAxMlcHJpLWhoZDgxdWFjLnZhdWx0LmNhLjExMTExMTExLmNvbnN1\nbDAeFw0yMjExMTcxNzQyMjBaFw0yMzExMTcxNzQyNTBaMDAxLjAsBgNVBAMTJXBy\naS0xanRyZDZ3Yi52YXVsdC5jYS4xMTExMTExMS5jb25zdWwwWTATBgcqhkjOPQIB\nBggqhkjOPQMBBwNCAATKxc/IyicWyhgazqFINH2LHxTPwV6/oyzJJL8ZUqie2VHX\nWQvuVm+SQ7YHT8Hv2/wd9Ji43OIqF/D2iZWX1F9Po4HMMIHJMA4GA1UdDwEB/wQE\nAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBT3BpRIx1gItM2dvzxCxk6K\nGZK+NTAfBgNVHSMEGDAWgBS4Klwhx8S2YHuEwR2pZSQM/S2jMTBmBgNVHREEXzBd\ngiVwcmktMWp0cmQ2d2IudmF1bHQuY2EuMTExMTExMTEuY29uc3VshjRzcGlmZmU6\nLy8xMTExMTExMS0yMjIyLTMzMzMtNDQ0NC01NTU1NTU1NTU1NTUuY29uc3VsMAoG\nCCqGSM49BAMCA0kAMEYCIQCFguARdz2ebI4Qz48tmuXp1/VgE94u+8pJK4wuMYAe\nZgIhAM//Dqv3ofZrmRtJbIx6VgjV15C9KqVOQUhwMlRcTalY\n-----END CERTIFICATE-----\n"
                Test:           TestVaultCAProvider_RenewIntermediate
[DEBUG] freeport: Test "TestVaultCAProvider_RenewIntermediate" returned ports [32501 32502]
--- FAIL: TestVaultCAProvider_RenewIntermediate (1.82s)
FAIL
FAIL    github.com/hashicorp/consul/agent/connect/ca    2.215s
FAIL

Vault 1.11.0 with changes

=== RUN   TestVaultCAProvider_RenewIntermediate
[INFO] freeport: detected ephemeral port range of [49152, 65535]
[INFO] freeport: reducing max blocks from 30 to 26 to avoid the ephemeral port range
[DEBUG] freeport: Test "TestVaultCAProvider_RenewIntermediate" took ports [29501 29502]
[INFO] agent/connect/ca: testing with vault server version: 1.11.0
[DEBUG] freeport: Test "TestVaultCAProvider_RenewIntermediate" returned ports [29501 29502]
--- PASS: TestVaultCAProvider_RenewIntermediate (1.30s)
PASS
ok      github.com/hashicorp/consul/agent/connect/ca    1.660s

Also tested with Vault 1.12.1 with same results as 1.11.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for including this detail :)


SkipIfVaultNotPresent(t)

provider, testVault := testVaultProviderWithConfig(t, true, nil)
_ = testVault

orig, err := provider.ActiveIntermediate()
require.NoError(t, err)

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow you weren't joking this did turn out to be easier than we thought! Nice work!

// This test was created to ensure that our calls to Vault
// returns a new Intermediate certificate and further calls
// to ActiveIntermediate return the same new cert.
new, err := provider.GenerateIntermediate()
require.NoError(t, err)

newActive, err := provider.ActiveIntermediate()
require.NoError(t, err)

require.Equal(t, new, newActive)
require.NotEqual(t, orig, new)
}

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

Expand Down