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

policyName longer than 128 characters for auto generated AWS::IAM::Policy for CodeBuild project #3402

Closed
4 tasks
juanrh opened this issue Jul 23, 2019 · 6 comments · Fixed by #3487
Closed
4 tasks
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug.

Comments

@juanrh
Copy link

juanrh commented Jul 23, 2019

Note: for support questions, please first reference our documentation, then use Stackoverflow. This repository's issues are intended for feature requests and bug reports.

  • I'm submitting a ...

    • [x ] 🪲 bug report
    • 🚀 feature request
    • 📚 construct library gap
    • ☎️ security issue or vulnerability => Please see policy
    • ❓ support request => Please see note at the top of this template.
  • What is the current behavior?
    If the current behavior is a 🪲bug🪲: Please provide the steps to reproduce
    new codebuild.Project
    I'm using CDK 1.0.0 to setup several code build projects. We have to run the same tests in different environments, so I have done something similar to travis build matrix, by having a TestMatrix construct that contains several instantiations of a TestSuite construct that creates a CodeBuild project with the required IAM permissions. As a result I get long cdk paths

The problem is that when I deploy the stack to my dev account the deployment fails because tje name of the IAM policies associated to each CodeBuild project is too long

1 validation error detected: Value '***********************' at 'policyName' failed to satisfy constraint: Member must have length less than or equal to 128 (Service: AmazonIdentityManagement; Status Code: 400; Error Code: ValidationError; Request ID: ad619787-ad6f-11e9-bfef-a3a40d689eaf)

However I haven't defined that policy explicitly, I just called codeBuildProject.addToRolePolicy to add additional PolicyStatement objects, and CDK added permissions automatically for accessing a CodeCommit repository that is used as source.

  • What is the expected behavior (or behavior of feature suggested)?
    There is no validation error when deploying the Cfn stack

  • What is the motivation / use case for changing the behavior or adding this feature?
    The policy name is defined automatically by CDK

  • Please tell us about your environment:

    • CDK CLI Version: 1.0.0
    • Module Version: 1.0.0
    • OS: Ubuntu
    • Language: TypeScript
  • Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, eg. associated pull-request, stackoverflow, gitter, etc)

@juanrh juanrh added the needs-triage This issue or PR still needs to be triaged. label Jul 23, 2019
@juanrh
Copy link
Author

juanrh commented Jul 23, 2019

This workaround seems to work. Please let confirm whether this is a reasonable solution or not

if (codeBuildProject.role) {
	const policy = codeBuildProject.role["defaultPolicy"].node.defaultChild as CfnPolicy ;
	// Override the policy name using Escape Hatches
	// https://docs.aws.amazon.com/cdk/latest/guide/cfn_layer.html
	policy.addOverride("Properties.PolicyName", `${projectName}-policy`);
}	

Thanks

@skinny85
Copy link
Contributor

skinny85 commented Jul 30, 2019

Hey @juanrh ,

sorry for the late response. If your workaround works fine, then I don't see a problem with it.

I'll work on a fix for this issue.

Thanks,
Adam

@skinny85 skinny85 added bug This issue is a bug. @aws-cdk/aws-iam Related to AWS Identity and Access Management and removed needs-triage This issue or PR still needs to be triaged. labels Jul 30, 2019
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Jul 30, 2019
Our logic for trimming the length of the default IAM policy name was not working,
as it wasn't updated when logicalId became a Token rather than a literate string,
and so it was never actually triggered
(it just checked that the display name of the Token was less than 128 characters,
which it always is).
The fix is to resolve the logical ID Token before applying the trimming logic.

Fixes aws#3402
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Jul 30, 2019
Our logic for trimming the length of the default IAM policy name was not working,
as it wasn't updated when logicalId became a Token rather than a literate string,
and so it was never actually triggered
(it just checked that the display name of the Token was less than 128 characters,
which it always is).
The fix is to resolve the logical ID Token before applying the trimming logic.

Fixes aws#3402
@juanrh
Copy link
Author

juanrh commented Jul 30, 2019

I'm having the same problem when defining a cw events rule for a CodeBuild project, as part of the same construct

new events.Rule(this, `trigger-${projectName}`, {
			schedule: events.Schedule.cron({
				// "All scheduled events use UTC time zone"
				// https://docs.aws.amazon.com/AmazonCloudWatch/latest/events/ScheduledEvents.html
				hour: "7" // daily at 07:00 UTC ~ 00:00 PDT
			}),
			targets: [
				new events_targets.CodeBuildProject(project)
			]

I'm having more problems to find a workaround here, any suggestion would be appreciated

@juanrh
Copy link
Author

juanrh commented Jul 31, 2019

Is there a simple way to traverse all resources in a cdk.Stack, so I can detect those corresponding to a IAM policy, and modify their name if needed?

@eladb
Copy link
Contributor

eladb commented Jul 31, 2019

stack.node.findAll

@juanrh
Copy link
Author

juanrh commented Jul 31, 2019

I'm using this as a workaround

this.node.findAll().forEach(child => {
    // HACK for https://github.com/aws/aws-cdk/issues/3402
    // Cannot check the actual policy name, because the token hasn't been expanded yet
    const childClass = child.constructor.name;
    if (childClass == 'CfnPolicy') {
        const policy = child as CfnPolicy;
        const maxPolicyNameLength = 128;					
        const newPolicyName = `policy-${uuid4()}${uuid4()}${uuid4()}${uuid4()}`.slice(0, maxPolicyNameLength);
        console.warn(`Renaming policy ${newPolicyName} to ensure the policy name passes validation`)
        policy.addOverride("Properties.PolicyName", newPolicyName);
    }
});

running from the stack constructor. I understand this means CDK will always generate new policy names, so each time I deploy a change for any resource, all the IAM policies will be regenerated too. That should be ok because there is no state associated to policies AFAIK, it will just make deployments slower.

Does this workaround sound good, or do you think there can be any problems with this approach?

Thanks

skinny85 added a commit to skinny85/aws-cdk that referenced this issue Jul 31, 2019
Our logic for trimming the length of the default IAM policy name was not working,
as it wasn't updated when logicalId became a Token rather than a literate string,
and so it was never actually triggered
(it just checked that the display name of the Token was less than 128 characters,
which it always is).
The fix is to resolve the logical ID Token before applying the trimming logic.

Fixes aws#3402
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Aug 1, 2019
Our logic for trimming the length of the default IAM policy name was not working,
as it wasn't updated when logicalId became a Token rather than a literate string,
and so it was never actually triggered
(it just checked that the display name of the Token was less than 128 characters,
which it always is).
The fix is to resolve the logical ID Token before applying the trimming logic.

Fixes aws#3402
skinny85 added a commit that referenced this issue Aug 2, 2019
…3487)

Our logic for trimming the length of the default IAM policy name was not working,
as it wasn't updated when logicalId became a Token rather than a literate string,
and so it was never actually triggered
(it just checked that the display name of the Token was less than 128 characters,
which it always is).
The fix is to resolve the logical ID Token before applying the trimming logic.

Fixes #3402
eladb pushed a commit that referenced this issue Aug 6, 2019
…3487)

Our logic for trimming the length of the default IAM policy name was not working,
as it wasn't updated when logicalId became a Token rather than a literate string,
and so it was never actually triggered
(it just checked that the display name of the Token was less than 128 characters,
which it always is).
The fix is to resolve the logical ID Token before applying the trimming logic.

Fixes #3402
mergify bot pushed a commit that referenced this issue Aug 7, 2019
* chore: update package-lock.json

* feat(eks): define kubernetes resources

This change allows defining arbitrary Kubernetes resources within an EKS cluster.

* nice!

* update readme

* Update README.md

* feat(events): ability to add cross-account targets (#3323)

This adds the capability of adding a target to an event rule
that belongs to a different account than the rule itself.
Required for things like cross-account CodePipelines with source actions triggered by events.

* chore(ci): add mergify config file (#3502)

* chore: update jsii to 0.14.3 (#3513)

* fix(iam): correctly limit the default PolicyName to 128 characters (#3487)

Our logic for trimming the length of the default IAM policy name was not working,
as it wasn't updated when logicalId became a Token rather than a literate string,
and so it was never actually triggered
(it just checked that the display name of the Token was less than 128 characters,
which it always is).
The fix is to resolve the logical ID Token before applying the trimming logic.

Fixes #3402

* v1.3.0 (#3516)

See CHANGELOG

* fix: typo in restapi.ts (#3530)

* feat(ecs): container dependencies (#3032)

Add new addContainerDependencies method to allow for container dependencies
Fixes #2490

* feat(s3-deployment): CloudFront invalidation (#3213)

see #3106

* docs(core): findChild gets direct child only (#3512)

* doc(iam): update references to addManagedPolicy (#3511)

* fix(sqs): do not emit grants to the AWS-managed encryption key (#3169)

Grants on the `alias/aws/sqs` KMS key alias are not necessary since the
key will implicitly allow for it's intended usage to be fulfilled (as
opposed to how you have to manage grants yourself when using a
user-managed key instead).

This removes the statement that was generated using an invalid resource
entry.

Fixes #2794

* fix(lambda): allow ArnPrincipal in grantInvoke (#3501)

Fixes #3264 

I'm trying to allow a lambda function in another account to be able to invoke my CDK generated lambda function. This works through the CLI like so:

    aws lambda add-permission --function-name=myFunction --statement-id=ABoldStatement --action=lambda:InvokeFunction --principal=arn:aws:iam::{account_id}:role/a_lambda_execution_role

But CDK doesn't seem to allow me to add an ArnPrincipal doing something like this:

    myFunction.grantInvoke(new iam.ArnPrincipal(props.myARN))

With the error:

    Invalid principal type for Lambda permission statement: ArnPrincipal. Supported: AccountPrincipal, ServicePrincipal

This PR allows ArnPrincipal to be passed to lambda.grantInvoke.

There might be some additional validation required on the exact ARN as I believe only some ARNs are supported by lambda add-permission

* chore(contrib): remove API stabilization disclaimer

* fix(ssm): add GetParameters action to grantRead() (#3546)

* misc

* rename `KubernetesManifest` to `KubernetesResource` and `addResource`
* move AWS Auth APIs to `cluster.awsAuth` and expose `AwsAuth`
* remove the yaml library (we can just use a JSON stream)
* add support for adding accounts to aws-auth
* fix cluster deletion bug
* move kubctl app info to constants

* addManifest => addResource

* update test expectations

* add unit test for customresrouce.ref

* fix sample link
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 bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants