Detect Vault 1.11+ import in secondary datacenters and update default issuer #15661
Conversation
github-actions
bot
added
theme/connect
| // | ||
| // The second return value is an opaque string meant to be passed back to | ||
| // the subsequent call to SetIntermediate. | ||
| GenerateIntermediateCSR() (string, string, error) |
There was a problem hiding this comment.
For the vault provider we needed a way to pass extra data from the GenerateIntermediateCSR step to the SetIntermediate step so an extra output/input pair was setup to thread an opaque string through.
The vault provider will pass a keyId.
| @@ -924,6 +924,76 @@ func TestVaultCAProvider_GenerateIntermediate(t *testing.T) { | |||
| require.NotEqual(t, orig, new) | |||
| } | |||
|
|
|||
| func TestVaultCAProvider_GenerateIntermediate_inSecondary(t *testing.T) { | |||
There was a problem hiding this comment.
This is a mirror of the test added in #15253 to validate the primary side of the bug.
| buf.WriteString(lib.EnsureTrailingNewline(rootPEM)) | ||
| buf.WriteString(lib.EnsureTrailingNewline(cert)) | ||
| if !strings.Contains(strings.TrimSpace(cert), strings.TrimSpace(rootPEM)) { | ||
| // Vault < v1.11 included the root in the output of sign-intermediate. |
There was a problem hiding this comment.
NOTE: this needed to be done to get the tests that depend on this to properly pass on vault 1.11+
| @@ -1052,7 +1052,7 @@ func (c *CAManager) primaryRenewIntermediate(provider ca.Provider, newActiveRoot | |||
| // provider. | |||
| // Should only be called while the state lock is held by setting the state to non-ready. | |||
| func (c *CAManager) secondaryRequestNewSigningCert(provider ca.Provider, newActiveRoot *structs.CARoot) error { | |||
| csr, err := provider.GenerateIntermediateCSR() | |||
| csr, opaque, err := provider.GenerateIntermediateCSR() | |||
There was a problem hiding this comment.
Here we see the actual data ferrying from one to the other.
| "certificate": intermediatePEM, | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Vault 1.11+ will return a non-nil response from intermediate/set-signed |
There was a problem hiding this comment.
This is lifted verbatim from the former fix.
There was a problem hiding this comment.
Should we wrap this shared bit of code in a common for DRYness?
There was a problem hiding this comment.
That already is in setDefaultIntermediateIssuer
There was a problem hiding this comment.
Yeah when I was refactoring setDefaultIntermediateIssuer I thought about bringing the nil check inside the function. I concluded that it's more useful to be explicitly loud about some behavioural assumptions than to call setDefaultIntermediateIssuer for all versions but secretly have it be a no-op for some versions.
| # run integration tests for the connect ca providers | ||
| test-connect-ca-providers: | ||
| # run integration tests for the connect ca providers with vault | ||
| vault-integration-test: |
There was a problem hiding this comment.
The new name is more right than the former name given what the make target does:
test-connect-ca-providers:
ifeq ("$(CIRCLECI)","true")
# Run in CI
gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report.xml" -- -cover -coverprofile=coverage.txt ./agent/connect/ca
# Run leader tests that require Vault
gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report-leader.xml" -- -cover -coverprofile=coverage-leader.txt -run Vault ./agent/consul
# Run agent tests that require Vault
gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report-agent.xml" -- -cover -coverprofile=coverage-agent.txt -run Vault ./agent
else
# Run locally
@echo "Running /agent/connect/ca tests in verbose mode"
@go test -v ./agent/connect/ca
@go test -v ./agent/consul -run Vault
@go test -v ./agent -run Vault
endif
rboyer
requested review from
cipherboy,
kisunji,
jkirschner-hashicorp and
skpratt
rboyer
force-pushed
the
fix-vault-ca-secondary-renewal
branch
from
876d610
to
e3b26c4
Compare
| @@ -1067,7 +1077,6 @@ workflows: | |||
| name: "lint-32bit" | |||
| go-arch: "386" | |||
| <<: *filter-ignore-non-go-branches | |||
| - test-connect-ca-providers: *filter-ignore-non-go-branches | |||
There was a problem hiding this comment.
I moved it to live with the other integrations.
| @@ -75,6 +75,8 @@ type AWSProvider struct { | |||
| logger hclog.Logger | |||
| } | |||
|
|
|||
| var _ Provider = (*AWSProvider)(nil) | |||
There was a problem hiding this comment.
I noticed this change was added to all 3 providers. What is the consequence of this change? (Why initialize a nil pointer to a Provider?)
There was a problem hiding this comment.
It's a compile-time no-op type assertion so a failure to make the provider match the Provider interface fails in the definition of the struct not when you try to use it.
There was a problem hiding this comment.
Ah - that's really cool!
Description
The fix outlined and merged in #15253 fixed the issue as it occurs in the primary DC. There is a similar issue that arises when vault is used as the Connect CA in a secondary datacenter that is fixed by this PR.
Additionally: this PR adds support to run the existing suite of vault related integration tests against the last 4 versions of vault (1.9, 1.10, 1.11, 1.12)