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

aws_ecs: overly broad permissions granted by enableExecuteCommand #31397

Closed
1 task
sandfox opened this issue Sep 10, 2024 · 5 comments · Fixed by #31475
Closed
1 task

aws_ecs: overly broad permissions granted by enableExecuteCommand #31397

sandfox opened this issue Sep 10, 2024 · 5 comments · Fixed by #31475
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@sandfox
Copy link

sandfox commented Sep 10, 2024

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

private executeCommandLogConfiguration() {
const logConfiguration = this.cluster.executeCommandConfiguration?.logConfiguration;
this.taskDefinition.addToTaskRolePolicy(new iam.PolicyStatement({
actions: [
'logs:DescribeLogGroups',
],
resources: ['*'],
}));
const logGroupArn = logConfiguration?.cloudWatchLogGroup ? `arn:${this.stack.partition}:logs:${this.env.region}:${this.env.account}:log-group:${logConfiguration.cloudWatchLogGroup.logGroupName}:*` : '*';
this.taskDefinition.addToTaskRolePolicy(new iam.PolicyStatement({
actions: [
'logs:CreateLogStream',
'logs:DescribeLogStreams',
'logs:PutLogEvents',
],
resources: [logGroupArn],
}));

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

  • Select this option if this issue appears to be a regression.

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_
@sandfox sandfox added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 10, 2024
@github-actions github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Sep 10, 2024
@khushail khushail added needs-reproduction This issue needs reproduction. p2 and removed needs-triage This issue or PR still needs to be triaged. labels Sep 11, 2024
@khushail khushail self-assigned this Sep 11, 2024
@khushail khushail added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Sep 17, 2024
@khushail
Copy link
Contributor

khushail commented Sep 17, 2024

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.

@khushail khushail added p1 effort/small Small work item – less than a day of effort and removed needs-reproduction This issue needs reproduction. investigating This issue is being investigated and/or work is in progress to resolve the issue. p2 labels Sep 17, 2024
@khushail khushail removed their assignment Sep 17, 2024
@pahud
Copy link
Contributor

pahud commented Sep 17, 2024

I think we should fix this line

const logging = this.cluster.executeCommandConfiguration?.logging ?? ExecuteCommandLogging.DEFAULT;

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
Copy link
Contributor

GavinZZ commented Sep 18, 2024

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.

@mergify mergify bot closed this as completed in #31475 Sep 18, 2024
@mergify mergify bot closed this as completed in de7ab7c Sep 18, 2024
Copy link

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.

1 similar comment
Copy link

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 github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
4 participants