Skip to content

Commit

Permalink
fix(ecs): downscope permissions required by instance draining hook (#…
Browse files Browse the repository at this point in the history 
…2761)

Minimize the IAM permissions required for the instance draining hook.
  • Loading branch information
@piradeepk @rix0rrr
piradeepk authored and rix0rrr committed Jun 12, 2019
1 parent 55d1bc8 commit 9ea6148
Show file tree
Hide file tree
Showing 10 changed files with 337 additions and 53 deletions.
24 changes: 23 additions & 1 deletion packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import elbv2 = require('@aws-cdk/aws-elasticloadbalancingv2');
import iam = require('@aws-cdk/aws-iam');
import sns = require('@aws-cdk/aws-sns');

import { AutoScalingRollingUpdate, Construct, Fn, IResource, Lazy, Resource, Tag } from '@aws-cdk/cdk';
import { AutoScalingRollingUpdate, Construct, Fn, IResource, Lazy, Resource, Stack, Tag } from '@aws-cdk/cdk';
import { CfnAutoScalingGroup, CfnAutoScalingGroupProps, CfnLaunchConfiguration } from './autoscaling.generated';
import { BasicLifecycleHookProps, LifecycleHook } from './lifecycle-hook';
import { BasicScheduledActionProps, ScheduledAction } from './scheduled-action';
Expand Down Expand Up @@ -196,6 +196,7 @@ export interface AutoScalingGroupProps extends CommonAutoScalingGroupProps {
abstract class AutoScalingGroupBase extends Resource implements IAutoScalingGroup {

public abstract autoScalingGroupName: string;
public abstract autoScalingGroupArn: string;
protected albTargetGroup?: elbv2.ApplicationTargetGroup;

/**
Expand Down Expand Up @@ -318,6 +319,11 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
public static fromAutoScalingGroupName(scope: Construct, id: string, autoScalingGroupName: string): IAutoScalingGroup {
class Import extends AutoScalingGroupBase {
public autoScalingGroupName = autoScalingGroupName;
public autoScalingGroupArn = Stack.of(this).formatArn({
service: 'autoscaling',
resource: 'autoScalingGroup:*:autoScalingGroupName',
resourceName: this.autoScalingGroupName
});
}

return new Import(scope, id);
Expand All @@ -343,6 +349,11 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
*/
public readonly autoScalingGroupName: string;

/**
* Arn of the AutoScalingGroup
*/
public readonly autoScalingGroupArn: string;

private readonly userDataLines = new Array<string>();
private readonly autoScalingGroup: CfnAutoScalingGroup;
private readonly securityGroup: ec2.ISecurityGroup;
Expand Down Expand Up @@ -432,6 +443,11 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
this.autoScalingGroup = new CfnAutoScalingGroup(this, 'ASG', asgProps);
this.osType = machineImage.os.type;
this.autoScalingGroupName = this.autoScalingGroup.autoScalingGroupName;
this.autoScalingGroupArn = Stack.of(this).formatArn({
service: 'autoscaling',
resource: 'autoScalingGroup:*:autoScalingGroupName',
resourceName: this.autoScalingGroupName
});

this.applyUpdatePolicies(props);
}
Expand Down Expand Up @@ -707,6 +723,12 @@ export interface IAutoScalingGroup extends IResource {
*/
readonly autoScalingGroupName: string;

/**
* The arn of the AutoScalingGroup
* @attribute
*/
readonly autoScalingGroupArn: string;

/**
* Send a message to either an SQS queue or SNS topic when instances launch or terminate
*/
Expand Down
47 changes: 42 additions & 5 deletions packages/@aws-cdk/aws-ecs-patterns/test/ec2/integ.scheduled-ecs-task.lit.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,6 @@
"Statement": [
{
"Action": [
"autoscaling:CompleteLifecycleAction",
"ec2:DescribeInstances",
"ec2:DescribeInstanceAttribute",
"ec2:DescribeInstanceStatus",
Expand All @@ -412,18 +411,56 @@
"Effect": "Allow",
"Resource": "*"
},
{
"Action": "autoscaling:CompleteLifecycleAction",
"Effect": "Allow",
"Resource": {
"Fn::Join": [
"",
[
"arn:",
{
"Ref": "AWS::Partition"
},
":autoscaling:",
{
"Ref": "AWS::Region"
},
":",
{
"Ref": "AWS::AccountId"
},
":autoScalingGroup:*:autoScalingGroupName/",
{
"Ref": "EcsClusterDefaultAutoScalingGroupASGC1A785DB"
}
]
]
}
},
{
"Action": [
"ecs:DescribeContainerInstances",
"ecs:DescribeTasks"
],
"Effect": "Allow",
"Resource": "*"
},
{
"Action": [
"ecs:ListContainerInstances",
"ecs:SubmitContainerStateChange",
"ecs:SubmitTaskStateChange",
"ecs:DescribeContainerInstances",
"ecs:UpdateContainerInstancesState",
"ecs:ListTasks",
"ecs:DescribeTasks"
"ecs:ListTasks"
],
"Effect": "Allow",
"Resource": "*"
"Resource": {
"Fn::GetAtt": [
"EcsCluster97242B84",
"Arn"
]
}
}
],
"Version": "2012-10-17"
Expand Down
8 changes: 1 addition & 7 deletions packages/@aws-cdk/aws-ecs/lib/cluster.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -396,13 +396,7 @@ class ImportedCluster extends Resource implements ICluster {
this.clusterArn = props.clusterArn !== undefined ? props.clusterArn : Stack.of(this).formatArn({
service: 'ecs',
resource: 'cluster',
resourceName: props.clusterName,
});

this.clusterArn = props.clusterArn !== undefined ? props.clusterArn : Stack.of(this).formatArn({
service: 'ecs',
resource: 'cluster',
resourceName: props.clusterName,
resourceName: props.clusterName
});

let i = 1;
Expand Down
29 changes: 19 additions & 10 deletions packages/@aws-cdk/aws-ecs/lib/drain-hook/instance-drain-hook.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,29 +69,38 @@ export class InstanceDrainHook extends cdk.Construct {
heartbeatTimeoutSec: drainTimeSeconds,
});

// FIXME: These should probably be restricted usefully in some way, but I don't exactly
// know how.
// Describe actions cannot be restricted and restrict the CompleteLifecycleAction to the ASG arn
// https://docs.aws.amazon.com/autoscaling/ec2/userguide/control-access-using-iam.html
fn.addToRolePolicy(new iam.PolicyStatement()
.addActions(
'autoscaling:CompleteLifecycleAction',
'ec2:DescribeInstances',
'ec2:DescribeInstanceAttribute',
'ec2:DescribeInstanceStatus',
'ec2:DescribeHosts',
'ec2:DescribeHosts'
)
.addAllResources());

// FIXME: These should be restricted to the ECS cluster probably, but I don't exactly
// know how.
// Restrict to the ASG
fn.addToRolePolicy(new iam.PolicyStatement()
.addActions(
'autoscaling:CompleteLifecycleAction'
)
.addResource(props.autoScalingGroup.autoScalingGroupArn));

fn.addToRolePolicy(new iam.PolicyStatement()
.addActions(
'ecs:DescribeContainerInstances',
'ecs:DescribeTasks')
.addAllResources());

// Restrict to the ECS Cluster
fn.addToRolePolicy(new iam.PolicyStatement()
.addActions(
'ecs:ListContainerInstances',
'ecs:SubmitContainerStateChange',
'ecs:SubmitTaskStateChange',
'ecs:DescribeContainerInstances',
'ecs:UpdateContainerInstancesState',
'ecs:ListTasks',
'ecs:DescribeTasks')
.addAllResources());
'ecs:ListTasks')
.addResource(props.cluster.clusterArn));
}
}
47 changes: 42 additions & 5 deletions packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-awsvpc-nw.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,6 @@
"Statement": [
{
"Action": [
"autoscaling:CompleteLifecycleAction",
"ec2:DescribeInstances",
"ec2:DescribeInstanceAttribute",
"ec2:DescribeInstanceStatus",
Expand All @@ -568,18 +567,56 @@
"Effect": "Allow",
"Resource": "*"
},
{
"Action": "autoscaling:CompleteLifecycleAction",
"Effect": "Allow",
"Resource": {
"Fn::Join": [
"",
[
"arn:",
{
"Ref": "AWS::Partition"
},
":autoscaling:",
{
"Ref": "AWS::Region"
},
":",
{
"Ref": "AWS::AccountId"
},
":autoScalingGroup:*:autoScalingGroupName/",
{
"Ref": "EcsClusterDefaultAutoScalingGroupASGC1A785DB"
}
]
]
}
},
{
"Action": [
"ecs:DescribeContainerInstances",
"ecs:DescribeTasks"
],
"Effect": "Allow",
"Resource": "*"
},
{
"Action": [
"ecs:ListContainerInstances",
"ecs:SubmitContainerStateChange",
"ecs:SubmitTaskStateChange",
"ecs:DescribeContainerInstances",
"ecs:UpdateContainerInstancesState",
"ecs:ListTasks",
"ecs:DescribeTasks"
"ecs:ListTasks"
],
"Effect": "Allow",
"Resource": "*"
"Resource": {
"Fn::GetAtt": [
"EcsCluster97242B84",
"Arn"
]
}
}
],
"Version": "2012-10-17"
Expand Down
47 changes: 42 additions & 5 deletions packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-bridge-nw.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,6 @@
"Statement": [
{
"Action": [
"autoscaling:CompleteLifecycleAction",
"ec2:DescribeInstances",
"ec2:DescribeInstanceAttribute",
"ec2:DescribeInstanceStatus",
Expand All @@ -589,18 +588,56 @@
"Effect": "Allow",
"Resource": "*"
},
{
"Action": "autoscaling:CompleteLifecycleAction",
"Effect": "Allow",
"Resource": {
"Fn::Join": [
"",
[
"arn:",
{
"Ref": "AWS::Partition"
},
":autoscaling:",
{
"Ref": "AWS::Region"
},
":",
{
"Ref": "AWS::AccountId"
},
":autoScalingGroup:*:autoScalingGroupName/",
{
"Ref": "EcsClusterDefaultAutoScalingGroupASGC1A785DB"
}
]
]
}
},
{
"Action": [
"ecs:DescribeContainerInstances",
"ecs:DescribeTasks"
],
"Effect": "Allow",
"Resource": "*"
},
{
"Action": [
"ecs:ListContainerInstances",
"ecs:SubmitContainerStateChange",
"ecs:SubmitTaskStateChange",
"ecs:DescribeContainerInstances",
"ecs:UpdateContainerInstancesState",
"ecs:ListTasks",
"ecs:DescribeTasks"
"ecs:ListTasks"
],
"Effect": "Allow",
"Resource": "*"
"Resource": {
"Fn::GetAtt": [
"EcsCluster97242B84",
"Arn"
]
}
}
],
"Version": "2012-10-17"
Expand Down
Loading

0 comments on commit 9ea6148

Please sign in to comment.