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

feat(kms): trustAccountIdentities avoids cyclic stack dependencies #5575

Merged
merged 23 commits into from
Feb 20, 2020

Conversation

moofish32
Copy link
Contributor

@moofish32 moofish32 commented Dec 29, 2019

The default KMS policy is designed for two approaches:

  1. IAM User based key administration
  2. Situations in which the stack containing the key is created after the stacks
    containing IAM roles that will use the key (by adding a dependency)

The problem is that many AWS consumers do not allow or use IAM users. If you
also want to use infrastructure as code this style of management is difficult
because commonly Grants are used. Key grants are not supported directly in
CloudFormation, but do enable fine grain programmatic key control. Another
reason for this control policy is to prevent the ability for the root user
(which might be a shared credential for account admins) access to key usage
permisssions. Again, that issue is mitigated when consumers choose to follow the
best practice of never creating AWS Credentials for root users.

The second solution of stack dependencies creates a dependency challenge. The
stack with the KMS key must now be created after your stacks that have IAM
Roles that need to use the key. This can be very problematic if you want to use
default encryption on AWS S3 bucket and then create a second stack that has a
lambda function reading from this bucket. The lambda role needs permission to
the s3 bucket which isn't created before the role. Yes, resource ARN's can be
created for some resources that do not exist, but extremely counter intuitive
and many potential issues with custom resources.

The answer to these problems is to enable IAM policy control of the CMK as
described
here.

Since KMS keys, and specifically CMKs, should default to secure over
ergonomically pleasing the default option for IAM policy control is false. If a
user opts in via properties like below:

    new Key(stack, 'MyKey', {enablePolicyControl: true});

Then the kms key policy will be:

{
  "Sid": "Enable IAM User Permissions",
  "Effect": "Allow",
  "Principal": {"AWS": "arn:aws:iam::111122223333:root"},
  "Action": "kms:*",
  "Resource": "*"
}

This would provide another solution for #5183 and reduce the likelihood of
hitting problems like #5190.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Commit Message

feat(kms): trustAccountIdentities avoids cyclic stack dependencies

Add a switch which will trust the entire account to use the key, subject to the identity policies of the identities making the calls (same as for all other AWS resources).

Since there is no way of updating a key policy after creating the key, AND bidirectional trust needs to be established between the resource and the identity, users are basically locked into creating both key and identities using the key in the same stack. The only way for cross-stack key usage where the key gets created first is by wildcarding. This switch allows that in an easy manner without introducing cyclic dependencies.

Copy link
Contributor Author

@moofish32 moofish32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming is difficult, and I'm not sure I have it right yet for this property.

*
* @default false
*/
readonly enablePolicyControl?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be enableIamPolicyControl I didn't like the way that reads with the capital I and lower am. However, I am not sure Policy is enough here because technically there is a Key Policy.

const actions = enablePolicyControl ?
['kms:*'] :
[
"kms:Create*",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these permissions are un-altered just moved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's interesting tagging and untagging permissions are not granted. This means that if you add or remove after the initial stack it will warn the user but not implement the change.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@solshark
Copy link

Looking very much forward for this.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 2, 2020

As far as I can tell, the change is to allow anyone in the account(*) to also USE the keys to encrypt/decrypt instead of only to administer the key. I don't like adding the parameter to allowAccountToAdmin(). I'd rather see a new method. To me it seems like the change comes down to the following:

if (props.enablePolicyControl) {
  this.allowEveryoneInAccountToAdminOrUse();
} else {
  this.allowAccountToAdmin();
}

First of all, am I reading the change correctly? Is it simply to completely manage key use from the identity policies of the identities that might want to, without calling key.grantDecrypt()?

The same could be achieved by use of addToResourcePolicy() after creating the key, or we could even have a wrapper for it:

key.grantUsageToAnyoneInThisAccount();

Which looks appropriately scary.

If the problem is about downstream consumers, a better alternative would be to make a custom resource KeyPolicy which behaves in the same way as AWS::S3::BucketPolicy and AWS::SQS::QueuePolicy, which can be attached from a downstream stack (or we can petition CloudFormation to add this resource).

(*) Does the use of "AccountRootPrincipal" here represent the actual "root" identity, or just it just mean "any identity in the account"? You're describing it to mean the former but according to the IAM docs, AWS:arn:aws:iam::123456789012:root (and also AWS:123456789012) mean the latter. Or is this is a difference in KMS vs other services?

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 2, 2020

Hmm I see KMS roughly recommends the same practice here: https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html

I'd still like to see this as an additive operator to the existing key management policy. There will be 2 statements:

  • One to allow anyone to manage
  • One to allow anyone to use

The latter one can be added via a construction property and also via a method (but won't be added twice).

For the property, I would propose something like:

/**
 * Allow access to this key using only the identity policy
 *
 * By default, permissions to use this key need to be added on the
 * key policy as well as the identity policy.
 *
 * Setting this to `true` adds a default statement which will allow
 * any identity in the same account to access the key, so that key
 * access is controlled completely from the identity's IAM policy
 * (similar to how it works for other AWS resources).
 *
 * You might be forced to use this if the identity that needs to access
 * this key will be created in a different Stack, as there is no
 * way to set up the permissions otherwise without creating a 
 * circular reference.
 *
 * @default false
 * @see https://github.com/aws-cloudformation/aws-cloudformation-coverage-roadmap/issues/322
 */
 readonly controlAccessViaIdentityPolicyAlone?: boolean;

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 2, 2020

Created a feature request for CloudFormation: aws-cloudformation/cloudformation-coverage-roadmap#322

@moofish32
Copy link
Contributor Author

(*) Does the use of "AccountRootPrincipal" here represent the actual "root" identity, or just it just mean "any identity in the account"? You're describing it to mean the former but according to the IAM docs, AWS:arn:aws:iam::123456789012:root (and also AWS:123456789012) mean the latter. Or is this is a difference in KMS vs other services?

The doc to me says can, but my understanding is that this is not defaulted. For example, I can create a kms key policy with kms:* on resources [*] to arn:aws:iam::123456789012:root. If I create a role for a lambda function and do not include any kms key permissions, then lambda will not be able to use the key.

For this reason I would propose a comment change

Setting this to true adds a default statement which will allow
any identity in the same account to access the key, so that key
access is controlled completely from the identity's IAM policy
(similar to how it works for other AWS resources).

Setting this to true adds a default statement which delegates
key access control completely to the identity's IAM policy
(similar to how it works for other AWS resources).

I'd still like to see this as an additive operator to the existing key management policy. There will be 2 statements:

What is the benefit to the separation for this one? Also, the IAM permissions can change without much notice which leaves many docs in AWS out dated. For example, we are missing the ImportKeyMaterial, TagResource, & UntagResource for management. GenerateDataKey I would think is key usage, but perhaps there is some dependency I don't know about. If we are opting into IAM management is there a new permission that we would not want to manage? I will implement which ever path the you want here.

I can get the name change up, 🏆 contender for longest property name in CDK. What are your final thoughts on custom resource and the permission separation above? If you are set with your initial position just drop that in a quick comment and I'll move on those.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 3, 2020

After reading up on the KMS docs some more this is what they more or less recommend themselves, after recognizing that they've made their permissions system hard to use. So I think I'm on board with your PR, with two changes:

  • I'd rather have 2 private methods to add the statements, rather than that argument to the current method.
  • Not a fan of the property names you've suggested or the one I came up with. Let's see if we can find a better one.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 3, 2020

Y'know what I have a hard time coming up with a better name. My current suggestion would be to pluralize, to controlAccessViaIdentityPoliciesAlone which I feel is slightly more correct.

Also interested in sourcing the opinions of my team mates, @eladb, @RomainMuller, @nija-at ?

@rix0rrr rix0rrr requested review from eladb and nija-at January 3, 2020 15:15
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 15, 2020

Nobody came to the bat signal. I confirmed that the right term for the policies is "Identity-based policy" (https://docs.aws.amazon.com/IAM/latest/UserGuide/access_policies.html). In my mind, "identity policy" work as well.

Let's try some alternatives:

  • controlAccessViaIdentityPoliciesAlone
  • onlyNeedIdentityPolicy
  • permissiveKeyPolicy
  • ... ?

@nija-at
Copy link
Contributor

nija-at commented Jan 15, 2020

controlAccessViaIdentityPoliciesAlone does sound a bit long.

I'll add one to the suggestion list - identityPolicyAccessControl

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 15, 2020

  • trustAccount
  • trustAccountIdentities

@moofish32
Copy link
Contributor Author

I'm looking at this holiday weekend to try to wrap this up, so the naming 🕐 isn't expired yet. Also I will not be offended if someone finishes this before me.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 17, 2020

When the boolean has been switched on, the grant...() methods need to switch on that boolean to avoid adding resource policy statements. We then need to switch to Grant.addToIdentityOrResource isntead of addToIdentityAndResource.

@moofish32
Copy link
Contributor Author

  • trustAccount
  • trustAccountIdentities

I'm leaning towards trustAccountIdentities

I could see trustAccountIdentityPolicies but I preferred the shorter because identities have to have policies.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…ccountIdentities for identity based kms key control
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@moofish32
Copy link
Contributor Author

@rix0rrr -- I changed the default admin policy to include what I think is the right administrative permissions. I've left GenerateDataKey only for backwards compatibility, but I strongly consider that a usage permission.

As a result many tests down stream are breaking. If we don't want to make this policy change right now, then drop me a note so I can revert and avoid the pain of integ tests plus many other test updates.

@moofish32
Copy link
Contributor Author

cc/ ⬆️ @nija-at

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 24, 2020

Let's not make a breaking change right now, we can discuss that in a separate PR.

@mergify mergify bot dismissed rix0rrr’s stale review February 18, 2020 22:52

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 55a28be
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr
rix0rrr previously approved these changes Feb 19, 2020
@mergify
Copy link
Contributor

mergify bot commented Feb 19, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 965c402
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 86a38b1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 19, 2020

Uhhh

npm ERR! notarget No matching version found for @aws-cdk/core@1.25.0.

@mergify mergify bot dismissed rix0rrr’s stale review February 19, 2020 14:52

Pull request has been modified.

rix0rrr
rix0rrr previously approved these changes Feb 20, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 20, 2020

Wow the system really doesn't want to cooperate with this one.

@mergify mergify bot dismissed rix0rrr’s stale review February 20, 2020 10:38

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 99aad54
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Feb 20, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: e26d6dd
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: ad75758
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Feb 20, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot merged commit 03f4ef2 into aws:master Feb 20, 2020
@moofish32
Copy link
Contributor Author

moofish32 commented Feb 20, 2020 via email

njlynch added a commit that referenced this pull request Dec 7, 2020
… (under feature flag)

In #5575, a new flag (`trustAccountIdentities`) was introduced which -- when set
-- changes the default key policy from a custom key admin policy to one that
grants all access to the key to the root account user. This key policy matches
the default policy when a key is created via the KMS APIs or console.

For backwards-compatibility reasons, the default for `trustAccountIdentities`
had to be set to `false`. Without the flag explicitly set, the default key
policy is one that (a) doesn't match the KMS-recommended admin policy and (b)
doesn't explicitly enable IAM principal policies to acccess the key. This means
that all usage operations (e.g., Encrypt, GenerateDataKey) must be added to both
the key policy and to the principal policy.

This change introduces a new feature flag to flip the default behavior of the
`trustAccountIdentities` flag, so new keys created will have the sane defaults
matching the KMS recommended best practices.

As a related change, this feature flag also changes the behavior when a user
passes in `policy` when creating a Key. Without the feature flag set, the policy
is always appended to the default key policy. With the feature flag set, the
policy will *override* the default key policy, enabling users to opt-out of the
default key policy to introduce a more restrictive policy if desired. This also
matches the KMS API behavior, where a policy provided by the user will override
the defaults.

Marking this PR as `requires-two-approvers` to ensure this PR gets an
appropriately-critical review.

BREAKING CHANGE: change the default value of trustAccountIdentities to true,
which will result in the key getting the KMS-recommended default key
policy. This is enabled through the '@aws-cdk/aws-kms:defaultKeyPolicies'
feature flag.

fixes #8977
fixes #10575
fixes #11309
njlynch added a commit that referenced this pull request Dec 7, 2020
… (under feature flag)

In #5575, a new flag (`trustAccountIdentities`) was introduced which -- when set
-- changes the default key policy from a custom key admin policy to one that
grants all access to the key to the root account user. This key policy matches
the default policy when a key is created via the KMS APIs or console.

For backwards-compatibility reasons, the default for `trustAccountIdentities`
had to be set to `false`. Without the flag explicitly set, the default key
policy is one that (a) doesn't match the KMS-recommended admin policy and (b)
doesn't explicitly enable IAM principal policies to acccess the key. This means
that all usage operations (e.g., Encrypt, GenerateDataKey) must be added to both
the key policy and to the principal policy.

This change introduces a new feature flag to flip the default behavior of the
`trustAccountIdentities` flag, so new keys created will have the sane defaults
matching the KMS recommended best practices.

As a related change, this feature flag also changes the behavior when a user
passes in `policy` when creating a Key. Without the feature flag set, the policy
is always appended to the default key policy. With the feature flag set, the
policy will *override* the default key policy, enabling users to opt-out of the
default key policy to introduce a more restrictive policy if desired. This also
matches the KMS API behavior, where a policy provided by the user will override
the defaults.

Marking this PR as `requires-two-approvers` to ensure this PR gets an
appropriately-critical review.

BREAKING CHANGE: change the default value of trustAccountIdentities to true,
which will result in the key getting the KMS-recommended default key
policy. This is enabled through the '@aws-cdk/aws-kms:defaultKeyPolicies'
feature flag.

fixes #8977
fixes #10575
fixes #11309
mergify bot pushed a commit that referenced this pull request Dec 9, 2020
… (under feature flag) (#11918)

In #5575, a new flag (`trustAccountIdentities`) was introduced which -- when set
-- changes the default key policy from a custom key admin policy to one that
grants all access to the key to the root account user. This key policy matches
the default policy when a key is created via the KMS APIs or console.

For backwards-compatibility reasons, the default for `trustAccountIdentities`
had to be set to `false`. Without the flag explicitly set, the default key
policy is one that (a) doesn't match the KMS-recommended admin policy and (b)
doesn't explicitly enable IAM principal policies to acccess the key. This means
that all usage operations (e.g., Encrypt, GenerateDataKey) must be added to both
the key policy and to the principal policy.

This change introduces a new feature flag to flip the default behavior of the
`trustAccountIdentities` flag, so new keys created will have the sane defaults
matching the KMS recommended best practices.

As a related change, this feature flag also changes the behavior when a user
passes in `policy` when creating a Key. Without the feature flag set, the policy
is always appended to the default key policy. With the feature flag set, the
policy will *override* the default key policy, enabling users to opt-out of the
default key policy to introduce a more restrictive policy if desired. This also
matches the KMS API behavior, where a policy provided by the user will override
the defaults.

Marking this PR as `requires-two-approvers` to ensure this PR gets an
appropriately-critical review.

BREAKING CHANGE: change the default value of trustAccountIdentities to true,
which will result in the key getting the KMS-recommended default key
policy. This is enabled through the '@aws-cdk/aws-kms:defaultKeyPolicies'
feature flag.

fixes #8977
fixes #10575
fixes #11309

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
flochaz pushed a commit to flochaz/aws-cdk that referenced this pull request Jan 5, 2021
… (under feature flag) (aws#11918)

In aws#5575, a new flag (`trustAccountIdentities`) was introduced which -- when set
-- changes the default key policy from a custom key admin policy to one that
grants all access to the key to the root account user. This key policy matches
the default policy when a key is created via the KMS APIs or console.

For backwards-compatibility reasons, the default for `trustAccountIdentities`
had to be set to `false`. Without the flag explicitly set, the default key
policy is one that (a) doesn't match the KMS-recommended admin policy and (b)
doesn't explicitly enable IAM principal policies to acccess the key. This means
that all usage operations (e.g., Encrypt, GenerateDataKey) must be added to both
the key policy and to the principal policy.

This change introduces a new feature flag to flip the default behavior of the
`trustAccountIdentities` flag, so new keys created will have the sane defaults
matching the KMS recommended best practices.

As a related change, this feature flag also changes the behavior when a user
passes in `policy` when creating a Key. Without the feature flag set, the policy
is always appended to the default key policy. With the feature flag set, the
policy will *override* the default key policy, enabling users to opt-out of the
default key policy to introduce a more restrictive policy if desired. This also
matches the KMS API behavior, where a policy provided by the user will override
the defaults.

Marking this PR as `requires-two-approvers` to ensure this PR gets an
appropriately-critical review.

BREAKING CHANGE: change the default value of trustAccountIdentities to true,
which will result in the key getting the KMS-recommended default key
policy. This is enabled through the '@aws-cdk/aws-kms:defaultKeyPolicies'
feature flag.

fixes aws#8977
fixes aws#10575
fixes aws#11309

----

*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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants