fix(logs): Faulty Resource Policy Generated

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<IPrincipal>();
+
   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
    *

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

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;
+  }
 }
 
 /**

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/*, Intrinsic*/, 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();