diff --git a/Dockerfile b/Dockerfile index cee69366d0..6cc46ab28d 100644 --- a/Dockerfile +++ b/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 diff --git a/api/types.pb.go b/api/types.pb.go index 61a544a6b7..3cfe583bb8 100644 --- a/api/types.pb.go +++ b/api/types.pb.go @@ -67,8 +67,9 @@ func (ResourceType) EnumDescriptor() ([]byte, []int) { // Only the manager create a NEW task, and move the task to PENDING and ASSIGNED. // Afterward, the manager must rely on the agent to update the task status // (pre-run: preparing, ready, starting; -// running; -// end-state: complete, shutdown, failed, rejected) +// +// running; +// end-state: complete, shutdown, failed, rejected) type TaskState int32 const ( @@ -1085,6 +1086,7 @@ var xxx_messageInfo_DiscreteGenericResource proto.InternalMessageInfo // be either an integer (e.g: SSD=3) or a string (e.g: SSD=sda1) type GenericResource struct { // Types that are valid to be assigned to Resource: + // // *GenericResource_NamedResourceSpec // *GenericResource_DiscreteResourceSpec Resource isGenericResource_Resource `protobuf_oneof:"resource"` @@ -2112,6 +2114,7 @@ type TaskStatus struct { // Container status contains container specific status information. // // Types that are valid to be assigned to RuntimeStatus: + // // *TaskStatus_Container RuntimeStatus isTaskStatus_RuntimeStatus `protobuf_oneof:"runtime_status"` // HostPorts provides a list of ports allocated at the host @@ -2504,6 +2507,7 @@ type IssuanceStatus struct { State IssuanceStatus_State `protobuf:"varint,1,opt,name=state,proto3,enum=docker.swarmkit.v1.IssuanceStatus_State" json:"state,omitempty"` // Err is set if the Certificate Issuance is in an error state. // The following states should report a companion error: + // // FAILED Err string `protobuf:"bytes,2,opt,name=err,proto3" json:"err,omitempty"` } @@ -3004,6 +3008,7 @@ var xxx_messageInfo_SpreadOver proto.InternalMessageInfo type PlacementPreference struct { // Types that are valid to be assigned to Preference: + // // *PlacementPreference_Spread Preference isPlacementPreference_Preference `protobuf_oneof:"Preference"` } @@ -3435,6 +3440,7 @@ type SecretReference struct { // Target specifies how this secret should be exposed to the task. // // Types that are valid to be assigned to Target: + // // *SecretReference_File Target isSecretReference_Target `protobuf_oneof:"target"` } @@ -3515,6 +3521,7 @@ type ConfigReference struct { // Target specifies how this config should be exposed to the task. // // Types that are valid to be assigned to Target: + // // *ConfigReference_File // *ConfigReference_Runtime Target isConfigReference_Target `protobuf_oneof:"target"` @@ -3824,6 +3831,7 @@ var xxx_messageInfo_Privileges proto.InternalMessageInfo // CredentialSpec for managed service account (Windows only). type Privileges_CredentialSpec struct { // Types that are valid to be assigned to Source: + // // *Privileges_CredentialSpec_File // *Privileges_CredentialSpec_Registry // *Privileges_CredentialSpec_Config @@ -4111,6 +4119,7 @@ type VolumeAccessMode struct { // but the upstream is free to do so. However, one of these MUST be set. // // Types that are valid to be assigned to AccessType: + // // *VolumeAccessMode_Block // *VolumeAccessMode_Mount AccessType isVolumeAccessMode_AccessType `protobuf_oneof:"access_type"` @@ -4336,12 +4345,12 @@ var xxx_messageInfo_VolumeSecret proto.InternalMessageInfo // // Without this two-step process, the following could happen: // -// 1. ControllerPublishVolume is called and the Volume is successfully -// published. -// 2. A crash or leadership change disrupts the cluster before -// the Volume with the updated VolumePublishStatus can be added to the -// store. -// 3. The Task that required the Volume to be published is deleted. +// 1. ControllerPublishVolume is called and the Volume is successfully +// published. +// 2. A crash or leadership change disrupts the cluster before +// the Volume with the updated VolumePublishStatus can be added to the +// store. +// 3. The Task that required the Volume to be published is deleted. // // In this case, the Volume would be published to the Node, but Swarm would be // unaware of this, and would additionally be unaware that the Volume _should_ @@ -4606,21 +4615,27 @@ type TopologyRequirement struct { // accessible from at least one of the requisite topologies. // // Given - // x = number of topologies provisioned volume is accessible from - // n = number of requisite topologies + // + // x = number of topologies provisioned volume is accessible from + // n = number of requisite topologies + // // The CO MUST ensure n >= 1. The SP MUST ensure x >= 1 // If x==n, then the SP MUST make the provisioned volume available to // all topologies from the list of requisite topologies. If it is // unable to do so, the SP MUST fail the CreateVolume call. // For example, if a volume should be accessible from a single zone, // and requisite = - // {"region": "R1", "zone": "Z2"} + // + // {"region": "R1", "zone": "Z2"} + // // then the provisioned volume MUST be accessible from the "region" // "R1" and the "zone" "Z2". // Similarly, if a volume should be accessible from two zones, and // requisite = - // {"region": "R1", "zone": "Z2"}, - // {"region": "R1", "zone": "Z3"} + // + // {"region": "R1", "zone": "Z2"}, + // {"region": "R1", "zone": "Z3"} + // // then the provisioned volume MUST be accessible from the "region" // "R1" and both "zone" "Z2" and "zone" "Z3". // @@ -4629,18 +4644,23 @@ type TopologyRequirement struct { // the CreateVolume call. // For example, if a volume should be accessible from a single zone, // and requisite = - // {"region": "R1", "zone": "Z2"}, - // {"region": "R1", "zone": "Z3"} + // + // {"region": "R1", "zone": "Z2"}, + // {"region": "R1", "zone": "Z3"} + // // then the SP may choose to make the provisioned volume available in // either the "zone" "Z2" or the "zone" "Z3" in the "region" "R1". // Similarly, if a volume should be accessible from two zones, and // requisite = - // {"region": "R1", "zone": "Z2"}, - // {"region": "R1", "zone": "Z3"}, - // {"region": "R1", "zone": "Z4"} + // + // {"region": "R1", "zone": "Z2"}, + // {"region": "R1", "zone": "Z3"}, + // {"region": "R1", "zone": "Z4"} + // // then the provisioned volume MUST be accessible from any combination // of two unique topologies: e.g. "R1/Z2" and "R1/Z3", or "R1/Z2" and - // "R1/Z4", or "R1/Z3" and "R1/Z4". + // + // "R1/Z4", or "R1/Z3" and "R1/Z4". // // If x>n, then the SP MUST make the provisioned volume available from // all topologies from the list of requisite topologies and MAY choose @@ -4649,7 +4669,9 @@ type TopologyRequirement struct { // CreateVolume call. // For example, if a volume should be accessible from two zones, and // requisite = - // {"region": "R1", "zone": "Z2"} + // + // {"region": "R1", "zone": "Z2"} + // // then the provisioned volume MUST be accessible from the "region" // "R1" and the "zone" "Z2" and the SP may select the second zone // independently, e.g. "R1/Z4". @@ -4678,10 +4700,14 @@ type TopologyRequirement struct { // Example 1: // Given a volume should be accessible from a single zone, and // requisite = - // {"region": "R1", "zone": "Z2"}, - // {"region": "R1", "zone": "Z3"} + // + // {"region": "R1", "zone": "Z2"}, + // {"region": "R1", "zone": "Z3"} + // // preferred = - // {"region": "R1", "zone": "Z3"} + // + // {"region": "R1", "zone": "Z3"} + // // then the the SP SHOULD first attempt to make the provisioned volume // available from "zone" "Z3" in the "region" "R1" and fall back to // "zone" "Z2" in the "region" "R1" if that is not possible. @@ -4689,13 +4715,17 @@ type TopologyRequirement struct { // Example 2: // Given a volume should be accessible from a single zone, and // requisite = - // {"region": "R1", "zone": "Z2"}, - // {"region": "R1", "zone": "Z3"}, - // {"region": "R1", "zone": "Z4"}, - // {"region": "R1", "zone": "Z5"} + // + // {"region": "R1", "zone": "Z2"}, + // {"region": "R1", "zone": "Z3"}, + // {"region": "R1", "zone": "Z4"}, + // {"region": "R1", "zone": "Z5"} + // // preferred = - // {"region": "R1", "zone": "Z4"}, - // {"region": "R1", "zone": "Z2"} + // + // {"region": "R1", "zone": "Z4"}, + // {"region": "R1", "zone": "Z2"} + // // then the the SP SHOULD first attempt to make the provisioned volume // accessible from "zone" "Z4" in the "region" "R1" and fall back to // "zone" "Z2" in the "region" "R1" if that is not possible. If that @@ -4708,13 +4738,17 @@ type TopologyRequirement struct { // the volume is accessible from two zones, aka synchronously // replicated), and // requisite = - // {"region": "R1", "zone": "Z2"}, - // {"region": "R1", "zone": "Z3"}, - // {"region": "R1", "zone": "Z4"}, - // {"region": "R1", "zone": "Z5"} + // + // {"region": "R1", "zone": "Z2"}, + // {"region": "R1", "zone": "Z3"}, + // {"region": "R1", "zone": "Z4"}, + // {"region": "R1", "zone": "Z5"} + // // preferred = - // {"region": "R1", "zone": "Z5"}, - // {"region": "R1", "zone": "Z3"} + // + // {"region": "R1", "zone": "Z5"}, + // {"region": "R1", "zone": "Z3"} + // // then the the SP SHOULD first attempt to make the provisioned volume // accessible from the combination of the two "zones" "Z5" and "Z3" in // the "region" "R1". If that's not possible, it should fall back to @@ -4829,6 +4863,7 @@ type VolumeCapability struct { // following fields MUST be specified. // // Types that are valid to be assigned to AccessType: + // // *VolumeCapability_Block // *VolumeCapability_Mount AccessType isVolumeCapability_AccessType `protobuf_oneof:"access_type"` diff --git a/ca/config_test.go b/ca/config_test.go index 728a141f8f..77f9ce2b4d 100644 --- a/ca/config_test.go +++ b/ca/config_test.go @@ -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 @@ -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) } } @@ -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], }, diff --git a/ca/server_test.go b/ca/server_test.go index 01a4ed71bd..6ee6fdc77d 100644 --- a/ca/server_test.go +++ b/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) { diff --git a/manager/controlapi/ca_rotation.go b/manager/controlapi/ca_rotation.go index 52802685cd..74c66789a0 100644 --- a/manager/controlapi/ca_rotation.go +++ b/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 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() diff --git a/manager/controlapi/ca_rotation_test.go b/manager/controlapi/ca_rotation_test.go index e339a5d92d..650a62d3ab 100644 --- a/manager/controlapi/ca_rotation_test.go +++ b/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,7 +319,11 @@ 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{ @@ -323,7 +331,7 @@ func runValidTestCases(t *testing.T, testcases []*rootCARotationTestCase, localR }, } 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), diff --git a/protobuf/plugin/deepcopy/test/deepcopy.pb.go b/protobuf/plugin/deepcopy/test/deepcopy.pb.go index 1b3d51dbe4..9c87ce7b6e 100644 --- a/protobuf/plugin/deepcopy/test/deepcopy.pb.go +++ b/protobuf/plugin/deepcopy/test/deepcopy.pb.go @@ -293,6 +293,7 @@ var xxx_messageInfo_MapStruct proto.InternalMessageInfo type OneOf struct { // Types that are valid to be assigned to Fields: + // // *OneOf_Field1 // *OneOf_Field2 // *OneOf_Field3 @@ -304,6 +305,7 @@ type OneOf struct { // *OneOf_Field9 Fields isOneOf_Fields `protobuf_oneof:"fields"` // Types that are valid to be assigned to FieldsTwo: + // // *OneOf_Field10 // *OneOf_Field11 FieldsTwo isOneOf_FieldsTwo `protobuf_oneof:"fieldsTwo"`