kubernetes-sigs · k8s-ci-robot · Jan 16, 2024 · Dec 20, 2023 · fiunchinho · Dec 21, 2023
diff --git 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
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 {