Skip to content

Commit

Permalink
Remove ingress and egress rules from vpc default security group
Browse files Browse the repository at this point in the history
fiunchinho committed Jan 15, 2024
1 parent bba04c4 commit c066167
Showing 10 changed files with 241 additions and 1 deletion.
2 changes: 1 addition & 1 deletion api/v1beta1/awscluster_conversion.go
Original file line number Diff line number Diff line change
@@ -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 {
1 change: 1 addition & 0 deletions api/v1beta1/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions api/v1beta2/network_types.go
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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.
11 changes: 11 additions & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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.
13 changes: 13 additions & 0 deletions pkg/cloud/awserrors/errors.go
Original file line number Diff line number Diff line change
@@ -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
}
7 changes: 7 additions & 0 deletions pkg/cloud/filter/ec2.go
Original file line number Diff line number Diff line change
@@ -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}),
}
}
70 changes: 70 additions & 0 deletions pkg/cloud/services/securitygroup/securitygroups.go
Original file line number Diff line number Diff line change
@@ -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)}}

92 changes: 92 additions & 0 deletions pkg/cloud/services/securitygroup/securitygroups_test.go
Original file line number Diff line number Diff line change
@@ -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 {

0 comments on commit c066167

Please sign in to comment.