Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ecs] Over-broad IAM permissions on ECS Cluster drain lambda #9501

Closed
ddneilson opened this issue Aug 7, 2020 · 0 comments · Fixed by #9502
Closed

[ecs] Over-broad IAM permissions on ECS Cluster drain lambda #9501

ddneilson opened this issue Aug 7, 2020 · 0 comments · Fixed by #9502
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@ddneilson
Copy link
Contributor

The task-draining lambda that is created for ECS clusters currently has over-broad permissions found here.

Reproduction Steps

Create ECS cluster with any capacity & task.

What did you expect to happen?

IAM Permissions that adhere to the principle of least privilege.

What actually happened?

Broad permissions

Environment

  • CLI Version : 1.52.0
  • Framework Version: 1.52.0
  • Node.js Version: all
  • OS : any
  • Language (Version): all

Other

Some specifics:

  • Both ecs:DescribeContainerInstances, and ecs:DescribeTasks support the ecs:cluster condition. So, these can be trivially down-scoped without affecting functionality by applying that condition.

I have a PR ready to roll on this, and will be posting it momentarily.


This is 🐛 Bug Report

@ddneilson ddneilson added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 7, 2020
@mergify mergify bot closed this as completed in #9502 Aug 7, 2020
mergify bot pushed a commit that referenced this issue Aug 7, 2020
Fixes #9501

### Testing

This was tested by deploying a simple app that was basically the sample from the ECS module readme, and then manually killing off instances from the ECS cluster's ASG. When I killed off an instance I then verified, from the lambda logs, that the task-draining lambda was able to complete its work with no errors.

The essentials of the app are:
```ts
const app = new cdk.App();

const env = {
    account: process.env.CDK_DEFAULT_ACCOUNT,
    region: process.env.CDK_DEFAULT_REGION
}

const stack = new cdk.Stack(app, 'Testing', { env });
const vpc = new ec2.Vpc(stack, 'Vpc');

// Create an ECS cluster
const cluster = new ecs.Cluster(stack, 'Cluster', {
  vpc,
});

// Add capacity to it
cluster.addCapacity('DefaultAutoScalingGroupCapacity', {
  instanceType: new ec2.InstanceType("t2.xlarge"),
  desiredCapacity: 2,
});

const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef');

taskDefinition.addContainer('DefaultContainer', {
  image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
  memoryLimitMiB: 512,
  logging: ecs.LogDriver.awsLogs({
    logGroup: new logs.LogGroup(stack, 'LogGroup', {
      logGroupName: '/test-group/',
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      retention: logs.RetentionDays.ONE_DAY,
    }),
    streamPrefix: 'testing-',
  }),
});

// Instantiate an Amazon ECS Service
const ecsService = new ecs.Ec2Service(stack, 'Service', {
  cluster,
  taskDefinition,
  desiredCount: 2,
});
```

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
eladb pushed a commit that referenced this issue Aug 10, 2020
Fixes #9501

### Testing

This was tested by deploying a simple app that was basically the sample from the ECS module readme, and then manually killing off instances from the ECS cluster's ASG. When I killed off an instance I then verified, from the lambda logs, that the task-draining lambda was able to complete its work with no errors.

The essentials of the app are:
```ts
const app = new cdk.App();

const env = {
    account: process.env.CDK_DEFAULT_ACCOUNT,
    region: process.env.CDK_DEFAULT_REGION
}

const stack = new cdk.Stack(app, 'Testing', { env });
const vpc = new ec2.Vpc(stack, 'Vpc');

// Create an ECS cluster
const cluster = new ecs.Cluster(stack, 'Cluster', {
  vpc,
});

// Add capacity to it
cluster.addCapacity('DefaultAutoScalingGroupCapacity', {
  instanceType: new ec2.InstanceType("t2.xlarge"),
  desiredCapacity: 2,
});

const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef');

taskDefinition.addContainer('DefaultContainer', {
  image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
  memoryLimitMiB: 512,
  logging: ecs.LogDriver.awsLogs({
    logGroup: new logs.LogGroup(stack, 'LogGroup', {
      logGroupName: '/test-group/',
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      retention: logs.RetentionDays.ONE_DAY,
    }),
    streamPrefix: 'testing-',
  }),
});

// Instantiate an Amazon ECS Service
const ecsService = new ecs.Ec2Service(stack, 'Service', {
  cluster,
  taskDefinition,
  desiredCount: 2,
});
```

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant