diff --git a/api/v1beta1/awscluster_conversion.go b/api/v1beta1/awscluster_conversion.go index 433c3bacc2..ce6b7ccf34 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.SecureDefaultVPCSecurityGroup = restored.Spec.NetworkSpec.VPC.SecureDefaultVPCSecurityGroup // 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..ff78d96b8c 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.SecureDefaultVPCSecurityGroup 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..b336689434 100644 --- a/api/v1beta2/network_types.go +++ b/api/v1beta2/network_types.go @@ -323,6 +323,11 @@ type VPCSpec struct { // +kubebuilder:default=Ordered // +kubebuilder:validation:Enum=Ordered;Random AvailabilityZoneSelection *AZSelectionScheme `json:"availabilityZoneSelection,omitempty"` + + // SecureDefaultVPCSecurityGroup specifies whether the default VPC security group ingress + // and egress rules should be removed. + // +optional + SecureDefaultVPCSecurityGroup bool `json:"secureDefaultVPCSecurityGroup,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..3cc12cec80 100644 --- a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml +++ b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml @@ -636,6 +636,11 @@ spec: is set. Mutually exclusive with IPAMPool. type: string type: object + secureDefaultVPCSecurityGroup: + description: SecureDefaultVPCSecurityGroup specifies whether + the default VPC security group ingress and egress rules + should be removed. + type: boolean tags: additionalProperties: type: string @@ -2222,6 +2227,11 @@ spec: is set. Mutually exclusive with IPAMPool. type: string type: object + secureDefaultVPCSecurityGroup: + description: SecureDefaultVPCSecurityGroup specifies whether + the default VPC security group ingress and egress rules + should be removed. + type: boolean tags: additionalProperties: type: string 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..7cce04b262 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml @@ -1468,6 +1468,11 @@ spec: is set. Mutually exclusive with IPAMPool. type: string type: object + secureDefaultVPCSecurityGroup: + description: SecureDefaultVPCSecurityGroup specifies whether + the default VPC security group ingress and egress rules + should be removed. + type: boolean tags: additionalProperties: type: string 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..a0aab7f6c6 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml @@ -1086,6 +1086,11 @@ spec: with IPAMPool. type: string type: object + secureDefaultVPCSecurityGroup: + description: SecureDefaultVPCSecurityGroup specifies + whether the default VPC security group ingress and + egress rules should be removed. + type: boolean tags: additionalProperties: type: string 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..5f47254bd1 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,52 @@ 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().SecureDefaultVPCSecurityGroup { + 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 + + ingressRules := infrav1.IngressRules{ + { + Protocol: infrav1.SecurityGroupProtocolAll, + FromPort: -1, + ToPort: -1, + CidrBlocks: []string{services.AnyIPv4CidrBlock}, + }, + } + err = s.revokeSecurityGroupIngressRules(defaultSecurityGroupID, ingressRules) + if err != nil { + return errors.Wrapf(err, "failed to revoke ingress rules from vpc default security group in vpc %q", 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 { + return errors.Wrapf(err, "failed to revoke egress rules from vpc default security group in vpc %q", s.scope.VPC().ID) + } + } + + return nil +} + func makeInfraSecurityGroup(ec2sg *ec2.SecurityGroup) infrav1.SecurityGroup { return infrav1.SecurityGroup{ ID: *ec2sg.GroupId, @@ -426,6 +477,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..614a30c71a 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", }, + SecureDefaultVPCSecurityGroup: 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)