connect: Enable renewing the intermediate cert in the primary DC #8784
Conversation
| // which means it must never take that lock itself or call anything that does. | ||
| func (s *Server) getIntermediateCAPrimary(provider ca.Provider, newActiveRoot *structs.CARoot) error { | ||
| connectLogger := s.loggers.Named(logging.Connect) | ||
| intermediatePEM, err := provider.GenerateIntermediate() |
There was a problem hiding this comment.
This generates and signs an intermediate cert. I had to look that up which is why I am leaving a note here for others.
kyhavlov
force-pushed
the
renew-intermediate-primary
branch
from
6a57211
to
e13f4af
Compare
kyhavlov
changed the title
kyhavlov
force-pushed
the
renew-intermediate-primary
branch
from
637dc2f
to
a7df749
Compare
kyhavlov
force-pushed
the
renew-intermediate-primary
branch
from
a7df749
to
01ce9f5
Compare
| } | ||
| if rootCert.NotAfter.Before(time.Now()) { | ||
| return "", fmt.Errorf("root certificate is expired") | ||
| } |
There was a problem hiding this comment.
I assume that means that Vault would happily sign a cert with an expired root CA?
There was a problem hiding this comment.
Right, it's because the sign-self-issued endpoint does minimal validation (only checks for CA cert and self-issued) so it was possible for the cert to expire and the provider would still cross-sign with it.
connect: Enable renewing the intermediate cert in the primary DC
| # Run leader tests that require Vault | ||
| gotestsum --format=short-verbose --junitfile "$(TEST_RESULTS_DIR)/gotestsum-report-leader.xml" -- -cover -coverprofile=coverage-leader.txt -run TestLeader_Vault_ ./agent/consul |
There was a problem hiding this comment.
I think we should try and move these tests that require vault into the agent/connect/ca or test/integration packages at some point. But I don't see any reason to block merging this PR. We can revisit once we have a better idea of how we would like to organize our integration tests.
I'm not sure if this extra coverage file will get sent to codecov, I suspect only the coverage.txt will get sent, but that's something we can fix in a follow up as well.
There was a problem hiding this comment.
It definitely feels strange in agent/consul, although it does test internal leader logic. It couldn't go directly in agent/connect/ca but maybe creating a new subpackage in one of these would be the way to go (like agent/consul/integration or something similar, that has tests like this which involve leader functionality and something external like a CA).
| var PrimaryIntermediateProviders = map[string]struct{}{ | ||
| "vault": {}, | ||
| } |
There was a problem hiding this comment.
I really like implementing this kind of thing as a method on the type. Then calling code can use an interface to check if the type has the method:
if _, ok := provider.(interface{
IntermediateCertInPrimaryDC()
}); ok {
...
}But I think we could easily change that later, so I don't want to hold up this bug fix.
There was a problem hiding this comment.
Good idea, I overlooked that pattern when thinking about this 👍
| func SkipIfVaultNotPresent(t testing.T) { | ||
| vaultBinaryName := os.Getenv("VAULT_BINARY_NAME") | ||
| if vaultBinaryName == "" { | ||
| vaultBinaryName = "vault" | ||
| } | ||
|
|
||
| path, err := exec.LookPath(vaultBinaryName) | ||
| if err != nil || path == "" { | ||
| t.Skipf("%q not found on $PATH - download and install to run this test", vaultBinaryName) | ||
| } | ||
| } | ||
|
|
||
| func NewTestVaultServer(t testing.T) *TestVaultServer { |
There was a problem hiding this comment.
I think these kind of helpers are great, but can sometimes make the package harder to use when they are in the godoc together with the production stuff. I generally try to put this type of thing into an internal/test/vaultca type package.
But not a big issue, and easy to do as a follow up.
This PR enables renewing the intermediate certificate in the primary DC for providers that depend on it (currently only Vault) - previously we were renewing intermediates periodically in the secondary DCs but not the primary.
This also adds a check to make sure we aren't using an out of date cert in the primary when cross-signing.