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

S3 Bucket grant* stopped working in 1.20.0 #5740

Closed
jeshan opened this issue Jan 10, 2020 · 7 comments · Fixed by #8280
Closed

S3 Bucket grant* stopped working in 1.20.0 #5740

jeshan opened this issue Jan 10, 2020 · 7 comments · Fixed by #8280
Assignees
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management @aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. effort/medium Medium work item – several days of effort in-progress This issue is being actively worked on. p1

Comments

@jeshan
Copy link

jeshan commented Jan 10, 2020

I've upgraded cdk (and aligned their modules as well) from 1.16.3(?) to 1.20.0 and I noticed that bucket policies that I created with the following stopped working

myBucket.grantReadWrite(myRole, `*/${templatesPublic}/*`);

Reproduction Steps

What I saw with cdk diff 1.20.0:
image
image

If I renamed the bucket, I'd get:
image
So, it suggests that it's broken for new and existing buckets as well.

It's not just the diff that was wrong; when I did cdk deploy, the bucket policy was actually gone.

Downgrading cdk (and the node modules) to 1.19.0:
I saw iam policies are back to what I had before:
image
image

Environment

  • **CLI Version :1.20.0
  • Framework Version:
  • **OS :Linux
  • **Language :JavaScript

I didn't spot any breaking change notice in the release notes at:
https://github.com/aws/aws-cdk/releases/tag/v1.20.0


This is 🐛 Bug Report

@jeshan jeshan added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 10, 2020
@netroy
Copy link
Contributor

netroy commented Jan 10, 2020

@jeshan is myRole imported using Role.fromRoleArn?
if not, can you please share some more information on how myRole is created?

@jeshan
Copy link
Author

jeshan commented Jan 10, 2020

Sorry, I didn't realise that that was relevant. It is indeed created with fromRoleArn:

Role.fromRoleArn(
            this,
            'deployment-account-role',
            `arn:aws:iam::123456789012:role/deployment-role`,
        );

and I'm creating the bucket with something like:

this.publicBucket = new Bucket(this, 'bucket', {
            publicReadAccess: true,
            bucketName: 'my-public-bucket',
        });

@netroy
Copy link
Contributor

netroy commented Jan 10, 2020

I believe the change that's causing this issue for you, landed in this PR #5568

@jeshan
Copy link
Author

jeshan commented Jan 10, 2020

OK. I'm trying with a newly created role just now (with 1.20) and I'm getting the same incorrect behaviour:

let myRole = new Role(this, 'dep-role', {
            assumedBy: new ServicePrincipal('...'),
            managedPolicies: [
                ManagedPolicy.fromAwsManagedPolicyName(
                    '...',
                ),
            ],
        });

@netroy
Copy link
Contributor

netroy commented Jan 10, 2020

For a new role, that's been the expected behaviour.
For imported roles, I'd recommend waiting for one of the repo owners to confirm if this is a regression or not.

@jeshan
Copy link
Author

jeshan commented Jan 10, 2020

For a new role, that's been the expected behaviour.

I don't understand. Why would it be different for new v/s imported ones? How else would you reference a role in a bucket policy?

@eladb eladb added the p0 label Jan 12, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 14, 2020

Normally you wouldn't add the permissions to the bucket but to the role.

Since this is an imported role we assume the permissions have already been added to the role and we don't add them to the bucket. On the other hand, this is an imported role from another account (seems like?), in which case the permissions need to be added to BOTH the resource and the role.

For the moment, you can work around this by using bucket.addToResourcePolicy() while we sort this out.

Sorry for the problems this has caused.

@SomayaB SomayaB added @aws-cdk/aws-iam Related to AWS Identity and Access Management @aws-cdk/aws-s3 Related to Amazon S3 and removed needs-triage This issue or PR still needs to be triaged. labels Jan 15, 2020
rix0rrr added a commit that referenced this issue Jan 15, 2020
Trust policies should be added to resources when trying to grant
permissions to a principal in a different account.  Typically this is a
Service Principal, but it could also be an imported Role from a
different account (and the same should not be done for a Role in the
same account).

Our previous API (`addToPolicy` returning `true|false`) was designed for
in-account Roles and cross-account ServicePrincipals, but was not rich
enough to make the distinction that Roles could be cross-account.

Add a function to `IPrincipal` to ask it whether it is in the same or a
different account from a given scope, and have `Grant` respect that.

Also in this commit: a bugfix for `ImmutableRole` to make it work
correctly with `Grant`

Also in this commit: `parseArn()` used to behave the same for
`parseArn(TOKEN)` and `parseArn("arn:{TOKEN}:svc:1234:...")`.
Make it (opportunistically) do the right thing in the latter case,
parsing out the concrete components and the TOKEN components
separately. This makes it possible to parse out the concrete
account number from an ARN that contains tokenized parts we don't
care about.

Fixes #5740.
rix0rrr added a commit that referenced this issue Jan 15, 2020
Trust policies should be added to resources when trying to grant
permissions to a principal in a different account.  Typically this is a
Service Principal, but it could also be an imported Role from a
different account (and the same should not be done for a Role in the
same account).

Our previous API (`addToPolicy` returning `true|false`) was designed for
in-account Roles and cross-account ServicePrincipals, but was not rich
enough to make the distinction that Roles could be cross-account.

Add a function to `IPrincipal` to ask it whether it is in the same or a
different account from a given scope, and have `Grant` respect that.

Also in this commit: a bugfix for `ImmutableRole` to make it work
correctly with `Grant`

Also in this commit: `parseArn()` used to behave the same for
`parseArn(TOKEN)` and `parseArn("arn:{TOKEN}:svc:1234:...")`.
Make it (opportunistically) do the right thing in the latter case,
parsing out the concrete components and the TOKEN components
separately. This makes it possible to parse out the concrete
account number from an ARN that contains tokenized parts we don't
care about.

Fixes #5740.
@rix0rrr rix0rrr added p1 and removed p0 labels Jan 15, 2020
@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Jun 1, 2020
@rix0rrr rix0rrr added the effort/medium Medium work item – several days of effort 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 @aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. effort/medium Medium work item – several days of effort in-progress This issue is being actively worked on. p1
Projects
None yet
5 participants