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

Kinesis Stream encryption should support the Master Kinesis key #751

Closed
ccurrie-amzn opened this issue Sep 20, 2018 · 6 comments · Fixed by #7057
Closed

Kinesis Stream encryption should support the Master Kinesis key #751

ccurrie-amzn opened this issue Sep 20, 2018 · 6 comments · Fixed by #7057
Assignees
Labels
@aws-cdk/aws-kinesis Related to Amazon Kinesis effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. needs-reproduction This issue needs reproduction.

Comments

@ccurrie-amzn
Copy link
Contributor

The L1 construct for Kinesis Stream accepts a string KeyId which can be any of arn, guid, or alias. The L2 construct only supports a keyArn, which hinders the use of Amazon-managed aliases such as alias/aws/kinesis.

This may be a change to EncryptionKey more than Kinesis, as EncryptionKey may need to understand the special nature of alias/aws keys, and make changes to such keys a no-op.

@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 21, 2018

Your issue may be solved in the latest release (0.9.2), as all attributes types have disappeared and are replaced with string.

That's not so say that EncryptionKey should not be smarter. Do you think we should model service default keys explicitly?

@ccurrie-amzn
Copy link
Contributor Author

I expect you will be forced to; the L2 constructs are helpful in adding permissions to the resource policy for keys when required, but I suspect that will likely fail for service default keys.

@ccurrie-amzn
Copy link
Contributor Author

ccurrie-amzn commented Oct 2, 2018

I have retested this with 0.10, and it does break the stack build when trying to use the service default keys:

    const key = EncryptionKeyRef.import(this, "SystemKey", {
      keyArn: "alias/aws/kinesis"
    });

    const stream = new Stream(this, "EncryptedStream", {
      encryption: StreamEncryption.Kms,
      encryptionKey: key
    });

    const func = new Function(this, "Lambda", {
      code: Code.directory("dist"),
      handler: "lib/index.handler",
      runtime: Runtime.NodeJS810,
    });

    stream.grantRead(func.role);

This ultimately generates a PolicyStatement on the function role in the form:

-
  Action: 'kms:Decrypt'
  Effect: Allow
  Resource: alias/aws/kinesis

Which fails with the error: Resource alias/aws/kinesis must be in ARN format or "*".

  1. I can work around this problem by changing the EncryptionKeyRef to:
    const key = EncryptionKeyRef.import(this, "SystemKey", {
      keyArn: new FnJoin(":", [
        "arn:aws:kms",
        new AwsRegion(),
        new AwsAccountId(),
        "alias/aws/kinesis"
      ]).toString(),
    });
  1. That said, IIUC the PolicyStatement is not needed for system keys, and making policy mods to the key itself may not be allowed.

@rix0rrr rix0rrr added feature-request A feature should be added or improved. and removed feature-request A feature should be added or improved. feature labels Nov 6, 2018
@debora-ito debora-ito added the @aws-cdk/aws-kinesis Related to Amazon Kinesis label Nov 8, 2018
@rix0rrr rix0rrr added the gap label Jan 4, 2019
@lambrospetrou
Copy link

Are there plans to support this before the 1.0 release is out? We are creating lots of streams in my team and without this we had to fallback to the CfnStream construct and use the raw StreamEncryption that provides keyId.

In addition, in CfnStream the links to StreamEncryptionProperty are broken.

Thanks

@skinny85
Copy link
Contributor

Now that Alias implements IKey, and can be used anywhere a KMS key can, is this issue now solved?

@lambrospetrou @ccurrie-amzn ?

@SomayaB SomayaB added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Sep 26, 2019
@ccurrie-amzn
Copy link
Contributor Author

We haven't updated the system that encountered this issue to the latest CDK, so it will be some time before I can verify that the use case is covered; that said, the fragment above isn't large, so perhaps I or someone else can rewrite it in the new syntax and verify the Cfn output.

@NGL321 NGL321 removed the gap label Oct 4, 2019
@SomayaB SomayaB added needs-reproduction This issue needs reproduction. and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Oct 17, 2019
@skinny85 skinny85 added the effort/medium Medium work item – several days of effort label Feb 6, 2020
@skinny85 skinny85 assigned iliapolo and unassigned skinny85 Mar 19, 2020
shivlaks added a commit that referenced this issue Mar 29, 2020
Adds a `StreamEncryption` option to specify that encryption should be enabled and managed by Kinesis.

Closes #751
@mergify mergify bot closed this as completed in #7057 Mar 31, 2020
mergify bot pushed a commit that referenced this issue Mar 31, 2020
feat(kinesis): stream encryption with the Kinesis master key

Adds a `StreamEncryption` option to specify that encryption should be enabled on a Stream with the master key managed by Kinesis.

Closes #751
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-kinesis Related to Amazon Kinesis effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. needs-reproduction This issue needs reproduction.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants