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
  • Loading branch information
fiunchinho committed Dec 21, 2023
1 parent 3784406 commit bd85118
Show file tree
Hide file tree
Showing 9 changed files with 127 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
Expand Up @@ -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 {
Expand Down
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.

5 changes: 5 additions & 0 deletions api/v1beta2/network_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions pkg/cloud/filter/ec2.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}),
}
}
68 changes: 68 additions & 0 deletions pkg/cloud/services/securitygroup/securitygroups.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -363,6 +368,53 @@ 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
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 awserrors.IsIgnorableSecurityGroupError(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 awserrors.IsIgnorableSecurityGroupError(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,
Expand Down Expand Up @@ -426,6 +478,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)}}

Expand Down
25 changes: 25 additions & 0 deletions pkg/cloud/services/securitygroup/securitygroups_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -74,6 +75,7 @@ func TestReconcileSecurityGroups(t *testing.T) {
Tags: infrav1.Tags{
infrav1.ClusterTagKey("test-cluster"): "owned",
},
SecureDefaultVPCSecurityGroup: true,
},
Subnets: infrav1.Subnets{
infrav1.SubnetSpec{
Expand All @@ -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)

Expand Down

0 comments on commit bd85118

Please sign in to comment.