diff --git a/manager/controlapi/ca_rotation.go b/manager/controlapi/ca_rotation.go index d39c7d2b69..91869a8d0a 100644 --- a/manager/controlapi/ca_rotation.go +++ b/manager/controlapi/ca_rotation.go @@ -203,6 +203,24 @@ func validateCAConfig(ctx context.Context, securityConfig *ca.SecurityConfig, cl var oldCertExtCAs []*api.ExternalCA if !hasSigningKey(&cluster.RootCA) { + + // If we are going from external -> internal, but providing the external CA's signing key, + // then we don't need to validate any external CAs. We can in fact abort any outstanding root + // rotations if we are just adding a key. Because we have a key, we don't care if there are + // no external CAs matching the certificate. + if bytes.Equal(normalizedRootCA, newConfig.SigningCACert) && hasSigningKey(newConfig) { + // validate that the key and cert indeed match - if they don't then just fail now rather + // than go through all the external CA URLs, which is a more expensive operation + if _, err := ca.NewRootCA(newConfig.SigningCACert, newConfig.SigningCACert, newConfig.SigningCAKey, ca.DefaultNodeCertExpiration, nil); err != nil { + return nil, err + } + copied := cluster.RootCA.Copy() + copied.CAKey = newConfig.SigningCAKey + copied.RootRotation = nil + copied.LastForcedRotation = newConfig.ForceRotate + return copied, nil + } + oldCertExtCAs, err = validateHasAtLeastOneExternalCA(ctx, extCAs, securityConfig, normalizedRootCA, "current") if err != nil { return nil, err diff --git a/manager/controlapi/ca_rotation_test.go b/manager/controlapi/ca_rotation_test.go index 1fccd14385..c330170c80 100644 --- a/manager/controlapi/ca_rotation_test.go +++ b/manager/controlapi/ca_rotation_test.go @@ -230,6 +230,11 @@ func TestValidateCAConfigInvalidValues(t *testing.T) { }, expectErrorString: "there must be at least one valid, reachable external CA corresponding to the next CA certificate", }, + { + rootCA: initialExternalRootCA, + caConfig: api.CAConfig{}, // removing the current external CA is not supported + expectErrorString: "there must be at least one valid, reachable external CA corresponding to the current CA certificate", + }, { rootCA: initialExternalRootCA, caConfig: api.CAConfig{ @@ -417,7 +422,17 @@ func TestValidateCAConfigValidValues(t *testing.T) { return init } - // These require no rotation, because the cert is exactly the same. + // no change in the CAConfig spec means no rotation + runValidTestCases(t, []*rootCARotationTestCase{ + { + description: "no specified config changes results no root rotation", + rootCA: initialLocalRootCA, + caConfig: api.CAConfig{}, + expectRootCA: initialLocalRootCA, + }, + }, &localRootCA) + + // These require no rotation, because the cert is exactly the same or there is no change specified. testcases := []*rootCARotationTestCase{ { description: "same desired cert and key as current Root CA results in no root rotation", @@ -430,25 +445,36 @@ func TestValidateCAConfigValidValues(t *testing.T) { expectRootCA: getExpectedRootCA(true), }, { - description: "same desired cert as current Root CA but external->internal results in no root rotation and no key -> key", + description: "same desired cert as current Root CA but external->internal (remove external CA is ok) results in no root rotation and no key -> key", rootCA: initialExternalRootCA, caConfig: api.CAConfig{ SigningCACert: uglifyOnePEM(initialLocalRootCA.CACert), SigningCAKey: initialLocalRootCA.CAKey, ForceRotate: 5, + }, + expectRootCA: getExpectedRootCA(true), + }, + { + description: "same desired cert as current Root CA but internal->external results in no root rotation and key -> no key", + rootCA: initialLocalRootCA, + caConfig: api.CAConfig{ + SigningCACert: initialLocalRootCA.CACert, ExternalCAs: []*api.ExternalCA{ { - URL: initExtServer.URL, + URL: initExtServer.URL, + CACert: uglifyOnePEM(initialLocalRootCA.CACert), }, }, + ForceRotate: 5, }, - expectRootCA: getExpectedRootCA(true), + expectRootCA: getExpectedRootCA(false), }, { - description: "same desired cert as current Root CA but internal->external results in no root rotation and key -> no key", + description: "same desired cert and key as current Root CA but adding an external CA results in no root rotation and no key change", rootCA: initialLocalRootCA, caConfig: api.CAConfig{ SigningCACert: initialLocalRootCA.CACert, + SigningCAKey: initialLocalRootCA.CAKey, ExternalCAs: []*api.ExternalCA{ { URL: initExtServer.URL, @@ -457,20 +483,22 @@ func TestValidateCAConfigValidValues(t *testing.T) { }, ForceRotate: 5, }, - expectRootCA: getExpectedRootCA(false), + expectRootCA: getExpectedRootCA(true), }, } runValidTestCases(t, testcases, &localRootCA) - // These will abort root rotation because the desired cert is the same as the current RootCA cert + // These are the same test cases as above, but we are testing that it will abort root rotation because + // the desired cert is the same as the current RootCA cert crossSigned, err := localRootCA.CrossSignCACertificate(rotationCert) require.NoError(t, err) for _, testcase := range testcases { testcase.rootCA = getRootCAWithRotation(testcase.rootCA, rotationCert, rotationKey, crossSigned) } testcases[0].description = "same desired cert and key as current RootCA results in aborting root rotation" - testcases[1].description = "same desired cert, even if external->internal, as current RootCA results in aborting root rotation and no key -> key" + testcases[1].description = "same desired cert as current Root CA but external->internal (remove external CA is ok) results in aborting root rotation and no key -> key" testcases[2].description = "same desired cert, even if internal->external, as current RootCA results in aborting root rotation and key -> no key" + testcases[3].description = "same desired cert and key as current Root CA but adding an external CA results in aborting root rotation and no key change" runValidTestCases(t, testcases, &localRootCA) // These will not change the root rotation because the desired cert is the same as the current to-be-rotated-to cert