aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.65k stars 3.91k forks source link

SecurityGroup.fromLookupByName() doesnt have properties to set mutable to false #22858

Open jashan05 opened 1 year ago

jashan05 commented 1 year ago

Describe the bug

We have exiting Security groups which we want to lookup and use by name, but there is no option to set properties while using method SecurityGroup.fromLookupByName(). So have couple of questions:

For reference we are doing the following work around for now:

this.vpc = ec2.Vpc.fromLookup(this, 'Vpc', { isDefault: false });
let mutablePrivateSg = ec2.SecurityGroup.fromLookupByName(this, 'MutableInternalSg', 'SG1Name', this.vpc);
let mutablePublicSg = ec2.SecurityGroup.fromLookupByName(this, 'MutablePublicSg', 'SG2Name', this.vpc);

this.privateSg =  ec2.SecurityGroup.fromSecurityGroupId(this, 'InternalSG', mutablePrivateSg.securityGroupId, {
        mutable: false
    });

    this.publicSg =  ec2.SecurityGroup.fromSecurityGroupId(this, 'PublicSG', mutablePublicSg.securityGroupId, {
        mutable: false
    });

Expected Behavior

Either ec2.SecurityGroup.fromLookupByName() also has properties to be defined or mutable is set to default by false

Current Behavior

Using ec2.SecurityGroup.fromLookupByName()doesnt allow to set properties like {mutable: false} which causes issues and overwrites our existing SG's rules.

Reproduction Steps

  1. Lookup an exiting SG by name, with existing rules
  2. Use that SG in a newly created ASG.
  3. Use this newly created ASG as a target to ELB

Outcome will be : SG's will be overwritten as per below generated CFT (impact seems to be not consistent):

{
 "Resources": {
  "xxxxxxxxxxxxxxxxx": {
   "Type": "AWS::EC2::SecurityGroupIngress",
   "Properties": {
    "IpProtocol": "tcp",
    "CidrIp": "0.0.0.0/0",
    "Description": "Allow from anyone on port 443",
    "FromPort": 443,
    "GroupId": "xxxxxxxxxxxxxxxxx",
    "ToPort": 443
   },
   "Metadata": {
    "aws:cdk:path": "xxxxxxxxxxxxxxxxx"
   }
  },
  "xxxxxxxxxxxxxxxxx": {
   "Type": "AWS::EC2::SecurityGroupIngress",
   "Properties": {
    "IpProtocol": "tcp",
    "Description": "Load balancer to target",
    "FromPort": 9000,
    "GroupId": "xxxxxxxxxxxxxxxxx",
    "SourceSecurityGroupId": "xxxxxxxxxxxxxxxxx",
    "ToPort": 9000
   },
   "Metadata": {
    "aws:cdk:path": "xxxxxxxxxxxxxxxxx"
   }
  },
  "xxxxxxxxxxxxxxxxx": {
   "Type": "AWS::EC2::SecurityGroupIngress",
   "Properties": {
    "IpProtocol": "tcp",
    "Description": "Load balancer to target",
    "FromPort": 9000,
    "GroupId": "xxxxxxxxxxxxxxxxx",
    "SourceSecurityGroupId": "xxxxxxxxxxxxxxxxx",
    "ToPort": 9000
   },
   "Metadata": {
    "aws:cdk:path": "xxxxxxxxxxxxxxxxx"
   }
  },
  "xxxxxxxxxxxxxxxxx": {
   "Type": "AWS::EC2::SecurityGroupEgress",
   "Properties": {
    "GroupId": "xxxxxxxxxxxxxxxxx",
    "IpProtocol": "tcp",
    "Description": "Load balancer to target",
    "DestinationSecurityGroupId": "xxxxxxxxxxxxxxxxx",
    "FromPort": 9000,
    "ToPort": 9000
   },
   "Metadata": {
    "aws:cdk:path": "xxxxxxxxxxxxxxxxx"
   }
  },
  "xxxxxxxxxxxxxxxxx": {
   "Type": "AWS::EC2::SecurityGroupIngress",
   "Properties": {
    "IpProtocol": "tcp",
    "CidrIp": "0.0.0.0/0",
    "Description": "Allow from anyone on port 443",
    "FromPort": 443,
    "GroupId": "xxxxxxxxxxxxxxxxx",
    "ToPort": 443
   },
   "Metadata": {
    "aws:cdk:path": "xxxxxxxxxxxxxxxxx"
   }
  },
  "configPublicSgt xxxxxxxxxxxxxxxxx": {
   "Type": "AWS::EC2::SecurityGroupEgress",
   "Properties": {
    "GroupId": "xxxxxxxxxxxxxxxxx",
    "IpProtocol": "tcp",
    "Description": "Load balancer to target",
    "DestinationSecurityGroupId": "xxxxxxxxxxxxxxxxx",
    "FromPort": 9000,
    "ToPort": 9000
   },
   "Metadata": {
    "aws:cdk:path": "xxxxxxxxxxxxxxxxx"
   }
  }
  }

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.45.0

Framework Version

No response

Node.js Version

16

OS

rhel 7

Language

Typescript

Language Version

TypeScript (4.8.4)

Other information

No response

pahud commented 1 year ago

Hi @jashan05

I was trying to reproduce your issue with the provided steps:

1. Lookup an exiting SG by name, with existing rules
2. Use that SG in a newly created ASG.
3. Use this newly created ASG as a target to ELB

So I first create a SG with a ingress rule:

    // create SG
    const sg = new ec2.SecurityGroup(this, 'SG', {
      vpc,
      securityGroupName: 'my-test-sg',
    });
    new ec2.CfnSecurityGroupIngress(this, 'Ing', {
      ipProtocol: 'tcp',
      fromPort: 8888,
      toPort: 8888,
      cidrIp: '0.0.0.0/0',
      groupId: sg.securityGroupId,
    });

Then I import the SG by its name

   // Lookup an exiting SG by name, with existing rules
    const importedSG = ec2.SecurityGroup.fromLookupByName(this, 'importedSg', 'my-test-sg', vpc);

I created an autoscaling group with the imported SG and add the ASG as the ELB target:

    // Use that SG in a newly created ASG.
    const asg = new autoscaling.AutoScalingGroup(this, 'ASG', {
      securityGroup: importedSG,
      machineImage: new ec2.AmazonLinuxImage(),
      instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE3, ec2.InstanceSize.LARGE),
      vpc,
    });
    // Use this newly created ASG as a target to ELB
    const lb = new elbv2.ApplicationLoadBalancer(this, 'ALB', {
      vpc,
    });
    const listener = lb.addListener('Listener', {
      port: 80,
      open: true,
    });
    listener.addTargets('ApplicationFleet', {
      port: 8080,
      targets: [asg],
    });

From what I can tell from the console, the imported SG is still having the original ingress rule with one additional ingress rule(tcp 8080 from the ELB SG).

I didn't see the SG was overwritten, in fact, additional ingress rules were created and associated to this imported SG so the http traffic from the ELB can access the ASG target.

image

However, if you destroy the CDK stack, the new ingress rule created by CDK will be removed from the imported SG.

github-actions[bot] commented 1 year ago

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

jashan05 commented 1 year ago

Hello @pahud ,

Thanks for the response. Sorry forgot to mention that in my case i had 2 SG's. I got problems with SG which was added afterwards as below:

     this.asg = new as.AutoScalingGroup(this, "xxxxx-asg", {
        autoScalingGroupName: `${props?.team}-${props?.stage}-xxxx-asg`,
        healthCheck: {
            type: "ELB",
        },
        vpc: props.vpc,
        role: xxxxxxxx,
        instanceType: props.instanceSize,
        groupMetrics: [new as.GroupMetrics(as.GroupMetric.TERMINATING_INSTANCES)],
        machineImage: props.ami,
        securityGroup: props.privateSg,
        keyName: props?.stage == 'acc' ? 'Acceptance' : 'Production',
        updatePolicy: as.UpdatePolicy.rollingUpdate(),
        minCapacity: 1,
        maxCapacity: 1,
        blockDevices: [{
            deviceName: '/dev/sda1',
            volume: as.BlockDeviceVolume.ebs(props?.stage == 'acc' ? 100 : 200, {
                deleteOnTermination: true,
                encrypted: true,
                iops: 400,
                volumeType: as.EbsDeviceVolumeType.IO1
            })
        }]
    });

// Adding below SG has issues where all traffic rules were removed and only ELB specific traffic rules were added
    this.asg.addSecurityGroup(props.publicSg);

where props.publicSG is coming from SecurityGroup.fromLookupByName()

Also going back to original query i have, cant we add properties in the method SecurityGroup.fromLookupByName(). Second - Shouldnt the mutable property be set to false by default while doing a lookup ?

Best Regards Jashan

pahud commented 1 year ago

Interesting. According to this:

https://github.com/aws/aws-cdk/blob/247d0f3f0a0f208391d4ed1480f9269c6941ae6b/packages/%40aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts#L1374-L1377

If you have defined props.securityGroup for your asg followed by asg.addSecurityGroup() it should throw an error. No?

github-actions[bot] commented 1 year ago

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

jashan05 commented 1 year ago

@pahud Not sure if you understood my problem. 😄 . I am not getting any errors. But the problem is just related to default value of mutable: true tag while looking up security groups by name ec2.SecurityGroup.fromLookupByName() .

My question/suggestion is to have default value of this tag as mutable: false.If that is not possible, Also would love to understand why !

nowosprz commented 1 year ago

I also need the possibility to do a fromLookupByName with mutable: false. Use case: creating RDS DB Proxy implicitely adds rules, but I don't want to give my stack permissions to tamper with SGs. I am able to use ec2.SecurityGroup.fromSecurityGroupId, but that is not really viable, since we want to have the possibility of destroying and recreating whole environment at any time, which would result in securityGroupId change

vihangshah8 commented 4 months ago

@pahud

CDK Version: 2.146.0

We recently had an outage due to a deployment where CDK modified the imported security group and revoked existing outbound rules which does not make any sense to us. The logic is that in our CDK app, we lookup an externally created security group by id and use that as a part of the autoscaling group and ec2 instance creation.

I would echo what is mentioned above - Can we have a flag where we do not allow CDK to mutate our existing security groups when using from_lookup_by_id or if there is a workaround, can you please share with us so we are not having this issues when we use import functionality of resources like this one?

Below is the cloudtrail event associated with the revoke:

{
    "eventVersion": "1.09",
    "userIdentity": {
        "type": "AssumedRole",
        "principalId": "AWSCloudFormation",
        "arn": "arn:aws:sts:::assumed-role/cdk-hnb659fds-cfn-exec-role--us-west-2/AWSCloudFormation",
        "accountId": "",
        "sessionContext": {
            "sessionIssuer": {
                "type": "Role",
                "principalId": "",
                "arn": "arn:aws:iam:::role/cdk-hnb659fds-cfn-exec-role--us-west-2",
                "accountId": "",
                "userName": "cdk-hnb659fds-cfn-exec-role--us-west-2"
            },
            "attributes": {
                "creationDate": "2024-06-17T23:01:31Z",
                "mfaAuthenticated": "false"
            }
        },
        "invokedBy": "cloudformation.amazonaws.com"
    },
    "eventTime": "2024-06-17T23:01:31Z",
    "eventSource": "ec2.amazonaws.com",
    "eventName": "RevokeSecurityGroupEgress",
    "awsRegion": "us-west-2",
    "sourceIPAddress": "cloudformation.amazonaws.com",
    "userAgent": "cloudformation.amazonaws.com",
    "requestParameters": {
        "groupId": "sg-id",
        "ipPermissions": {
            "items": [
                {
                    "ipProtocol": "-1",
                    "groups": {},
                    "ipRanges": {
                        "items": [
                            {
                                "cidrIp": "0.0.0.0/0"
                            }
                        ]
                    },
                    "ipv6Ranges": {},
                    "prefixListIds": {}
                }
            ]
        }
    },
    "responseElements": {
        "requestId": "970867d7-f761-4b49-b8f1-1e3ec3daba62",
        "_return": true,
        "revokedSecurityGroupRuleSet": {
            "items": [
                {
                    "groupId": "sg-id",
                    "securityGroupRuleId": "sgr-0e76e51d8386434b0",
                    "isEgress": true,
                    "ipProtocol": "-1",
                    "fromPort": -1,
                    "toPort": -1,
                    "cidrIpv4": "0.0.0.0/0"
                }
            ]
        }
    },
    "requestID": "970867d7-f761-4b49-b8f1-1e3ec3daba62",
    "eventID": "d5be1271-2a26-449f-a6d8-74dac4c26438",
    "readOnly": false,
    "eventType": "AwsApiCall",
    "managementEvent": true,
    "recipientAccountId": "",
    "eventCategory": "Management"
}

Below is the code which imports existing security group in the ASG and same for ELBv2:

autoscaling_group=_autoscaling.AutoScalingGroup(self,"AutoScalingGroup",
                                                        vpc=_ec2.Vpc.from_lookup(self,f"{stage}-VPCLookup",vpc_id=vpc_id),
                                                        vpc_subnets = _ec2.SubnetSelection(subnets=[_ec2.Subnet.from_subnet_attributes(self,f"{stage}-SubnetReference1",availability_zone="us-west-2a",subnet_id=private_subnet_id_us_west_2a),
                                                                                                    _ec2.Subnet.from_subnet_attributes(self,f"{stage}-SubnetReference2",availability_zone="us-west-2b",subnet_id=private_subnet_id_us_west_2b)]),
                                                        instance_type=_ec2.InstanceType(ec2_instance_type),
                                                        security_group=_ec2.SecurityGroup.from_lookup_by_id(self,f"{stage}-InstanceSG",security_group_id=instance_security_group),
                                                        machine_image=_ec2.MachineImage.from_ssm_parameter("/aws/service/ami-amazon-linux-latest/al2023-ami-kernel-default-arm64"),
                                                        desired_capacity=4,
                                                        role=autoscaling_group_instance_roles,
                                                        max_capacity=5,
                                                        min_capacity=1)
load_balancer=_elbv2.ApplicationLoadBalancer(self,f"{stage}-LoadBalancerV2",
                                                     vpc=_ec2.Vpc.from_lookup(self,f"{stage}-ELBV2VPCLookup",vpc_id=vpc_id),
                                                     security_group=_ec2.SecurityGroup.from_lookup_by_id(self,f"{stage}-SGELBV2",security_group_id=elbv2_security_group),
                                                     internet_facing=True,
                                                     vpc_subnets=_ec2.SubnetSelection(subnet_type=_ec2.SubnetType.PUBLIC))