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 all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion packages/@aws-cdk/aws-lambda/lib/function-base.ts
Original file line number Diff line number Diff line change
@@ -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 @@ -274,12 +278,45 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC

private _latestVersion?: LatestVersion;

/**
* Flag to delay adding a warning message until current version is invoked.
* @internal
*/
protected _warnIfCurrentVersionCalled: boolean = false;

/**
* Mapping of invocation principals to grants. Used to de-dupe `grantInvoke()` calls.
* @internal
*/
protected _invocationGrants: Record<string, iam.Grant> = {};

/**
* A warning will be added to functions under the following conditions:
* - permissions that include `lambda:InvokeFunction` are added to the unqualified function.
* - function.currentVersion is invoked before or after the permission is created.
*
* This applies only to permissions on Lambda functions, not versions or aliases.
* This function is overridden as a noOp for QualifiedFunctionBase.
*/
public considerWarningOnInvokeFunctionPermissions(scope: Construct, action: string) {
const affectedPermissions = ['lambda:InvokeFunction', 'lambda:*', 'lambda:Invoke*'];
if (affectedPermissions.includes(action)) {
if (scope.node.tryFindChild('CurrentVersion')) {
this.warnInvokeFunctionPermissions(scope);
} else {
this._warnIfCurrentVersionCalled = true;
}
}
}

protected warnInvokeFunctionPermissions(scope: Construct): void {
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 @@ -296,6 +333,8 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC
const action = permission.action ?? 'lambda:InvokeFunction';
const scope = permission.scope ?? this;

this.considerWarningOnInvokeFunctionPermissions(scope, action);

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

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

/**
Expand Down
4 changes: 4 additions & 0 deletions packages/@aws-cdk/aws-lambda/lib/function.ts
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,10 @@ export class Function extends FunctionBase {
return this._currentVersion;
}

if (this._warnIfCurrentVersionCalled) {
this.warnInvokeFunctionPermissions(this);
};

this._currentVersion = new Version(this, 'CurrentVersion', {
lambda: this,
...this.currentVersionOptions,
Expand Down
150 changes: 149 additions & 1 deletion packages/@aws-cdk/aws-lambda/test/function.test.ts
Original file line number Diff line number Diff line change
@@ -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,154 @@ 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,
});
});

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

// Simulate a workflow where a user has created a currentVersion with the intent to invoke it later.
fn.currentVersion;

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

describe('with lambda:InvokeFunction', () => {
test('without invoking currentVersion', () => {
// WHEN
fn.addPermission('MyPermission', {
principal: new iam.ServicePrincipal('lambda.amazonaws.com'),
});

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

test('with currentVersion invoked first', () => {
// GIVEN
// Simulate a workflow where a user has created a currentVersion with the intent to invoke it later.
fn.currentVersion;

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

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

test('with currentVersion invoked after permissions created', () => {
// WHEN
fn.addPermission('MyPermission', {
principal: new iam.ServicePrincipal('lambda.amazonaws.com'),
});

// Simulate a workflow where a user has created a currentVersion after adding permissions to the function.
fn.currentVersion;

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

test('multiple currentVersion calls does not result in multiple warnings', () => {
// WHEN
fn.currentVersion;

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

fn.currentVersion;

// THEN
const warns = Annotations.fromStack(stack).findWarning('/Default/MyLambda', Match.stringLikeRegexp(warningMessage));
expect(warns).toHaveLength(1);
});
});
});

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

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

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

test('permission on 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));
});

describe('permission on alias', () => {
test('of current version', () => {
// GIVEN
const version = new lambda.Version(stack, 'MyVersion', {
lambda: fn.currentVersion,
});
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('of 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('Lambda code can be read from a local directory via an asset', () => {
Expand Down