feat(lambda): add grantInvokeLatestVersion to grant invoke only to latest function version #29856

roger-zhangg · 2024-04-16T17:59:28Z

Issue # (if applicable)

Closes #20177

Reason for this change

fn.grantInvoke() will grant invoke permission to invoke both the latest version and all pervious version of the lambda function. We can see this behavior could bring some security concern for some of our customers.

Description of changes

We provides a new function fn.grantInvokeLatestVersion() to grant invoke only to the Latest version of function and the unqualified lambda arn

Example:

// Grant permissions to a service
declare const fn: lambda.Function;
const principal = new iam.ServicePrincipal('my-service');

fn.grantInvokeLatestVersion(principal);

Description of how you validated changes

Added unit tests and integration tests.
When using fn.grantInvokeLatestVersion() granted principle to invoke a function's past version, it will get the following error:

An error occurred (AccessDeniedException) when calling the Invoke operation: User: {$principle} is not authorized to perform: lambda:InvokeFunction on resource: {$LambdaArn:$version} because no identity-based policy allows the lambda:InvokeFunction action

Alternative design (to discuss)

setup a grantInvokeProp including grantVersionAccess flag to pass in the grantInvokeLatestVersion instead using grantVersionAccess flag directly on grantInvokeLatestVersion
-> This is discussed in the comments, I agree having props will have future extensibility but usually for grant methods specifically we haven't seen before. So we will not add prop to the new function grantInvokeLatestVersion

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

…o grant_invoke_v2

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

nmussy

A cleaner way to implement this feature would be to extend the current implementation of grantInvoke and to add a onlyGrantLatestVersion parameter with a default value. Using a Props object is also preferable for future extensibility:

interface GrantInvokeProps {
  onlyGrantLatestVersion?: boolean;
}
grantInvoke(grantee: iam.IGrantable, { onlyGrantLatestVersion = false }: GrantInvokeProps = {}): iam.Grant;

This would not cause a breaking change, given existing calls to grantInvoke would remain correct and unchanged.

EDIT: Sorry, I didn't notice you proposed the same solution at the end of your PR comment. I would definitely favor it

nmussy · 2024-04-18T03:11:41Z

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

    const hash = createHash('sha256')
      .update(JSON.stringify({
        principal: grantee.grantPrincipal.toString(),
        conditions: grantee.grantPrincipal.policyFragment.conditions,
+        onlyGrantLatestVersion: props.onlyGrantLatestVersion,


Switching between grantInvoke(grantee) and grantInvoke(grantee, { onlyGrantLatestVersion: false }) would cause an unnecessary hash change

Suggested change

onlyGrantLatestVersion: props.onlyGrantLatestVersion,

onlyGrantLatestVersion: props.onlyGrantLatestVersion ?? false,

nmussy · 2024-04-18T03:11:55Z

packages/aws-cdk-lib/aws-lambda/README.md

@@ -201,6 +201,15 @@ You can also restrict permissions given to AWS services by providing
 a source account or ARN (representing the account and identifier of the resource
 that accesses the function or layer).

+**Important**: By default `fn.grantInvoke()` grants permission to the principal to invoke any version of the function, including all past ones. If you only want the principal to invoke the latest version, use `grantInvoke(grantee, { onlyGrantLatestVersion:true })`.


Suggested change

**Important**: By default `fn.grantInvoke()` grants permission to the principal to invoke any version of the function, including all past ones. If you only want the principal to invoke the latest version, use `grantInvoke(grantee, { onlyGrantLatestVersion:true })`.

**Important**: By default `fn.grantInvoke()` grants permission to the principal to invoke any version of the function, including all past ones. If you only want the principal to be granted permission to invoke the latest version, use `grantInvoke(grantee, { onlyGrantLatestVersion: true })`.

You can also use a Markdown quote to increase the visibility, e.g. this note

> By default...

nmussy · 2024-04-18T03:20:28Z

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

Could you add unit tests with imported Lambda functions as well? I am especially anticipating issues with the following:

lambda.Function.fromFunctionArn( stack, 'unqualified', 'arn:aws:lambda:us-east-1:123456789012:function:my-function' ); lambda.Function.fromFunctionArn( stack, 'v1', 'arn:aws:lambda:us-east-1:123456789012:function:my-function:1' ); lambda.Function.fromFunctionArn( stack, 'latest', 'arn:aws:lambda:us-east-1:123456789012:function:my-function:$LATEST' );

If you onlyGrantLatestVersion for the first example, I would assume it would actually only grant the ARN version, which might not actually be the latest version

Thanks for the hint, and this is interesting. For the current design onlyGrantLatestVersion will grant permission to this.functionArn. While the previous design without onlyGrantLatestVersion grants to [this.functionArn, ${this.functionArn}:*]. I would assume if this.functionArn is something like arn:aws:lambda:us-east-1:123456789012:function:my-function:1 the previous method won't work either. I'll update once I checked this out.

Update: I tried out fromFunctionArn and seems CDK doesn't allow calling grantInvoke on a imported Lambda function

sample code

const func2 = lambda.Function.fromFunctionArn( this, 'v1', 'arn:aws:lambda:us-east-1:123456789012:function:my-function:1' ); const lambdaRole = new iam.Role(this, 'LambdaRole', { assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'), }); func2.grantInvoke(lambdaRole);

error

/local/home/ruojiazh/proj/aws-cdk/packages/aws-cdk-lib/aws-lambda/lib/function-base.ts:563 throw new Error('Cannot modify permission to lambda function. Function is either imported or $LATEST version.\n' ^ Error: Cannot modify permission to lambda function. Function is either imported or $LATEST version. If the function is imported from the same account use `fromFunctionAttributes()` API with the `sameEnvironment` flag. If the function is imported from a different account and already has the correct permissions use `fromFunctionAttributes()` API with the `skipPermissions` flag.

Using the suggested method fromFunctionAttributes

const func2 = lambda.Function.fromFunctionAttributes(this, 'Function', { functionArn: 'arn:aws:lambda:us-east-1:123456789012:function:my-function:1', sameEnvironment: true, }); const lambdaRole = new iam.Role(this, 'LambdaRole', { assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'), }); func2.grantInvoke(lambdaRole);

output

PolicyDocument: Statement: - Action: lambda:InvokeFunction Effect: Allow Resource: - arn:aws:lambda:us-east-1:123456789012:function:my-function:1 - arn:aws:lambda:us-east-1:123456789012:function:my-function:1:* Version: "2012-10-17"

You can see the output here is actually wrong, However because it still have allow on arn:aws:lambda:us-east-1:123456789012:function:my-function:1, this will allow the principle to invoke the given version. I assume CDK doesn't expect you to passin the function arn with a version in fromFunctionAttributes and CDK also didn't (or unable to) check if this ARN is valid or not. If we follow this design, we can also assume the Arn passed in shouldn't have a version.

nmussy · 2024-04-18T03:21:15Z

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

-      grant = this.grant(grantee, identifier, 'lambda:InvokeFunction', this.resourceArnsForGrantInvoke);
+      let resouceArns = this.resourceArnsForGrantInvoke;
+      if (props.onlyGrantLatestVersion) {
+        resouceArns = [this.functionArn];


Is functionArn always fully qualified? If it is, what happens if functionArn points to a specific version, e.g. my-function:1, but other versions were published since? You are no longer granting the latest version, just "a" version

See the comment on the unit tests file below

Refer to the comment above, the previous design doesn't expect a functionArn with version

nmussy · 2024-04-18T03:27:48Z

packages/@aws-cdk-testing/framework-integ/test/aws-lambda/test/integ.permissions.ts

The integration feels a bit lackluster here, we're only testing that the policy is synthetically correct, not that the grants work as expected

Adding integration assertions to check that the functions are invokable with the provided examples would be ideal, either with AwsApiCalls, or by adding an API Gateway to integrate the Lambdas and running HttpApiCalls

I do agree only testing synthetically is kind of wired, but doing AwsApiCalls seems to be testing against Lambda/IAM rather than testing CDK. So I just followed the previous test behavior

nmussy · 2024-04-18T03:30:23Z

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

+  /**
+   * Controls whether to grant invoke access to all function versions. Defaults to `false`.
+   *  - When set to `false`, both the function and functions with specific versions can be invoked.
+   *  - When set to `true`, only the function without a specific version (`$Latest`) can be invoked.


Is this accurate? I would imagine that at least functionname:$LATEST would be invokable, and probably even functionname:3, if 3 was the latest published version

Thanks for catching this, I think you are right👍

godwingrs22 · 2024-04-19T23:25:46Z

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

@@ -95,12 +95,12 @@ export interface IFunction extends IResource, ec2.IConnectable, iam.IGrantable {
  /**
   * Grant the given identity permissions to invoke this Lambda
   */
-  grantInvoke(identity: iam.IGrantable): iam.Grant;
+  grantInvoke(grantee: iam.IGrantable, props?: GrantInvokeProps): iam.Grant;


Seems like we add the props to the existing grantInvoke method which differs from the description of this PR. Are we not creating as new helper method grantInvokeV2 with the props without touching the existing method?

We provides a new function fn.grantInvokeV2() with a flag grantVersionAccess to controls whether to grant access to all function versions. Defaults to false.

If this is the propsed solution in updating the existing method grantInvoke with optional props, can we also update the PR description and update in the original issue?

updating the existing parameter name identity would it cause a breaking change?

I'm also confused on this part, this interface uses identity as param, however the implementation uses grantee. I'm not sure if we should just keep it as is or should we change one to match the other?

godwingrs22 · 2024-04-19T23:39:14Z

packages/aws-cdk-lib/aws-cloudfront/lib/experimental/edge-function.ts

-  public grantInvoke(identity: iam.IGrantable): iam.Grant {
-    return this.lambda.grantInvoke(identity);
+  public grantInvoke(identity: iam.IGrantable, props?: lambda.GrantInvokeProps): iam.Grant {
+    return this.lambda.grantInvoke(identity, props);


nit: do we need to update the identity parameter to grantee as defined in the lambda grantInvoke method?

godwingrs22 · 2024-04-19T23:56:07Z

packages/@aws-cdk-testing/framework-integ/test/aws-lambda/test/integ.permissions.ts

@@ -17,6 +17,10 @@ fn.grantInvoke(new iam.AnyPrincipal().inOrganization('o-yyyyyyyyyy'));

 fn.grantInvoke(new iam.OrganizationPrincipal('o-xxxxxxxxxx'));

+fn.grantInvoke(new iam.AnyPrincipal().inOrganization('o-yyyyyyyyyy2'), { onlyGrantLatestVersion: true });


Can we able to assert that onlyGrantLatestVersion: true grants only to the specific latest version and other versions should get an error on invocation?

Hi @godwingrs22 I'm looking into this but could you give me a headup on how to write integ test on this? I think I need to assume to the role I created here. But I didn't find how can I assume role in integration test. I checked invokeFucntion here and invokefunctionprop here but nothing related to assume role is found.

I had a inspiration on this from my teammate:

Setting up integration test

I can setup 2 lambda function - fn1, fn2. And a role in the integ-test.
fn1 should have multiple versions, and grants invoke lastest version to role
fn2 uses this role, and the handler of fn2 will invoke fn1 with the given version then return the result

running the integration test

In the integration test, we will only invoke fn2 by integ.assertions.invokeFunction(fn2, version). In this way we will know if the role attached to fn2 can invoke fn1 or not. That is:

When

fn1.grantInvokeLatestVersion(role)

Then
integ.assertions.invokeFunction(fn2, '$LATEST') should success
integ.assertions.invokeFunction(fn2, 1) should fail

When
fn1.grantInvokeVersion(role, 1)

Then
integ.assertions.invokeFunction(fn2, '$LATEST') should fail
integ.assertions.invokeFunction(fn2, 1) should success

What do you think?

godwingrs22 · 2024-04-22T21:50:50Z

Instead of adding props to the existing grantInvoke method can we create new method instead as stated in the description? Because I saw most of our grant methods in other modules accepts only one parameter grantee and doesn't take props as a param. Hence we propose to follow the similar approach.

Examples:
grantReadData(grantee) in TableV2 Construct of DynamoDB
grantManageConnections(identity) in WebSocketApi construct of apigateway

Also for naming the new method, we suggest not to have V2 in the method name and have something like grantInvokeLatestVersion() or grantInvokeOnlyLatestVersion()

roger-zhangg · 2024-04-22T22:14:05Z

Hi @godwingrs22 thanks for the review, I can see your point. For the naming, If we name the new function like grantInvokeLatestVersion() Then maybe we don't need to provide props to the new function. But this also eliminates possible extensibility. What do you think?

godwingrs22 · 2024-04-22T22:36:00Z

Hi @roger-zhangg, Yes with new method approach we don't need to provide props. I agree having props will have future extensibility but usually for grant methods specifically we haven't seen before.

grantInvokeLatestVersion(grantee: iam.IGrantable)

roger-zhangg · 2024-04-24T17:30:32Z

Hi @godwingrs22 should we consider supporting granting invoke to a certain version? Could be something like grantInvokeVersion(grantee: iam.IGrantable, version?: IVersion) and defaults to $LATEST

nmussy · 2024-04-24T18:18:53Z

A good compromise might be to have both grantInvokeLatestVersion(grantee: iam.IGrantable) and grantInvokeVersion(grantee: iam.IGrantable, version: IVersion), with the former calling the latter. We'd get both the more commonly used $LATEST grant ,and the flexibility of pinpointing a specific version

roger-zhangg · 2024-05-01T00:08:43Z

Hi @nmussy , @godwingrs22 I'm really grateful for your great and helpful ideas, I have updated and replied to some of the conversation above. Please help to review so I can proceed and merge it sooner. Thanks!

In my opinion, it's hard to simulate the whole scenario in the integration test.
1: as stated above, there's no standard way to assume role to invoke function in the current integration test
2: CDK/CFN can't deploy multiple versions in one deployments

Given these facts, I think it's reasonable to just test if the permission is correctly set, rather than actually invoking the Lambda function with version.

aws-cdk-automation · 2024-05-06T23:08:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 36ee9a9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

roger-zhangg · 2024-05-09T17:56:16Z

Hi @godwingrs22 , could you please help to take a look when available, Thanks!

add grantInvokeV2

4d90557

github-actions bot added repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 labels Apr 16, 2024

aws-cdk-automation requested a review from a team April 16, 2024 17:59

aws-cdk-automation previously requested changes Apr 16, 2024

View reviewed changes

roger-zhangg changed the title ~~(lambda): add grantInvokeV2 to grant for only latest version~~ feat(lambda): add grantInvokeV2 to grant invoke only to latest function version Apr 16, 2024

roger-zhangg added 4 commits April 16, 2024 21:33

add readme

a2ca2f2

Merge branch 'main' into grant_invoke_v2

287ccc5

Merge branch 'grant_invoke_v2' of github.com:roger-zhangg/aws-cdk int…

1052cf1

…o grant_invoke_v2

add integ test

aa9d4ca

roger-zhangg marked this pull request as ready for review April 16, 2024 22:25

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 16, 2024

nmussy suggested changes Apr 17, 2024

View reviewed changes

godwingrs22 self-requested a review April 17, 2024 17:49

godwingrs22 self-assigned this Apr 17, 2024

change to grantinvoke

fc612e3

roger-zhangg force-pushed the grant_invoke_v2 branch from 46dba87 to fc612e3 Compare April 18, 2024 00:22

roger-zhangg added 5 commits April 17, 2024 17:22

Merge branch 'main' into grant_invoke_v2

3aa8b6f

lint

679e6a6

typo

0857f65

add fn

34d1506

test

a502cd0

nmussy suggested changes Apr 18, 2024

View reviewed changes

godwingrs22 reviewed Apr 20, 2024

View reviewed changes

github-actions bot mentioned this pull request Apr 22, 2024

Weekly PR metrics report #29922

Closed

roger-zhangg changed the title ~~feat(lambda): add grantInvokeV2 to grant invoke only to latest function version~~ feat(lambda): add grantInvokeLatestVersion to grant invoke only to latest function version Apr 22, 2024

github-actions bot mentioned this pull request May 1, 2024

Monthly PR metrics report #30026

Closed

roger-zhangg added 8 commits May 3, 2024 01:29

add grantInvokeLatestVersion

5e5e6c9

add grantInvokeLatestVersion

c734032

Merge branch 'main' into grant_invoke_v2

74130bb

fix lint

f4b873f

fix integ

a7b2b36

Merge branch 'main' into grant_invoke_v2

cd2f33b

revert to original snapshot

2e60174

fix integ test

36ee9a9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lambda): add grantInvokeLatestVersion to grant invoke only to latest function version #29856

feat(lambda): add grantInvokeLatestVersion to grant invoke only to latest function version #29856

roger-zhangg commented Apr 16, 2024 •

edited

aws-cdk-automation left a comment

nmussy left a comment •

edited

nmussy Apr 18, 2024

nmussy Apr 18, 2024

nmussy Apr 18, 2024

roger-zhangg Apr 18, 2024

roger-zhangg Apr 30, 2024 •

edited

nmussy Apr 18, 2024

roger-zhangg Apr 30, 2024

nmussy Apr 18, 2024

roger-zhangg Apr 18, 2024

nmussy Apr 18, 2024

roger-zhangg Apr 18, 2024

godwingrs22 Apr 19, 2024

godwingrs22 Apr 19, 2024

godwingrs22 Apr 20, 2024

roger-zhangg May 2, 2024

godwingrs22 Apr 19, 2024

godwingrs22 Apr 19, 2024

roger-zhangg Apr 30, 2024

roger-zhangg Apr 30, 2024 •

edited

godwingrs22 commented Apr 22, 2024

roger-zhangg commented Apr 22, 2024 •

edited

godwingrs22 commented Apr 22, 2024

roger-zhangg commented Apr 24, 2024

nmussy commented Apr 24, 2024

roger-zhangg commented May 1, 2024 •

edited

aws-cdk-automation commented May 6, 2024

roger-zhangg commented May 9, 2024

	onlyGrantLatestVersion: props.onlyGrantLatestVersion,
	onlyGrantLatestVersion: props.onlyGrantLatestVersion ?? false,

	Important: By default `fn.grantInvoke()` grants permission to the principal to invoke any version of the function, including all past ones. If you only want the principal to invoke the latest version, use `grantInvoke(grantee, { onlyGrantLatestVersion:true })`.
	Important: By default `fn.grantInvoke()` grants permission to the principal to invoke any version of the function, including all past ones. If you only want the principal to be granted permission to invoke the latest version, use `grantInvoke(grantee, { onlyGrantLatestVersion: true })`.

		@@ -17,6 +17,10 @@ fn.grantInvoke(new iam.AnyPrincipal().inOrganization('o-yyyyyyyyyy'));

		fn.grantInvoke(new iam.OrganizationPrincipal('o-xxxxxxxxxx'));

		fn.grantInvoke(new iam.AnyPrincipal().inOrganization('o-yyyyyyyyyy2'), { onlyGrantLatestVersion: true });

feat(lambda): add grantInvokeLatestVersion to grant invoke only to latest function version #29856

Are you sure you want to change the base?

feat(lambda): add grantInvokeLatestVersion to grant invoke only to latest function version #29856

Conversation

roger-zhangg commented Apr 16, 2024 • edited

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Alternative design (to discuss)

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

nmussy left a comment • edited

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

roger-zhangg Apr 30, 2024 • edited

Choose a reason for hiding this comment

sample code

error

Using the suggested method fromFunctionAttributes

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

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

roger-zhangg Apr 30, 2024 • edited

Choose a reason for hiding this comment

Setting up integration test

running the integration test

godwingrs22 commented Apr 22, 2024

roger-zhangg commented Apr 22, 2024 • edited

godwingrs22 commented Apr 22, 2024

roger-zhangg commented Apr 24, 2024

nmussy commented Apr 24, 2024

roger-zhangg commented May 1, 2024 • edited

aws-cdk-automation commented May 6, 2024

AWS CodeBuild CI Report

roger-zhangg commented May 9, 2024

roger-zhangg commented Apr 16, 2024 •

edited

nmussy left a comment •

edited

roger-zhangg Apr 30, 2024 •

edited

Using the suggested method `fromFunctionAttributes`

roger-zhangg Apr 30, 2024 •

edited

roger-zhangg commented Apr 22, 2024 •

edited

roger-zhangg commented May 1, 2024 •

edited