Skip to content

Commit 8a140cf

Browse files
committed
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]>
1 parent 911c976 commit 8a140cf

File tree

7 files changed

+131
-94
lines changed

7 files changed

+131
-94
lines changed

Dockerfile

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# syntax=docker/dockerfile:1
22

3-
ARG GO_VERSION=1.18.9
3+
ARG GO_VERSION=1.21.6
44
ARG PROTOC_VERSION=3.11.4
55
ARG GOLANGCI_LINT_VERSION=v1.50.1
66
ARG DEBIAN_FRONTEND=noninteractive

api/types.pb.go

+70-35
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

ca/config_test.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -464,14 +464,14 @@ func TestSecurityConfigUpdateRootCA(t *testing.T) {
464464
require.Error(t, err)
465465
err = <-actualErrChan
466466
require.Error(t, err)
467-
require.IsType(t, x509.UnknownAuthorityError{}, err)
467+
require.ErrorAs(t, err, &x509.UnknownAuthorityError{})
468468

469469
_, actualErrChan, err = tlsGRPCDial(tc.Context, l.Addr().String(), tcConfig.ClientTLSCreds)
470470
defer close(actualErrChan)
471471
require.Error(t, err)
472472
err = <-actualErrChan
473473
require.Error(t, err)
474-
require.IsType(t, x509.UnknownAuthorityError{}, err)
474+
require.ErrorAs(t, err, &x509.UnknownAuthorityError{})
475475

476476
// update the root CA on the "original security config to support both the old root
477477
// and the "new root" (the testing CA root). Also make sure this root CA has an
@@ -640,7 +640,7 @@ func TestRenewTLSConfigUpdatesRootOnUnknownAuthError(t *testing.T) {
640640
default:
641641
crossSigneds[i], err = cas[i-1].CrossSignCACertificate(certs[i])
642642
require.NoError(t, err)
643-
cas[i], err = ca.NewRootCA(certs[i-1], certs[i], keys[i], ca.DefaultNodeCertExpiration, crossSigneds[i])
643+
cas[i], err = ca.NewRootCA(certs[i-1], crossSigneds[i], keys[i], ca.DefaultNodeCertExpiration, crossSigneds[i])
644644
require.NoError(t, err)
645645
}
646646
}
@@ -652,7 +652,7 @@ func TestRenewTLSConfigUpdatesRootOnUnknownAuthError(t *testing.T) {
652652
CACert: certs[0],
653653
CAKey: keys[0],
654654
RootRotation: &api.RootRotation{
655-
CACert: certs[1],
655+
CACert: crossSigneds[1],
656656
CAKey: keys[1],
657657
CrossSignedCACert: crossSigneds[1],
658658
},

ca/server_test.go

+5-3
Original file line numberDiff line numberDiff line change
@@ -429,8 +429,9 @@ type clusterObjToUpdate struct {
429429
externalCertSignedBy []byte
430430
}
431431

432-
// When the SecurityConfig is updated with a new TLS keypair, the server automatically uses that keypair to contact
433-
// the external CA
432+
// TestServerExternalCAGetsTLSKeypairUpdates tests that when the SecurityConfig
433+
// is updated with a new TLS keypair, the server automatically uses that
434+
// keypair to contact the external CA
434435
func TestServerExternalCAGetsTLSKeypairUpdates(t *testing.T) {
435436
t.Parallel()
436437

@@ -473,12 +474,13 @@ func TestServerExternalCAGetsTLSKeypairUpdates(t *testing.T) {
473474
require.NoError(t, testutils.PollFuncWithTimeout(nil, func() error {
474475
externalCA := tc.CAServer.ExternalCA()
475476
// wait for the credentials for the external CA to update
477+
log.G(tc.Context).Warn("making external CA sign request")
476478
if _, err = externalCA.Sign(tc.Context, req); err == nil {
477479
return errors.New("external CA creds haven't updated yet to be invalid")
478480
}
479481
return nil
480482
}, 2*time.Second))
481-
require.Contains(t, errors.Cause(err).Error(), "remote error: tls: bad certificate")
483+
require.Contains(t, errors.Cause(err).Error(), "remote error: tls: expired certificate")
482484
}
483485

484486
func TestCAServerUpdateRootCA(t *testing.T) {

manager/controlapi/ca_rotation.go

+16-2
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ func hasSigningKey(a interface{}) bool {
3737
// Creates a cross-signed intermediate and new api.RootRotation object.
3838
// This function assumes that the root cert and key and the external CAs have already been validated.
3939
func newRootRotationObject(ctx context.Context, securityConfig *ca.SecurityConfig, apiRootCA *api.RootCA, newCARootCA ca.RootCA, extCAs []*api.ExternalCA, version uint64) (*api.RootCA, error) {
40+
log.G(ctx).Info("calls newRootRotationObject")
4041
var (
4142
rootCert, rootKey, crossSignedCert []byte
4243
newRootHasSigner bool
@@ -53,6 +54,7 @@ func newRootRotationObject(ctx context.Context, securityConfig *ca.SecurityConfi
5354
// a root rotation is already in progress)
5455
switch {
5556
case hasSigningKey(apiRootCA):
57+
log.G(ctx).Info("takes hasSigningKey branch")
5658
var oldRootCA ca.RootCA
5759
oldRootCA, err = ca.NewRootCA(apiRootCA.CACert, apiRootCA.CACert, apiRootCA.CAKey, ca.DefaultNodeCertExpiration, nil)
5860
if err == nil {
@@ -175,8 +177,14 @@ func getNormalizedExtCAs(caConfig *api.CAConfig, normalizedCurrentRootCACert []b
175177
// object as is
176178
// - we want to generate a new internal CA cert and key (force rotation value has changed), and we return the updated RootCA
177179
// object
178-
// 3. Signing cert and key have been provided: validate that these match (the cert and key match). Otherwise, return an error.
179-
// 4. Return the updated RootCA object according to the following criteria:
180+
// 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
181+
// trust chain validation changed, and a chain including two certs with the same key will not validate. This case would
182+
// usually occur when reissuing the same cert with a later expiration date. Because of this validation failure, our root
183+
// rotation algorithm fails. While it might be possible to adjust the rotation procedure to accommodate such a cert change,
184+
// it is somewhat of an edge case, and, more importantly, we do not currently possess the cryptographic expertise to safely
185+
// make such a change. So, as a result, this operation is disallowed. The new root cert must have a new key.
186+
// 4. Signing cert and key have been provided: validate that these match (the cert and key match). Otherwise, return an error.
187+
// 5. Return the updated RootCA object according to the following criteria:
180188
// - If the desired cert is the same as the current CA cert then abort any outstanding rotations. The current signing key
181189
// is replaced with the desired signing key (this could lets us switch between external->internal or internal->external
182190
// 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
289297
return copied, nil
290298
}
291299

300+
// See step 3 in the doc comment. We cannot upgrade a cert with the same
301+
// key.
302+
if len(newConfig.SigningCAKey) > 0 && bytes.Equal(newConfig.SigningCAKey, cluster.RootCA.CAKey) {
303+
return nil, status.Errorf(codes.InvalidArgument, "Cannot update to a cert with an identical key")
304+
}
305+
292306
// check if this is the same desired cert as an existing root rotation
293307
if r := cluster.RootCA.RootRotation; r != nil && bytes.Equal(ca.NormalizePEMs(r.CACert), newConfig.SigningCACert) {
294308
copied := cluster.RootCA.Copy()

0 commit comments

Comments
 (0)