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 only work on IAM objects, not arbitrary principals #236

Closed
rix0rrr opened this issue Jul 6, 2018 · 6 comments · Fixed by #1623
Closed

iam: Grants only work on IAM objects, not arbitrary principals #236

rix0rrr opened this issue Jul 6, 2018 · 6 comments · Fixed by #1623
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version.

Comments

@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 6, 2018

# Works
bucket.grantReadWrite(lambda.role)

# Does not work
bucket.grantReadWrite(new ArnPrincipal('arn:aws:...'));

From the point of view of the user, it makes sense to be able to add a grant to a principal, because that's what you do in IAM.

Right now, this grantReadWrite() uses

  • identity.addToPolicy()
  • identity.principal

The addToPolicy() call should be a no-op. Better yet, it should probably return true|false on whether it did something, and if it returns false the permissions should be added on the resource instead of on the identity. This makes most use cases just work.

While we're at it:

# Should actually also work
bucket.grantReadWrite(lambda)
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jul 6, 2018

This came up in #49

@rix0rrr rix0rrr changed the title [iam] It should be possible to grant to a Principal iam: Grants only work on IAM objects, not arbitrary principals Sep 13, 2018
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Sep 13, 2018

Came up again while wanting to write the following code:

bucket.grantPut(new AccountPrincipal('12345'));

Doesn't work, because AccountPrincipal is not an IAM object.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 3, 2018

And it should do something different for cross-account permissions (add to both resource and identity).

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Oct 16, 2018

Came up again when @leepa was working on a workshop:

        bucket.grantRead(new ArnPrincipal("arn:aws:iam::cloudfront:user/CloudFront Origin Access Identity " + webDistribution.getId()));

@debora-ito debora-ito added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Nov 8, 2018
@fulghum fulghum added the pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version. label Jan 10, 2019
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jan 17, 2019

Another one that came up on Gitter:

In the ECR Repository construct, grantPull expects an IPrincipal. But how can I grant everyone in the org pull access? In classic cfn it's a Condition inside the policy.

Again a principal that is not an IAM identity, but a virtual thing which means the resource policy needs updatin'.

@Tehnix
Copy link
Contributor

Tehnix commented Jan 25, 2019

Is there any workarounds w.r.t. to this? Like adding it through the SDK/CLI in a post-hook? :/

rix0rrr pushed a commit that referenced this issue Jan 28, 2019
Add support for non-identity Principals in grants (for example,
principals that represent accounts or organization IDs). For resources
that support them, the required IAM statements will be added to the
resource policy. For resources that don't support them (because they
don't have resource policies) an error will be thrown.

Add a new `OrganizationPrincipal` principal which represents all
identities in the given AWS Organization.

Fixes #236.
rix0rrr added a commit that referenced this issue Apr 3, 2019
Add support for non-identity Principals in grants (for example,
principals that represent accounts or organization IDs). For resources
that support them, the required IAM statements will be added to the
resource policy. For resources that don't support them (because they
don't have resource policies) an error will be thrown.

Add a new `OrganizationPrincipal` principal which represents all
identities in the given AWS Organization.

Grant methods no longer accept an optional principal. Instead, they
accept an `IGrantable`, which encodes constructs that have a principal
to grant to. This principal must be always present, but may be a principal
that can't do any work other than emitting warnings for imported resources.

For construct authors, all grant methods must now return an `iam.Grant` object,
and all should be implemented by calling one of the static factory methods on
`iam.Grant` and returning its result.

Fixes #236.

BREAKING CHANGE: `grant(function.role)` and `grant(project.role)` are now
`grant(function)` and `grant(role)`.
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 pr/breaking-change This PR is a breaking change. It needs to be modified to be allowed in the current major version.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants