fix(pipelines): artifact bucket permissions missing for in-account deployments

[rix0rrr]

A recent change to the default deploy-role policies required that the
required permissions statements are added to the artifacts bucket
instead.

This happened to be working for stacks which had NO account env property
set, but not for stacks that DID have an account property set which
was the same as the pipeline's account property.

Ultimately this was caused by an account comparison deep in the bowels of the
Grants system, which happened to work out because in cases of mismatched
accounts it would think there was a cross-account access and it was trying to
add permissions to both principal and resource. But in this case, it's much more
accurate to say that we can NEVER add permissions to the deploy-role, and
instead always want the permissions added to the bucket (rather than dropping
them).

Fixes #15307.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[skinny85]

@rix0rrr I wonder: shouldn't we simply always return statementAdded: false in addToPrincipalPolicy() for immutable Roles? What's the consequence of changing this behavior now?

[rix0rrr]

@rix0rrr I wonder: shouldn't we simply always return statementAdded: false in addToPrincipalPolicy() for immutable Roles? What's the consequence of changing this behavior now?

It would add the permissions to the resource always, which may or may not be intended. Bit dangerous to change.

[skinny85]

My opinion of this change is that I would switch to return statementAdded: false always for immutable Roles. I think this actually makes sense, because the statements are not added to an immutable Role. So I would consider the current behavior a bug.

I really don't like the addition of the addGrantsToResources property. I understand that it's the only way to do this in a fully, 100% backwards-compatible way, but I think it's a confusing and weird property, and makes the IAM library more complicated than it should be.

However, since you're the owner of both of these libraries, the ultimate decision is yours to make. I don't have any comments on the actual code (assuming you disagree with me that the current behavior of returning statementAdded: true for immutable Roles is a bug), so I'm approving the PR.

I trust you to make the decision you think is best 🙂.

[rix0rrr]

I think it still makes sense to give this control to users, but I agree with you that in an ideal world the defaults would have been reversed. I seem to remember there was a use case where we needed the current behavior, and so that kinda just stayed around...

Fortunately, by adding this flag we now have a clear mechanism of how to do that: a feature flag that controls the default of this property.

And I don't think we can change this default retroactively, especially not since it pertains to IAM.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 8322f2a
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository