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

Issue:
Does not work with existing buckets
the solution doesn't seem to work with existing buckets. Our workaround is to change the signature of the `addSourceBucket` method and cast:

```js
  addSourceBucket(bucket: IBucket) {
    this._scanFunction.addEventSource(
      new S3EventSource(bucket as Bucket, { events: [EventType.OBJECT_CREATED] }),
  // ...
```


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:
This is intended since the bucket policy is rather important for this construct and mutating existing buckets can have unintended consequences. Without the policy other services may attempt to download files that have not finished the scan process, which can have consequences if the file is registered as infected.

Additionally the CDK can not add bucket policies to imported buckets
Thank you. Still, there's huge value in making the solution work with existing infrastructure. What about allowing that  subject to an explicit confirmation from the bucket owner? This can be enforced in the code by requiring users to set e.g. `acceptResponsibilityForUsingOwnBucket=true`.

I guess you could create an output containing the policy to make the integration easier.

Alternatively - it should be possible to add policies to imported buckets using [CfnBucketPolicy](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_s3.CfnBucketPolicy.html#construct-props). Yes, this might have unintended consequences but IMO the bucket owner should be given the choice to use it if they accept the risk.
I'm okay with this as long as we have a two step explicit acknowledgement .

1. `acceptResponsibilityForUsingExistingBucket`
   - Allows user to use an existing bucket
   - Warn users that the bucket policy will not be updated unless they use `acceptResponsibilityForBucketPolicyReplacement`  and about the potential consequences of not updating the bucket policy
 2. `acceptResponsibilityForBucketPolicyReplacement` 
    -  Replaces the existing bucket policy
    - Warns users that the existing bucket policy will be replaced and they accept responsibility for it
    
 Happy to further workshop this over a PR
@dontirun I've implemented the requested changes, thanks for reviewing!
@michelesr  You need to [authorize modification on the branch](https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork) for mergify to do it's thing
> @michelesr You need to [authorize modification on the branch](https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork) for mergify to do it's thing

Looks like that can only be done from a fork in a personal (not org) account. I'll open a PR from my personal account and then if everything works fine I'll close this.

Fix patch:
diff --git a/.projen/deps.json b/.projen/deps.json
index 61827578..cf44c5ce 100644
--- a/.projen/deps.json
+++ b/.projen/deps.json
@@ -138,6 +138,11 @@
       "version": "^1.101.0",
       "type": "peer"
     },
+    {
+      "name": "@aws-cdk/aws-s3-notifications",
+      "version": "^1.101.0",
+      "type": "peer"
+    },
     {
       "name": "@aws-cdk/aws-s3",
       "version": "^1.101.0",
@@ -203,6 +208,11 @@
       "version": "^1.101.0",
       "type": "runtime"
     },
+    {
+      "name": "@aws-cdk/aws-s3-notifications",
+      "version": "^1.101.0",
+      "type": "runtime"
+    },
     {
       "name": "@aws-cdk/aws-s3",
       "version": "^1.101.0",
diff --git a/.projenrc.js b/.projenrc.js
index 033c4b65..ff59938c 100644
--- a/.projenrc.js
+++ b/.projenrc.js
@@ -25,6 +25,7 @@ const project = new awscdk.AwsCdkConstructLibrary({
     '@aws-cdk/aws-lambda-destinations',
     '@aws-cdk/aws-lambda-event-sources',
     '@aws-cdk/aws-s3',
+    '@aws-cdk/aws-s3-notifications',
     '@aws-cdk/aws-sqs',
     '@aws-cdk/core',
   ],
diff --git a/API.md b/API.md
index ecfd228a..994a2ea4 100644
--- a/API.md
+++ b/API.md
@@ -64,7 +64,8 @@ new ServerlessClamscan(scope: Construct, id: string, props: ServerlessClamscanPr
 * **scope** (<code>[Construct](#aws-cdk-core-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<[Bucket](#aws-cdk-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<[IBucket](#aws-cdk-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>[IDestination](#aws-cdk-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>[IDestination](#aws-cdk-aws-lambda-idestination)</code> | The Lambda Destination for failed on erred scans [ERROR, IN PROGRESS (If error is due to Lambda timeout)].
 **resultDest** | <code>[IDestination](#aws-cdk-aws-lambda-idestination)</code> | The Lambda Destination for completed ClamAV scans [CLEAN, INFECTED].
+**scanAssumedPrincipal** | <code>[ArnPrincipal](#aws-cdk-aws-iam-arnprincipal)</code> | <span></span>
 **cleanRule**? | <code>[Rule](#aws-cdk-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>[IBucket](#aws-cdk-aws-s3-ibucket)</code> | Conditional: The Bucket for access logs for the virus definitions bucket if logging is enabled (defsBucketAccessLogsConfig).<br/>__*Optional*__
 **errorDeadLetterQueue**? | <code>[Queue](#aws-cdk-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>[Queue](#aws-cdk-aws-sqs-queue)</code> | Conditional: The SQS Queue for erred scans if a failure (onError) destination was not specified.<br/>__*Optional*__
 **infectedRule**? | <code>[Rule](#aws-cdk-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>[EventBus](#aws-cdk-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>[Bucket](#aws-cdk-aws-s3-bucket)</code>)  The bucket to add the scanning bucket policy and s3:ObjectCreate* trigger to.
+* **bucket** (<code>[IBucket](#aws-cdk-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>[IBucket](#aws-cdk-aws-s3-ibucket)</code>)  The bucket which you need to protect with the policy.
+
+__Returns__:
+* <code>[PolicyStatement](#aws-cdk-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<[Bucket](#aws-cdk-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<[IBucket](#aws-cdk-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>[IDestination](#aws-cdk-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/package.json b/package.json
index 6f7ffc39..e20fbee6 100644
--- a/package.json
+++ b/package.json
@@ -70,6 +70,7 @@
     "@aws-cdk/aws-lambda-destinations": "^1.101.0",
     "@aws-cdk/aws-lambda-event-sources": "^1.101.0",
     "@aws-cdk/aws-s3": "^1.101.0",
+    "@aws-cdk/aws-s3-notifications": "^1.101.0",
     "@aws-cdk/aws-sqs": "^1.101.0",
     "@aws-cdk/core": "^1.101.0",
     "constructs": "^3.2.27"
@@ -85,6 +86,7 @@
     "@aws-cdk/aws-lambda-destinations": "^1.101.0",
     "@aws-cdk/aws-lambda-event-sources": "^1.101.0",
     "@aws-cdk/aws-s3": "^1.101.0",
+    "@aws-cdk/aws-s3-notifications": "^1.101.0",
     "@aws-cdk/aws-sqs": "^1.101.0",
     "@aws-cdk/core": "^1.101.0",
     "cdk-nag": "^1.6.1"
diff --git a/src/index.ts b/src/index.ts
index 54808db2..52ec328b 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -3,44 +3,33 @@
 
 import * as path from 'path';
 import {
-  Vpc,
-  SubnetType,
   GatewayVpcEndpoint,
   GatewayVpcEndpointAwsService,
   Port,
-  SecurityGroup,
+  SecurityGroup, SubnetType, Vpc,
 } from '@aws-cdk/aws-ec2';
 import { FileSystem, LifecyclePolicy, PerformanceMode } from '@aws-cdk/aws-efs';
 import { EventBus, Rule, Schedule } from '@aws-cdk/aws-events';
 import { LambdaFunction } from '@aws-cdk/aws-events-targets';
 import {
-  Effect,
-  PolicyStatement,
-  ArnPrincipal,
-  AnyPrincipal,
   AccountRootPrincipal,
+  AddToResourcePolicyResult, AnyPrincipal, ArnPrincipal, Effect,
+  PolicyStatement,
 } from '@aws-cdk/aws-iam';
 import {
-  DockerImageCode,
-  DockerImageFunction,
-  Function,
-  IDestination,
-  FileSystem as LambdaFileSystem,
-  Runtime,
-  Code,
+  Code, DockerImageCode,
+  DockerImageFunction, FileSystem as LambdaFileSystem, Function,
+  IDestination, Runtime,
 } from '@aws-cdk/aws-lambda';
 import {
   EventBridgeDestination,
   SqsDestination,
 } from '@aws-cdk/aws-lambda-destinations';
-import { S3EventSource } from '@aws-cdk/aws-lambda-event-sources';
-import { IBucket, Bucket, BucketEncryption, EventType } from '@aws-cdk/aws-s3';
+import { Bucket, BucketEncryption, EventType, IBucket } from '@aws-cdk/aws-s3';
+import { LambdaDestination } from '@aws-cdk/aws-s3-notifications';
 import { Queue, QueueEncryption } from '@aws-cdk/aws-sqs';
 import {
-  Construct,
-  Duration,
-  CustomResource,
-  RemovalPolicy,
+  Construct, CustomResource, Duration, RemovalPolicy,
   Stack,
 } from '@aws-cdk/core';
 import { NagSuppressions } from 'cdk-nag';
@@ -65,7 +54,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
@@ -87,6 +76,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;
 }
 
 /**
@@ -164,6 +158,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';
@@ -179,6 +178,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);
@@ -540,16 +541,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({
@@ -560,9 +608,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,
@@ -580,24 +626,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');
+      }
     }
   }
 }
diff --git a/yarn.lock b/yarn.lock
index 2bf83627..5ea66ba1 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -921,7 +921,7 @@
     "@aws-cdk/cx-api" "1.155.0"
     constructs "^3.3.69"
 
-"@aws-cdk/aws-s3-notifications@1.155.0":
+"@aws-cdk/aws-s3-notifications@1.155.0", "@aws-cdk/aws-s3-notifications@^1.101.0":
   version "1.155.0"
   resolved "https://registry.yarnpkg.com/@aws-cdk/aws-s3-notifications/-/aws-s3-notifications-1.155.0.tgz#c61bf4f7acb22d36c263a67ae611ce585937873a"
   integrity sha512-mgJfCbkXU0k2HrJsUms8B28rY2djXPTFrTcoKR4XMv2YD93f09sejlv1Yk5Eh996f01P8CRE6/GOAOb9vwdz4Q==


Test patch:
diff --git a/test/ServerlessClamscan.test.ts b/test/ServerlessClamscan.test.ts
index 3f58bae1..e107ebac 100644
--- a/test/ServerlessClamscan.test.ts
+++ b/test/ServerlessClamscan.test.ts
@@ -293,6 +293,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();