Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cryptographic certificates for post go 1.19 #3161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
@@ -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
Expand Down
105 changes: 70 additions & 35 deletions api/types.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions ca/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -464,14 +464,14 @@ func TestSecurityConfigUpdateRootCA(t *testing.T) {
require.Error(t, err)
err = <-actualErrChan
require.Error(t, err)
require.IsType(t, x509.UnknownAuthorityError{}, err)
require.ErrorAs(t, err, &x509.UnknownAuthorityError{})

_, actualErrChan, err = tlsGRPCDial(tc.Context, l.Addr().String(), tcConfig.ClientTLSCreds)
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
Expand Down Expand Up @@ -640,7 +640,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)
}
}
Expand All @@ -652,7 +652,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],
},
Expand Down
8 changes: 5 additions & 3 deletions ca/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -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) {
Expand Down
18 changes: 16 additions & 2 deletions manager/controlapi/ca_rotation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 len(newConfig.SigningCAKey) > 0 && 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()
Expand Down