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

Add KMS permission(s) that may be optionally needed #214

Open
KaanErturk opened this issue Sep 20, 2023 · 11 comments
Open

Add KMS permission(s) that may be optionally needed #214

KaanErturk opened this issue Sep 20, 2023 · 11 comments

Comments

@KaanErturk
Copy link

I had to create an IAM policy with kms:CreateGrant permission for the KMS key I was using with my Lambda functions, and then attach this manually to the solution's Lambda IAM roles.

It would be great to be able to optionally pass the KMS key ARN and add the necessary permission(s) as part of the solution.

@alexcasalboni
Copy link
Owner

Hi @KaanErturk 👋 thanks for reaching out!

Could you please share more about your use case? Why did the Lambda Power Tuning function need the kms:CreateGrant permission to use a KMS key? Or was it required by the function you were power-tuning?

@alexcasalboni
Copy link
Owner

@KaanErturk any updates? I'd like to address this, if you could share more details 😄

@KaanErturk
Copy link
Author

Hi @alexcasalboni , kms:CreateGrant permission was required by the state machine task that invoked my Lambda function(s), for the KMS key they were using (to encrypt environment variables).

I think an ideal solution would be to allow attaching an additional IAM policy to the state machine tasks' IAM role(s) via configuration at the deployment stage.

@alexcasalboni
Copy link
Owner

@KaanErturk thanks for the additional info!

I'm still struggling to understand the main reason/action for this issue though.

The required permission is for your own Lambda functions, right? (not for Lambda Power Tuning's functions)

What does Lambda Power Tuning have to do with the IAM policies of your Lambda function? Why would that kms:CreateGrant permission be required for Lambda Power Tuning's IAM roles too?

Is that a requirement for your AWS account(s)? Does the deployment of Lambda Power Tuning fail without that permission?

@KaanErturk
Copy link
Author

The required permission is for your solution's IAM roles, NOT mine.

I deployed your solution without an issue. When I ran it, it failed. When I checked the CloudTrail logs I saw the error about the missing permission. I added it to your Lambda functions' IAM roles (all of them but IIRC it was needed by the first function) and then it worked, hence my suggestion about users (us, not you) being able to attach additional (custom) IAM policies to your solution, so you don't have to add a particular permission everytime someone has a similar issue.

@alexcasalboni
Copy link
Owner

I see.

Indeed that would makes sense and add a lot of flexibility. Not sure if it's a best practice to implement IAM policies in the form of CloudFormation Parameters, as you probably want to include/validate/track those in proper IaC. The best (most secure) option for these use cases might still be to fork the repo and customize permissions in the YAML template.

And have you figured out why these permissions were required to run successfully? Is that required by your company's AWS account with something like AWS Config Rules?

@KaanErturk
Copy link
Author

I was thinking more of just attaching an IAM policy that I created as part of my infrastructure. You don't need to support creating that additional IAM policy, just be able to attach it.

I don't know why that permission was required by your Lambda functions. But it has nothing to do with AWS account setup or Config rules, etc. I just keep my KMS and IAM policies quite tight.

Have you tested your solution with Lambda functions that use KMS keys with tight policies? I shouldn't be the only one to use those, right? 🙂

@alexcasalboni
Copy link
Owner

alexcasalboni commented Jan 8, 2024

I admit I haven't tested Lambda Power Tuning with functions that use KMS keys, but so far I had assumed that whatever IAM policy the target function is using, it shouldn't affect who can invoke it (as long as the Lambda Power Tuning functions have the right permissions such as lambda:InvokeFunction, lambda:GetFunctionConfiguration, lambda:GetAlias, etc.).

Could you please provide a simple CloudFormation template with your function definition (and IAM/KMS policies) so I can deploy it and run a few tests?

@patmeiler
Copy link
Collaborator

Hi @KaanErturk , I'm about to investigate the issue, but I can't replicate it. Could you clarify, how I can replicate it?

  • You have a Lambda function that has an explicit Allow to fetch a specific KMS key
  • When invoking Power Tuning, the first Lambda function runs into a missing permissions error

@KaanErturk
Copy link
Author

I think I know why the extra KMS permission is needed to invoke the Lambda function.

First of all, this is the KMS key I'm talking about: https://docs.aws.amazon.com/lambda/latest/api/API_CreateFunction.html#lambda-CreateFunction-request-KMSKeyArn . I'm using it to encrypt the environment variables of my Lambda function. It's not fetched (!) or anything by the function. It's just part of its creation.

But, and I didn't mention this before, my Lambda function is using a container image. And the documentation above says If you deploy your function using a container image, Lambda also uses this key to encrypt your function when it's deployed. So I think that's the reason that the Lambda functon in this tool requires that KMS permission on my KMS key in order to be able to invoke my function.

As I said before, all that is needed is to be able to define (preferably a list of) IAM policy (attachments) so additional permissions like this can be added when needed, without bloating the tool.

If this still can't be replicated then maybe it's time to close this issue.

Thank you for your efforts so far.

@patmeiler
Copy link
Collaborator

Hi @KaanErturk , thank you so much for the explanation. This makes indeed a lot of sense to attach the specific policies, when the you deploy a container image. I will try to replicate the behavior and work on a fix.

I'll keep you updated 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants