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

IAM Grants still not quite right #2807

Closed
rix0rrr opened this issue Jun 10, 2019 · 1 comment · Fixed by #8280
Closed

IAM Grants still not quite right #2807

rix0rrr opened this issue Jun 10, 2019 · 1 comment · Fixed by #8280
Assignees
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1

Comments

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 10, 2019

The IAM policy statement support is still not quite right. Right now, there are 3 grant methods:

  • Grant.addToPrincipal().
  • Grant.addToPrincipalOrResource() -- note: OR
  • Grant.addToPrincipalAndResource()-- note: AND

And some of the mechanics are made possible by IPrincipal.addToPolicy(): boolean, which is supposed to return true or false based on whether or not it added the policy statement. The thinking here was that we can represent principals like myResource.grantWrite(new ServicePrincipal('events.amazonaws.com')), and because its addToPolicy() would return false, the statement would be added to myResources resource policy.

Behavior is supposedly:

Identity-based policies and resource-based policies are both permissions policies and are evaluated together. [...] If at least one policy statement allows the action in the request, the request is allowed. It doesn't matter whether the Allow is in the identity-based policy or the resource-based policy.

Important

This logic applies only when the request is made within a single AWS account. For requests made from one account to another, the requester in Account A must have an identity-based policy that allows them to make a request to the resource in Account B.

Source: https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies_identity-vs-resource.html

Grant.addToPrincipalOrResource() tries to do the right thing, but mostly does so by accident. If it can't add the statement to the principal, it will add it to the resource.

  • Works for created IAM roles: statement gets added to the role.
  • Works for service principals: statement gets added to the resource (adding a cross-account trust), and we'll assume that the service principal already has the appropriate permissions on their end, which is probably true for supported AWS service interactions.
  • Works for imported IAM role in same account (before application of fix(iam): support adding permissions to imported roles #2805, and modulo some bugs), statement gets added to resource which is good enough for "same account" access
  • Does not work for cross-account created IAM roles (inside the same CDK app): the statement will be added to the role policy, but should also be added to the resource policy to support cross-account access.

The latter case is probably not exercised a lot, and if due to a user's extensive experience with manual IAM configuration they weren't expecting it to work just as magically as the rest of the CDK, they would just add the resource permissions by hand and be none the wiser that we have a bug here. But I would argue that we have a bug nonetheless.

The fact is, the trigger for automatically adding to the resource policy is wrong. The current trigger is: "did we failed to add permissions to the identity?", but the trigger should be: "does the principal live in another account OR did we fail to add permissions?".

Then there is Grant.addToPrincipalAndResource() which is yet another crazy beast, and mostly designed to fit the use case of KMS. By default, most (all?) resources exhibit the "inside the same account a statement on either resource or identity is enough" rule, but KMS keys do not. They seem to require explicit statements on the key for every action.

@rix0rrr rix0rrr added the gap label Jun 10, 2019
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jun 10, 2019

@skinny85 something to be aware of when implementing cross-account CodePipeline

@rix0rrr rix0rrr added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Jun 10, 2019
@NGL321 NGL321 added the needs-triage This issue or PR still needs to be triaged. label Jun 17, 2019
@rix0rrr rix0rrr removed the needs-triage This issue or PR still needs to be triaged. label Aug 28, 2019
@SomayaB SomayaB added feature-request A feature should be added or improved. and removed gap labels Nov 11, 2019
@rix0rrr rix0rrr added the effort/medium Medium work item – several days of effort label Jan 23, 2020
@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Jun 1, 2020
@rix0rrr rix0rrr added the p1 label Aug 12, 2020
@mergify mergify bot closed this as completed in #8280 Aug 19, 2020
mergify bot pushed a commit that referenced this issue Aug 19, 2020
Add the `account` and `region` properties to the `IResource` interface and `Resource` class.
By default, these are equal to the account and region of the Stack the resource belongs to;
however, they can be set to different values in resources that are imported.

Use those new properties in two places:
* In CodePipeline, to determine whether a given action is cross-account
  (with support for specifying the account and region in S3's `BucketAttributes`,
  as a first use case).
* IAM's `addToPrincipalOrResource()`, to correctly know when to modify the receiver's resource policy.
  This is aided by adding an optional `principalAccount` property to `IPrincipal`,
as a way to compare to the account present in the passed `IResource` instance.

Fixes #2807
Fixes #5740
Fixes #7012

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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 effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants