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

StepFunction/Lambda PolicyDocument duplicate permissions (Maximum policy size exceeded) #1777

Closed
0xdevalias opened this issue Feb 16, 2019 · 11 comments · Fixed by #2254
Closed
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management @aws-cdk/aws-stepfunctions Related to AWS StepFunctions bug This issue is a bug.

Comments

@0xdevalias
Copy link
Contributor

0xdevalias commented Feb 16, 2019

I have a lambda function, that is called by a number of parallel tasks in an AWS step function. Code (with irrelevant bits snipped out) looks something like this:

const checkDomainsFunc = new lambda.Function(this, 'CheckDomainsFunction', {
..snip..
});

const newCheckDomainsTask = (sliceNum: number) => {
      const checkDomainsTask = new stepfunctions.Task(this, `CheckDomainsTask${sliceNum}`, {
        resource: checkDomainsFunc,
        inputPath: `$.slice.${sliceNum}`,
        resultPath: `$.result.${sliceNum}`,
      });
..snip..
      return checkDomainsTask;
    };

const checkDomainsParallelStep = new stepfunctions.Parallel(this, 'CheckDomainsParallelStep', {
      resultPath: '$.result'
    });

    for(let i = 0; i < Object.keys(sliceConfig).length; i++) {
      checkDomainsParallelStep.branch(newCheckDomainsTask(i));
    }

const definition = stepfunctions.Chain
        .start(configureSlicesStep)
        .next(checkDomainsParallelStep);

new stepfunctions.StateMachine(this, 'FooStateMachine', {
        definition,
        timeoutSec: 60*60
});

I end up getting an error like the following:

 2/4 | 21:47:03 | UPDATE_FAILED        | AWS::IAM::Policy                 | FooStateMachine/Role/DefaultPolicy (FooStateMachineRoleDefaultPolicy3ED6D243) Maximum policy size of 10240 bytes exceeded for role FooStack-FooStateMachineRole725DD6EF-752AIIF1U5GZ (Service: AmazonIdentityManagement; Status Code: 409; Error Code: LimitExceeded; Request ID: f5369744-31e8-11e9-88af-795178e442ff)

Looking at cdk diff, it seems that the same permission for the statemachine to execute the lambda function is repeated for each of the parallel tasks:

[~] AWS::IAM::Policy FooStateMachine/Role/DefaultPolicy FooStateMachineRoleDefaultPolicy3ED6D243
 └─ [~] PolicyDocument
     └─ [~] .Statement:
         └─ @@ -198,5 +198,805 @@
            [ ]         "Arn"
            [ ]       ]
            [ ]     }
            [+]   },
            [+]   {
            [+]     "Action": "lambda:InvokeFunction",
            [+]     "Effect": "Allow",
            [+]     "Resource": {
            [+]       "Fn::GetAtt": [
            [+]         "CheckDomainsFunction9CC80B3F",
            [+]         "Arn"
            [+]       ]
            [+]     }
            [+]   },
            [+]   {
            [+]     "Action": "lambda:InvokeFunction",
            [+]     "Effect": "Allow",
            [+]     "Resource": {
            [+]       "Fn::GetAtt": [
            [+]         "CheckDomainsFunction9CC80B3F",
            [+]         "Arn"
            [+]       ]
            [+]     }
            [+]   },
            [+]   {
            [+]     "Action": "lambda:InvokeFunction",
            [+]     "Effect": "Allow",
            [+]     "Resource": {
            [+]       "Fn::GetAtt": [
            [+]         "CheckDomainsFunction9CC80B3F",
            [+]         "Arn"
            [+]       ]
            [+]     }
            [+]   },
..snip..

I would have expected the policy to be treated like a set (eg. this permission would only be added once when it's exactly the same).

While it would be nice to have this solved 'properly' at some point, some form of workaround in the meantime would also be awesome. I assume I'll be able to override/replace the policy document somehow, but haven't quite figured that out yet.

@rix0rrr rix0rrr added bug This issue is a bug. @aws-cdk/aws-iam Related to AWS Identity and Access Management @aws-cdk/aws-stepfunctions Related to AWS StepFunctions labels Feb 18, 2019
@0xdevalias
Copy link
Contributor Author

0xdevalias commented Feb 19, 2019

What is the correct way to override deeper elements now? I know I can access the Policy like this:

const stateMachinePolicy = stateMachine.role.node.findChild('DefaultPolicy') as iam.Policy;

But not sure how I can override the state machine's role/policy now. It used to be possibly with something such as addOverride or propertyOverrides if I remember correctly.

Also, where is the canonical location for 'how to' do an override/access a nested element now? It used to be available from the main docs page, but I can't seem to see it there anymore..?

It looks like it's possible on a Resource at least according to these tests.. but no idea how to make that work for the state machine?

Edit: Figured out how to do the overrides on the resource:

const stateMachineResource = stateMachine.role.node.findChild('Resource') as stepfunctions.CfnStateMachine;

stateMachineResource.addOverride(...)
stateMachineResource.addPropertyOverride(...)

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 19, 2019

You want to be getting the StateMachine/Role/DefaultPolicy/Resource object (which is of type iam.CfnPolicy. You can then override the PolicyDocument property according to this schema:

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-policy.html

@0xdevalias
Copy link
Contributor Author

0xdevalias commented Feb 19, 2019

Thanks @rix0rrr.

Based on that I so far have this, though don't seem to quite have it working right just yet:

const stateMachinePolicyDocumentOverride = new iam.PolicyDocument().addStatement(
      new iam.PolicyStatement()
        .allow()
        .addAction('lambda:InvokeFunction')
        .addResource(fooFunc.functionArn)
        .addResource(barFunc.functionArn)
    );

    const stateMachinePolicyResource = stateMachine.node.findChild('Role/DefaultPolicy/Resource') as iam.CfnPolicy;
    stateMachinePolicyResource.addPropertyOverride('PolicyDocument', stateMachinePolicyDocumentOverride)

Edit:
If I play around with the following, I can see that I remove the 'bad' document and add my new document properly (under a differently named key: PolicyDocumentA)

stateMachinePolicyResource.addPropertyDeletionOverride('PolicyDocument');
stateMachinePolicyResource.addPropertyOverride('PolicyDocumentA', stateMachinePolicyDocumentOverride);
SecmapsStateMachineRoleDefaultPolicy3ED6D243:
  Type: AWS::IAM::Policy
  Properties:
..snip..
    PolicyDocumentA:
      statements:
        - Action: lambda:InvokeFunction
          Effect: Allow
          Resource:
            - Fn::GetAtt:
                - FooFuncD8B69655
                - Arn
            - Fn::GetAtt:
                - BarFunc9CC80B3F
                - Arn

But if I just add the property override (as below), or try to delete it then add it, it seems to just keep the old PolicyDocument (with all of it's many many repeated entries)

stateMachinePolicyResource.addPropertyOverride('PolicyDocument', stateMachinePolicyDocumentOverride);

Any thoughts on how to make it actually apply?

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 19, 2019

Huh. Honestly, no.

A raw override (addOverride('Properties.PolicyDocument', ...)) might work better?

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 19, 2019

But I think the problem might be that it merges the documents :x

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 19, 2019

A hacky workaround solution for you right now is to supply your own class that implements IRole which does nothing in its addToRolePolicy() function.

@0xdevalias
Copy link
Contributor Author

0xdevalias commented Feb 20, 2019

Will have a bit of a play around. It looks like using addOverride still has the same issue, so your merge theory is probably on the right track.

While I get that the find child/override stuff is designed to allow us to break out, while still 'protecting' the inner workings, I wonder if it would be nicer to have an easier ability to substitute aspects of a higher level component. eg.

statemachine.HereThereBeDragons.role = new iam.Role(...)

All of this override stuff (to my knowledge) is applied to the yaml/etc at synthesis. Is there currently a method to say "Give me a state machine, but use this custom Role class i'm providing"? Because that would be super handy in situations like this.

Edit: Reading a bit deeper, it seems that the StateMachine actually accepts a role prop, which is exactly what I need.

@0xdevalias
Copy link
Contributor Author

Ok, here's my workaround. I decided to run with the Set theory:

class RoleWithUniquePolicyStatements extends iam.Role {
  constructor(scope: cdk.Construct, id: string, props: iam.RoleProps, suppressWarnings: boolean = false) {
    super(scope, id, props);

    this.suppressWarnings = suppressWarnings;
    this.uniqueStatements = new Set<string>()
  }

  suppressWarnings: boolean;
  uniqueStatements: Set<string>;

  addToPolicy(statement: iam.PolicyStatement): void {
    const statementJson = JSON.stringify(statement.toJson(), null, 0);

    if (this.uniqueStatements.has(statementJson)) {
      if (!this.suppressWarnings) {
        console.warn(`Prevented attempt to add duplicate PolicyStatement:\n${statementJson}\n`)
      }
    } else {
      this.uniqueStatements = this.uniqueStatements.add(statementJson);
      super.addToPolicy(statement);
    }
  }

  // sneakyAddToPolicy(statement: iam.PolicyStatement): void {
  //   super.addToPolicy(statement)
  // }
}

Usage:

new stepfunctions.StateMachine(this, 'FooStateMachine', {
    definition,
    role: new RoleWithUniquePolicyStatements(this, 'FooStateMachineRole', {
      assumedBy: new iam.ServicePrincipal(`states.${this.region}.amazonaws.com`),
    })
});

@eladb
Copy link
Contributor

eladb commented Apr 2, 2019

@rix0rrr what do you think we should do with this?

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 2, 2019

I think we should deduplicate policies upon rendering (in effect, render them as the set they are).

@skinny85
Copy link
Contributor

skinny85 commented Apr 2, 2019

I think we should deduplicate policies upon rendering (in effect, render them as the set they are).

+1. This is an issue that will crop up again and again in different contexts (in fact, it already has, with permissions in CodePipeline for CloudFormation deployments). If we want to solve it once and for all, I believe it needs to be changed at the source (i.e., the IAM library).

jogold added a commit to jogold/aws-cdk that referenced this issue Apr 11, 2019
jogold added a commit to jogold/aws-cdk that referenced this issue Apr 11, 2019
rix0rrr pushed a commit that referenced this issue Apr 16, 2019
Remove duplicate statements from policy documents

Fixes #1777
Fixes #2168
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management @aws-cdk/aws-stepfunctions Related to AWS StepFunctions bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants