Fix cryptographic certificates for post go 1.19

Go 1.19 changed the way cryptographic certificates were verified, which
broke a certain edge case of root CA rotation. This edge case is now
disallowed.

Signed-off-by: Drew Erny <[email protected]>

Author: dperny

Dockerfile

@@ -1,6 +1,6 @@
 # syntax=docker/dockerfile:1
 
-ARG GO_VERSION=1.18.9
+ARG GO_VERSION=1.21.6
 ARG PROTOC_VERSION=3.11.4
 ARG GOLANGCI_LINT_VERSION=v1.50.1
 ARG DEBIAN_FRONTEND=noninteractive

ca/config_test.go

@@ -460,18 +460,19 @@ func TestSecurityConfigUpdateRootCA(t *testing.T) {
 	// dialer, so that grpc does not attempt to load balance/retry the connection - this way the x509 errors can be
 	// surfaced.
 	_, actualErrChan, err := tlsGRPCDial(tc.Context, tc.Addr, secConfig.ClientTLSCreds)
-	defer close(actualErrChan)
+	// defer close(actualErrChan)
 	require.Error(t, err)
 	err = <-actualErrChan
 	require.Error(t, err)
-	require.IsType(t, x509.UnknownAuthorityError{}, err)
+	require.ErrorAs(t, err, &x509.UnknownAuthorityError{})
+	// require.IsType(t, x509.UnknownAuthorityError{}, err)
 
 	_, actualErrChan, err = tlsGRPCDial(tc.Context, l.Addr().String(), tcConfig.ClientTLSCreds)
-	defer close(actualErrChan)
+	// defer close(actualErrChan)
 	require.Error(t, err)
 	err = <-actualErrChan
 	require.Error(t, err)
-	require.IsType(t, x509.UnknownAuthorityError{}, err)
+	require.ErrorAs(t, err, &x509.UnknownAuthorityError{})
 
 	// update the root CA on the "original security config to support both the old root
 	// and the "new root" (the testing CA root).  Also make sure this root CA has an
@@ -640,7 +641,7 @@ func TestRenewTLSConfigUpdatesRootOnUnknownAuthError(t *testing.T) {
 		default:
 			crossSigneds[i], err = cas[i-1].CrossSignCACertificate(certs[i])
 			require.NoError(t, err)
-			cas[i], err = ca.NewRootCA(certs[i-1], certs[i], keys[i], ca.DefaultNodeCertExpiration, crossSigneds[i])
+			cas[i], err = ca.NewRootCA(certs[i-1], crossSigneds[i], keys[i], ca.DefaultNodeCertExpiration, crossSigneds[i])
 			require.NoError(t, err)
 		}
 	}
@@ -652,7 +653,7 @@ func TestRenewTLSConfigUpdatesRootOnUnknownAuthError(t *testing.T) {
 		CACert: certs[0],
 		CAKey:  keys[0],
 		RootRotation: &api.RootRotation{
-			CACert:            certs[1],
+			CACert:            crossSigneds[1],
 			CAKey:             keys[1],
 			CrossSignedCACert: crossSigneds[1],
 		},

ca/server_test.go

@@ -429,8 +429,9 @@ type clusterObjToUpdate struct {
 	externalCertSignedBy []byte
 }
 
-// When the SecurityConfig is updated with a new TLS keypair, the server automatically uses that keypair to contact
-// the external CA
+// TestServerExternalCAGetsTLSKeypairUpdates tests that when the SecurityConfig
+// is updated with a new TLS keypair, the server automatically uses that
+// keypair to contact the external CA
 func TestServerExternalCAGetsTLSKeypairUpdates(t *testing.T) {
 	t.Parallel()
 
@@ -473,12 +474,13 @@ func TestServerExternalCAGetsTLSKeypairUpdates(t *testing.T) {
 	require.NoError(t, testutils.PollFuncWithTimeout(nil, func() error {
 		externalCA := tc.CAServer.ExternalCA()
 		// wait for the credentials for the external CA to update
+		log.G(tc.Context).Warn("making external CA sign request")
 		if _, err = externalCA.Sign(tc.Context, req); err == nil {
 			return errors.New("external CA creds haven't updated yet to be invalid")
 		}
 		return nil
 	}, 2*time.Second))
-	require.Contains(t, errors.Cause(err).Error(), "remote error: tls: bad certificate")
+	require.Contains(t, errors.Cause(err).Error(), "remote error: tls: expired certificate")
 }
 
 func TestCAServerUpdateRootCA(t *testing.T) {

manager/controlapi/ca_rotation.go

@@ -37,6 +37,7 @@ func hasSigningKey(a interface{}) bool {
 // Creates a cross-signed intermediate and new api.RootRotation object.
 // This function assumes that the root cert and key and the external CAs have already been validated.
 func newRootRotationObject(ctx context.Context, securityConfig *ca.SecurityConfig, apiRootCA *api.RootCA, newCARootCA ca.RootCA, extCAs []*api.ExternalCA, version uint64) (*api.RootCA, error) {
+	log.G(ctx).Info("calls newRootRotationObject")
 	var (
 		rootCert, rootKey, crossSignedCert []byte
 		newRootHasSigner                   bool
@@ -53,6 +54,7 @@ func newRootRotationObject(ctx context.Context, securityConfig *ca.SecurityConfi
 	// a root rotation is already in progress)
 	switch {
 	case hasSigningKey(apiRootCA):
+		log.G(ctx).Info("takes hasSigningKey branch")
 		var oldRootCA ca.RootCA
 		oldRootCA, err = ca.NewRootCA(apiRootCA.CACert, apiRootCA.CACert, apiRootCA.CAKey, ca.DefaultNodeCertExpiration, nil)
 		if err == nil {
@@ -175,8 +177,14 @@ func getNormalizedExtCAs(caConfig *api.CAConfig, normalizedCurrentRootCACert []b
 //     object as is
 //     - we want to generate a new internal CA cert and key (force rotation value has changed), and we return the updated RootCA
 //     object
-//  3. Signing cert and key have been provided: validate that these match (the cert and key match). Otherwise, return an error.
-//  4. Return the updated RootCA object according to the following criteria:
+//  3. Check if the cert is the same key. We cannot rotate to a cert with the same key. As of go 1.19, the logic for certificate
+//     trust chain validation changed, and a chain including two certs with the same key will not validate. This case would
+//     usually occur when reissuing the same cert with a later expiration date. Because of this validation failure, our root
+//     rotation algorithm fails. While it might be possible to adjust the rotation procedure to accommodate such a cert change,
+//     it is somewhat of an edge case, and, more importantly, we do not currently possess the cryptographic expertise to safely
+//     make such a change. So, as a result, this operation is disallowed. The new root cert must have a new key.
+//  4. Signing cert and key have been provided: validate that these match (the cert and key match). Otherwise, return an error.
+//  5. Return the updated RootCA object according to the following criteria:
 //     - If the desired cert is the same as the current CA cert then abort any outstanding rotations. The current signing key
 //     is replaced with the desired signing key (this could lets us switch between external->internal or internal->external
 //     without an actual CA rotation, which is not needed because any leaf cert issued with one CA cert can be validated using
@@ -289,6 +297,12 @@ func validateCAConfig(ctx context.Context, securityConfig *ca.SecurityConfig, cl
 		return copied, nil
 	}
 
+	// See step 3 in the doc comment. We cannot upgrade a cert with the same
+	// key.
+	if bytes.Equal(newConfig.SigningCAKey, cluster.RootCA.CAKey) {
+		return nil, status.Errorf(codes.InvalidArgument, "Cannot update to a cert with an identical key")
+	}
+
 	// check if this is the same desired cert as an existing root rotation
 	if r := cluster.RootCA.RootRotation; r != nil && bytes.Equal(ca.NormalizePEMs(r.CACert), newConfig.SigningCACert) {
 		copied := cluster.RootCA.Copy()

manager/controlapi/ca_rotation_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto/x509"
 	"encoding/pem"
+	"os"
 	"testing"
 	"time"
 
@@ -12,10 +13,13 @@ import (
 	"github.com/moby/swarmkit/v2/api"
 	"github.com/moby/swarmkit/v2/ca"
 	"github.com/moby/swarmkit/v2/ca/testutils"
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
+
+	"github.com/moby/swarmkit/v2/log"
 )
 
 type rootCARotationTestCase struct {
@@ -315,15 +319,19 @@ func TestValidateCAConfigInvalidValues(t *testing.T) {
 }
 
 func runValidTestCases(t *testing.T, testcases []*rootCARotationTestCase, localRootCA *ca.RootCA) {
+	logrus.SetLevel(logrus.DebugLevel)
+	logrus.SetOutput(os.Stdout)
+	ctx := log.WithLogger(context.Background(), log.L.WithField("testname", t.Name()))
 	for _, valid := range testcases {
+		casectx := log.WithField(ctx, "testcase", valid.description)
 		cluster := &api.Cluster{
 			RootCA: *valid.rootCA.Copy(),
 			Spec: api.ClusterSpec{
 				CAConfig: valid.caConfig,
 			},
 		}
 		secConfig := getSecurityConfig(t, localRootCA, cluster)
-		result, err := validateCAConfig(context.Background(), secConfig, cluster)
+		result, err := validateCAConfig(casectx, secConfig, cluster)
 		require.NoError(t, err, valid.description)
 
 		// ensure that the cluster was not mutated
@@ -346,8 +354,12 @@ func runValidTestCases(t *testing.T, testcases []*rootCARotationTestCase, localR
 			// make sure the cross-signed cert is signed by the current root CA (and not an intermediate, if a root rotation is in progress)
 			parsedCross, err := helpers.ParseCertificatePEM(result.RootRotation.CrossSignedCACert) // there should just be one
 			require.NoError(t, err)
+
+			log.G(casectx).Debugf("localRootCA:%s", localRootCA.Certs)
+			log.G(casectx).Debugf("CACert:%s", result.RootRotation.CACert)
+			log.G(casectx).Debugf("CrossSigned:%s", result.RootRotation.CrossSignedCACert)
 			_, err = parsedCross.Verify(x509.VerifyOptions{Roots: localRootCA.Pool})
-			require.NoError(t, err, valid.description)
+			assert.NoError(t, err, valid.description)
 
 			// if we are expecting generated certs or root rotation, we can expect the expected root CA has a root rotation
 			result.RootRotation.CrossSignedCACert = valid.expectRootCA.RootRotation.CrossSignedCACert
@@ -365,14 +377,30 @@ func runValidTestCases(t *testing.T, testcases []*rootCARotationTestCase, localR
 	}
 }
 
+func printCert(t *testing.T, pemData []byte) {
+	t.Helper()
+
+	block, _ := pem.Decode(pemData)
+	cert, err := x509.ParseCertificate(block.Bytes)
+	if err != nil {
+		t.Error(err)
+	}
+
+	cert.RawSubject = nil
+	cert.Raw = nil
+	cert.RawIssuer = nil
+	cert.RawSubjectPublicKeyInfo = nil
+	cert.RawTBSCertificate = nil
+	cert.Signature = nil
+	t.Logf("%+v", cert)
+}
+
 func TestValidateCAConfigValidValues(t *testing.T) {
 	t.Parallel()
 	localRootCA, err := ca.NewRootCA(testutils.ECDSA256SHA256Cert, testutils.ECDSA256SHA256Cert, testutils.ECDSA256Key,
 		ca.DefaultNodeCertExpiration, nil)
 	require.NoError(t, err)
 
-	parsedCert, err := helpers.ParseCertificatePEM(testutils.ECDSA256SHA256Cert)
-	require.NoError(t, err)
 	parsedKey, err := helpers.ParsePrivateKeyPEM(testutils.ECDSA256Key)
 	require.NoError(t, err)
 
@@ -536,8 +564,7 @@ func TestValidateCAConfigValidValues(t *testing.T) {
 
 	// These all require a new root rotation because the desired cert is different, even if it has the same key and/or subject as the current
 	// cert or the current-to-be-rotated cert.
-	renewedInitialCert, err := initca.RenewFromSigner(parsedCert, parsedKey)
-	require.NoError(t, err)
+	time.Sleep(5 * time.Second)
 	parsedRotationCert, err := helpers.ParseCertificatePEM(rotationCert)
 	require.NoError(t, err)
 	parsedRotationKey, err := helpers.ParsePrivateKeyPEM(rotationKey)
@@ -554,49 +581,6 @@ func TestValidateCAConfigValidValues(t *testing.T) {
 	defer differentExtServer.Stop()
 	require.NoError(t, differentExtServer.EnableCASigning())
 	testcases = []*rootCARotationTestCase{
-		{
-			description: "desired cert being a renewed current cert and key results in a root rotation because the cert has changed",
-			rootCA:      initialLocalRootCA,
-			caConfig: api.CAConfig{
-				SigningCACert: uglifyOnePEM(renewedInitialCert),
-				SigningCAKey:  initialLocalRootCA.CAKey,
-				ForceRotate:   5,
-			},
-			expectRootCA:         getRootCAWithRotation(expectedBaseRootCA, renewedInitialCert, initialLocalRootCA.CAKey, nil),
-			expectGeneratedCross: true,
-		},
-		{
-			description: "desired cert being a renewed current cert, external->internal results in a root rotation because the cert has changed",
-			rootCA:      initialExternalRootCA,
-			caConfig: api.CAConfig{
-				SigningCACert: uglifyOnePEM(renewedInitialCert),
-				SigningCAKey:  initialLocalRootCA.CAKey,
-				ForceRotate:   5,
-				ExternalCAs: []*api.ExternalCA{
-					{
-						URL: initExtServer.URL,
-					},
-				},
-			},
-			expectRootCA:         getRootCAWithRotation(getExpectedRootCA(false), renewedInitialCert, initialLocalRootCA.CAKey, nil),
-			expectGeneratedCross: true,
-		},
-		{
-			description: "desired cert being a renewed current cert, internal->external results in a root rotation because the cert has changed",
-			rootCA:      initialLocalRootCA,
-			caConfig: api.CAConfig{
-				SigningCACert: append([]byte("\n\n"), renewedInitialCert...),
-				ForceRotate:   5,
-				ExternalCAs: []*api.ExternalCA{
-					{
-						URL:    initExtServer.URL,
-						CACert: uglifyOnePEM(renewedInitialCert),
-					},
-				},
-			},
-			expectRootCA:         getRootCAWithRotation(expectedBaseRootCA, renewedInitialCert, nil, nil),
-			expectGeneratedCross: true,
-		},
 		{
 			description: "desired cert being a renewed rotation RootCA cert + rotation key results in replaced root rotation because the cert has changed",
 			rootCA:      getRootCAWithRotation(initialLocalRootCA, rotationCert, rotationKey, crossSigned),