Skip to content

Commit c066167

Browse files
fiunchinhofiunchinho
committed
Remove ingress and egress rules from vpc default security group
1 parent bba04c4 commit c066167

File tree

10 files changed

+241
-1
lines changed

10 files changed

+241
-1
lines changed

‎api/v1beta1/awscluster_conversion.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error {
9090
restoreIPAMPool(restored.Spec.NetworkSpec.VPC.IPv6.IPAMPool, dst.Spec.NetworkSpec.VPC.IPv6.IPAMPool)
9191
}
9292

93-
dst.Spec.NetworkSpec.AdditionalControlPlaneIngressRules = restored.Spec.NetworkSpec.AdditionalControlPlaneIngressRules
93+
dst.Spec.NetworkSpec.VPC.EmptyRoutesDefaultVPCSecurityGroup = restored.Spec.NetworkSpec.VPC.EmptyRoutesDefaultVPCSecurityGroup
9494

9595
// Restore SubnetSpec.ResourceID field, if any.
9696
for _, subnet := range restored.Spec.NetworkSpec.Subnets {

‎api/v1beta1/zz_generated.conversion.go‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎api/v1beta2/network_types.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,18 @@ type VPCSpec struct {
323323
// +kubebuilder:default=Ordered
324324
// +kubebuilder:validation:Enum=Ordered;Random
325325
AvailabilityZoneSelection *AZSelectionScheme `json:"availabilityZoneSelection,omitempty"`
326+
327+
// EmptyRoutesDefaultVPCSecurityGroup specifies whether the default VPC security group ingress
328+
// and egress rules should be removed.
329+
//
330+
// By default, when creating a VPC, AWS creates a security group called `default` with ingress and egress
331+
// rules that allow traffic from anywhere. The group could be used as a potential surface attack and
332+
// it's generally suggested that the group rules are removed or modified appropriately.
333+
//
334+
// NOTE: This only applies when the VPC is managed by the Cluster API AWS controller.
335+
//
336+
// +optional
337+
EmptyRoutesDefaultVPCSecurityGroup bool `json:"emptyRoutesDefaultVPCSecurityGroup,omitempty"`
326338
}
327339

328340
// String returns a string representation of the VPC.

‎config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -569,6 +569,17 @@ spec:
569569
provider creates a managed VPC. Defaults to 10.0.0.0/16.
570570
Mutually exclusive with IPAMPool.
571571
type: string
572+
emptyRoutesDefaultVPCSecurityGroup:
573+
description: "EmptyRoutesDefaultVPCSecurityGroup specifies
574+
whether the default VPC security group ingress and egress
575+
rules should be removed. \n By default, when creating a
576+
VPC, AWS creates a security group called `default` with
577+
ingress and egress rules that allow traffic from anywhere.
578+
The group could be used as a potential surface attack and
579+
it's generally suggested that the group rules are removed
580+
or modified appropriately. \n NOTE: This only applies when
581+
the VPC is managed by the Cluster API AWS controller."
582+
type: boolean
572583
id:
573584
description: ID is the vpc-id of the VPC this provider should
574585
use to create resources.
@@ -2155,6 +2166,17 @@ spec:
21552166
provider creates a managed VPC. Defaults to 10.0.0.0/16.
21562167
Mutually exclusive with IPAMPool.
21572168
type: string
2169+
emptyRoutesDefaultVPCSecurityGroup:
2170+
description: "EmptyRoutesDefaultVPCSecurityGroup specifies
2171+
whether the default VPC security group ingress and egress
2172+
rules should be removed. \n By default, when creating a
2173+
VPC, AWS creates a security group called `default` with
2174+
ingress and egress rules that allow traffic from anywhere.
2175+
The group could be used as a potential surface attack and
2176+
it's generally suggested that the group rules are removed
2177+
or modified appropriately. \n NOTE: This only applies when
2178+
the VPC is managed by the Cluster API AWS controller."
2179+
type: boolean
21582180
id:
21592181
description: ID is the vpc-id of the VPC this provider should
21602182
use to create resources.

‎config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1401,6 +1401,17 @@ spec:
14011401
provider creates a managed VPC. Defaults to 10.0.0.0/16.
14021402
Mutually exclusive with IPAMPool.
14031403
type: string
1404+
emptyRoutesDefaultVPCSecurityGroup:
1405+
description: "EmptyRoutesDefaultVPCSecurityGroup specifies
1406+
whether the default VPC security group ingress and egress
1407+
rules should be removed. \n By default, when creating a
1408+
VPC, AWS creates a security group called `default` with
1409+
ingress and egress rules that allow traffic from anywhere.
1410+
The group could be used as a potential surface attack and
1411+
it's generally suggested that the group rules are removed
1412+
or modified appropriately. \n NOTE: This only applies when
1413+
the VPC is managed by the Cluster API AWS controller."
1414+
type: boolean
14041415
id:
14051416
description: ID is the vpc-id of the VPC this provider should
14061417
use to create resources.

‎config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,6 +1014,18 @@ spec:
10141014
when the provider creates a managed VPC. Defaults
10151015
to 10.0.0.0/16. Mutually exclusive with IPAMPool.
10161016
type: string
1017+
emptyRoutesDefaultVPCSecurityGroup:
1018+
description: "EmptyRoutesDefaultVPCSecurityGroup specifies
1019+
whether the default VPC security group ingress and
1020+
egress rules should be removed. \n By default, when
1021+
creating a VPC, AWS creates a security group called
1022+
`default` with ingress and egress rules that allow
1023+
traffic from anywhere. The group could be used as
1024+
a potential surface attack and it's generally suggested
1025+
that the group rules are removed or modified appropriately.
1026+
\n NOTE: This only applies when the VPC is managed
1027+
by the Cluster API AWS controller."
1028+
type: boolean
10171029
id:
10181030
description: ID is the vpc-id of the VPC this provider
10191031
should use to create resources.

‎pkg/cloud/awserrors/errors.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,3 +205,16 @@ func IsIgnorableSecurityGroupError(err error) error {
205205
}
206206
return nil
207207
}
208+
209+
// IsPermissionNotFoundError returns whether the error is InvalidPermission.NotFound.
210+
func IsPermissionNotFoundError(err error) bool {
211+
if code, ok := Code(err); ok {
212+
switch code {
213+
case PermissionNotFound:
214+
return true
215+
default:
216+
return false
217+
}
218+
}
219+
return false
220+
}

‎pkg/cloud/filter/ec2.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,3 +166,10 @@ func (ec2Filters) IgnoreLocalZones() *ec2.Filter {
166166
Values: aws.StringSlice([]string{"opt-in-not-required"}),
167167
}
168168
}
169+
170+
func (ec2Filters) SecurityGroupName(name string) *ec2.Filter {
171+
return &ec2.Filter{
172+
Name: aws.String("group-name"),
173+
Values: aws.StringSlice([]string{name}),
174+
}
175+
}

‎pkg/cloud/services/securitygroup/securitygroups.go‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,11 @@ func (s *Service) ReconcileSecurityGroups() error {
6464

6565
var err error
6666

67+
err = s.revokeIngressAndEgressRulesFromVPCDefaultSecurityGroup()
68+
if err != nil {
69+
return err
70+
}
71+
6772
// Security group overrides are mapped by Role rather than their security group name
6873
// They are copied into the main 'sgs' list by their group name later
6974
var securityGroupOverrides map[infrav1.SecurityGroupRole]*ec2.SecurityGroup
@@ -363,6 +368,55 @@ func (s *Service) describeSecurityGroupsByName() (map[string]infrav1.SecurityGro
363368
return res, nil
364369
}
365370

371+
// revokeIngressAndEgressRulesFromVPCDefaultSecurityGroup revokes ingress and egress rules from the VPC default security group.
372+
// The VPC default security group is created by AWS and cannot be deleted.
373+
// But we can revoke all ingress and egress rules from it to make it more secure. This security group is not used by CAPA.
374+
func (s *Service) revokeIngressAndEgressRulesFromVPCDefaultSecurityGroup() error {
375+
if !s.scope.VPC().EmptyRoutesDefaultVPCSecurityGroup {
376+
return nil
377+
}
378+
379+
securityGroups, err := s.EC2Client.DescribeSecurityGroupsWithContext(context.TODO(), &ec2.DescribeSecurityGroupsInput{
380+
Filters: []*ec2.Filter{
381+
filter.EC2.VPC(s.scope.VPC().ID),
382+
filter.EC2.SecurityGroupName("default"),
383+
},
384+
})
385+
if err != nil {
386+
return errors.Wrapf(err, "failed to find default security group in vpc %q", s.scope.VPC().ID)
387+
}
388+
defaultSecurityGroupID := *securityGroups.SecurityGroups[0].GroupId
389+
s.scope.Debug("Removing ingress and egress rules from default security group in VPC", "defaultSecurityGroupID", defaultSecurityGroupID, "vpc-id", s.scope.VPC().ID)
390+
391+
ingressRules := infrav1.IngressRules{
392+
{
393+
Protocol: infrav1.SecurityGroupProtocolAll,
394+
FromPort: -1,
395+
ToPort: -1,
396+
SourceSecurityGroupIDs: []string{defaultSecurityGroupID},
397+
},
398+
}
399+
err = s.revokeSecurityGroupIngressRules(defaultSecurityGroupID, ingressRules)
400+
if err != nil && !awserrors.IsPermissionNotFoundError(errors.Cause(err)) {
401+
return errors.Wrapf(err, "failed to revoke ingress rules from vpc default security group %q in VPC %q", defaultSecurityGroupID, s.scope.VPC().ID)
402+
}
403+
404+
egressRules := infrav1.IngressRules{
405+
{
406+
Protocol: infrav1.SecurityGroupProtocolAll,
407+
FromPort: -1,
408+
ToPort: -1,
409+
CidrBlocks: []string{services.AnyIPv4CidrBlock},
410+
},
411+
}
412+
err = s.revokeSecurityGroupEgressRules(defaultSecurityGroupID, egressRules)
413+
if err != nil && !awserrors.IsPermissionNotFoundError(errors.Cause(err)) {
414+
return errors.Wrapf(err, "failed to revoke egress rules from vpc default security group %q in VPC %q", defaultSecurityGroupID, s.scope.VPC().ID)
415+
}
416+
417+
return nil
418+
}
419+
366420
func makeInfraSecurityGroup(ec2sg *ec2.SecurityGroup) infrav1.SecurityGroup {
367421
return infrav1.SecurityGroup{
368422
ID: *ec2sg.GroupId,
@@ -426,6 +480,22 @@ func (s *Service) revokeSecurityGroupIngressRules(id string, rules infrav1.Ingre
426480
return nil
427481
}
428482

483+
func (s *Service) revokeSecurityGroupEgressRules(id string, rules infrav1.IngressRules) error {
484+
input := &ec2.RevokeSecurityGroupEgressInput{GroupId: aws.String(id)}
485+
for i := range rules {
486+
rule := rules[i]
487+
input.IpPermissions = append(input.IpPermissions, ingressRuleToSDKType(s.scope, &rule))
488+
}
489+
490+
if _, err := s.EC2Client.RevokeSecurityGroupEgressWithContext(context.TODO(), input); err != nil {
491+
record.Warnf(s.scope.InfraCluster(), "FailedRevokeSecurityGroupEgressRules", "Failed to revoke security group egress rules %v for SecurityGroup %q: %v", rules, id, err)
492+
return errors.Wrapf(err, "failed to revoke security group %q egress rules: %v", id, rules)
493+
}
494+
495+
record.Eventf(s.scope.InfraCluster(), "SuccessfulRevokeSecurityGroupEgressRules", "Revoked security group egress rules %v for SecurityGroup %q", rules, id)
496+
return nil
497+
}
498+
429499
func (s *Service) revokeAllSecurityGroupIngressRules(id string) error {
430500
describeInput := &ec2.DescribeSecurityGroupsInput{GroupIds: []*string{aws.String(id)}}
431501

‎pkg/cloud/services/securitygroup/securitygroups_test.go‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535

3636
infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
3737
"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/awserrors"
38+
"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/filter"
3839
"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope"
3940
"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services"
4041
"sigs.k8s.io/cluster-api-provider-aws/v2/test/mocks"
@@ -74,6 +75,7 @@ func TestReconcileSecurityGroups(t *testing.T) {
7475
Tags: infrav1.Tags{
7576
infrav1.ClusterTagKey("test-cluster"): "owned",
7677
},
78+
EmptyRoutesDefaultVPCSecurityGroup: true,
7779
},
7880
Subnets: infrav1.Subnets{
7981
infrav1.SubnetSpec{
@@ -90,6 +92,29 @@ func TestReconcileSecurityGroups(t *testing.T) {
9092
},
9193
},
9294
expect: func(m *mocks.MockEC2APIMockRecorder) {
95+
m.DescribeSecurityGroupsWithContext(context.TODO(), &ec2.DescribeSecurityGroupsInput{
96+
Filters: []*ec2.Filter{
97+
filter.EC2.VPC("vpc-securitygroups"),
98+
filter.EC2.SecurityGroupName("default"),
99+
},
100+
}).
101+
Return(&ec2.DescribeSecurityGroupsOutput{
102+
SecurityGroups: []*ec2.SecurityGroup{
103+
{
104+
Description: aws.String("default VPC security group"),
105+
GroupName: aws.String("default"),
106+
GroupId: aws.String("sg-default"),
107+
},
108+
},
109+
}, nil)
110+
m.RevokeSecurityGroupIngressWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.RevokeSecurityGroupIngressInput{
111+
GroupId: aws.String("sg-default"),
112+
}))
113+
114+
m.RevokeSecurityGroupEgressWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.RevokeSecurityGroupEgressInput{
115+
GroupId: aws.String("sg-default"),
116+
}))
117+
93118
m.DescribeSecurityGroupsWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{})).
94119
Return(&ec2.DescribeSecurityGroupsOutput{}, nil)
95120

@@ -735,6 +760,73 @@ func TestReconcileSecurityGroups(t *testing.T) {
735760
},
736761
err: errors.New(`security group overrides provided for managed vpc "test-cluster"`),
737762
},
763+
{
764+
name: "when VPC default security group has no rules then no errors are returned",
765+
awsCluster: func(acl infrav1.AWSCluster) infrav1.AWSCluster {
766+
return acl
767+
},
768+
input: &infrav1.NetworkSpec{
769+
VPC: infrav1.VPCSpec{
770+
ID: "vpc-securitygroups",
771+
InternetGatewayID: aws.String("igw-01"),
772+
Tags: infrav1.Tags{
773+
infrav1.ClusterTagKey("test-cluster"): "owned",
774+
},
775+
EmptyRoutesDefaultVPCSecurityGroup: true,
776+
},
777+
Subnets: infrav1.Subnets{
778+
infrav1.SubnetSpec{
779+
ID: "subnet-securitygroups-private",
780+
IsPublic: false,
781+
AvailabilityZone: "us-east-1a",
782+
},
783+
infrav1.SubnetSpec{
784+
ID: "subnet-securitygroups-public",
785+
IsPublic: true,
786+
NatGatewayID: aws.String("nat-01"),
787+
AvailabilityZone: "us-east-1a",
788+
},
789+
},
790+
},
791+
expect: func(m *mocks.MockEC2APIMockRecorder) {
792+
m.DescribeSecurityGroupsWithContext(context.TODO(), &ec2.DescribeSecurityGroupsInput{
793+
Filters: []*ec2.Filter{
794+
filter.EC2.VPC("vpc-securitygroups"),
795+
filter.EC2.SecurityGroupName("default"),
796+
},
797+
}).
798+
Return(&ec2.DescribeSecurityGroupsOutput{
799+
SecurityGroups: []*ec2.SecurityGroup{
800+
{
801+
Description: aws.String("default VPC security group"),
802+
GroupName: aws.String("default"),
803+
GroupId: aws.String("sg-default"),
804+
},
805+
},
806+
}, nil)
807+
808+
m.RevokeSecurityGroupIngressWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.RevokeSecurityGroupIngressInput{
809+
GroupId: aws.String("sg-default"),
810+
})).Return(&ec2.RevokeSecurityGroupIngressOutput{}, awserr.New("InvalidPermission.NotFound", "rules not found in security group", nil))
811+
812+
m.RevokeSecurityGroupEgressWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.RevokeSecurityGroupEgressInput{
813+
GroupId: aws.String("sg-default"),
814+
})).Return(&ec2.RevokeSecurityGroupEgressOutput{}, awserr.New("InvalidPermission.NotFound", "rules not found in security group", nil))
815+
816+
m.DescribeSecurityGroupsWithContext(context.TODO(), &ec2.DescribeSecurityGroupsInput{
817+
Filters: []*ec2.Filter{
818+
filter.EC2.VPC("vpc-securitygroups"),
819+
filter.EC2.Cluster("test-cluster"),
820+
},
821+
}).Return(&ec2.DescribeSecurityGroupsOutput{}, nil)
822+
823+
m.CreateSecurityGroupWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.CreateSecurityGroupInput{})).
824+
Return(&ec2.CreateSecurityGroupOutput{GroupId: aws.String("sg-node")}, nil).AnyTimes()
825+
826+
m.AuthorizeSecurityGroupIngressWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.AuthorizeSecurityGroupIngressInput{})).
827+
Return(&ec2.AuthorizeSecurityGroupIngressOutput{}, nil).AnyTimes()
828+
},
829+
},
738830
}
739831

740832
for _, tc := range testCases {

0 commit comments

Comments
 (0)