feat(lambda): warn if you use `function.grantInvoke` while also using `currentVersion` #19464

kaizencc · 2022-03-18T16:48:49Z

‼️ Lambda is changing their authorization strategy, which means that some behavior that was previously valid now results in access-denied errors.

Under the new behavior, customer lambda invocations will fail if the CDK generates a policy with an unqualified ARN as the resource, and the customer invokes lambda with the unqualified ARN and the Qualifier request parameter.

Example of an affected setup:

Statement: 
{
  Effect: "Allow",
  Action: "lambda:InvokeFunction",
  Resource: "arn:aws:lambda:...:function:MyFunction",
}

API Call:
lambda.Invoke({
  FunctionName: "MyFunction",
  Qualifier: "1234",
})

This Invoke call used to succeed, but under the new authorization strategy it will fail. The required statement to make the call succeed would be (note the qualified ARN):

{
  Effect: "Allow",
  Action: "lambda:InvokeFunction",
  Resource: "arn:aws:lambda:...:function:MyFunction:1234",
}

This PR aims to warn users who could be using an affected setup. Users will receive the a warning message under the following circumstances:

they grant lambda:InvokeFunction to an unqualified function arn
they call lambda.currentVersion somewhere in their code

This is part of #19273. Related is #19318.

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

gitpod-io · 2022-03-18T16:48:56Z

kaizencc · 2022-03-18T17:22:55Z

PR failing for unrelated reason. Will fix malformed test in events in another PR.

Please approve #19465 to fix this build :).

kaizencc · 2022-03-18T17:25:27Z

packages/@aws-cdk/aws-lambda/test/function.test.ts

+        });
+
+        // THEN
+        // cannot add permissions on latest version, so no warning necessary


Do all of the Qualified functions actually use this base class? Looks like 'LatestVersion' does not.

LatestVersion is a special case that cannot have permissions added to it, since it is a dynamic qualifier. The typical use case is to alias the latest version, and then apply permissions to the alias. All other qualified functions use the QualifiedFunctionBase class.

The typical use case is to alias the latest version, and then apply permissions to the alias.

Is this actually something people do? What's the value over just adding permissions to the function?

Is this actually something people do? IDK, but I'm reading from the LatestVersion docstring that says:

/** * The $LATEST version of a function, useful when attempting to create aliases. */

At any rate you cannot add permissions to $LATEST.

…class (#19465) Also removes unnecessary assertion on number of messages, as this breaks when other messages are introduced. See #19464, which introduces a relevant message and breaks this test. Fixing in one other place as well. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

packages/@aws-cdk/aws-lambda/lib/function-base.ts

madeline-k · 2022-03-22T06:12:54Z

packages/@aws-cdk/aws-lambda/lib/function-base.ts

@@ -283,6 +290,14 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC
    const action = permission.action ?? 'lambda:InvokeFunction';
    const scope = permission.scope ?? this;

+    if (['lambda:InvokeFunction', 'lambda:*'].includes(action) && !qualified) {


Is the scope of this narrow enough? I am concerned we might end up giving unnecessary warnings to customers. But, I can't think of a way to narrow this down further (without crazy amounts of effort).

I think since lambda:* gives permission to lambda:InvokeFunction we have to give the warning since it's possible that these customers are broken by the new change. I think we should consider adding lambda:Invoke* as well to the list (even though there's no other lambda:InvokeXxx permission, which is why I didn't include it in the first place).

madeline-k · 2022-03-22T06:22:22Z

packages/@aws-cdk/aws-lambda/lib/function-base.ts

@@ -273,6 +273,13 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC
   * @param permission The permission to grant to this Lambda function. @see Permission for details.
   */
  public addPermission(id: string, permission: Permission) {
+    this.addPermissionHelper(id, permission, false);


Passing around boolean literals here makes it kindof hard to decipher what is going on. I think it would be more readable to include a protected function in the base class called something like addAnnotationsForPermissionsIfNeeded (sorry, awfully long name). The default implementation would include the Annotation. Then the Qualified Function can override it to do nothing. (This is basically the template pattern)

Great suggestion and thanks for the link to the pattern. I've updated the code to do this instead.

madeline-k · 2022-03-22T06:24:28Z

packages/@aws-cdk/aws-lambda/lib/function-base.ts

@@ -283,6 +290,14 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC
    const action = permission.action ?? 'lambda:InvokeFunction';
    const scope = permission.scope ?? this;

+    if (['lambda:InvokeFunction', 'lambda:*'].includes(action) && !qualified) {
+      Annotations.of(this).addWarning([
+        'AWS Lambda has changed their authorization strategy, which may cause client invocations of the lambda function to fail with Access Denied errors.',


Suggested change

'AWS Lambda has changed their authorization strategy, which may cause client invocations of the lambda function to fail with Access Denied errors.',

'AWS Lambda has changed their authorization strategy, which may cause client invocations using the `Qualifier` parameter of the lambda function to fail with Access Denied errors.',

This is clunky writing, but I think we need to mention that only clients using Qualifer will get the errors. I'd welcome better suggestions here.

Updating a test that requires metadata to be the first item returned in `node.metadataEntry`. This is not resilient because we could add additional metadata in the future that changes the order of `node.metadatEntry`. Instead, these tests should use assertions. I've found that we do this quite a bit elsewhere in the CDK. Because this test is actively blocking #19464, I'm including this as a separate PR to get it in and will fast-follow with other changes later. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

rix0rrr

It could be this PR or a different PR, but can you also @deprecate all qualifier parameters in integration classes that take Lambdas (with the message something like "pass a Version or Alias object as IFunction instead")? Examples:

aws-cdk/packages/@aws-cdk/aws-stepfunctions-tasks/lib/lambda/run-lambda-task.ts

Line 52 in 3c9ea5f

readonly qualifier?: string;

aws-cdk/packages/@aws-cdk/aws-stepfunctions-tasks/lib/lambda/invoke.ts

Line 49 in 3c9ea5f

readonly qualifier?: string;

aws-cdk/packages/@aws-cdk/aws-lambda/lib/event-invoke-config.ts

Line 61 in 3c9ea5f

readonly qualifier?: string;

rix0rrr · 2022-03-24T09:00:03Z

packages/@aws-cdk/aws-lambda/test/function.test.ts

+        });
+
+        // THEN
+        // cannot add permissions on latest version, so no warning necessary


The typical use case is to alias the latest version, and then apply permissions to the alias.

Is this actually something people do? What's the value over just adding permissions to the function?

rix0rrr · 2022-03-24T09:02:48Z

packages/@aws-cdk/aws-lambda/lib/function-base.ts

@@ -283,6 +303,8 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC
    const action = permission.action ?? 'lambda:InvokeFunction';
    const scope = permission.scope ?? this;

+    this.addWarningOnInvokeFunctionPermissions(scope, action);


This looks like it will add the warning always, even if you're not using aliases or versions at all. Do I have that right?

If so, that is probably overly spammy. I'd prefer adding the warning if we detect the following:

grantInvoke is called on the function; AND

currentVersion is called/has been called

If combine those, you're probably doing something wrong.

I am happy to do this, but what about the scenarios that this will miss? I'm thinking:

const fn = new lambda.Function(); const version = new lambda.Version(this, 'v', { lambda: fn, }); fn.grantInvoke();

Shoudn't we warn users who do this that they should grantInvoke on the version?

To minimize the amount of support requests, I'd prefer to err on the side of caution: tell people when they're definitely doing something wrong, rather than when they might be doing something wrong.

So: I don't want false positives. We can talk about how to reduce false negatives.

There might be ways, we might care to invest in them (we might also not).

Okay! I think I've done exactly as you've described. Moved this from reducing false negatives to reducing false positives.

There is an addVersion() on Functions that is @deprecated in favor of currentVersion so I decided not to warn those users. I think @deprecated is signal enough that you should not be using it. But open to feedback here.

‼️ Lambda is changing their authorization strategy. Under this new behavior customer lambda invocations will fail in this scenario: - the invocation is requested using an IAM Permission with an unqualified ARN as the FunctionName - the invocation is requested with an unqualified ARN and a Qualifier parameter The idea is to steer away from invoking lambdas with a Qualifier request parameter altogether, hence the deprecations. Instead, customers should be requesting permissions on qualified ARNs (versions and aliases) if they want to invoke versions/aliases. See #19464. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Addressed

mergify · 2022-03-31T12:46:46Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2022-03-31T13:29:58Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-03-31T13:30:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 0fc25bd
Result: SUCCEEDED
Build Logs (available for 30 days)

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

Updating a test that requires metadata to be the first item returned in `node.metadataEntry`. This is not resilient because we could add additional metadata in the future that changes the order of `node.metadatEntry`. Instead, these tests should use assertions. I've found that we do this quite a bit elsewhere in the CDK. Because this test is actively blocking aws#19464, I'm including this as a separate PR to get it in and will fast-follow with other changes later. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

‼️ Lambda is changing their authorization strategy. Under this new behavior customer lambda invocations will fail in this scenario: - the invocation is requested using an IAM Permission with an unqualified ARN as the FunctionName - the invocation is requested with an unqualified ARN and a Qualifier parameter The idea is to steer away from invoking lambdas with a Qualifier request parameter altogether, hence the deprecations. Instead, customers should be requesting permissions on qualified ARNs (versions and aliases) if they want to invoke versions/aliases. See aws#19464. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

… `currentVersion` (aws#19464) ‼️ Lambda is changing their authorization strategy, which means that some behavior that was previously valid now results in `access-denied` errors. Under the new behavior, customer lambda invocations will fail if the CDK generates a policy with an unqualified ARN as the resource, and the customer invokes lambda with the unqualified ARN and the `Qualifier` request parameter. Example of an affected setup: ``` Statement: { Effect: "Allow", Action: "lambda:InvokeFunction", Resource: "arn:aws:lambda:...:function:MyFunction", } API Call: lambda.Invoke({ FunctionName: "MyFunction", Qualifier: "1234", }) ``` This `Invoke` call *used* to succeed, but under the new authorization strategy it will fail. The required statement to make the call succeed would be (note the qualified ARN): ``` { Effect: "Allow", Action: "lambda:InvokeFunction", Resource: "arn:aws:lambda:...:function:MyFunction:1234", } ``` This PR aims to warn users who could be using an affected setup. Users will receive the a warning message under the following circumstances: - they grant `lambda:InvokeFunction` to an unqualified function arn - they call `lambda.currentVersion` somewhere in their code This is part of aws#19273. Related is aws#19318. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

kaizencc and others added 2 commits March 18, 2022 12:18

chnages to add permission

cafdc6b

Merge branch 'master' into conroy/update-lambda-resource-policies

d6c183f

github-actions bot added the @aws-cdk/aws-lambda Related to AWS Lambda label Mar 18, 2022

github-actions bot assigned kaizencc Mar 18, 2022

mergify bot added the contribution/core This is a PR that came from AWS. label Mar 18, 2022

kaizencc added the pr-linter/exempt-readme The PR linter will not require README changes label Mar 18, 2022

kaizencc added 2 commits March 18, 2022 13:07

test latest version

e6b4610

test latest version

50bbab1

kaizencc changed the title ~~feat(lambda): support Lambda's new IAM authorization behavior for resource polices~~ feat(lambda): warn relevant users of Lambda's new IAM authorization behavior for resource polices Mar 18, 2022

kaizencc commented Mar 18, 2022

View reviewed changes

kaizencc requested review from rix0rrr, madeline-k and a team March 18, 2022 17:25

kaizencc mentioned this pull request Mar 18, 2022

chore(events-targets): migrate a test to use assertions' annotations class #19465

Merged

madeline-k previously requested changes Mar 22, 2022

View reviewed changes

kaizencc and others added 2 commits March 22, 2022 10:13

Merge branch 'master' into conroy/update-lambda-resource-policies

ef23549

use template pattern for overriding base class functions

7204adc

kaizencc added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Mar 22, 2022

kaizencc mentioned this pull request Mar 23, 2022

chore(events-targets): update lambda annotation test #19527

Merged

4 tasks

Merge branch 'master' into conroy/update-lambda-resource-policies

be42368

kaizencc requested a review from madeline-k March 24, 2022 01:02

rix0rrr reviewed Mar 24, 2022

View reviewed changes

This was referenced Mar 24, 2022

many tests targeting annotations are brittle #19544

Closed

chore: deprecate qualifier parameter for lambda integrations #19548

Merged

kaizencc added 2 commits March 24, 2022 19:24

only add warning when currentVersion invoked

1b7f94b

update documentation

3c250f3

kaizencc and others added 2 commits March 24, 2022 19:29

remove duplicate test

3b03c84

Merge branch 'master' into conroy/update-lambda-resource-policies

bae6a87

kaizencc requested a review from rix0rrr March 24, 2022 23:33

github-actions bot added the p2 label Mar 24, 2022

rix0rrr approved these changes Mar 25, 2022

View reviewed changes

rix0rrr changed the title ~~feat(lambda): warn relevant users of Lambda's new IAM authorization behavior for resource polices~~ feat(lambda): warn if you call function.grantInvoke while also calling currentVersion Mar 25, 2022

rix0rrr changed the title ~~feat(lambda): warn if you call function.grantInvoke while also calling currentVersion~~ feat(lambda): warn if you use function.grantInvoke while also using currentVersion Mar 25, 2022

Merge branch 'master' into conroy/update-lambda-resource-policies

465279d

rix0rrr approved these changes Mar 25, 2022

View reviewed changes

Merge branch 'master' into conroy/update-lambda-resource-policies

f974469

Merge branch 'master' into conroy/update-lambda-resource-policies

0fc25bd

mergify bot merged commit fd1fff9 into master Mar 31, 2022

mergify bot deleted the conroy/update-lambda-resource-policies branch March 31, 2022 13:29

kaizencc mentioned this pull request Mar 31, 2022

(lambda): CDK-generated permissions will not work on Lambda invoked with Qualifier= #19273

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lambda): warn if you use `function.grantInvoke` while also using `currentVersion` #19464

feat(lambda): warn if you use `function.grantInvoke` while also using `currentVersion` #19464

kaizencc commented Mar 18, 2022 •

edited

Loading

gitpod-io bot commented Mar 18, 2022

kaizencc commented Mar 18, 2022 •

edited

Loading

kaizencc Mar 18, 2022

rix0rrr Mar 24, 2022

kaizencc Mar 24, 2022

madeline-k Mar 22, 2022

kaizencc Mar 22, 2022

madeline-k Mar 22, 2022

kaizencc Mar 22, 2022

madeline-k Mar 22, 2022

rix0rrr left a comment

rix0rrr Mar 24, 2022

rix0rrr Mar 24, 2022

kaizencc Mar 24, 2022

rix0rrr Mar 24, 2022

rix0rrr Mar 24, 2022

kaizencc Mar 24, 2022

mergify bot commented Mar 31, 2022

mergify bot commented Mar 31, 2022

aws-cdk-automation commented Mar 31, 2022

	'AWS Lambda has changed their authorization strategy, which may cause client invocations of the lambda function to fail with Access Denied errors.',
	'AWS Lambda has changed their authorization strategy, which may cause client invocations using the `Qualifier` parameter of the lambda function to fail with Access Denied errors.',

feat(lambda): warn if you use function.grantInvoke while also using currentVersion #19464

feat(lambda): warn if you use function.grantInvoke while also using currentVersion #19464

Conversation

kaizencc commented Mar 18, 2022 • edited Loading

gitpod-io bot commented Mar 18, 2022

kaizencc commented Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Mar 31, 2022

mergify bot commented Mar 31, 2022

aws-cdk-automation commented Mar 31, 2022

AWS CodeBuild CI Report

feat(lambda): warn if you use `function.grantInvoke` while also using `currentVersion` #19464

feat(lambda): warn if you use `function.grantInvoke` while also using `currentVersion` #19464

kaizencc commented Mar 18, 2022 •

edited

Loading

kaizencc commented Mar 18, 2022 •

edited

Loading