-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(iam): immutable IAM roles #4501
Conversation
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -901,8 +901,7 @@ export class Project extends ProjectBase { | |||
}, | |||
})); | |||
|
|||
const policy = new iam.Policy(this, 'PolicyDocument', { | |||
policyName: 'CodeBuildEC2Policy', | |||
const policy = this.role.createAndAttachPolicy('CodeBuildVpcPolicy', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that this can return nothing is a bit of a gotcha and feels like an unnatural API.
Can it not return a special kind of Policy
object that doesn't render to CFN, or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the change should be that unattached Policy
objects just don't render? This new method feels unnatural.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Why is returning something optionally "unnatural"...?
Returning a magical Policy
that won't render if it's unattached seems a lot more unnatural to me (we don't have a single resource in the library that behaves that way - doesn't render under some conditions), and also won't work in this case - we are making the Project
depend on the Policy
, so we can't not render it (the resulting template would be invalid).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Why is returning something optionally "unnatural"...?
How is it to be expected that a function that is called createThing
doesn't actually create and return a thing? What I don't like about this is:
- Consumers have to write additional ifs. I will fight against any design that requires my consumers to add an
if
to their code. - We could only conceivably have typed this function this way once we have the requirement that there may be a class which doesn't do the thing. If we had made this function BEFORE we had the requirement of the no-op class, the API would have been locked in to the normal API (which always returns a thing) and then we wouldn't have been able to add this in this way. That leads me to think we are confusing layers of functionality.
we don't have a single resource in the library that behaves that way - doesn't render under some conditions
We do have objects that render differently under certain conditions (references turn into { Ref: xxx }
or { Fn::ImportValue: Yyy }
under certain conditions) so that is not without precedent. And in fact, we also have policy objects that automatically get added to the document if they're non-empty (see BucketPolicy
, QueuePolicy
, TopicPolicy
).
Automatic adding on non-empty is roughly what I'm asking for here as well. The difference being that in the existing case, bucket.addToResourcePolicy()
adds the AWS::IAM::Policy
resource to the template, and in this case it would be policy.addToPolicy()
that adds the AWS::IAM::Policy
to the template.
Yes, the letter of the code will be different (because the class structure is slightly different), but I would argue it's in the same spirit.
And in any case, CDK is intended to make it easier to work with CloudFormation templates, not push the same problems and need for case analysis down to the user if we can deal with them.
we are making the
Project
depend on thePolicy
, so we can't not render it
Obviously we would have to not render the dependency in the case we're not rendering the resource. Yes, I understand it is more work, but it's not like that is impossible to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consumers have to write additional ifs. I will fight against any design that requires my consumers to add an if to their code.
That's a noble point, only slightly undercut by the fact that we're adding a method to an interface with an existing method addToPolicy(statement: PolicyStatement): boolean
...
Obviously we would have to not render the dependency in the case we're not rendering the resource. Yes, I understand it is more work, but it's not like that is impossible to do.
How? CfnResource.addDependsOn(resource)
adds resource
to a dependsOn
Set field of CfnResource
. How would you not render that?
And other cases - what if somebody does CfnOutput(this, 'Output', { value: policy.ref })
? How could we possibly not renderpolicy
in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So probably this is a bad idea for CfnPolicy
, but it could be true for Policy
.
As for the second question, we could render it as { Ref: AWS::NoValue }
. Not saying it's the best answer but it is an answer. Another thing we could do is reify the policy when someone refers to it. There is precedent for something like this btw, see iam.LazyRole
.
As for addDependsOn()
... that is an issue. We would really have to use addDependency
, which has a mechanism (in IDependable
) to control what gets depended on when you take a dependency on a thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just spent basically the entire day implementing LazyPolicy
(which I believe is basically what you asked). And it was horrible. Feel free to judge yourself (I've updated the PR with what I came up with).
We turned a clean, simple API into a Frankenstein monster full of hacks, throwing exceptions, and weird edge cases. I don't even know how to write the docs for this new method, it's so convoluted what it does now. And I'm still not able to actually make it work - the build fails on the codebuild
package.
I really urge you to reconsider. I think we're going down the wrong path here.
} | ||
|
||
public addToPolicy(_statement: PolicyStatement): boolean { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might need to return true
. I'm sorry :(.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this comment. Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/**
* Add to the policy of this principal.
*
* @returns true if the statement was added, false if the principal in
* question does not have a policy document to add the statement to.
*/
addToPolicy(statement: PolicyStatement): boolean;
I know that the statement wasn't really added, but false
means that a policy statement could NEVER be added and one should be added to the resource instead.
In this case I guess it's a toss-up between "the user has already prepared all statements so don't do anything" and "this role is immutable so add to the resource instead."
I would say we either make it configurable between those 2, or default to the do-less option (with an option to make it configurable later). Right now, the do-less default is returning "true" (pretend everything worked fine attaching to the principal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be true
here as well then...? (for roles imported with { mutable: false }
)
aws-cdk/packages/@aws-cdk/aws-iam/lib/role.ts
Lines 220 to 228 in ddb05f9
class ImmutableImport extends Import { | |
public addToPolicy(_statement: PolicyStatement): boolean { | |
return false; | |
} | |
public attachInlinePolicy(_policy: Policy): void { | |
// do nothing | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the PR description to include the motivation for the change
// otherwise, creating the Project fails, | ||
// as it requires these permissions to be already attached to the Project's Role | ||
const cfnPolicy = policy.node.defaultChild as CfnResource; | ||
project.addDependsOn(cfnPolicy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it work to add the dependency on the policy
object? I believe it should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
project.addDependsOn(policy)
doesn't compile (Policy
is not aCfnResource
)this.node.addDependency(policy)
adds way too much dependencies, including a circular one betweenpolicy
and itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this.node.addDependency(cfnPolicy)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
this.node.addDependency(cfnPolicy)
?
Nope. Same as this.node.addDependency(policy)
- a million dependencies, including a cyclical one.
* It will be created in the scope of this principal (if it's mutable) | ||
* @param props the construction properties of the new policy | ||
*/ | ||
createAndAttachPolicy(id: string, props?: PolicyProps): Policy | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name is weird... "xAndY" is usually a sign of a bad name. Maybe just addPolicy
? Semantically it means the same thing: role.addPolicy('foo', { xxx })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also thinking createAttachedPolicy
might be a better name. What do you think?
For now, changed to addPolicy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also worried that addPolicy
looks too similar to addToPolicy
(which is a method present on the superinterface of IIdentity
, IPrincipal
).
a47c6e2
to
cfd8a27
Compare
Changed the name of the new method to |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
cfd8a27
to
f40edeb
Compare
This time, actually changed it. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
This adds an IRole implementation that ignores all mutating operations. To accommodate this new behavior, add a new method to IIdentity: createAndAttachPolicy, which is meant to replace attachInlinePolicy, which can leave Policy resources unattached, which is illegal in CloudFormation. Fixes aws#2985 Fixes aws#4465
f40edeb
to
021bc24
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
* This is the same as calling `policy.addToXxx(principal)`. | ||
* @param policy The policy resource to attach to this principal [disable-awslint:ref-via-interface] | ||
* Creates and attaches a Policy to this principal, if it is mutable. | ||
* If this principal is immutable, does nothing, and returns undefined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This docstring can't be correct anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'm actually struggling to write these docs, like I said here: #4501 (comment)
import { IRole } from './role'; | ||
import { IUser } from './user'; | ||
|
||
export class LazyPolicy extends Resource implements IPolicy { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need a docstring explaining what it does.
Does it work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not currently, because of the magical overrides of ref
and logicalId
of the custom L1 here: https://github.com/aws/aws-cdk/pull/4501/files?file-filters%5B%5D=.ts#diff-f60a11b2b65df07bd241981fe0a626d7R37-R55
Hey @rix0rrr, like I said before, I believe we're heading down the wrong path here. Even putting aside the fact that I can't get the code working with the hacky overrides needed for the L1 in Is there any way I can convince you to back out from this direction? I strongly feel this is the wrong thing to do. |
I implemented If this approach does turn out to work, I'm still not sure this shouldn't be the default behavior of |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
The DynamoDB global table construct was incorrectly declaring its dependencies. That resulted in resources in the coordinator stack depending on the global tables, which cannot work, as those are from different stacks. Changed the logic to declare dependencies between the stacks explicitly. Fixes aws#4501
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few things:
- I don't believe this does what we need in CodeBuild's
Project
class. - If we're going down the
LazyPolicy
route, maybe addingaddToPolicy
doesn't make much sense, and we should instead have clients ofattachInlinePolicy
create aLazyPolicy
, and pass it there?
} | ||
} | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a test that shows the dependency is there if the policy was attached to anything?
// otherwise, creating the Project fails, | ||
// as it requires these permissions to be already attached to the Project's Role | ||
const cfnPolicy = policy.node.findChild('Resource') as CfnResource; | ||
const cfnPolicy = policy.node.defaultChild as CfnResource; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work in the current version? From what I can tell, LazyPolicy
extends Policy
, and so this will leave a dependency in the template to a resource that will be later removed, if I'm correct.
Please stop overwriting previous commits with I'm trying to find old comments back to see what your problems with |
I can't retrieve why
Not exactly sure what you mean here, but if it's up to me we roll |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
I doubt it has to do anything with
The issue was I was doing |
I'm saying that:
If you want to roll the capabilities of Is this clearer now? |
Yep, although I would have I am fine with removing Who is going to finish this PR, you or me? I have a feeling our roles got reversed over the last few commits :)
Yes but I've also seen it show old comments as "out-of-date". My current theory is that because the commit the comments are linked to is gone, it's not showing them anymore. |
I think that's fine. It's not strictly backwards-compatible, but I think the old behavior was purely an irritation, and didn't add much value.
Go for it. I think that's fine that we switched roles, that's what collaboration is all about :). I think you can also roll back all of the changes I did to |
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
5 similar comments
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
Subsumed by new PR. |
Add a new method to `Role` called `withoutPolicyUpdates()`, which returns a wrapper object that will drop all policy updates. This can be used in situations where you want to skip the default IAM permission adding behavior of CDK. Needs only be used with roles that are created in the same CDK application; for imported roles, supplying `mutable=false` when calling `Role.fromRoleArn()` is sufficient. Fixes #2985. Fixes #4465. Closes #4501.
This adds an
IRole
implementation that ignores all mutating operations.Useful when you want to turn off CDK's automatic permissions management,
and have full control over a role's policy
(and/or if the CDK generated policy exceeds the size limit).
To accommodate this new behavior, add a new method to
IIdentity
:addPolicy
, which is meant to replaceattachInlinePolicy
,which can leave Policy resources unattached, resulting in an invalid CloudFormation template.
Fixes #2985
Fixes #4465
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license