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 Secrets from SSM ParamStore exceeds IAM Policy size #8435

Closed
cynicaljoy opened this issue Jun 8, 2020 · 9 comments
Closed

ECS Secrets from SSM ParamStore exceeds IAM Policy size #8435

cynicaljoy opened this issue Jun 8, 2020 · 9 comments
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container @aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@cynicaljoy
Copy link

I'm unable to deploy my stack that makes use of ECS Secrets. I'm trying to use 58 ECS Secrets (current hard limit is 60) but how CDK is currently writing the IAM Policy for the Execution Role I'm hitting a limit with the IAM Policy.

The limit is being hit because CDK is adding a statement to the policy per parameter which creates a lot of duplication within the statement.

            Resource:
              Fn::Join:
                - ""
                - - "arn:"
                  - Ref: AWS::Partition
                  - :ssm:us-east-2:065449092177:parameter/application/parameter1
          - Action:
              - ssm:DescribeParameters
              - ssm:GetParameters
              - ssm:GetParameter
              - ssm:GetParameterHistory
            Effect: Allow

Reproduction Steps

Sorry, I don't have a stack that I can share.

  1. Create ECS Application w/60 secrets
  2. cdk deploy

Error Log

Maximum policy size of 10240 bytes exceeded for role

Environment

  • CLI Version : 1.44.0
  • Framework Version: 1.44.0
  • Node.js Version: v12.16.1
  • OS : macOS 10.15.5
  • Language (Version): TypeScript (3.7.2)

Other

My ideal solution would be an additional parameter for ecs.Secret.fromSsmParameter that gives the option to skip the grant. My stack already appends a Managed Policy to the Execution Role that has all of the access necessary for my application to pull the parameters.

A more immediate option would be to group all of the SSM parameter grants into a single statement to dramatically reduce the duplication of the Action statements 60 times


This is 🐛 Bug Report

@cynicaljoy cynicaljoy added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 8, 2020
@SomayaB SomayaB added @aws-cdk/aws-ecs Related to Amazon Elastic Container @aws-cdk/aws-iam Related to AWS Identity and Access Management labels Jun 8, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 16, 2020

As a workaround you can always pass role.withoutPolicyUpdates() to avoid policies from getting added.

@rix0rrr rix0rrr added the p2 label Jun 16, 2020
@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Jun 18, 2020
@rix0rrr rix0rrr added the effort/medium Medium work item – several days of effort label Aug 12, 2020
@mriccia
Copy link

mriccia commented Sep 11, 2020

I've come across a similar issue, I found that when using ecs.Secret.fromSSMParameter or ecs.Secret.fromSecretsManager an IAM grant is automatically added to the IAM role assigned to the TaskDefinition.
You can see the implementation here and here
The function is then invoked during the object creation here

CDK could make this better by adding an optional flag to disable autocreation of the additional IAM statements.

As a workaround you can do the following:

secretsObj = ecs.Secret.fromSecretsManager(secret);
//add a no-op function to avoid IAM statement creation
secretsObj.grantRead = function(){};

@LeandroSoares
Copy link

LeandroSoares commented Oct 1, 2020

Guys i had the same problem, when using an existing role, until i found the option mutable:

Role.fromRoleArn(this, 'ECSTaskExecutionRole', 'MyExecutionRoleArn', { mutable: false }),

@kyler-hyuna
Copy link

@rix0rrr Would it take significant work to group the ssm secrets into one statement

@Plasma
Copy link
Contributor

Plasma commented Aug 23, 2021

I've hit this too with around 40-50 secrets. The workarounds proposed don't seem to work for me for C# (trying to inherit IParameter to make GrantRead a no-op when used throws a Could not convert argument ... to Jsii (Parameter 'arguments') conversion exception etc).

Does anyone have a reliable workaround at the moment?

@Plasma
Copy link
Contributor

Plasma commented Aug 25, 2021

I've managed to get a workaround working as expected for me in C#, for anyone else interested:

When assigning the list of Secrets to the taskDef/taskImageOptions.Secrets list, first wrap each Secret with an adapter class that causes GrantRead to no-op/do-nothing:

    using EcsSecret = Amazon.CDK.AWS.ECS.Secret;

    /// <summary>
    /// Wrapper class that causes GrantRead() to do nothing
    /// </summary>
    class NoOpGrantReadSecret : EcsSecret
    {
        readonly EcsSecret _secretImplementation;

        public NoOpGrantReadSecret(EcsSecret secretImplementation)
        {
            _secretImplementation = secretImplementation;
        }

        public override Grant GrantRead(IGrantable grantee)
        {
            // No Op
            return Grant.Drop(grantee, "ignored");
        }

        public override string Arn => _secretImplementation.Arn;

        public override bool? HasField => _secretImplementation.HasField;
    }

Example:

var ecsService = new ApplicationLoadBalancedFargateService(..., new ApplicationLoadBalancedFargateServiceProps {
    TaskImageOptions = new ApplicationLoadBalancedTaskImageOptions {
        ...,
        Secrets = existingSecrets.ToDictionary(x => x.Key, x => (EcsSecret) new NoOpGrantReadSecret(x.Value))
    }
})

That removes all the individual grants, but now we need to add them back in a wildcard read grant:

const string ParamNamePrefix = "/your/ssm/param/hierarchy/*";
ecsService.TaskDefinition.ExecutionRole.AddToPrincipalPolicy(new PolicyStatement(new PolicyStatementProps
    {
        Effect = Effect.ALLOW,
        Actions = new[] { "ssm:DescribeParameters", "ssm:GetParameter", "ssm:GetParameterHistory", "ssm:GetParameters" },
        Resources = new[] { $"arn:aws:ssm:{props.Env!.Region}:{props.Env.Account}:parameter/{ParamNamePrefix}" }
    })
);

@metlaivan
Copy link

@Plasma thx a lot for your example. It is working for Java too.
Example on the Java:

public class NoOpGrantReadSecret extends Secret {

  private final Secret secretImplementation;

  public NoOpGrantReadSecret(Secret secretImplementation) {
    this.secretImplementation = secretImplementation;
  }

  @Override
  public @NotNull Grant grantRead(@NotNull IGrantable grantee) {
    return Grant.drop(grantee, "ignored");
  }

  @Override
  public @NotNull String getArn() {
    return secretImplementation.getArn();
  }

  @Override
  public @Nullable Boolean getHasField() {
    return secretImplementation.getHasField();
  }
}

Call:

  final Map<String, Secret> secretVariables = new HashMap<>();
  var secret = Secret.fromSsmParameter(StringParameter.fromSecureStringParameterAttributes(
      this,
      "service-parameter-name",
      SecureStringParameterAttributes.builder()
          .parameterName(serviceSecrets.get("/path/to/ssm"))
          .version(1)
          .build()
  ));
    secretVariables.put("ENV_VAR_NAME",new NoOpGrantReadSecret(secret));

  var ssmReadOnlyPolicy = PolicyStatement.Builder.create()
      .actions(Arrays.asList("ssm:Describe*", "ssm:Get*", "ssm:List*"))
      .resources(Collections.singletonList("*"))
      .build();

  final TaskDefinition taskDefinition = TaskDefinition.Builder.create().build();
    taskDefinition.addToExecutionRolePolicy(ssmReadOnlyPolicy);

  ContainerDefinitionOptions containerDefinitionOptions = ContainerDefinitionOptions.builder()
      .secrets(secretVariables)
      .build();

@madeline-k
Copy link
Contributor

There was a project completed last year to reduce the size of IAM policies across the board. See the tracking issue for more detail #19276. I believe this should be resolved. Please create a new issue if you continue to experience this issue.

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container @aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

No branches or pull requests