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.55k stars 3.87k forks source link

aws_ecs: overly broad permissions granted by enableExecuteCommand #31397

Closed sandfox closed 1 week ago

sandfox commented 2 weeks ago

Describe the bug

If a FargateService has enableExecuteCommand: true and the ECS cluster it runs on has executeCommandConfiguration.logging set to anything but ecs.ExecuteCommandLogging.NONE then the CDK automatically grants the underlying TaskDefinition overly broad cloudwatch logs permissions regardless of need. If the logging configuration has no cloudwatch logs config set then it allows CreateLogStream, DescribeLogStreams, PutLogEvents on resource: ["*"]

https://github.com/aws/aws-cdk/blob/af9e6bae94c0c303364c2c4f2033eb3823fb59c9/packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts#L754C1-L756C8

https://github.com/aws/aws-cdk/blob/af9e6bae94c0c303364c2c4f2033eb3823fb59c9/packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts#L1035-L1052

As best I understand it, the CDK is automatically granting cloudwatch logs permissions if any kind executeCommandConfiguration.logging is enabled, even if there is no configuration set to send to logs to cloudwatch. I'm not aware of any reason why these permissions need to be automatically granted if there is no config to send logs to cloudwatch. It seems to me that these permissions should at least be behind some kind of "is cloudwatch logging enabled" check, and potentially not even be needed unless logging is set to ecs.ExecuteCommandLogging.OVERRIDE (based on my understanding of https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ecs-cluster-executecommandconfiguration.html)

The current behaviour feels bad from a security point of view and does indeed trigger various security tooling to complain about overly broad write permissions. e.g checkov

check: CKV_AWS_111: "Ensure IAM policies does not allow write access without constraints"

Regression Issue

Last Known Working CDK Version

No response

Expected Behavior

No extra cloudwatch logs permissions to be added to the Task Role

Current Behavior

lots of cloudwatch logs permissions get added to the task role.

Reproduction Steps


import * as cdk from "aws-cdk-lib";
import { Construct } from "constructs";
import * as ec2 from "aws-cdk-lib/aws-ec2";
import * as ecs from "aws-cdk-lib/aws-ecs";

import { Bastion } from "./bastion-construct";

export class DemoStack extends cdk.Stack { 
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const cluster = new ecs.Cluster(this, "cluster", {
      enableFargateCapacityProviders: true,
    });

    const taskDefinition = new ecs.FargateTaskDefinition(this, "TaskDef", {
      cpu: 256,
      memoryLimitMiB: 512,
      runtimePlatform: {
        operatingSystemFamily: ecs.OperatingSystemFamily.LINUX,
        cpuArchitecture: ecs.CpuArchitecture.ARM64, 
      },
    });

    const containerDef = taskDefinition.addContainer("Container", {
      image: ecs.ContainerImage.fromRegistry(
        "public.ecr.aws/amazonlinux/amazonlinux:2023-minimal",
      ),
      logging: new ecs.AwsLogDriver({
        logRetention: logs.RetentionDays.ONE_MONTH,
        mode: ecs.AwsLogDriverMode.NON_BLOCKING,
        streamPrefix: "demo",
      }),
      command: ["sleep", 360],
      linuxParameters: new ecs.LinuxParameters(this, "LinuxParameters", {
        initProcessEnabled: true,
      }),
    });

    const service = new ecs.FargateService(this, "Service", {
      cluster,,
      taskDefinition,
      enableExecuteCommand: true, 
    });
}

### Possible Solution

See initial comment - only add addition cloudwatch logs permissions when required.

### Additional Information/Context

_No response_

### CDK CLI Version

2.157.0 (build 7315a59)

### Framework Version

_No response_

### Node.js Version

v20.13.1

### OS

OSX

### Language

TypeScript

### Language Version

5.5.3

### Other information

_No response_
khushail commented 1 week ago

Hi @sandfox , thanks for reaching out.

The issue is reproducible with the shared code sample. When enableExecuteCommand:true, and the logging config enabled, the synthesized template has -

the synthesized template has this DefaultPolicy -

"FargateIssueTaskDefinitionv2ExecutionRoleDefaultPolicy1F84A7F4": {
   "Type": "AWS::IAM::Policy",
   "Properties": {
    "PolicyDocument": {
     "Statement": [
      {
       "Action": [
        "logs:CreateLogStream",
        "logs:PutLogEvents"
       ],
       "Effect": "Allow",
       "Resource": {
        "Fn::GetAtt": [
         "FargateIssueTaskDefinitionv2FargateIssueContainerv2LogGroup5E0366CD",
         "Arn"
        ]
       }
      }
     ],
     "Version": "2012-10-17"
    },
    .......
  "FargateIssueTaskDefinitionv2TaskRoleDefaultPolicyE18491F5": {
   "Type": "AWS::IAM::Policy",
   "Properties": {
    "PolicyDocument": {
     "Statement": [
      {
       "Action": [
        "logs:CreateLogStream",
        "logs:DescribeLogGroups",
        "logs:DescribeLogStreams",
        "logs:PutLogEvents",
        "ssmmessages:CreateControlChannel",
        "ssmmessages:CreateDataChannel",
        "ssmmessages:OpenControlChannel",
        "ssmmessages:OpenDataChannel"
       ],
       "Effect": "Allow",
       "Resource": "*"
      }
     ],
     "Version": "2012-10-17"
    },
    "PolicyName": "FargateIssueTaskDefinitionv2TaskRoleDefaultPolicyE18491F5",
    "Roles": [
     {
      "Ref": "FargateIssueTaskDefinitionv2TaskRoleBF6E7591"
     }
    ]
   },

and when its changed without any log configuration, still the policy has these statements -

"FargateIssueTaskDefinitionv2TaskRoleDefaultPolicyE18491F5": {
   "Type": "AWS::IAM::Policy",
   "Properties": {
    "PolicyDocument": {
     "Statement": [
      {
       "Action": [
        "logs:CreateLogStream",
        "logs:DescribeLogGroups",
        "logs:DescribeLogStreams",
        "logs:PutLogEvents",
        "ssmmessages:CreateControlChannel",
        "ssmmessages:CreateDataChannel",
        "ssmmessages:OpenControlChannel",
        "ssmmessages:OpenDataChannel"
       ],
       "Effect": "Allow",
       "Resource": "*"
      }
     ],
     "Version": "2012-10-17"
    },
    "PolicyName": "FargateIssueTaskDefinitionv2TaskRoleDefaultPolicyE18491F5",
    "Roles": [
     {
      "Ref": "FargateIssueTaskDefinitionv2TaskRoleBF6E7591"
     }
    ]

Looking at the cdk code , this block is getting executed if executeCommandLogging is anything but NONE -

https://github.com/aws/aws-cdk/blob/af9e6bae94c0c303364c2c4f2033eb3823fb59c9/packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts#L754C1-L757C6

which might need to be updated with another check for log config.

As per Docs, when the logging is not configured, output should not be logged. So this seems to be an issue. Marking this as P1 for team's immediate attention.

pahud commented 1 week ago

I think we should fix this line

https://github.com/aws/aws-cdk/blob/af9e6bae94c0c303364c2c4f2033eb3823fb59c9/packages/aws-cdk-lib/aws-ecs/lib/base/base-service.ts#L749

when logging is undefined, at this moment, it would default to ExecuteCommandLogging.DEFAULT which != ExecuteCommandLogging.NONE and this would always be true.

Maybe we should just leave

const logging = this.cluster.executeCommandConfiguration?.logging

or

const logging = this.cluster.executeCommandConfiguration?.logging ?? ExecuteCommandLogging.NONE
GavinZZ commented 1 week ago

Thanks Pahud and Shailja for the deep dive. Also thanks to @sandfox for reporting the issue. I think this is a valid p1 issue and it's definitely against the best practices.

I pushed a PR to fix it but instead of using the suggested solution by Pahud, I chose to wrap a feature flag inside the method executeCommandLogConfiguration. Only if a valid cloudwatch log group is given to the construct through cluster's logConfiguration will then result in Cloudwatch permissions attached to the IAM Policy.

github-actions[bot] commented 1 week ago

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

github-actions[bot] commented 1 week ago

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.