Skip to content

Commit

Permalink
fix(lambda): grantInvoke fails for imported IAM identities (#9957)
Browse files Browse the repository at this point in the history
We used to erroneously assume that IAM identities imported into
the same Stack object (imported Roles specifically) would always
belong to the same account as the resources in the stack, and so
try to add `Invoke` permissions to the identity policy, which
would silently fail.

In a recent change, we started recognizing the account of the
Role properly and so now we detect that we must actually ALSO
add permission to the Lambda itself (resource policies).

Unfortunately the Lambda IAM-to-Lambda-Permissions translator had a list
of special recognized classes that did not include imported Roles, and
so this would fail.

Add another case where we try a more generic fallback by parsing
the policy principal. This should catch most simple principals
that Lambda Permissions supports.

Fixes #9883.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr committed Aug 28, 2020
1 parent 95c0332 commit d748f44
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 2 deletions.
27 changes: 26 additions & 1 deletion packages/@aws-cdk/aws-lambda/lib/function-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -346,12 +346,24 @@ export abstract class FunctionBase extends Resource implements IFunction {
return this.node;
}

/**
* Translate IPrincipal to something we can pass to AWS::Lambda::Permissions
*
* Do some nasty things because `Permission` supports a subset of what the
* full IAM principal language supports, and we may not be able to parse strings
* outright because they may be tokens.
*
* Try to recognize some specific Principal classes first, then try a generic
* fallback.
*/
private parsePermissionPrincipal(principal?: iam.IPrincipal) {
if (!principal) {
return undefined;
}
// use duck-typing, not instance of

// Try some specific common classes first.
// use duck-typing, not instance of
// @deprecated: after v2, we can change these to 'instanceof'
if ('accountId' in principal) {
return (principal as iam.AccountPrincipal).accountId;
}
Expand All @@ -364,6 +376,19 @@ export abstract class FunctionBase extends Resource implements IFunction {
return (principal as iam.ArnPrincipal).arn;
}

// Try a best-effort approach to support simple principals that are not any of the predefined
// classes, but are simple enough that they will fit into the Permission model. Main target
// here: imported Roles, Users, Groups.
//
// The principal cannot have conditions and must have a single { AWS: [arn] } entry.
const json = principal.policyFragment.principalJson;
if (Object.keys(principal.policyFragment.conditions).length === 0 && json.AWS) {
if (typeof json.AWS === 'string') { return json.AWS; }
if (Array.isArray(json.AWS) && json.AWS.length === 1 && typeof json.AWS[0] === 'string') {
return json.AWS[0];
}
}

throw new Error(`Invalid principal type for Lambda permission statement: ${principal.constructor.name}. ` +
'Supported: AccountPrincipal, ArnPrincipal, ServicePrincipal');
}
Expand Down
62 changes: 61 additions & 1 deletion packages/@aws-cdk/aws-lambda/test/test.lambda.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as path from 'path';
import { ABSENT, expect, haveResource, MatchStyle, ResourcePart } from '@aws-cdk/assert';
import { ABSENT, expect, haveResource, MatchStyle, ResourcePart, arrayWith, objectLike } from '@aws-cdk/assert';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import * as logs from '@aws-cdk/aws-logs';
Expand Down Expand Up @@ -1168,6 +1168,66 @@ export = {
},
},

'grantInvoke with an imported role (in the same account)'(test: Test) {
// GIVEN
const stack = new cdk.Stack(undefined, undefined, {
env: { account: '123456789012' },
});
const fn = new lambda.Function(stack, 'Function', {
code: lambda.Code.fromInline('xxx'),
handler: 'index.handler',
runtime: lambda.Runtime.NODEJS_10_X,
});

// WHEN
fn.grantInvoke(iam.Role.fromRoleArn(stack, 'ForeignRole', 'arn:aws:iam::123456789012:role/someRole'));

// THEN
expect(stack).to(haveResource('AWS::IAM::Policy', {
PolicyDocument: objectLike({
Statement: arrayWith(
{
Action: 'lambda:InvokeFunction',
Effect: 'Allow',
Resource: { 'Fn::GetAtt': ['Function76856677', 'Arn'] },
},
),
}),
Roles: ['someRole'],
}));

test.done();
},

'grantInvoke with an imported role (from a different account)'(test: Test) {
// GIVEN
const stack = new cdk.Stack(undefined, undefined, {
env: { account: '3333' },
});
const fn = new lambda.Function(stack, 'Function', {
code: lambda.Code.fromInline('xxx'),
handler: 'index.handler',
runtime: lambda.Runtime.NODEJS_10_X,
});

// WHEN
fn.grantInvoke(iam.Role.fromRoleArn(stack, 'ForeignRole', 'arn:aws:iam::123456789012:role/someRole'));

// THEN
expect(stack).to(haveResource('AWS::Lambda::Permission', {
Action: 'lambda:InvokeFunction',
FunctionName: {
'Fn::GetAtt': [
'Function76856677',
'Arn',
],
},
Principal: 'arn:aws:iam::123456789012:role/someRole',
}));

test.done();
},

'Can use metricErrors on a lambda Function'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
Expand Down

0 comments on commit d748f44

Please sign in to comment.