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
base: main
Are you sure you want to change the base?
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?
Examples:
|
Hi @godwingrs22 thanks for the review, I can see your point. For the naming, If we name the new function like |
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.
|
Hi @godwingrs22 should we consider supporting granting invoke to a certain version? Could be something like |
A good compromise might be to have both |
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. 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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Hi @godwingrs22 , could you please help to take a look when available, Thanks! |
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