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 encryption cross-stack permission causes cyclic reference #3732

Closed
1 of 6 tasks
robwettach opened this issue Aug 20, 2019 · 9 comments
Closed
1 of 6 tasks

S3 encryption cross-stack permission causes cyclic reference #3732

robwettach opened this issue Aug 20, 2019 · 9 comments
Assignees
Labels
@aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. cross-stack Related to cross-stack resource sharing p2

Comments

@robwettach
Copy link

robwettach commented Aug 20, 2019

I'm submitting a ...

  • πŸͺ² bug report
  • πŸš€ feature request
  • πŸ“š construct library gap
  • πŸ“• documentation issue => If regarding developer guide, please create issue here
  • ☎️ security issue or vulnerability => Please see policy
  • ❓ support request => Please see note at the top of this template.

What is the current behavior?

When attempting to grant read/write access to an encrypted S3 bucket exported from a separate stack, I'm receiving a "cyclic reference" error.

Minimal sample code:

#!/usr/bin/env node
import "source-map-support/register";

import cdk = require("@aws-cdk/core");
import iam = require("@aws-cdk/aws-iam");
import s3 = require("@aws-cdk/aws-s3");

class S3HostStack extends cdk.Stack {
  public readonly bucket: s3.Bucket;
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    this.bucket = new s3.Bucket(this, "EncryptedBucket", {
      encryption: s3.BucketEncryption.KMS
    });
  }
}

interface S3ConsumerStackProps extends cdk.StackProps {
  readonly bucket: s3.Bucket;
}

class S3ConsumerStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props: S3ConsumerStackProps) {
    super(scope, id, props);

    const accessRole = new iam.Role(this, "Access", {
      assumedBy: new iam.ServicePrincipal("ecs-tasks.amazonaws.com")
    });
    
    props.bucket.grantRead(accessRole);
  }
}

const app = new cdk.App();
const host = new S3HostStack(app, 'S3HostStack');
new S3ConsumerStack(app, 'S3ConsumerStack', {
  bucket: host.bucket
});

Error output:

$ cdk synth

.../node_modules/@aws-cdk/core/lib/stack.ts:273
        throw new Error(`'${stack.node.path}' depends on '${this.node.path}' (${dep.join(', ')}). Adding this dependency (${reason}) would create a cyclic reference.`);
              ^
Error: 'S3ConsumerStack' depends on 'S3HostStack' (S3ConsumerStack/Access/DefaultPolicy/Resource -> S3HostStack/EncryptedBucket/Resource.Arn). Adding this dependency (S3HostStack/EncryptedBucket/Key/Resource -> S3ConsumerStack/Access/Resource.Arn) would create a cyclic reference.
    at S3HostStack.addDependency (.../node_modules/@aws-cdk/core/lib/stack.ts:273:15)
    at CfnReference.consumeFromStack (.../node_modules/@aws-cdk/core/lib/private/cfn-reference.ts:131:22)
    at S3HostStack.prepare (.../node_modules/@aws-cdk/core/lib/stack.ts:490:23)
    at Function.prepare (.../node_modules/@aws-cdk/core/lib/construct.ts:84:28)
    at Function.synth (.../node_modules/@aws-cdk/core/lib/construct.ts:41:10)
    at App.synth (.../node_modules/@aws-cdk/core/lib/app.ts:128:36)
    at process.App.process.once (.../node_modules/@aws-cdk/core/lib/app.ts:111:45)
    at Object.onceWrapper (events.js:285:13)
    at process.emit (events.js:197:13)
    at process.EventEmitter.emit

What is the expected behavior (or behavior of feature suggested)?

Granting access to a bucket from a separate stack should not cause a cyclic reference. I believe what's happening is that CDK is trying to modify the KMS key's Resource Policy to reference the Role's ARN. Is there any way to do that in the context of the consumer stack, or avoid that all together? In particular, I think this code is what's causing the cyclic reference, so even if I supplied my own KMS key to the S3 bucket it would probably have a similar effect if I simply called grantEncryptDecrypt from the consumer stack. Previously in CloudFormation I would simply export the KMS Key ARN and grant my IAM Role access without modifying the Key's Resource Policy. Is there any way to get that behavior by default in CDK?

What is the motivation / use case for changing the behavior or adding this feature?

Environment

  • CDK CLI Version: 1.4.0 (build 175471f)
  • Module Version: 1.4.0
  • OS: OSX High Sierra
  • Language: TypeScript
@rix0rrr rix0rrr added the bug This issue is a bug. label Aug 28, 2019
@eladb eladb added the @aws-cdk/aws-s3 Related to Amazon S3 label Sep 4, 2019
@moofish32
Copy link
Contributor

@skinny85 -- did you fix this already? (#3694)

@skinny85
Copy link
Contributor

@skinny85 -- did you fix this already? (#3694)

I'm actually not sure - @robwettach, can you try upgrading to 1.9.0 and see if that helps?

Thanks,
Adam

@robwettach
Copy link
Author

Doesn't look like it, no. I copied my "minimal sample code" into new 1.8.0 and 1.9.0 applications and I'm getting the same error still.

@Artmobile
Copy link

I have the same issue in 1.13.1. Currently the only way is not to use CMK on buckets (which is a show stopper for us). @skinny85 can you please update? The issue can be easily reproduced from the snippet provided by @robwettach so you do not have to guess.

@eladb eladb added the p2 label Oct 23, 2019
@SomayaB SomayaB added the cross-stack Related to cross-stack resource sharing label Oct 23, 2019
@Artmobile
Copy link

Hi Elad,

Is there any update on that? Same thing happens in 1.19.0.
Should we move all our lambdas, databases and buckets into a single stack?

Thanks

@MrArnoldPalmer
Copy link
Contributor

Hey @Artmobile.

Can you create a bucket access policy (with iam.Policy) from the bucket stack and then reference it from the other stacks that have constructs that need access? This is a workaround but hopefully can satisfy your use-case in the interim.

@petermeansrock
Copy link
Contributor

Unless I'm mistaken, it looks like Adam's commit (#3694) covers this case. From the commit message:

Granting permissions to a key works if the principal belongs to a stack that is a dependent of the key stack

Thanks to this code snippet, as long as S3ConsumerStack declares a dependency on S3HostStack before IBucket.grantRead is invoked, the KMS key's policy will only reference the consumer stack's account (rather than its role), avoiding the cyclic reference. Here's an updated version of your S3ConsumerStack example class that should synthesize correctly:

class S3ConsumerStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props: S3ConsumerStackProps) {
    super(scope, id, props);

    const accessRole = new iam.Role(this, "Access", {
      assumedBy: new iam.ServicePrincipal("ecs-tasks.amazonaws.com")
    });

    this.addDependency(cdk.Stack.of(props.bucket));
    props.bucket.grantRead(accessRole);
  }
}

@Artmobile
Copy link

@petermeansrock your suggestion worked. Thank you!

@eladb eladb assigned iliapolo and unassigned eladb Jan 22, 2020
@iliapolo
Copy link
Contributor

Seems like this was resolved in #3694

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. cross-stack Related to cross-stack resource sharing p2
Projects
None yet
Development

No branches or pull requests

10 participants