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

fix(kms) default key policy: give all permissions to root account #2329

Closed
wants to merge 1 commit into from

Conversation

spg
Copy link
Contributor

@spg spg commented Apr 19, 2019

Enables all kms:* actions on the KMS key's default policy.

Context:
I was trying to trigger a Lambda function to receive messages from an encrypted (customer-managed KMS key) SQS queue.
When using the default policy for the KMS key:

const newObjectQueueKey = new kms.EncryptionKey(this, 'NewObjectQueueKey', {
  enableKeyRotation: true
});

the Lambda function was not able to receive messages from the encrypted SQS Queue. To fix this problem, I had to override the KMS key's default policy:

const newObjectQueueKey = new kms.EncryptionKey(this, 'NewObjectQueueKey', {
  enableKeyRotation: true,
  policy: new iam.PolicyDocument().addStatement(
    new iam.PolicyStatement()
      .addAllResources()
      .addAction('kms:*')
      .addAccountRootPrincipal()
  )
});

Using the above fix, the Lambda function was able to receive messages from the encrypted SQS Queue.

Source: https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default-allow-root-enable-iam


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

@spg spg requested a review from a team as a code owner April 19, 2019 00:26
@spg spg changed the title fix(kms) Default key policy: give all permissions to root account fix(kms) default key policy: give all permissions to root account Apr 19, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 22, 2019

It might be that you have a problem, but this is not the way to fix it. This gives all IAM users/roles in your account permissions to do ANYTHING to the key.

You are free to use this in your own application (though I would advise against it), but we need more fine-grained permissions than that in the library, both in terms of API calls allowed as well as principals they are allowed to.

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Permissions are unacceptably widened this way.

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 23, 2019

In fact, just seeing this PR open makes me eerily uncomfortable, for fear of someone merging it accidentally. It's like standing at the edge of a cliff face :).

I'm sorry, but I'm going to close this. Feel free to reopen once the code gets changed.

@rix0rrr rix0rrr closed this Apr 23, 2019
@spg
Copy link
Contributor Author

spg commented Apr 24, 2019

Hey @rix0rrr , thanks for having a look at my PR.

I want to clear out some points:

This gives all IAM users/roles in your account permissions to do ANYTHING to the key.

This is false. The KMS key policy generated by this pull request gives full access only to the root user.
This PR will generate a key policy similar to this:

KeyPolicy:
  Version: '2012-10-17'
  Statement:
    - Sid: Allow root user to manage key
      Effect: Allow
      Principal:
        AWS: !Sub 'arn:aws:iam::${AWS::AccountId}:root'
      Action: kms:*
      Resource: "*"

In fact, using such a default key policy is a practice recommended by the KMS documentation: https://docs.aws.amazon.com/kms/latest/developerguide/key-policies.html#key-policy-default-allow-root-enable-iam . Furthermore, if you create a KMS key manually from the console, and leave everything to default values, the exact same policy will be generated.

To verify my claim that such a key policy does not give access to every user/role in the current AWS account, you can deploy the following stack:

---
AWSTemplateFormatVersion: '2010-09-09'
Resources:
  MyKey:
    Type: AWS::KMS::Key
    Properties:
      KeyPolicy:
        Version: '2012-10-17'
        Statement:
          - Sid: Allow root user to manage key
            Effect: Allow
            Principal:
              AWS: !Sub 'arn:aws:iam::${AWS::AccountId}:root'
            Action: kms:*
            Resource: "*"

  UserWithoutAccess:
    Type: AWS::IAM::User

  UserWithAccess:
    Type: AWS::IAM::User
    Properties:
      Policies:
        - PolicyName: KMSaccess
          PolicyDocument:
            Version: '2012-10-17'
            Statement:
              - Effect: Allow
                Action:
                  - kms:Encrypt
                Resource:
                  - !GetAtt MyKey.Arn

Once this stack is deployed, create access keys for both users. Using credentials for UserWithoutAccess, access to the KMS key is denied:

AWS_ACCESS_KEY_ID=AKIA... AWS_SECRET_ACCESS_KEY=fIaAJ... aws kms encrypt --key-id 65b26f4f-e68b-4316-9b77-199884b70486 --plaintext hello --region us-east-1

An error occurred (AccessDeniedException) when calling the Encrypt operation: User: arn:aws:iam::4987XXXXXX:user/test-keys-UserWithoutAccess-10P1QO7VHONNM is not authorized to perform: kms:Encrypt on resource: arn:aws:kms:us-east-1:4987XXXXXX:key/65b26f4f-e68b-4316-9b77-199884b70486

However, using credentials for UserWithAccess, encryption is allowed:

 AWS_ACCESS_KEY_ID=AKIA... AWS_SECRET_ACCESS_KEY=HVExXE... aws kms encrypt --key-id 65b26f4f-e68b-4316-9b77-199884b70486 --plaintext hello --region us-east-1
{
    "CiphertextBlob": "AQICAHigUirQ9yveupk17dD2q0I+KKX8kSmHMVBkR0XEqo8GUQFNUjKLKq8MCf/BxM6KpbIHAAAAYzBhBgkqhkiG9w0BBwagVDBSAgEAME0GCSqGSIb3DQEHATAeBglghkgBZQMEAS4wEQQMc/QVs0qiaWyRhyTSAgEQgCAprgvs0N3Pr+eh3TSKcjfUB4XELhUHJA42q3yFps+lTQ==",
    "KeyId": "arn:aws:kms:us-east-1:4987XXXXXX:key/65b26f4f-e68b-4316-9b77-199884b70486"
}

@rix0rrr rix0rrr reopened this Apr 25, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 25, 2019

Hi SP,

Thanks for getting back to me. Apparently I misunderstood what the root account means in this context, but to be fair, I think its meaning deviates from other IAM policies? I will pass this on to KMS team and security to see what they have to say.

In any case, doesn't it make more sense for the queue.grantConsume() and queue.grantSend() methods to also add key permissions to the appropriate role, rather than this blanket kms:*?

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 25, 2019

By the way, I'm still confused; if your example is correct and UserWithoutAccess does NOT have permissions to the key given this policy... then how could it have made any difference to the user/role pulling from your encrypted queue?

@spg
Copy link
Contributor Author

spg commented Apr 27, 2019

Hey Rico,

I made a few experiments. Here are my conclusions.

In all cases, for the Lambda Function to be able to receive messages from the encrypted queue,
the Lambda Function's IAM Role needs to have the kms:Decrypt permission for the queue's KMS key:

const myFunction = new lambda.Function(this, 'MyFunction', {
  ...
  initialPolicy: [
    new iam.PolicyStatement()
      .addAction('kms:Decrypt')
      .addResource(queueKey.keyArn)
  ],
});

Assuming the function above, I have found only 2 solutions that allow my Lambda function to receive messages from the encrypted queue:

solution 1: Give all permissions to the root account on the key's policy (exactly what this PR does)

KeyPolicy:
  Version: '2012-10-17'
  Statement:
    - Sid: Allow root user to manage key
      Effect: Allow
      Principal:
        AWS: !Sub 'arn:aws:iam::${AWS::AccountId}:root'
      Action: kms:*
      Resource: "*"

solution 2: Add the 'kms:GenerateDataKey*', 'kms:Decrypt' actions to the key policy for the root user. Bear in mind that I have not found this solution anywhere in the SQS or KMS or Lambda documentation. Users might have a hard time figuring out this one by themselves:

const queueKey = new kms.EncryptionKey(this, 'QueueKey', {
  enableKeyRotation: true,
});

queueKey.addToResourcePolicy(
  new iam.PolicyStatement()
    .addAccountRootPrincipal()
    .addActions('kms:GenerateDataKey*', 'kms:Decrypt')
    .addAllResources()
);

Here are other failed attempts that did not allow my Lambda function to receive messages from the encrypted queue:

failed 1: Add the 'kms:GenerateDataKey*', 'kms:Decrypt' actions to the key policy for the lambda Service principal:

const queueKey = new kms.EncryptionKey(this, 'QueueKey', {
  enableKeyRotation: true,
});

queueKey.addToResourcePolicy(
  new iam.PolicyStatement()
    .addPrincipal(new iam.ServicePrincipal(`lambda.${cdk.Aws.urlSuffix}`))
    .addActions('kms:GenerateDataKey*', 'kms:Decrypt')
    .addAllResources()
);

failed 2: Giving all kms:* permissions on the Lambda function's IAM Role for the queue's KMS Key:

const myFunction = new lambda.Function(this, 'MyFunction', {
  ...
  initialPolicy: [
    new iam.PolicyStatement()
      .addAction('kms:*')
      .addResource(queueKey.keyArn)
  ],
});

I did not find any documentation on setting up a Lambda function to receive messages from an encrypted SQS queue. If you have ideas for other solutions please let me know.

@rix0rrr
Copy link
Contributor

rix0rrr commented May 6, 2019

I did not find any documentation on setting up a Lambda function to receive messages from an encrypted SQS queue. If you have ideas for other solutions please let me know.

I think it's going to boil down to kms:Decrypt permissions need to be given to the Lambda's role, BOTH on the key policy AND on the Lambda role policy.

Can you try making it so that the grantConsumeMessages() method on the queue will add both permissions?

@rix0rrr
Copy link
Contributor

rix0rrr commented May 31, 2019

Has your issue potentially been solved by #2659 ?

@spg
Copy link
Contributor Author

spg commented May 31, 2019

Yes, #2659 fixed it! Thanks a lot :)

@spg spg closed this May 31, 2019
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.

2 participants