From c0661679ea640d2e94737829c07c831bce095124 Mon Sep 17 00:00:00 2001 From: Jose Armesto Date: Wed, 20 Dec 2023 13:33:09 +0100 Subject: [PATCH] Remove ingress and egress rules from vpc default security group --- api/v1beta1/awscluster_conversion.go | 2 +- api/v1beta1/zz_generated.conversion.go | 1 + api/v1beta2/network_types.go | 12 +++ ...ster.x-k8s.io_awsmanagedcontrolplanes.yaml | 22 +++++ ...tructure.cluster.x-k8s.io_awsclusters.yaml | 11 +++ ....cluster.x-k8s.io_awsclustertemplates.yaml | 12 +++ pkg/cloud/awserrors/errors.go | 13 +++ pkg/cloud/filter/ec2.go | 7 ++ .../services/securitygroup/securitygroups.go | 70 ++++++++++++++ .../securitygroup/securitygroups_test.go | 92 +++++++++++++++++++ 10 files changed, 241 insertions(+), 1 deletion(-) diff --git a/api/v1beta1/awscluster_conversion.go b/api/v1beta1/awscluster_conversion.go index 433c3bacc2..ca199770ec 100644 --- a/api/v1beta1/awscluster_conversion.go +++ b/api/v1beta1/awscluster_conversion.go @@ -90,7 +90,7 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error { restoreIPAMPool(restored.Spec.NetworkSpec.VPC.IPv6.IPAMPool, dst.Spec.NetworkSpec.VPC.IPv6.IPAMPool) } - dst.Spec.NetworkSpec.AdditionalControlPlaneIngressRules = restored.Spec.NetworkSpec.AdditionalControlPlaneIngressRules + dst.Spec.NetworkSpec.VPC.EmptyRoutesDefaultVPCSecurityGroup = restored.Spec.NetworkSpec.VPC.EmptyRoutesDefaultVPCSecurityGroup // Restore SubnetSpec.ResourceID field, if any. for _, subnet := range restored.Spec.NetworkSpec.Subnets { diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go index a00d63e148..347e67fd35 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -2284,6 +2284,7 @@ func autoConvert_v1beta2_VPCSpec_To_v1beta1_VPCSpec(in *v1beta2.VPCSpec, out *VP out.Tags = *(*Tags)(unsafe.Pointer(&in.Tags)) out.AvailabilityZoneUsageLimit = (*int)(unsafe.Pointer(in.AvailabilityZoneUsageLimit)) out.AvailabilityZoneSelection = (*AZSelectionScheme)(unsafe.Pointer(in.AvailabilityZoneSelection)) + // WARNING: in.EmptyRoutesDefaultVPCSecurityGroup requires manual conversion: does not exist in peer-type return nil } diff --git a/api/v1beta2/network_types.go b/api/v1beta2/network_types.go index b964a4614d..8b4ba3ac4e 100644 --- a/api/v1beta2/network_types.go +++ b/api/v1beta2/network_types.go @@ -323,6 +323,18 @@ type VPCSpec struct { // +kubebuilder:default=Ordered // +kubebuilder:validation:Enum=Ordered;Random AvailabilityZoneSelection *AZSelectionScheme `json:"availabilityZoneSelection,omitempty"` + + // EmptyRoutesDefaultVPCSecurityGroup specifies whether the default VPC security group ingress + // and egress rules should be removed. + // + // By default, when creating a VPC, AWS creates a security group called `default` with ingress and egress + // rules that allow traffic from anywhere. The group could be used as a potential surface attack and + // it's generally suggested that the group rules are removed or modified appropriately. + // + // NOTE: This only applies when the VPC is managed by the Cluster API AWS controller. + // + // +optional + EmptyRoutesDefaultVPCSecurityGroup bool `json:"emptyRoutesDefaultVPCSecurityGroup,omitempty"` } // String returns a string representation of the VPC. diff --git a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml index d5cca03753..6fde25fdea 100644 --- a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml +++ b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml @@ -569,6 +569,17 @@ spec: provider creates a managed VPC. Defaults to 10.0.0.0/16. Mutually exclusive with IPAMPool. type: string + emptyRoutesDefaultVPCSecurityGroup: + description: "EmptyRoutesDefaultVPCSecurityGroup specifies + whether the default VPC security group ingress and egress + rules should be removed. \n By default, when creating a + VPC, AWS creates a security group called `default` with + ingress and egress rules that allow traffic from anywhere. + The group could be used as a potential surface attack and + it's generally suggested that the group rules are removed + or modified appropriately. \n NOTE: This only applies when + the VPC is managed by the Cluster API AWS controller." + type: boolean id: description: ID is the vpc-id of the VPC this provider should use to create resources. @@ -2155,6 +2166,17 @@ spec: provider creates a managed VPC. Defaults to 10.0.0.0/16. Mutually exclusive with IPAMPool. type: string + emptyRoutesDefaultVPCSecurityGroup: + description: "EmptyRoutesDefaultVPCSecurityGroup specifies + whether the default VPC security group ingress and egress + rules should be removed. \n By default, when creating a + VPC, AWS creates a security group called `default` with + ingress and egress rules that allow traffic from anywhere. + The group could be used as a potential surface attack and + it's generally suggested that the group rules are removed + or modified appropriately. \n NOTE: This only applies when + the VPC is managed by the Cluster API AWS controller." + type: boolean id: description: ID is the vpc-id of the VPC this provider should use to create resources. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml index 400268cae9..e8ff7c2191 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml @@ -1401,6 +1401,17 @@ spec: provider creates a managed VPC. Defaults to 10.0.0.0/16. Mutually exclusive with IPAMPool. type: string + emptyRoutesDefaultVPCSecurityGroup: + description: "EmptyRoutesDefaultVPCSecurityGroup specifies + whether the default VPC security group ingress and egress + rules should be removed. \n By default, when creating a + VPC, AWS creates a security group called `default` with + ingress and egress rules that allow traffic from anywhere. + The group could be used as a potential surface attack and + it's generally suggested that the group rules are removed + or modified appropriately. \n NOTE: This only applies when + the VPC is managed by the Cluster API AWS controller." + type: boolean id: description: ID is the vpc-id of the VPC this provider should use to create resources. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml index 49ba3ef552..36d6677db4 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml @@ -1014,6 +1014,18 @@ spec: when the provider creates a managed VPC. Defaults to 10.0.0.0/16. Mutually exclusive with IPAMPool. type: string + emptyRoutesDefaultVPCSecurityGroup: + description: "EmptyRoutesDefaultVPCSecurityGroup specifies + whether the default VPC security group ingress and + egress rules should be removed. \n By default, when + creating a VPC, AWS creates a security group called + `default` with ingress and egress rules that allow + traffic from anywhere. The group could be used as + a potential surface attack and it's generally suggested + that the group rules are removed or modified appropriately. + \n NOTE: This only applies when the VPC is managed + by the Cluster API AWS controller." + type: boolean id: description: ID is the vpc-id of the VPC this provider should use to create resources. diff --git a/pkg/cloud/awserrors/errors.go b/pkg/cloud/awserrors/errors.go index e1888e25b1..b7ff53b654 100644 --- a/pkg/cloud/awserrors/errors.go +++ b/pkg/cloud/awserrors/errors.go @@ -205,3 +205,16 @@ func IsIgnorableSecurityGroupError(err error) error { } return nil } + +// IsPermissionNotFoundError returns whether the error is InvalidPermission.NotFound. +func IsPermissionNotFoundError(err error) bool { + if code, ok := Code(err); ok { + switch code { + case PermissionNotFound: + return true + default: + return false + } + } + return false +} diff --git a/pkg/cloud/filter/ec2.go b/pkg/cloud/filter/ec2.go index d1d886f73b..b3122039cc 100644 --- a/pkg/cloud/filter/ec2.go +++ b/pkg/cloud/filter/ec2.go @@ -166,3 +166,10 @@ func (ec2Filters) IgnoreLocalZones() *ec2.Filter { Values: aws.StringSlice([]string{"opt-in-not-required"}), } } + +func (ec2Filters) SecurityGroupName(name string) *ec2.Filter { + return &ec2.Filter{ + Name: aws.String("group-name"), + Values: aws.StringSlice([]string{name}), + } +} diff --git a/pkg/cloud/services/securitygroup/securitygroups.go b/pkg/cloud/services/securitygroup/securitygroups.go index 5ba7e77bbe..1a3c9440e1 100644 --- a/pkg/cloud/services/securitygroup/securitygroups.go +++ b/pkg/cloud/services/securitygroup/securitygroups.go @@ -64,6 +64,11 @@ func (s *Service) ReconcileSecurityGroups() error { var err error + err = s.revokeIngressAndEgressRulesFromVPCDefaultSecurityGroup() + if err != nil { + return err + } + // Security group overrides are mapped by Role rather than their security group name // They are copied into the main 'sgs' list by their group name later var securityGroupOverrides map[infrav1.SecurityGroupRole]*ec2.SecurityGroup @@ -363,6 +368,55 @@ func (s *Service) describeSecurityGroupsByName() (map[string]infrav1.SecurityGro return res, nil } +// revokeIngressAndEgressRulesFromVPCDefaultSecurityGroup revokes ingress and egress rules from the VPC default security group. +// The VPC default security group is created by AWS and cannot be deleted. +// But we can revoke all ingress and egress rules from it to make it more secure. This security group is not used by CAPA. +func (s *Service) revokeIngressAndEgressRulesFromVPCDefaultSecurityGroup() error { + if !s.scope.VPC().EmptyRoutesDefaultVPCSecurityGroup { + return nil + } + + securityGroups, err := s.EC2Client.DescribeSecurityGroupsWithContext(context.TODO(), &ec2.DescribeSecurityGroupsInput{ + Filters: []*ec2.Filter{ + filter.EC2.VPC(s.scope.VPC().ID), + filter.EC2.SecurityGroupName("default"), + }, + }) + if err != nil { + return errors.Wrapf(err, "failed to find default security group in vpc %q", s.scope.VPC().ID) + } + defaultSecurityGroupID := *securityGroups.SecurityGroups[0].GroupId + s.scope.Debug("Removing ingress and egress rules from default security group in VPC", "defaultSecurityGroupID", defaultSecurityGroupID, "vpc-id", s.scope.VPC().ID) + + ingressRules := infrav1.IngressRules{ + { + Protocol: infrav1.SecurityGroupProtocolAll, + FromPort: -1, + ToPort: -1, + SourceSecurityGroupIDs: []string{defaultSecurityGroupID}, + }, + } + err = s.revokeSecurityGroupIngressRules(defaultSecurityGroupID, ingressRules) + if err != nil && !awserrors.IsPermissionNotFoundError(errors.Cause(err)) { + return errors.Wrapf(err, "failed to revoke ingress rules from vpc default security group %q in VPC %q", defaultSecurityGroupID, s.scope.VPC().ID) + } + + egressRules := infrav1.IngressRules{ + { + Protocol: infrav1.SecurityGroupProtocolAll, + FromPort: -1, + ToPort: -1, + CidrBlocks: []string{services.AnyIPv4CidrBlock}, + }, + } + err = s.revokeSecurityGroupEgressRules(defaultSecurityGroupID, egressRules) + if err != nil && !awserrors.IsPermissionNotFoundError(errors.Cause(err)) { + return errors.Wrapf(err, "failed to revoke egress rules from vpc default security group %q in VPC %q", defaultSecurityGroupID, s.scope.VPC().ID) + } + + return nil +} + func makeInfraSecurityGroup(ec2sg *ec2.SecurityGroup) infrav1.SecurityGroup { return infrav1.SecurityGroup{ ID: *ec2sg.GroupId, @@ -426,6 +480,22 @@ func (s *Service) revokeSecurityGroupIngressRules(id string, rules infrav1.Ingre return nil } +func (s *Service) revokeSecurityGroupEgressRules(id string, rules infrav1.IngressRules) error { + input := &ec2.RevokeSecurityGroupEgressInput{GroupId: aws.String(id)} + for i := range rules { + rule := rules[i] + input.IpPermissions = append(input.IpPermissions, ingressRuleToSDKType(s.scope, &rule)) + } + + if _, err := s.EC2Client.RevokeSecurityGroupEgressWithContext(context.TODO(), input); err != nil { + record.Warnf(s.scope.InfraCluster(), "FailedRevokeSecurityGroupEgressRules", "Failed to revoke security group egress rules %v for SecurityGroup %q: %v", rules, id, err) + return errors.Wrapf(err, "failed to revoke security group %q egress rules: %v", id, rules) + } + + record.Eventf(s.scope.InfraCluster(), "SuccessfulRevokeSecurityGroupEgressRules", "Revoked security group egress rules %v for SecurityGroup %q", rules, id) + return nil +} + func (s *Service) revokeAllSecurityGroupIngressRules(id string) error { describeInput := &ec2.DescribeSecurityGroupsInput{GroupIds: []*string{aws.String(id)}} diff --git a/pkg/cloud/services/securitygroup/securitygroups_test.go b/pkg/cloud/services/securitygroup/securitygroups_test.go index a292da31ee..50fe7007dc 100644 --- a/pkg/cloud/services/securitygroup/securitygroups_test.go +++ b/pkg/cloud/services/securitygroup/securitygroups_test.go @@ -35,6 +35,7 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/awserrors" + "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/filter" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services" "sigs.k8s.io/cluster-api-provider-aws/v2/test/mocks" @@ -74,6 +75,7 @@ func TestReconcileSecurityGroups(t *testing.T) { Tags: infrav1.Tags{ infrav1.ClusterTagKey("test-cluster"): "owned", }, + EmptyRoutesDefaultVPCSecurityGroup: true, }, Subnets: infrav1.Subnets{ infrav1.SubnetSpec{ @@ -90,6 +92,29 @@ func TestReconcileSecurityGroups(t *testing.T) { }, }, expect: func(m *mocks.MockEC2APIMockRecorder) { + m.DescribeSecurityGroupsWithContext(context.TODO(), &ec2.DescribeSecurityGroupsInput{ + Filters: []*ec2.Filter{ + filter.EC2.VPC("vpc-securitygroups"), + filter.EC2.SecurityGroupName("default"), + }, + }). + Return(&ec2.DescribeSecurityGroupsOutput{ + SecurityGroups: []*ec2.SecurityGroup{ + { + Description: aws.String("default VPC security group"), + GroupName: aws.String("default"), + GroupId: aws.String("sg-default"), + }, + }, + }, nil) + m.RevokeSecurityGroupIngressWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.RevokeSecurityGroupIngressInput{ + GroupId: aws.String("sg-default"), + })) + + m.RevokeSecurityGroupEgressWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.RevokeSecurityGroupEgressInput{ + GroupId: aws.String("sg-default"), + })) + m.DescribeSecurityGroupsWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{})). Return(&ec2.DescribeSecurityGroupsOutput{}, nil) @@ -735,6 +760,73 @@ func TestReconcileSecurityGroups(t *testing.T) { }, err: errors.New(`security group overrides provided for managed vpc "test-cluster"`), }, + { + name: "when VPC default security group has no rules then no errors are returned", + awsCluster: func(acl infrav1.AWSCluster) infrav1.AWSCluster { + return acl + }, + input: &infrav1.NetworkSpec{ + VPC: infrav1.VPCSpec{ + ID: "vpc-securitygroups", + InternetGatewayID: aws.String("igw-01"), + Tags: infrav1.Tags{ + infrav1.ClusterTagKey("test-cluster"): "owned", + }, + EmptyRoutesDefaultVPCSecurityGroup: true, + }, + Subnets: infrav1.Subnets{ + infrav1.SubnetSpec{ + ID: "subnet-securitygroups-private", + IsPublic: false, + AvailabilityZone: "us-east-1a", + }, + infrav1.SubnetSpec{ + ID: "subnet-securitygroups-public", + IsPublic: true, + NatGatewayID: aws.String("nat-01"), + AvailabilityZone: "us-east-1a", + }, + }, + }, + expect: func(m *mocks.MockEC2APIMockRecorder) { + m.DescribeSecurityGroupsWithContext(context.TODO(), &ec2.DescribeSecurityGroupsInput{ + Filters: []*ec2.Filter{ + filter.EC2.VPC("vpc-securitygroups"), + filter.EC2.SecurityGroupName("default"), + }, + }). + Return(&ec2.DescribeSecurityGroupsOutput{ + SecurityGroups: []*ec2.SecurityGroup{ + { + Description: aws.String("default VPC security group"), + GroupName: aws.String("default"), + GroupId: aws.String("sg-default"), + }, + }, + }, nil) + + m.RevokeSecurityGroupIngressWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.RevokeSecurityGroupIngressInput{ + GroupId: aws.String("sg-default"), + })).Return(&ec2.RevokeSecurityGroupIngressOutput{}, awserr.New("InvalidPermission.NotFound", "rules not found in security group", nil)) + + m.RevokeSecurityGroupEgressWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.RevokeSecurityGroupEgressInput{ + GroupId: aws.String("sg-default"), + })).Return(&ec2.RevokeSecurityGroupEgressOutput{}, awserr.New("InvalidPermission.NotFound", "rules not found in security group", nil)) + + m.DescribeSecurityGroupsWithContext(context.TODO(), &ec2.DescribeSecurityGroupsInput{ + Filters: []*ec2.Filter{ + filter.EC2.VPC("vpc-securitygroups"), + filter.EC2.Cluster("test-cluster"), + }, + }).Return(&ec2.DescribeSecurityGroupsOutput{}, nil) + + m.CreateSecurityGroupWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.CreateSecurityGroupInput{})). + Return(&ec2.CreateSecurityGroupOutput{GroupId: aws.String("sg-node")}, nil).AnyTimes() + + m.AuthorizeSecurityGroupIngressWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.AuthorizeSecurityGroupIngressInput{})). + Return(&ec2.AuthorizeSecurityGroupIngressOutput{}, nil).AnyTimes() + }, + }, } for _, tc := range testCases {