ASG Instance Security Group AllowOutbound Bug

[moofish32]

If you create an ASG with the L2 in version 0.13.0 and set allowOutbound: true (also the default) and later set up rules that can not be inlined, the outbound access is removed. The CFN template does not remove it, but it also does not work.

This EKS Example is a working a reproduction of the bug: https://github.com/moofish32/cdk-eks-example/blob/test/lib/eks-node-group.ts#L46-L94

You can see the synth output has the outbound but if you build this stack (you'll need to build the cluster first).

This might be solve-able with a documentation update, but it seems we should likely fix this? @rix0rrr I think you have an open doc task.

[rix0rrr]

If this really behaves that way that's certainly a bug.

The doc task I have open is about the default behavior of Security Groups, which behaves by allowing all outbound if you don't add any other rules, but I believe that's default CloudFormation behavior.

Wondering if we should work around that, but that's another issue. This must be about an explicit addEgressRule() that would have to be dropped on the floor.

[rix0rrr]

I don't see the allowAllOutbound: true in your example though?

[moofish32]

I can set it that way too, sorry testing locally getting ready to push. The template output and failing userdata are pretty good indications. I don't believe you can combine egress rules and inline egress. Other tools have prevented this due to inconsistent behaviors. Let me get another push and I'll validate.

[rix0rrr]

/me expresses surprise

[moofish32]

Without pasting the whole YAML file, here is the key snippet:

    WorkersInstanceSecurityGroup65472717:
        Type: 'AWS::EC2::SecurityGroup'
        Properties:
            GroupDescription: EksWorkers/Workers/InstanceSecurityGroup
            SecurityGroupEgress:
                -
                    CidrIp: 0.0.0.0/0
                    Description: Outbound traffic allowed by default
                    FromPort: -1
                    IpProtocol: '-1'
                    ToPort: -1
            SecurityGroupIngress: []
            Tags:
                -
                    Key: Name
                    Value: EksWorkers/Workers
                -
                    Key: kubernetes.io/cluster/EksExample
                    Value: owned
                -
                    Key: NodeType
                    Value: Worker
            VpcId:
                'Fn::ImportValue': 'EksCluster:EksVpcVpcId48D9E242'

This screen shot has the only outbound rule.

[moofish32]

@rix0rrr these were the changes I had to make moofish32/cdk-eks-example#2

[rix0rrr]

Maybe inline rules and object rules cannot be combined or somesuch

[rix0rrr]

Well yeah the extra sg makes it work because empty sg's come with all outbound traffic enabled by default

[rix0rrr]

Maybe it's the '-1' protocol string that doesn't work

[moofish32]

Maybe inline rules and object rules cannot be combined or somesuch

Perhaps we should isolate this to test.

[rix0rrr]

Oh I bet I know what the issue is.

In order to achieve the behavior "empty SGs start out with everything enabled by default" but then lose that default once rules are added, I bet it works like this:

1. Security Group is created, gets created with "all outbound traffic" rule if there are no static egress rules.
2. Dynamic egress rule is added, if there is an "all outbound traffic" rule, it gets removed.

It just so happens that the second step does not make a distinction between an "all outbound" rule added as a default, vs an "all outbound" rule added by us on purpose. It deletes it in either case.

[rix0rrr]

Confirmed. The following template shows the behavior:

If the Egress rule is absent, the SG shows the outbound rule just fine, but if you add the egress rule the default rule disappears.

---
Resources:
    WorkersInstanceSecurityGroup65472717:
        Type: 'AWS::EC2::SecurityGroup'
        Properties:
            GroupDescription: EksWorkers/Workers/InstanceSecurityGroup
            SecurityGroupEgress:
                -
                    CidrIp: 0.0.0.0/0
                    Description: Outbound traffic allowed by default
                    FromPort: -1
                    IpProtocol: '-1'
                    ToPort: -1
            SecurityGroupIngress: []
            Tags:
                -
                    Key: Name
                    Value: EksWorkers/Workers
                -
                    Key: kubernetes.io/cluster/EksExample
                    Value: owned
                -
                    Key: NodeType
                    Value: Worker
            VpcId: vpc-60900905
    WorkersInstanceSecurityGrouptoEksWorkersControlPlaneSG070CB121102565535E8766439:
        Type: 'AWS::EC2::SecurityGroupEgress'
        Properties:
            GroupId:
                'Fn::GetAtt':
                    - WorkersInstanceSecurityGroup65472717
                    - GroupId
            IpProtocol: tcp
            Description: 'to somewhere else'
            FromPort: 1234
            ToPort: 1234
            CidrIp: '1.2.3.4/32'

[rix0rrr]

It's actually EC2 itself that's doing this.

Just moving the egress rule to a separate Rule object doesn't help, because if the ordering is "just-so", the rule still gets removed:

---
Resources:
    Group111:
        Type: 'AWS::EC2::SecurityGroup'
        Properties:
            GroupDescription: Test security group
            SecurityGroupEgress: []
            SecurityGroupIngress: []
            VpcId: vpc-60900905
    Everything111:
        Type: 'AWS::EC2::SecurityGroupEgress'
        Properties:
            GroupId:
                'Fn::GetAtt':
                    - Group111
                    - GroupId
            IpProtocol: '-1'
            Description: 'Yeah'
            FromPort: -1
            ToPort: -1
            CidrIp: 0.0.0.0/0
    Specific111:
        Type: 'AWS::EC2::SecurityGroupEgress'
        Properties:
            GroupId:
                'Fn::GetAtt':
                    - Group111
                    - GroupId
            IpProtocol: tcp
            Description: 'to somewhere else'
            FromPort: 1234
            ToPort: 1234
            CidrIp: '1.2.3.4/32'
        DependsOn:  [Everything111]

Still leaves the rule nonexistent (because the specific rule gets applied later on).

The only solution is to apply the "all outbound" rule last, or make a situation in which the "default delete" doesn't apply anymore, probably by making more rules or by changing the format ever so slightly (get rid of some of the -1s, for example).

[rix0rrr]

Looks like it's the CIDR 0.0.0.0/0 and the protocol -1 that are the identifiers for this rule.

Changing the CIDR to 0.0.0.1/0 doesn't help, because it gets normalized back to 0.0.0.0/0.

[rix0rrr]

It's not about the default rule being the only one either, because if I add a bogus rule in (so that there are 2 at the moment that AuthorizeSecurityGroupEgress gets called) the all traffic rule still gets picked out and deleted.

[rix0rrr]

A colleague helpfully pointed out that any other rule in addition to "allow all outbound traffic" is useless anyway. Which is true.

But to solve that, we need to lift this concept into the SecurityGroup class, and what's worse: we then also need to allow disabling it, which requires adding a bogus rule that matches NO traffic (so that the default "all traffic" rule gets cleaned up).

[mhuebner]

I'm wondering why this SecurityGroup is added to the LaunchConfiguration. Wouldn't it be better to just be able to set a securityGroup to have better control?
Currently I'm using AutoScalingGroup.addSecurityGroup to add my own SecurityGroup (with outbound traffic allowed) to I don't want a default group to be added. Would it be possible to just not add any default groups to the LaunchConfiguration (but support setting you own)?

[erikdebruin]

+1 on that suggestion, @mhuebner