feat(ec2): imported SecurityGroups don't create egress rules

[ayazhussein]

Security Groups are created with allowAllOutbound: true by default, and so imported security groups default to that as well. This means that no egress rules will be created for them, because that will undo the default of allowAllOutbound.

This can be configured by setting allowAllOutbound: false upon importing.

[ayazhussein]

attempt to fix #3355

[rix0rrr]

Not 100% opposed, but is adding ingress and egress rules to the securitygroup directly not good enough?

[ayazhussein]

you can manipulate the security group but for example, you cannot remove it and instead of creating a new security group and adding it or accessing the security group in the array, it would be easier/nicer to either offer this way or have a defaultSecurityGroup obj in connections.

having ingress/egress output might affect existing security groups because they might have default all outbound active and adding the egress rule that was generated removes the default all outbound rule (happened to me in production and it wasn't ok, especially when someone might not know that adding any egress rule removes the default all outbound one).

ex:

const asg = new AutoScalingGroup(this, `AutoScalingGroup`, {...props})
// 1st option: connection one-way
asg.connections.allowFrom(Peer.anyIpv4(), Port.allTraffic(), "bla bla", true)
// 2nd option: default security group
asg.connections.defaultSecurityGroup.addIngressRule(Peer.anyIpv4(), Port.allTraffic(), "bla bla")
// current way:
asg.connections.securityGroups[0].addIngressRule(Peer.anyIpv4(), Port.allTraffic(), "bla bla", true)

[rix0rrr]

adding the egress rule that was generated removes the default all outbound rule

Wait, this shouldn't be happening. There is an "allowAllOutbound" property on a security group that makes it so that adding egress rules becomes a no-op (or at least it should be).

Can you confirm that that behavior is broken?

[ayazhussein]

The issue appears when there is an existing Security group outside aws-cdk and you import it.

ex:

const existingSg = SecurityGroup.fromSecurityGroupId(this, "demoSG", "sg-1234567");

const asg = new AutoScalingGroup(this, `AutoScalingGroup`, {...props})

asg.connections.allowFrom(existingSg, Port.tcp(22), "ssh from demo instances")

this will create ingress/egress rule and the issue is that if "demoSG" has all traffic/all ports to 0.0.0.0/0 (being default security group egress rule), Cloudformation removes it and adds the new egress rule only, which is an issue in an existing infrastructure

[rix0rrr]

Yep, I got you. I think the proper solution will be to allow specifying that attribute when importing the SG then.

[ayazhussein]

How would that work?
Maybe I could give it a try.

[rix0rrr]

I think SecurityGroup.fromSecurityGroupId() would take an additional (optional) struct like this:

interface SecurityGroupImportOptions {
  /**
   * Whether the security group has been created to allow all outbound traffic.
   *
   * Unless set to `false`, no new outbound rules will be added to the security group.
   * 
   * @default true
   * @experimental
   */
  allowAllOutbound?: boolean;
}

class SecurityGroup {
  public static fromSecurityGroupId(scope: Construct, id: string, securityGroupId: string, options: SecurityGroupImportOptions = {}): ISecurityGroup {
   // ...
  }
}

Marking the property @experimental as I'm not sure yet whether I want it to be called allowAllOutbound or something like ignoreEgressRules.

[rix0rrr]

@ayazhussein will this satisfy your use case as well?

[mergify]

Pull Request Checklist

• Testing

• Unit test added (prefer to add a new test rather than modify existing tests)
• CLI change? Re-run/add CLI integration tests

• Documentation

• Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
• README: update module README
• Design: for significant features, follow the design process

• Title uses the format type(scope): text

• Type: fix, feat, refactor go into CHANGELOG, chore is hidden
• Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
• Style: use all lower-case, do not end with a period

• Description

• Rationale: describe rationale of change and approach taken
• Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
• Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

• Sensitive Modules (requires 2 PR approvers)

• IAM document library (in @aws-cdk/aws-iam)
• EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
• Grant APIs (if not based on official documentation with a reference)

[mergify]

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

[mergify]

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

[ayazhussein]

@rix0rrr Hi, I couldn't test because of the build failing

error: [awslint:from-signature:@aws-cdk/aws-ec2.SecurityGroup.fromSecurityGroupId] invalid method signature for fromXxx method  (expected="3",actual="4")

[mergify]

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

[ayazhussein]

yes, it is working great, no more CfnSecurityGroupIngress

[mergify]

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

[mergify]

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

[jogold]

Hey @rix0rrr

FYI this change is "infrastructure" breaking because it deletes existing AWS::EC2::SecurityGroupEgress resources for stacks with imported security groups. I just encountered the case in one my stacks after upgrading to v1.6.0. This should be documented in the CHANGELOG.

[jogold]

Also this doesn't cover the case of imported clusters/instances in @aws-cdk/aws-rds where the allowAllOutbound prop is not exposed in the fromDatabaseInstanceAttributes()/fromDatabaseClusterAttributes() methods where there is an implicit import of a security group via its id.

aws-cdk/packages/@aws-cdk/aws-rds/lib/instance.ts

Lines 91 to 103 in 3a0cde0

	public static fromDatabaseInstanceAttributes(scope: Construct, id: string, attrs: DatabaseInstanceAttributes): IDatabaseInstance {
	class Import extends DatabaseInstanceBase implements IDatabaseInstance {
	public readonly defaultPort = ec2.Port.tcp(attrs.port);
	public readonly connections = new ec2.Connections({
	securityGroups: [ec2.SecurityGroup.fromSecurityGroupId(this, 'SecurityGroup', attrs.securityGroupId)],
	defaultPort: this.defaultPort
	});
	public readonly instanceIdentifier = attrs.instanceIdentifier;
	public readonly dbInstanceEndpointAddress = attrs.instanceEndpointAddress;
	public readonly dbInstanceEndpointPort = attrs.port.toString();
	public readonly instanceEndpoint = new Endpoint(attrs.instanceEndpointAddress, attrs.port);
	public readonly securityGroupId = attrs.securityGroupId;
	}