From 1fdf1223304e15d905723553a40640b8bcb0ec56 Mon Sep 17 00:00:00 2001 From: Calvin Combs <66279577+comcalvi@users.noreply.github.com> Date: Mon, 4 Apr 2022 11:41:24 -0700 Subject: [PATCH] fix(logs): Faulty Resource Policy Generated (#19640) Closes #17544. Cloudwatch logs resource policies do not accept ARNs of any kind as principals. This PR adds logic to convert any ARN principals to account ID strings for resource policies and provides methods to do so if needed in other modules, even if those ARNs are encoded as tokens (for example, if using an imported value to retrieve an ARN principal). Shout-out to @rix0rrr for coauthoring this. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license --- .../@aws-cdk/aws-iam/lib/policy-statement.ts | 32 ++++++++++++++++ .../aws-lambda/test/singleton-lambda.test.ts | 10 ++--- packages/@aws-cdk/aws-logs/README.md | 4 ++ packages/@aws-cdk/aws-logs/lib/log-group.ts | 28 +++++++++++++- .../@aws-cdk/aws-logs/test/loggroup.test.ts | 38 +++++++++++++++++-- 5 files changed, 102 insertions(+), 10 deletions(-) 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();