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

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

Merged
merged 14 commits into from Mar 31, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 28 additions & 1 deletion packages/@aws-cdk/aws-lambda/lib/function-base.ts
@@ -1,7 +1,7 @@
import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import { ArnFormat, ConstructNode, IResource, Resource, Token } from '@aws-cdk/core';
import { Annotations, ArnFormat, ConstructNode, IResource, Resource, Token } from '@aws-cdk/core';
import { AliasOptions } from './alias';
import { Architecture } from './architecture';
import { EventInvokeConfig, EventInvokeConfigOptions } from './event-invoke-config';
Expand All @@ -12,6 +12,10 @@ import { CfnPermission } from './lambda.generated';
import { Permission } from './permission';
import { addAlias, flatMap } from './util';

// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
// eslint-disable-next-line no-duplicate-imports, import/order
import { Construct } from '@aws-cdk/core';

export interface IFunction extends IResource, ec2.IConnectable, iam.IGrantable {

/**
Expand Down Expand Up @@ -267,6 +271,22 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC
*/
protected _invocationGrants: Record<string, iam.Grant> = {};

/**
* Adds a warning to function permissions that include `lambda:InvokeFunction`.
* This should apply only to permissions on Lambda functions, not versions or aliases.
* This function is overridden as a noOp for QualifiedFunctionBase.
*/
public addWarningOnInvokeFunctionPermissions(scope: Construct, action: string) {
const affectedPermissions = ['lambda:InvokeFunction', 'lambda:*', 'lambda:Invoke*'];
if (affectedPermissions.includes(action)) {
Annotations.of(scope).addWarning([
"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.",
"If you are using a lambda Version or Alias, make sure to call 'grantInvoke' or 'addPermission' on the Version or Alias, not the underlying Function",
'See: https://github.com/aws/aws-cdk/issues/19273',
].join('\n'));
}
}

/**
* Adds a permission to the Lambda resource policy.
* @param id The id for the permission construct
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


new CfnPermission(scope, id, {
action,
principal,
Expand Down Expand Up @@ -537,6 +559,11 @@ export abstract class QualifiedFunctionBase extends FunctionBase {
...options,
});
}

public addWarningOnInvokeFunctionPermissions(_scope: Construct, _action: string): void {
// noOp
return;
}
}

/**
Expand Down
99 changes: 98 additions & 1 deletion packages/@aws-cdk/aws-lambda/test/function.test.ts
@@ -1,5 +1,5 @@
import * as path from 'path';
import { Match, Template } from '@aws-cdk/assertions';
import { Annotations, Match, Template } from '@aws-cdk/assertions';
import { ProfilingGroup } from '@aws-cdk/aws-codeguruprofiler';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as efs from '@aws-cdk/aws-efs';
Expand Down Expand Up @@ -435,6 +435,103 @@ describe('function', () => {
// THEN
Template.fromStack(stack).resourceCountIs('AWS::Lambda::Permission', 0);
});

describe('annotations on different IFunctions', () => {
let stack: cdk.Stack;
let fn: lambda.Function;
let warningMessage: string;
beforeEach(() => {
warningMessage = 'AWS Lambda has changed their authorization strategy';
stack = new cdk.Stack();
fn = new lambda.Function(stack, 'MyLambda', {
code: lambda.Code.fromAsset(path.join(__dirname, 'my-lambda-handler')),
handler: 'index.handler',
runtime: lambda.Runtime.PYTHON_3_6,
});
});

test('function', () => {
// WHEN
fn.addPermission('MyPermission', {
principal: new iam.ServicePrincipal('lambda.amazonaws.com'),
});

// THEN
Annotations.fromStack(stack).hasWarning('/Default/MyLambda', Match.stringLikeRegexp(warningMessage));
});

test('version', () => {
// GIVEN
const version = new lambda.Version(stack, 'MyVersion', {
lambda: fn,
});

// WHEN
version.addPermission('MyPermission', {
principal: new iam.ServicePrincipal('lambda.amazonaws.com'),
});

// THEN
Annotations.fromStack(stack).hasNoWarning('/Default/MyVersion', Match.stringLikeRegexp(warningMessage));
});

test('latest version', () => {
// WHEN
fn.latestVersion.addPermission('MyPermission', {
principal: new iam.ServicePrincipal('lambda.amazonaws.com'),
});

// THEN
// cannot add permissions on latest version, so no warning necessary
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Annotations.fromStack(stack).hasNoWarning('/Default/MyLambda/$LATEST', Match.stringLikeRegexp(warningMessage));
});

test('alias', () => {
// GIVEN
const version = new lambda.Version(stack, 'MyVersion', {
lambda: fn,
});
const alias = new lambda.Alias(stack, 'MyAlias', {
aliasName: 'alias',
version,
});

// WHEN
alias.addPermission('MyPermission', {
principal: new iam.ServicePrincipal('lambda.amazonaws.com'),
});

// THEN
Annotations.fromStack(stack).hasNoWarning('/Default/MyAlias', Match.stringLikeRegexp(warningMessage));
});

test('alias on latest version', () => {
// GIVEN
const alias = new lambda.Alias(stack, 'MyAlias', {
aliasName: 'alias',
version: fn.latestVersion,
});

// WHEN
alias.addPermission('MyPermission', {
principal: new iam.ServicePrincipal('lambda.amazonaws.com'),
});

// THEN
Annotations.fromStack(stack).hasNoWarning('/Default/MyAlias', Match.stringLikeRegexp(warningMessage));
});

test('function without lambda:InvokeFunction', () => {
// WHEN
fn.addPermission('MyPermission', {
action: 'lambda.GetFunction',
principal: new iam.ServicePrincipal('lambda.amazonaws.com'),
});

// THEN
Annotations.fromStack(stack).hasNoWarning('/Default/MyLambda', Match.stringLikeRegexp(warningMessage));
});
});
});

test('Lambda code can be read from a local directory via an asset', () => {
Expand Down