diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts index 5e3b7eaabfbc7..80ff191613e0e 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts @@ -70,6 +70,9 @@ export class PolicyStatement { private readonly condition: { [key: string]: any } = { }; private principalConditionsJson?: string; + // Hold on to those principals + private readonly _principals = new Array(); + constructor(props: PolicyStatementProps = {}) { // Validate actions for (const action of [...props.actions || [], ...props.notActions || []]) { @@ -145,6 +148,7 @@ export class PolicyStatement { * @param principals IAM principals that will be added */ public addPrincipals(...principals: IPrincipal[]) { + this._principals.push(...principals); if (Object.keys(principals).length > 0 && Object.keys(this.notPrincipal).length > 0) { throw new Error('Cannot add \'Principals\' to policy statement if \'NotPrincipals\' have been added'); } @@ -156,6 +160,15 @@ export class PolicyStatement { } } + /** + * Expose principals to allow their ARNs to be replaced by account ID strings + * in policy statements for resources policies that don't allow full account ARNs, + * such as AWS::Logs::ResourcePolicy. + */ + public get principals(): IPrincipal[] { + return [...this._principals]; + } + /** * Specify principals that is not allowed or denied access to the "NotPrincipal" section of * a policy statement. @@ -319,6 +332,25 @@ export class PolicyStatement { this.addCondition('StringEquals', { 'sts:ExternalId': accountId }); } + /** + * Create a new `PolicyStatement` with the same exact properties + * as this one, except for the overrides + */ + public copy(overrides: PolicyStatementProps = {}) { + return new PolicyStatement({ + sid: overrides.sid ?? this.sid, + effect: overrides.effect ?? this.effect, + actions: overrides.actions ?? this.action, + notActions: overrides.notActions ?? this.notAction, + + principals: overrides.principals, + notPrincipals: overrides.notPrincipals, + + resources: overrides.resources ?? this.resource, + notResources: overrides.notResources ?? this.notResource, + }); + } + /** * JSON-ify the policy statement * diff --git a/packages/@aws-cdk/aws-lambda/test/singleton-lambda.test.ts b/packages/@aws-cdk/aws-lambda/test/singleton-lambda.test.ts index 0f0a864a4c173..0f259e1866abd 100644 --- a/packages/@aws-cdk/aws-lambda/test/singleton-lambda.test.ts +++ b/packages/@aws-cdk/aws-lambda/test/singleton-lambda.test.ts @@ -172,17 +172,17 @@ describe('singleton lambda', () => { // WHEN const invokeResult = singleton.grantInvoke(new iam.ServicePrincipal('events.amazonaws.com')); - const statement = stack.resolve(invokeResult.resourceStatement); + const statement = stack.resolve(invokeResult.resourceStatement?.toJSON()); // THEN Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Permission', { Action: 'lambda:InvokeFunction', Principal: 'events.amazonaws.com', }); - expect(statement.action).toEqual(['lambda:InvokeFunction']); - expect(statement.principal).toEqual({ Service: ['events.amazonaws.com'] }); - expect(statement.effect).toEqual('Allow'); - expect(statement.resource).toEqual([ + expect(statement.Action).toEqual('lambda:InvokeFunction'); + expect(statement.Principal).toEqual({ Service: 'events.amazonaws.com' }); + expect(statement.Effect).toEqual('Allow'); + expect(statement.Resource).toEqual([ { 'Fn::GetAtt': ['SingletonLambda84c0de93353f42179b0b45b6c993251a840BCC38', 'Arn'] }, { 'Fn::Join': ['', [{ 'Fn::GetAtt': ['SingletonLambda84c0de93353f42179b0b45b6c993251a840BCC38', 'Arn'] }, ':*']] }, ]); diff --git a/packages/@aws-cdk/aws-logs/README.md b/packages/@aws-cdk/aws-logs/README.md index 804b5c56c4eb3..4e04059638138 100644 --- a/packages/@aws-cdk/aws-logs/README.md +++ b/packages/@aws-cdk/aws-logs/README.md @@ -69,6 +69,10 @@ const logGroup = new logs.LogGroup(this, 'LogGroup'); logGroup.grantWrite(new iam.ServicePrincipal('es.amazonaws.com')); ``` +Be aware that any ARNs or tokenized values passed to the resource policy will be converted into AWS Account IDs. +This is because CloudWatch Logs Resource Policies do not accept ARNs as principals, but they do accept +Account ID strings. Non-ARN principals, like Service principals or Any princpals, are accepted by CloudWatch. + ## Encrypting Log Groups By default, log group data is always encrypted in CloudWatch Logs. You have the diff --git a/packages/@aws-cdk/aws-logs/lib/log-group.ts b/packages/@aws-cdk/aws-logs/lib/log-group.ts index 9baf950f92f85..026f00092087a 100644 --- a/packages/@aws-cdk/aws-logs/lib/log-group.ts +++ b/packages/@aws-cdk/aws-logs/lib/log-group.ts @@ -1,7 +1,7 @@ import * as cloudwatch from '@aws-cdk/aws-cloudwatch'; import * as iam from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; -import { ArnFormat, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core'; +import { Arn, ArnFormat, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core'; import { Construct } from 'constructs'; import { LogStream } from './log-stream'; import { CfnLogGroup } from './logs.generated'; @@ -194,15 +194,39 @@ abstract class LogGroupBase extends Resource implements ILogGroup { /** * Adds a statement to the resource policy associated with this log group. * A resource policy will be automatically created upon the first call to `addToResourcePolicy`. + * + * Any ARN Principals inside of the statement will be converted into AWS Account ID strings + * because CloudWatch Logs Resource Policies do not accept ARN principals. + * * @param statement The policy statement to add */ public addToResourcePolicy(statement: iam.PolicyStatement): iam.AddToResourcePolicyResult { if (!this.policy) { this.policy = new ResourcePolicy(this, 'Policy'); } - this.policy.document.addStatements(statement); + this.policy.document.addStatements(statement.copy({ + principals: statement.principals.map(p => this.convertArnPrincpalToAccountId(p)), + })); return { statementAdded: true, policyDependable: this.policy }; } + + private convertArnPrincpalToAccountId(principal: iam.IPrincipal) { + if (principal.principalAccount) { + // we use ArnPrincipal here because the constructor inserts the argument + // into the template without mutating it, which means that there is no + // ARN created by this call. + return new iam.ArnPrincipal(principal.principalAccount); + } + + if (principal instanceof iam.ArnPrincipal) { + const parsedArn = Arn.split(principal.arn, ArnFormat.SLASH_RESOURCE_NAME); + if (parsedArn.account) { + return new iam.ArnPrincipal(parsedArn.account); + } + } + + return principal; + } } /** diff --git a/packages/@aws-cdk/aws-logs/test/loggroup.test.ts b/packages/@aws-cdk/aws-logs/test/loggroup.test.ts index 4fefc67272d5f..2ba10fdd38f86 100644 --- a/packages/@aws-cdk/aws-logs/test/loggroup.test.ts +++ b/packages/@aws-cdk/aws-logs/test/loggroup.test.ts @@ -1,7 +1,7 @@ import { Template } from '@aws-cdk/assertions'; import * as iam from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; -import { CfnParameter, RemovalPolicy, Stack } from '@aws-cdk/core'; +import { CfnParameter, Fn, RemovalPolicy, Stack } from '@aws-cdk/core'; import { LogGroup, RetentionDays } from '../lib'; describe('log group', () => { @@ -364,7 +364,7 @@ describe('log group', () => { }); }); - test('can add a policy to the log group', () => { + test('when added to log groups, IAM users are converted into account IDs in the resource policy', () => { // GIVEN const stack = new Stack(); const lg = new LogGroup(stack, 'LogGroup'); @@ -378,11 +378,43 @@ describe('log group', () => { // THEN Template.fromStack(stack).hasResourceProperties('AWS::Logs::ResourcePolicy', { - PolicyDocument: '{"Statement":[{"Action":"logs:PutLogEvents","Effect":"Allow","Principal":{"AWS":"arn:aws:iam::123456789012:user/user-name"},"Resource":"*"}],"Version":"2012-10-17"}', + PolicyDocument: '{"Statement":[{"Action":"logs:PutLogEvents","Effect":"Allow","Principal":{"AWS":"123456789012"},"Resource":"*"}],"Version":"2012-10-17"}', PolicyName: 'LogGroupPolicy643B329C', }); }); + test('imported values are treated as if they are ARNs and converted to account IDs via CFN pseudo parameters', () => { + // GIVEN + const stack = new Stack(); + const lg = new LogGroup(stack, 'LogGroup'); + + // WHEN + lg.addToResourcePolicy(new iam.PolicyStatement({ + resources: ['*'], + actions: ['logs:PutLogEvents'], + principals: [iam.Role.fromRoleArn(stack, 'Role', Fn.importValue('SomeRole'))], + })); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::Logs::ResourcePolicy', { + PolicyDocument: { + 'Fn::Join': [ + '', + [ + '{\"Statement\":[{\"Action\":\"logs:PutLogEvents\",\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"', + { + 'Fn::Select': [ + 4, + { 'Fn::Split': [':', { 'Fn::ImportValue': 'SomeRole' }] }, + ], + }, + '\"},\"Resource\":\"*\"}],\"Version\":\"2012-10-17\"}', + ], + ], + }, + }); + }); + test('correctly returns physical name of the log group', () => { // GIVEN const stack = new Stack();