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

[kms] overiding policy default statement #10575

Closed
pproux opened this issue Sep 28, 2020 · 5 comments · Fixed by #11918
Closed

[kms] overiding policy default statement #10575

pproux opened this issue Sep 28, 2020 · 5 comments · Fixed by #11918
Assignees
Labels
@aws-cdk/aws-kms Related to AWS Key Management effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2

Comments

@pproux
Copy link

pproux commented Sep 28, 2020

Setting a KMS policy parameter add a statement to the default policy instead of overriding the whole policy.

Reproduction Steps

    aws_kms.Key(
        self, 'mykey',
        policy = aws_iam.PolicyDocument(
            statements = [
                aws_iam.PolicyStatement(
                    effect = aws_iam.Effect('ALLOW'),
                    actions = [ 'kms:*' ],
                    principals = [ aws_iam.ArnPrincipal('arn:aws:iam::xxxxxxxxxx:root') ] 
                )
            ]
        )
    )

What did you expect to happen?

I was expecting the new KMS policy to be exactly what i set in the policy parameter :

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::xxxxxxxx:root"
            },
            "Action": "kms:*"
        }
    ]
}

What actually happened?

Instead it was added as a new statement along the default one.

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::xxxxxxx:root"
            },
            "Action": "kms:*"
        },
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::xxxxxxxx:root"
            },
            "Action": [
                "kms:Create*",
                "kms:Describe*",
                "kms:Enable*",
                "kms:List*",
                "kms:Put*",
                "kms:Update*",
                "kms:Revoke*",
                "kms:Disable*",
                "kms:Get*",
                "kms:Delete*",
                "kms:ScheduleKeyDeletion",
                "kms:CancelKeyDeletion",
                "kms:GenerateDataKey",
                "kms:TagResource",
                "kms:UntagResource"
            ],
            "Resource": "*"
        }
    ]
}

Environment

  • CLI Version : (cdk --version) 1.64.1
  • Framework Version: (pip3 list | grep aws-cdk.aws-kms) 1.64.1
  • Node.js Version: (node -v) v10.16.3
  • **OS : ** (uname -r) 4.14.146-93.123.amzn1.x86_64
  • Language (Version): (python --version) Python 3.6.8

This is 🐛 Bug Report

@pproux pproux added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 28, 2020
@github-actions github-actions bot added the @aws-cdk/aws-kms Related to AWS Key Management label Sep 28, 2020
@pproux pproux changed the title [kms] [kms] overiding policy default statements Sep 28, 2020
@pproux pproux changed the title [kms] overiding policy default statements [kms] overiding policy default statement Sep 28, 2020
@njlynch
Copy link
Contributor

njlynch commented Sep 30, 2020

Related to #8977 (possibly a complete dupe of it). I'll keep this one open for now.

@njlynch njlynch added p2 feature-request A feature should be added or improved. effort/small Small work item – less than a day of effort and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 30, 2020
@njlynch
Copy link
Contributor

njlynch commented Dec 3, 2020

@pproux - I'm circling back around to this and several related issues right now, and I wanted to clarify your use case. The key policy you are providing is to grant all permissions to the root account. This is identical to the policy you'll get if you supply trustAccountIdentities = True when creating the key. Does that actually solve your use case, or are you granting all permissions to an account other than the current stack/environment account? If so, would you mind briefly explaining your use case? I'd like to know more about why users might want to opt out of the default policies.

njlynch added a commit that referenced this issue 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 issue 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 mergify bot closed this as completed in #11918 Dec 9, 2020
mergify bot pushed a commit that referenced this issue 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*
@github-actions
Copy link

github-actions bot commented Dec 9, 2020

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@pproux
Copy link
Author

pproux commented Dec 14, 2020

Hi @njlynch,

I have no problem with the default policy.

But i think as soon as you provide a custom one (like i did), then the the default policy should not be added.

@njlynch
Copy link
Contributor

njlynch commented Dec 14, 2020

@pproux - That's exactly what the fix (released in v1.78.0) enables. Default policies will not be added if you provide a policy, if the '@aws-cdk/aws-kms:defaultKeyPolicies' feature flag is set. The flag will be enabled by default for all new CDK projects, or can be manually enabled for existing projects.

flochaz pushed a commit to flochaz/aws-cdk that referenced this issue 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
@aws-cdk/aws-kms Related to AWS Key Management effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
2 participants