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

Author: comcalvi

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-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'] }, ':*']] },
     ]);

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