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

(lambda): Function.grant_invoke should not grant for all versions #20177

Open
rittneje opened this issue May 2, 2022 · 9 comments · May be fixed by #29856
Open

(lambda): Function.grant_invoke should not grant for all versions #20177

rittneje opened this issue May 2, 2022 · 9 comments · May be fixed by #29856
Labels
@aws-cdk/aws-lambda Related to AWS Lambda effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1

Comments

@rittneje
Copy link

rittneje commented May 2, 2022

Describe the bug

While upgrading from 1.140.0 to 2.20.0, we noticed that the behavior of aws_lambda.Function.grant_invoke has changed. Previously it granted lambda:InvokeFunction just on the function ARN itself. Now it also grants lambda:InvokeFunction on ${arn}:*, meaning all function versions.

This change does not make sense. At best, granting this wildcard permission is pointless, because CDK doesn't publish function versions. At worst, this is actually a potential security issue, because when I grant invoke rights to a principal, I mean for them to invoke it without any version number, which means $LATEST. If they have permission to invoke any version, then it is possible for them to invoke an older version that is not intended to be used anymore.

Expected Behavior

grant_invoke should retain its original behavior of only granting the ability to invoke the function itself, without an explicit version. If for some reason someone wants to grant wildcard access to all versions, there can be an optional boolean parameter to grant_invoke.

Current Behavior

See above.

Reproduction Steps

Create an aws_lambda.Function and use grant_invoke to grant invoke rights to some principal. Then look at the IAM policy that is generated.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.20.0

Framework Version

No response

Node.js Version

16.14.2

OS

Alpine 3.15

Language

Python

Language Version

3.9.12

Other information

No response

@rittneje rittneje added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 2, 2022
@github-actions github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label May 2, 2022
@kaizencc kaizencc changed the title (aws_lambda): Function.grant_invoke should not grant for all versions (lambda): Function.grant_invoke should not grant for all versions May 9, 2022
@kaizencc
Copy link
Contributor

Hi @rittneje! This is an intentional change. See this PR for additional context.

tl;dr: lambda changed their authorization strategy, the CDK updated to reflect this. The user behavior we want to see is to grantInvoke on specific versions and aliases instead.

@kaizencc kaizencc added guidance Question that needs advice or information. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels May 10, 2022
@rittneje
Copy link
Author

@kaizencc I'm confused. The new lambda behavior your are describing seems like exactly what CDK should do be default. Namely, grant_invoke should allow access to the unqualified ARN only, and they are not allowed to specify Qualifier. Consequently, they are forced to use $LATEST always, implicitly. With this CDK change, I don't see a way to generate such a policy via grant_invoke, and am now forced to write it manually all the time.

In short, CDK is kind of contradicting the entire point of the Lambda team's change here. If a user actually does intend to grant access to all versions/aliases, they should be forced to say so explicitly. It should not happen as part of grant_invoke by default.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 11, 2022
@kaizencc
Copy link
Contributor

Hi @rittneje! It boils down to not wanting to break our customers. Without change, customer lambda invocations would fail when the customer invokes the lambda with unqualified ARN and a Qualifier request parameter. We can't let this happen, so we have to change the policy to [ARN, ARN:*]. In our view, it's slightly more permissible but works for all use cases.

The clear direction we're taking here is that if you want to grant less permissible policies, grant them on the version or alias you've created, not the function itself. So, our suggestion for your use case is to create an alias on the latest version and then grant invoke permissions on the alias (instead of the function). You shouldn't have to write a manual policy for that.

@kaizencc kaizencc added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed bug This issue is a bug. labels May 12, 2022
@rittneje
Copy link
Author

rittneje commented May 14, 2022

@kaizencc Until this CDK change, I could use grant_invoke to grant lambda:InvokeFunction against the unqualified ARN, and then the application in question could use the unqualified ARN to invoke the lambda function. The Lambda team has now (rightfully) decided that allowing applications to invoke specific lambda function versions via a backdoor when the IAM role only granted access to the unqualified function was a mistake, and is correcting that mistake. The CDK team has decided to contradict that decision, under the guise of "backwards compatibility". However, the Lambda team decided breaking backwards compatibility was the correct approach. The CDK team absolutely should not be attempting to contradict this.

Furthermore, your suggestion to grant against the specific version/alias is not workable. If I grant access specifically to the $LATEST version (i.e., arn:aws:lambda:<region>:<account>:function:MyFunctionName:$LATEST), then the application in question is no longer permitted to invoke the unqualified ARN. So either I am forced to also make changes to the application, or I have to manually write a policy that grants access only to the unqualified ARN instead of using grant_invoke.

What the CDK team needs to do is add an optional boolean parameter to grant_invoke that indicates whether access should also be granted to all versions/aliases. (Alternatively, create a separate grant_invoke_all_versions method.) And if you are really concerned about backwards compatibility, add a feature flag to control the default behavior.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 14, 2022
@madeline-k
Copy link
Contributor

Hello @rittneje, I want to add some additional info and address some of your comments individually:

At worst, this is actually a potential security issue, because when I grant invoke rights to a principal, I mean for them to invoke it without any version number, which means $LATEST. If they have permission to invoke any version, then it is possible for them to invoke an older version that is not intended to be used anymore.

This is somewhat subjective. Without any bias towards the previous behavior, calling grantInvoke() on a lambda.Function Construct could easily be interpreted as it should give the principal invoke-access to that Construct and all of it's "subordinates" (Versions and Aliases).

The new lambda behavior your are describing seems like exactly what CDK should do be default. Namely, grant_invoke should allow access to the unqualified ARN only, and they are not allowed to specify Qualifier.

This is conflating the meaning of lambda.Function.grantInvoke() with the behavior of including { "Action": "lambda:InvokeFunction", "Resource": "unqualifiedFunctionARN" } in an IAM principal's policy. These two things do not necessarily need to be the same. Although, I understand that you are surprised by the change in behavior, since these two things used to have the same meaning.

With this CDK change, I don't see a way to generate such a policy via grant_invoke, and am now forced to write it manually all the time.

This is a fair callout. We don't want you to have to manually rewrite this all over your CDK application. You can either use myFunction.latestVersion.grantInvoke() to restrict access to the latest version. Or, you can of course create your own abstraction to encapsulate this and re-use it. I'd also suggest we change this issue to a feature request to add either a parameter to grantInvoke() which allows restricting the permissions to only the unqualified Function ARN, or introduce an additional API to do this such as grantInvokeUnqualifiedFunctionOnly().

The Lambda team has now (rightfully) decided that allowing applications to invoke specific lambda function versions via a backdoor when the IAM role only granted access to the unqualified function was a mistake, and is correcting that mistake. The CDK team has decided to contradict that decision, under the guise of "backwards compatibility". However, the Lambda team decided breaking backwards compatibility was the correct approach. The CDK team absolutely should not be attempting to contradict this.

The Lambda team actually put a lot of effort into introducing this new functionality without breaking any existing customers. They spent months on notification campaigns to inform customers and working directly with customers that were using the Qualifer parameter to invoke Lambda Functions with an IAM policy that only granted permission to the unqualified ARN. Some of these customers had used the CDK to provision the relevant IAM policies. We worked together to design this solution which would enable customers to upgrade their CDK version, re-deploy their infrastructure, and subsequently NOT experience a breaking change or a degradation in service for their customers when Lambda finally made the new authorization strategy live in all accounts. The goal of all of the aws-cdk L2 construct libraries is to provide opinionated defaults that make it easier for customers to use the AWS services, it is not necessarily to align exactly with the behavior of AWS Service APIs. Saying that the CDK is contradicting Lambda in this case, does not make much sense because Lambda does not have a concept of calling grantInvoke() on a Function. This is a functionality provided only by the CDK (today).

Furthermore, your suggestion to grant against the specific version/alias is not workable. If I grant access specifically to the $LATEST version (i.e., arn:aws:lambda:::function:MyFunctionName:$LATEST), then the application in question is no longer permitted to invoke the unqualified ARN. So either I am forced to also make changes to the application, or I have to manually write a policy that grants access only to the unqualified ARN instead of using grant_invoke.

Thanks for this explanation on why our recommended solution is not satisfactory for your use case. I propose we implement a new parameter or API to allow the old behavior.

What the CDK team needs to do is add an optional boolean parameter to grant_invoke that indicates whether access should also be granted to all versions/aliases. (Alternatively, create a separate grant_invoke_all_versions method.) And if you are really concerned about backwards compatibility, add a feature flag to control the default behavior.

I agree with you we can make a change to enable granting invoke access only on the unqualified ARN. But we cannot make this the default behavior. No feature flag should be necessary here, we can add a new parameter or API for this.

Thanks for your patience with this one, @rittneje! I understand this is frustrating, but unfortunuately we can't revert to the previous behavior.

@kaizencc kaizencc added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 18, 2022
@github-actions
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label May 19, 2022
@rittneje
Copy link
Author

rittneje commented May 22, 2022

Saying that the CDK is contradicting Lambda in this case, does not make much sense because Lambda does not have a concept of calling grantInvoke() on a Function. This is a functionality provided only by the CDK (today).

grantInvoke literally means to grant lambda:InvokeFunction to the principal in question. CDK has redefined the semantics in order to cater to users that were taking advantage of what essentially was a bug in AWS Lambda, instead of forcing those users to be explicit in their intent. Since you are already telling them to upgrade CDK, I don't see why you cannot also tell them to fix their CDK code.

I agree with you we can make a change to enable granting invoke access only on the unqualified ARN. But we cannot make this the default behavior. No feature flag should be necessary here, we can add a new parameter or API for this.

I strongly disagree. In our experience, it is extremely uncommon (read: it never happens) for someone to actually intend to grant access to invoke all versions/aliases of a function. And it is dangerous for CDK to redefine grantInvoke to mean this, because unsuspecting developers will call that method without realizing this. In such situations, CDK should always fall to the more restrictive interpretation, because if someone did actually mean to grant for all versions/aliases, they will find out when their code doesn't work. However, if someone did mean the more restrictive interpretation and CDK falls to the less restrictive one, then no one will know unless they manually inspect the policy, so the likelihood of a security issue is high.

Regardless, this issue should not be closed until CDK once again provides a mechanism to grant lambda:InvokeFunction strictly against the unqualified ARN.

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels May 22, 2022
@peterwoodworth peterwoodworth added p1 feature-request A feature should be added or improved. and removed guidance Question that needs advice or information. labels Jun 9, 2022
@kaizencc kaizencc removed their assignment Jul 20, 2022
@kaizencc kaizencc added the effort/small Small work item – less than a day of effort label Jul 20, 2022
@roger-zhangg
Copy link
Member

Update: Our current plan is to provide a new helper function grant_invoke_v2 to just grant permission for the latest lambda version. We will keep grant_invoke as is to avoid breaking changes.

@alexiswl
Copy link

alexiswl commented May 8, 2024

We've been able to work around this with:

// CDK Version 2.139.1 (build b88f959)
// Old: 
// lambda_obj.grantInvoke(<iam.Role>stateMachine.role)
// New:
lambda_obj.currentVersion.grantInvoke(<iam.Role>stateMachine.role)

@reisingerf @victorskl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda Related to AWS Lambda effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1
Projects
None yet
6 participants