From 632aab1ae434c2e55f1ac8e5e3e1542667ebabe9 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 2 Jul 2018 12:07:46 -0700 Subject: [PATCH] Add a test for the edge case for CA rotation where the current root CA has an external CA URL, but the root CA also has a key (the external URL is preferred), and the swarm update for removes the external URL without changing the certificate and key. This should succeed. Support the edge case for CA rotation where the current root CA has an external CA URL but not key, and the swarm update adds the same cert and key and removes the external CA URL. Previously this would error, because it first made sure that the external CA URLs needed by the old root CA were present, but this now succeeds because no root rotation is needed, a key is just being provided where none was present before. Signed-off-by: Ying Li --- manager/controlapi/ca_rotation.go | 18 +++++++++++ manager/controlapi/ca_rotation_test.go | 44 +++++++++++++++++++++----- 2 files changed, 54 insertions(+), 8 deletions(-) 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