feat(aws-ec2): Fixes #7094 include ipv6 traffic in allowAllOutbound #7827
Conversation
Attempting to fix the issue I created #7094
t0gre
changed the title
Mainly just to pass the pull request test. I don't think these changes to the readme are actually necessary.
|
Hi @tomgreenwood1! About tests, the first test that needs updating is the one verifying that by default security groups allows all outbound traffic, sepcifically the test exceptions should be updated to add the IPv6 traffic. Additional test that will be nice to have is a test which verifies an error is thrown when calling the |
| function isAllTrafficRule(rule: any) { | ||
| return rule.cidrIp === '0.0.0.0/0' && rule.ipProtocol === '-1'; | ||
| return (['0.0.0.0/0','::/0'].includes(rule.cidrIp) && rule.ipProtocol === '-1'; | ||
| } |
There was a problem hiding this comment.
Should this check for all traffic? Meaning both IPv4 and IPv6 allowed? Otherwise this isn't coherent with the (new and fixed) behavior of allowAllOutbound... This is used to guard against some problems, I'm not sure if the check must be "as strict" or not. It could be problematic as it appears to limit what users are able to express using the API:
allowAllOutbound: truewill allow both IPv4 and IPv6allowAllOutbound: falsewould disallow allowing only one of IPv6 xor IPv6
There was a problem hiding this comment.
You're right, the 'includes' check is too loose. I will amend it when I have time. I've been very busy with work, hence not getting around to writing any tests yet..
There was a problem hiding this comment.
An Egress rule Resource includes CidrIp for setting IPv4 and CidrIpv6 for setting IPv6:
Type: AWS::EC2::SecurityGroupEgress
Properties:
CidrIp: String
CidrIpv6: String
Description: String
DestinationPrefixListId: String
DestinationSecurityGroupId: String
FromPort: Integer
GroupId: String
IpProtocol: String
ToPort: Integer
A rule can include CidrIp or CidrIpv6 but not both, so there is no way to test both on the same rule, which makes this check kinda of tricky now
There was a problem hiding this comment.
Did some digging into this, changing the default to allow all IPv6 traffic and all IPv4 traffic requires changing isAllTrafficRule to return true if the rule being added either allow all IPv4 traffic OR allow all IPv6 traffic. But doing so means that we don't allow creating a security group that we don't allow creating a security group that allows only one of them, as it will throw:
'Cannot add an "all traffic" egress rule in this way; set allowAllOutbound=true on the SecurityGroup instead.'
When setting allowAllOutbound to false and trying to add an all traffic rule for, WLOG IPv4.
The idea behind not allowing all traffic rule to be added using addEgressRule was that these rules will be removed by ec2 if other rules exists, but this is only true for same protocol rules:
Additionally, changing the isAllTrafficRule to return true if the rule is either all IPv4 or all IPv6 will break customers that use the addEgressRule method to allow all IPv6 traffic:
const mySecurityGroup = new ec2.SecurityGroup(this, 'SecurityGroup', {
vpc,
description: 'Allow ssh access to ec2 instances',
allowAllOutbound: true // Allow all IPv4 traffic
});
mySecurityGroup.addIngressRule(ec2.Peer.anyIpv6(), Port.allTraffic()); // Allow IPv6
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Closing this one, we will change the default in v2 |
Attempting to fix the issue I created #7094
Commit Message
COMMIT/PR TITLE HERE (must follow conventionalcommits.org)
COMMIT MESSAGE HERE
End Commit Message
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license