-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(lambda): add grantInvokeLatestVersion to grant invoke only to latest function version #29856
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
46dba87
to
fc612e3
Compare
const hash = createHash('sha256') | ||
.update(JSON.stringify({ | ||
principal: grantee.grantPrincipal.toString(), | ||
conditions: grantee.grantPrincipal.policyFragment.conditions, | ||
onlyGrantLatestVersion: props.onlyGrantLatestVersion, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching between grantInvoke(grantee)
and grantInvoke(grantee, { onlyGrantLatestVersion: false })
would cause an unnecessary hash change
onlyGrantLatestVersion: props.onlyGrantLatestVersion, | |
onlyGrantLatestVersion: props.onlyGrantLatestVersion ?? false, |
@@ -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 })`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
**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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
grant = this.grant(grantee, identifier, 'lambda:InvokeFunction', this.resourceArnsForGrantInvoke); | ||
let resouceArns = this.resourceArnsForGrantInvoke; | ||
if (props.onlyGrantLatestVersion) { | ||
resouceArns = [this.functionArn]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to the comment above, the previous design doesn't expect a functionArn
with version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 AwsApiCall
s, or by adding an API Gateway to integrate the Lambdas and running HttpApiCall
s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
/** | ||
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this, I think you are right👍
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updating the existing parameter name identity
would it cause a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do we need to update the identity
parameter to grantee
as defined in the lambda grantInvoke
method?
@@ -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 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we able to assert that onlyGrantLatestVersion: true
grants only to the specific latest version and other versions should get an error on invocation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this should work. However let me get a second review within my team and get back to you on this integ test handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roger-zhangg , Seems the current integ test framework is designed for handling success cases. Hence we can write a success case and perform assertion if the right permission is set. This should be good enough to cover the integ test.
Examples:
|
Hi @godwingrs22 thanks for the review, I can see your point. For the naming, If we name the new function like |
Hi @godwingrs22 , could you please help to take a look when available, Thanks! |
😴 |
Hi @roger-zhangg , Apologies for the delay. I'll review within this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roger-zhangg Apologies for the delay in response. Thank you for making the changes. I've few followup comments left.
if (!grant) { | ||
let resouceArns = [`${this.functionArn}:${version.version}`]; | ||
if (version == this.latestVersion) { | ||
resouceArns = [this.functionArn, `${this.functionArn}:$LATEST`]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if i'm wrong. If it is a latest version, we are adding both specific version and $latest version function arns. In that case, can we do like this?
if (check for latest version) {
resouceArns.push(`${this.functionArn}:$LATEST`);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean resouceArns.push(this.functionArn);
Cause we need to enable the customer to invoke through unquantified Function Arn and through Arn:$LATEST
Effect: 'Allow', | ||
Resource: [ | ||
{ 'Fn::GetAtt': ['Function76856677', 'Arn'] }, | ||
{ 'Fn::Join': ['', [{ 'Fn::GetAtt': ['Function76856677', 'Arn'] }, ':$LATEST']] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would be able to call the latest lambda by also specfying the version right? In that case, can we assert for the role contains specific latest version also along with $LATEST?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not the goal. Imagine if we have a version 33, and it's currently the latest version. If we add a allow for Arn:33 here. And then after customer publishes Version 34, the allow on Arn:33 is still there. This violates our goal for only allow invoking on latest version
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
Will resolve comments this week |
Pull request has been modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed comments
if (!grant) { | ||
let resouceArns = [`${this.functionArn}:${version.version}`]; | ||
if (version == this.latestVersion) { | ||
resouceArns = [this.functionArn, `${this.functionArn}:$LATEST`]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean resouceArns.push(this.functionArn);
Cause we need to enable the customer to invoke through unquantified Function Arn and through Arn:$LATEST
Effect: 'Allow', | ||
Resource: [ | ||
{ 'Fn::GetAtt': ['Function76856677', 'Arn'] }, | ||
{ 'Fn::Join': ['', [{ 'Fn::GetAtt': ['Function76856677', 'Arn'] }, ':$LATEST']] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not the goal. Imagine if we have a version 33, and it's currently the latest version. If we add a allow for Arn:33 here. And then after customer publishes Version 34, the allow on Arn:33 is still there. This violates our goal for only allow invoking on latest version
Hi @godwingrs22 , could you help with a final review? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @roger-zhangg for your effort in implementing this feature.
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
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 arnExample:
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:Alternative design (to discuss)
setup a
grantInvokeProp
includinggrantVersionAccess
flag to pass in thegrantInvokeLatestVersion
instead usinggrantVersionAccess
flag directly ongrantInvokeLatestVersion
-> 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
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license