Repository: awslabs/cdk-serverless-clamscan (language: python)

Issue:
feat: support use of existing buckets as sources
Fix #451 

Passing an existing bucket to the construct is now supported, but the user must explicitly set the `acceptResponsibilityForUsingExistingBucket` or an error is thrown. When using an existing bucket, the bucket policy is not added automatically, but the relevant statement can be retrieved with `getPolicyStatementForBucket(bucket)` and the user is responsible to add that statement to the bucket. That can be done in different ways, but choice and responsibility is left to the user. For example a `CfnOutput` containing the policy statement can be created to expose the statement, or the policy can be explicitly replaced in the CDK project.

Edit: push-forced to fix invalid commit name

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*


Discussion / hints:

Fix patch:
diff --git a/API.md b/API.md
index a95577a3..7d145fbd 100644
--- a/API.md
+++ b/API.md
@@ -64,7 +64,8 @@ new ServerlessClamscan(scope: Construct, id: string, props: ServerlessClamscanPr
 * **scope** (<code>[Construct](#constructs-construct)</code>)  The parent creating construct (usually `this`).
 * **id** (<code>string</code>)  The construct's name.
 * **props** (<code>[ServerlessClamscanProps](#cdk-serverless-clamscan-serverlessclamscanprops)</code>)  A `ServerlessClamscanProps` interface.
-  * **buckets** (<code>Array<[aws_s3.Bucket](#aws-cdk-lib-aws-s3-bucket)></code>)  An optional list of S3 buckets to configure for ClamAV Virus Scanning; __*Optional*__
+  * **acceptResponsibilityForUsingImportedBucket** (<code>boolean</code>)  Allows the use of imported buckets. __*Optional*__
+  * **buckets** (<code>Array<[aws_s3.IBucket](#aws-cdk-lib-aws-s3-ibucket)></code>)  An optional list of S3 buckets to configure for ClamAV Virus Scanning; __*Optional*__
   * **defsBucketAccessLogsConfig** (<code>[ServerlessClamscanLoggingProps](#cdk-serverless-clamscan-serverlessclamscanloggingprops)</code>)  Whether or not to enable Access Logging for the Virus Definitions bucket, you can specify an existing bucket and prefix (Default: Creates a new S3 Bucket for access logs ). __*Optional*__
   * **efsEncryption** (<code>boolean</code>)  Whether or not to enable encryption on EFS filesystem (Default: enabled). __*Optional*__
   * **onError** (<code>[aws_lambda.IDestination](#aws-cdk-lib-aws-lambda-idestination)</code>)  The Lambda Destination for files that fail to scan and are marked 'ERROR' or stuck 'IN PROGRESS' due to a Lambda timeout (Default: Creates and publishes to a new SQS queue if unspecified). __*Optional*__
@@ -80,12 +81,14 @@ Name | Type | Description
 -----|------|-------------
 **errorDest** | <code>[aws_lambda.IDestination](#aws-cdk-lib-aws-lambda-idestination)</code> | The Lambda Destination for failed on erred scans [ERROR, IN PROGRESS (If error is due to Lambda timeout)].
 **resultDest** | <code>[aws_lambda.IDestination](#aws-cdk-lib-aws-lambda-idestination)</code> | The Lambda Destination for completed ClamAV scans [CLEAN, INFECTED].
+**scanAssumedPrincipal** | <code>[aws_iam.ArnPrincipal](#aws-cdk-lib-aws-iam-arnprincipal)</code> | <span></span>
 **cleanRule**? | <code>[aws_events.Rule](#aws-cdk-lib-aws-events-rule)</code> | Conditional: An Event Bridge Rule for files that are marked 'CLEAN' by ClamAV if a success destination was not specified.<br/>__*Optional*__
 **defsAccessLogsBucket**? | <code>[aws_s3.IBucket](#aws-cdk-lib-aws-s3-ibucket)</code> | Conditional: The Bucket for access logs for the virus definitions bucket if logging is enabled (defsBucketAccessLogsConfig).<br/>__*Optional*__
 **errorDeadLetterQueue**? | <code>[aws_sqs.Queue](#aws-cdk-lib-aws-sqs-queue)</code> | Conditional: The SQS Dead Letter Queue for the errorQueue if a failure (onError) destination was not specified.<br/>__*Optional*__
 **errorQueue**? | <code>[aws_sqs.Queue](#aws-cdk-lib-aws-sqs-queue)</code> | Conditional: The SQS Queue for erred scans if a failure (onError) destination was not specified.<br/>__*Optional*__
 **infectedRule**? | <code>[aws_events.Rule](#aws-cdk-lib-aws-events-rule)</code> | Conditional: An Event Bridge Rule for files that are marked 'INFECTED' by ClamAV if a success destination was not specified.<br/>__*Optional*__
 **resultBus**? | <code>[aws_events.EventBus](#aws-cdk-lib-aws-events-eventbus)</code> | Conditional: The Event Bridge Bus for completed ClamAV scans if a success (onResult) destination was not specified.<br/>__*Optional*__
+**useImportedBuckets**? | <code>boolean</code> | Conditional: When true, the user accepted the responsibility for using imported buckets.<br/>__*Optional*__
 
 ### Methods
 
@@ -98,14 +101,27 @@ Grants the ClamAV function permissions to get and tag objects.
 Adds a bucket policy to disallow GetObject operations on files that are tagged 'IN PROGRESS', 'INFECTED', or 'ERROR'.
 
 ```ts
-addSourceBucket(bucket: Bucket): void
+addSourceBucket(bucket: IBucket): void
 ```
 
-* **bucket** (<code>[aws_s3.Bucket](#aws-cdk-lib-aws-s3-bucket)</code>)  The bucket to add the scanning bucket policy and s3:ObjectCreate* trigger to.
+* **bucket** (<code>[aws_s3.IBucket](#aws-cdk-lib-aws-s3-ibucket)</code>)  The bucket to add the scanning bucket policy and s3:ObjectCreate* trigger to.
 
 
 
 
+#### getPolicyStatementForBucket(bucket) <a id="cdk-serverless-clamscan-serverlessclamscan-getpolicystatementforbucket"></a>
+
+Returns the statement that should be added to the bucket policy in order to prevent objects to be accessed when they are not clean or there have been scanning errors: this policy should be added manually if external buckets are passed to addSourceBucket().
+
+```ts
+getPolicyStatementForBucket(bucket: IBucket): PolicyStatement
+```
+
+* **bucket** (<code>[aws_s3.IBucket](#aws-cdk-lib-aws-s3-ibucket)</code>)  The bucket which you need to protect with the policy.
+
+__Returns__:
+* <code>[aws_iam.PolicyStatement](#aws-cdk-lib-aws-iam-policystatement)</code>
+
 
 
 ## struct ServerlessClamscanLoggingProps  <a id="cdk-serverless-clamscan-serverlessclamscanloggingprops"></a>
@@ -131,7 +147,8 @@ Interface for creating a ServerlessClamscan.
 
 Name | Type | Description 
 -----|------|-------------
-**buckets**? | <code>Array<[aws_s3.Bucket](#aws-cdk-lib-aws-s3-bucket)></code> | An optional list of S3 buckets to configure for ClamAV Virus Scanning;<br/>__*Optional*__
+**acceptResponsibilityForUsingImportedBucket**? | <code>boolean</code> | Allows the use of imported buckets.<br/>__*Optional*__
+**buckets**? | <code>Array<[aws_s3.IBucket](#aws-cdk-lib-aws-s3-ibucket)></code> | An optional list of S3 buckets to configure for ClamAV Virus Scanning;<br/>__*Optional*__
 **defsBucketAccessLogsConfig**? | <code>[ServerlessClamscanLoggingProps](#cdk-serverless-clamscan-serverlessclamscanloggingprops)</code> | Whether or not to enable Access Logging for the Virus Definitions bucket, you can specify an existing bucket and prefix (Default: Creates a new S3 Bucket for access logs ).<br/>__*Optional*__
 **efsEncryption**? | <code>boolean</code> | Whether or not to enable encryption on EFS filesystem (Default: enabled).<br/>__*Optional*__
 **onError**? | <code>[aws_lambda.IDestination](#aws-cdk-lib-aws-lambda-idestination)</code> | The Lambda Destination for files that fail to scan and are marked 'ERROR' or stuck 'IN PROGRESS' due to a Lambda timeout (Default: Creates and publishes to a new SQS queue if unspecified).<br/>__*Optional*__
diff --git a/src/index.ts b/src/index.ts
index f6890cd9..3549c670 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -25,6 +25,7 @@ import {
   ArnPrincipal,
   AnyPrincipal,
   AccountRootPrincipal,
+  AddToResourcePolicyResult,
 } from 'aws-cdk-lib/aws-iam';
 import {
   DockerImageCode,
@@ -39,8 +40,8 @@ import {
   EventBridgeDestination,
   SqsDestination,
 } from 'aws-cdk-lib/aws-lambda-destinations';
-import { S3EventSource } from 'aws-cdk-lib/aws-lambda-event-sources';
 import { IBucket, Bucket, BucketEncryption, EventType } from 'aws-cdk-lib/aws-s3';
+import { LambdaDestination } from 'aws-cdk-lib/aws-s3-notifications';
 import { Queue, QueueEncryption } from 'aws-cdk-lib/aws-sqs';
 import { NagSuppressions } from 'cdk-nag';
 import { Construct } from 'constructs';
@@ -66,7 +67,7 @@ export interface ServerlessClamscanProps {
   /**
    * An optional list of S3 buckets to configure for ClamAV Virus Scanning; buckets can be added later by calling addSourceBucket.
    */
-  readonly buckets?: Bucket[];
+  readonly buckets?: IBucket[];
   /**
    * Optionally set a reserved concurrency for the virus scanning Lambda.
    * @see https://docs.aws.amazon.com/lambda/latest/operatorguide/reserved-concurrency.html
@@ -88,6 +89,11 @@ export interface ServerlessClamscanProps {
    * Whether or not to enable Access Logging for the Virus Definitions bucket, you can specify an existing bucket and prefix (Default: Creates a new S3 Bucket for access logs ).
    */
   readonly defsBucketAccessLogsConfig?: ServerlessClamscanLoggingProps;
+
+  /**
+   * Allows the use of imported buckets. When using imported buckets the user is responsible for adding the required policy statement to the bucket policy: `getPolicyStatementForBucket()` can be used to retrieve the policy statement required by the solution.
+   */
+  readonly acceptResponsibilityForUsingImportedBucket?: boolean;
 }
 
 /**
@@ -165,6 +171,11 @@ export class ServerlessClamscan extends Construct {
    */
   public readonly defsAccessLogsBucket?: IBucket;
 
+  /**
+    Conditional: When true, the user accepted the responsibility for using imported buckets
+   */
+  public readonly useImportedBuckets?: boolean;
+
   private _scanFunction: DockerImageFunction;
   private _s3Gw: GatewayVpcEndpoint;
   private _efsRootPath = '/lambda';
@@ -180,6 +191,8 @@ export class ServerlessClamscan extends Construct {
   constructor(scope: Construct, id: string, props: ServerlessClamscanProps) {
     super(scope, id);
 
+    this.useImportedBuckets = props.acceptResponsibilityForUsingImportedBucket;
+
     if (!props.onResult) {
       this.resultBus = new EventBus(this, 'ScanResultBus');
       this.resultDest = new EventBridgeDestination(this.resultBus);
@@ -508,16 +521,63 @@ export class ServerlessClamscan extends Construct {
     }
   }
 
+  /**
+   * @returns ArnPrincipal the ARN of the assumed role principal for the scan function
+   */
+  get scanAssumedPrincipal(): ArnPrincipal {
+    if (this._scanFunction.role) {
+      const stack = Stack.of(this);
+      const scan_assumed_role = `arn:${stack.partition}:sts::${stack.account}:assumed-role/${this._scanFunction.role.roleName}/${this._scanFunction.functionName}`;
+      return new ArnPrincipal(scan_assumed_role);
+    } else {
+      throw new Error('The scan function role is undefined');
+    }
+  }
+
+
+  /**
+   * Returns the statement that should be added to the bucket policy
+     in order to prevent objects to be accessed when they are not clean
+     or there have been scanning errors: this policy should be added
+     manually if external buckets are passed to addSourceBucket()
+   * @param bucket The bucket which you need to protect with the policy
+   * @returns PolicyStatement the policy statement if available
+   */
+  getPolicyStatementForBucket(bucket: IBucket): PolicyStatement {
+    if (this._scanFunction.role) {
+      const scan_assumed_principal = this.scanAssumedPrincipal;
+      return new PolicyStatement({
+        effect: Effect.DENY,
+        actions: ['s3:GetObject'],
+        resources: [bucket.arnForObjects('*')],
+        notPrincipals: [this._scanFunction.role, scan_assumed_principal],
+        conditions: {
+          StringEquals: {
+            's3:ExistingObjectTag/scan-status': [
+              'IN PROGRESS',
+              'INFECTED',
+              'ERROR',
+            ],
+          },
+        },
+      });
+    } else {
+      throw new Error("Can't generate a valid S3 bucket policy, the scan function role is undefined");
+    }
+  }
+
   /**
    * Sets the specified S3 Bucket as a s3:ObjectCreate* for the ClamAV function.
      Grants the ClamAV function permissions to get and tag objects.
      Adds a bucket policy to disallow GetObject operations on files that are tagged 'IN PROGRESS', 'INFECTED', or 'ERROR'.
    * @param bucket The bucket to add the scanning bucket policy and s3:ObjectCreate* trigger to.
    */
-  addSourceBucket(bucket: Bucket) {
-    this._scanFunction.addEventSource(
-      new S3EventSource(bucket, { events: [EventType.OBJECT_CREATED] }),
+  addSourceBucket(bucket: IBucket) {
+    bucket.addEventNotification(
+      EventType.OBJECT_CREATED,
+      new LambdaDestination(this._scanFunction),
     );
+
     bucket.grantRead(this._scanFunction);
     this._scanFunction.addToRolePolicy(
       new PolicyStatement({
@@ -528,9 +588,7 @@ export class ServerlessClamscan extends Construct {
     );
 
     if (this._scanFunction.role) {
-      const stack = Stack.of(this);
-      const scan_assumed_role = `arn:${stack.partition}:sts::${stack.account}:assumed-role/${this._scanFunction.role.roleName}/${this._scanFunction.functionName}`;
-      const scan_assumed_principal = new ArnPrincipal(scan_assumed_role);
+      const scan_assumed_principal = this.scanAssumedPrincipal;
       this._s3Gw.addToPolicy(
         new PolicyStatement({
           effect: Effect.ALLOW,
@@ -548,24 +606,13 @@ export class ServerlessClamscan extends Construct {
         }),
       );
 
-      // Need the assumed role for the not Principal Action with Lambda
-      bucket.addToResourcePolicy(
-        new PolicyStatement({
-          effect: Effect.DENY,
-          actions: ['s3:GetObject'],
-          resources: [bucket.arnForObjects('*')],
-          notPrincipals: [this._scanFunction.role, scan_assumed_principal],
-          conditions: {
-            StringEquals: {
-              's3:ExistingObjectTag/scan-status': [
-                'IN PROGRESS',
-                'INFECTED',
-                'ERROR',
-              ],
-            },
-          },
-        }),
+      const result : AddToResourcePolicyResult = bucket.addToResourcePolicy(
+        this.getPolicyStatementForBucket(bucket),
       );
+
+      if (!result.statementAdded && !this.useImportedBuckets) {
+        throw new Error('acceptResponsibilityForUsingImportedBucket must be set when adding an imported bucket. When using imported buckets the user is responsible for adding the required policy statement to the bucket policy: `getPolicyStatementForBucket()` can be used to retrieve the policy statement required by the solution');
+      }
     }
   }
 }


Test patch:
diff --git a/test/ServerlessClamscan.test.ts b/test/ServerlessClamscan.test.ts
index 58715b5c..8fbf053b 100644
--- a/test/ServerlessClamscan.test.ts
+++ b/test/ServerlessClamscan.test.ts
@@ -282,6 +282,65 @@ test('check bucket triggers and policies for source buckets ', () => {
   });
 });
 
+test('Check bucket triggers and policies for imported bucket', () => {
+  const stack = new Stack();
+  const importedBucket = Bucket.fromBucketName(stack, 'ImportedBucket', 'imported-bucket-name');
+  new ServerlessClamscan(stack, 'default', {
+    buckets: [importedBucket],
+    acceptResponsibilityForUsingImportedBucket: true,
+  });
+
+  // policy for the source bucket shouldn't be added
+  expect(stack).toCountResources('AWS::S3::BucketPolicy', 2);
+
+  expect(stack).toHaveResource('AWS::Lambda::Permission', {
+    Action: 'lambda:InvokeFunction',
+    FunctionName: {
+      'Fn::GetAtt': [
+        stringLike('*ServerlessClamscan*'),
+        'Arn',
+      ],
+    },
+    Principal: 's3.amazonaws.com',
+    SourceAccount: {
+      Ref: 'AWS::AccountId',
+    },
+    SourceArn: {
+      'Fn::Join': [
+        '',
+        [
+          'arn:',
+          {
+            Ref: 'AWS::Partition',
+          },
+          ':s3:::imported-bucket-name',
+        ],
+      ],
+    },
+  });
+});
+
+test('Check error is raised when imported bucket is used without accepting responsibility', () => {
+  const stack = new Stack();
+  const importedBucket = Bucket.fromBucketName(stack, 'ImportedBucket', 'imported-bucket-name');
+
+  const errorMessage = 'acceptResponsibilityForUsingImportedBucket must be set when adding an imported bucket. When using imported buckets the user is responsible for adding the required policy statement to the bucket policy: `getPolicyStatementForBucket()` can be used to retrieve the policy statement required by the solution';
+
+  const f = () => {
+    new ServerlessClamscan(stack, 'default', {
+      buckets: [importedBucket],
+    });
+  };
+
+  const g = () => {
+    const sc = new ServerlessClamscan(stack, 'default_2', {});
+    sc.addSourceBucket(importedBucket);
+  };
+
+  expect(f).toThrow(errorMessage);
+  expect(g).toThrow(errorMessage);
+});
+
 test('check Virus Definition buckets policy security and S3 Gateway endpoint policy', () => {
   const stack = new Stack();