Handle an edge case in CA rotation where we reclaim CA service from an external CA

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

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