Add support for specifying an existing S3 bucket

[vardanmat]

Summary

• Add optional ExistingBucketName parameter to single-account, multi-account, and SAM templates
• When provided, no new S3 buckets are created — the existing bucket is used for SAM artifacts, Lambda assessment results, and final report storage
• Updated buildspec.yml to pass --s3-bucket and --parameter-overrides to SAM deploy when an existing bucket is specified
• Updated README with documentation and usage examples for both single and multi-account deployments
• Supports customer environments with SCPs that restrict S3 bucket creation

Files changed

• deployment/aiml-security-single-account.yaml — ExistingBucketName parameter, conditional bucket/policy, updated IAM and env vars
• deployment/2-aiml-security-codebuild.yaml — same changes for multi-account template
• aiml-security-assessment/template.yaml — conditional AIMLAssessmentBucket, updated Lambda env vars and policies
• buildspec.yml — conditional SAM deploy flags based on EXISTING_ASSESSMENT_BUCKET env var
• README.md — documentation for the new parameter

Test plan

• Single-account: deployed with ExistingBucketName empty — new bucket created, assessment completed successfully
• Single-account: deployed with ExistingBucketName set — no new buckets created, assessment completed, results in specified bucket
• Tested in customer environment with SCP restricting bucket creation — deployment and assessment succeeded

[agasthik]

@vardanmat - Thanks for submitting this PR. Can you please review the impact / changes on the SAM template for the scenario for a multi-account environment too. Documentation update will also be needed. Thank you !

[vardanmat]

@agasthik Updated this PR with: multi-account template support (2-aiml-security-codebuild.yaml), SAM template + buildspec changes for zero bucket creation, and README documentation. All tested in Isengard and customer environment. Ready for review.

[agasthik]

@vardanmat — One item to review and fix

CleanupBucketFunction may delete user data when using an existing bucket

The CleanupBucketFunction in aiml-security-assessment/template.yaml now points at the user's existing bucket (via the conditional AIML_ASSESSMENT_BUCKET_NAME). If this function purges bucket contents on stack deletion, it could destroy pre-existing data that has nothing to do with the assessment.

Suggested fix: Add Condition: CreateNewBucket to the CleanupBucketFunction resource, same as you did for AIMLAssessmentBucket and AssessmentBucketPolicy. The cleanup function only exists to empty the bucket so CloudFormation can delete it on stack teardown — when the bucket is user-owned, the stack doesn't delete it, so there's nothing to clean up.

  CleanupBucketFunction:
    Condition: CreateNewBucket
    Type: AWS::Serverless::Function
    ...

One-line change, no Lambda code modifications needed.

[agasthik]

@vardanmat — Minor UX suggestion (non-blocking):

Consider adding an AllowedPattern to the ExistingBucketName parameter so invalid bucket names fail at stack creation time with a clear validation error, rather than failing mid-deploy when S3 API calls reject the name:

  ExistingBucketName:
    Type: String
    Default: ""
    Description: "Name of an existing S3 bucket to store assessment results (leave empty to create a new bucket)"
    AllowedPattern: "^([a-z0-9][a-z0-9.-]{1,61}[a-z0-9])?$"
    ConstraintDescription: "Bucket name must be 3-63 characters, lowercase letters/numbers/hyphens/dots only, starting and ending with a letter or number. Leave empty to create a new bucket."

This isn't a security concern (IAM policy alone can't grant access to a bucket you don't own, and invalid names just 403 at runtime), but it gives users faster feedback if they typo the bucket name.

[mehtadman87]

@vardanmat — Good work on the feature. I ran a full review (static analysis with cfn-lint/sam validate/sam build, automated code analysis, and live deployment testing in a dev account across three test passes including the stack-update edge cases). The core single-account logic works correctly — the conditions gate as expected, stack outputs resolve to the right bucket, and CodeBuild env vars are set correctly. Below is everything I found, ordered by severity.

Agasthi already flagged the CleanupBucketFunction data-deletion risk and the AllowedPattern — skipping those here to avoid duplication.

---

🔴 Critical

C-01 — template-multi-account.yaml not updated — ExistingBucketName has no effect in multi-account deployments

Location: aiml-security-assessment/template-multi-account.yaml

Live-tested and confirmed. template-multi-account.yaml has zero occurrences of ExistingBucketName, UseExistingBucket, or CreateNewBucket — no Parameters block, no conditions, no !If substitutions. All Lambda env vars and S3CrudPolicy references still use !Ref AIMLAssessmentBucket unconditionally.

buildspec.yml builds template-multi-account.yaml when MULTI_ACCOUNT_SCAN=true (line 49: sam build --template template-multi-account.yaml), then passes --parameter-overrides ExistingBucketName=$EXISTING_ASSESSMENT_BUCKET to sam deploy. SAM CLI silently ignores --parameter-overrides for parameters that don't exist in the template — no error is thrown. The deploy succeeds, but AIMLAssessmentBucket is always created regardless of what you pass.

I reproduced this by deploying template-multi-account.yaml with --parameter-overrides ExistingBucketName=my-existing-bucket — the changeset showed AIMLAssessmentBucket: CREATE_IN_PROGRESS and the stack output returned the auto-generated bucket name, not the existing one.

This doesn't mean multi-account assessments are broken — the assessment itself runs fine. It means the ExistingBucketName parameter specifically has no effect for multi-account deployments. A customer using multi-account in an SCP-restricted environment will still get a CreateBucket call and will hit the same SCP denial the feature was designed to avoid.

Fix: Apply the same changes to template-multi-account.yaml that were made to template.yaml:

• Add Parameters.ExistingBucketName (identical block)
• Add Conditions.UseExistingBucket and Conditions.CreateNewBucket (identical block)
• Add Condition: CreateNewBucket to AIMLAssessmentBucket and AssessmentBucketPolicy
• Replace all !Ref AIMLAssessmentBucket with !If [UseExistingBucket, !Ref ExistingBucketName, !Ref AIMLAssessmentBucket] in DefinitionSubstitutions, all S3CrudPolicy entries, and all Lambda AIML_ASSESSMENT_BUCKET_NAME env vars
• Update the AssessmentBucketName Output to use !If

---

🟠 High

H-01 — All six Lambda functions have S3CrudPolicy granting s3:DeleteObject on the existing customer bucket

Location: aiml-security-assessment/template.yaml — all six AWS::Serverless::Function resources

SAM's S3CrudPolicy macro expands to s3:GetObject, s3:PutObject, s3:DeleteObject, s3:DeleteObjectVersion, and s3:ListBucket. When UseExistingBucket=true, the bucket is customer-owned and may contain data that pre-dates this assessment. Granting s3:DeleteObject to all six Lambda functions — especially CleanupBucketFunction which explicitly purges objects — means a logic bug could permanently destroy customer data.

Fix: Replace S3CrudPolicy with a scoped inline statement that omits DeleteObject and DeleteObjectVersion when UseExistingBucket=true. One approach: use !If to switch between S3CrudPolicy (new-bucket path) and a minimal inline policy (s3:PutObject, s3:GetObject, s3:ListBucket only) for the existing-bucket path.

H-02 — Hardcoded IAM role names prevent two stacks in the same account/region

Location: deployment/aiml-security-single-account.yaml — AIMLSecurityMemberRole, CodeBuildRole; deployment/2-aiml-security-codebuild.yaml — AIMLSecurityMemberRole, MultiAccountCodeBuildRole

Confirmed by live test: Deploying two stacks simultaneously in the same account/region fails with: AIMLSecurityMemberRole already exists in stack arn:aws:cloudformation:.... This is pre-existing behavior, but it's particularly sharp for the ExistingBucketName scenario — a customer might already have a prior deployment running and want to deploy a second stack that reuses their existing bucket. The second stack fails immediately.

Fix: Scope role names to the stack: RoleName: !Sub 'AIMLSecurityMemberRole-${AWS::StackName}'. Or at minimum, document clearly that only one stack can exist per account/region.

H-03 — Single-account sam deploy missing --region $AWS_DEFAULT_REGION

Location: buildspec.yml line ~185

Both multi-account sam deploy calls include --region $AWS_DEFAULT_REGION. The single-account path omits it:

# Current (missing --region):
if ! sam deploy --template-file .aws-sam/build/template.yaml --stack-name $STACK_NAME --capabilities CAPABILITY_IAM --no-confirm-changeset --no-fail-on-empty-changeset $SAM_S3_FLAG $SAM_PARAMS; then

# Should be:
if ! sam deploy --template-file .aws-sam/build/template.yaml --stack-name $STACK_NAME --capabilities CAPABILITY_IAM --no-confirm-changeset --no-fail-on-empty-changeset $SAM_S3_FLAG $SAM_PARAMS --region $AWS_DEFAULT_REGION; then

If AWS_DEFAULT_REGION is unset in a manually-triggered CodeBuild run, SAM fails with a confusing region error instead of a clear message.

H-04 — SAM_PARAMS uses unquoted $EXISTING_ASSESSMENT_BUCKET

Location: buildspec.yml

# Current:
SAM_PARAMS="--parameter-overrides ExistingBucketName=$EXISTING_ASSESSMENT_BUCKET"

# Should be:
SAM_PARAMS="--parameter-overrides ExistingBucketName='$EXISTING_ASSESSMENT_BUCKET'"

S3 bucket names cannot contain spaces, but unquoted variables are still bad shell practice. If EXISTING_ASSESSMENT_BUCKET is ever set by an upstream process or sourced from an env file, shell word-splitting could inject extra SAM CLI flags.

---

🟡 Medium

M-01 — ACCOUNT_BUCKET != BUCKET_REPORT comparison in post_build is fragile

Location: buildspec.yml post_build, single-account block

When ExistingBucketName is provided, both BUCKET_REPORT (from CodeBuild env var set by CloudFormation) and ACCOUNT_BUCKET (from aws cloudformation describe-stacks output) should resolve to the same bucket name. The equality check works when both strings are identical, but any trailing whitespace from the describe-stacks output, a None return if the output key is missing, or a failed stack lookup causes incorrect behavior:

• If ACCOUNT_BUCKET is None or empty, the != branch triggers and the script attempts to sync from a non-existent source
• The else-branch prints "no sync needed" even if ACCOUNT_BUCKET is empty, masking a lookup failure

Fix:

# Add before the comparison:
echo "DEBUG: ACCOUNT_BUCKET='$ACCOUNT_BUCKET' BUCKET_REPORT='$BUCKET_REPORT'"
if [[ -z "$ACCOUNT_BUCKET" || "$ACCOUNT_BUCKET" == "None" ]]; then
  echo "WARNING: Could not determine assessment bucket from stack outputs"
elif [[ $ACCOUNT_BUCKET != $BUCKET_REPORT ]]; then
  # ... existing sync logic
fi

M-02 — README CLI examples have wrong file paths for both templates

Location: README.md — "Using an Existing S3 Bucket" section

# Current (both will fail from repo root):
--template-body file://aiml-security-single-account.yaml
--template-body file://2-aiml-security-codebuild.yaml

# Should be:
--template-body file://deployment/aiml-security-single-account.yaml
--template-body file://deployment/2-aiml-security-codebuild.yaml

Both files live in the deployment/ subdirectory. Running the CLI commands from the repo root produces [Errno 2] No such file or directory.

M-03 — README "Using an Existing S3 Bucket" section missing four critical prerequisites

Location: README.md — "Using an Existing S3 Bucket" section

The section explains the parameter and shows CLI examples but omits the following — all of which will block users in SCP-restricted environments:

1. Required bucket permissions: The existing bucket needs at minimum s3:PutObject, s3:GetObject, s3:ListBucket, and s3:CreateMultipartUpload (for SAM Lambda packaging). If the bucket has a restrictive bucket policy, deployment will fail with AccessDenied after the stack creates successfully — a very confusing failure mode.

2. Same-region requirement: sam deploy --s3-bucket requires the bucket to be in the same AWS Region as the CloudFormation stack. A cross-region bucket fails with a region mismatch error.

3. SAM deployment artifacts in the bucket: The same bucket receives both SAM Lambda packaging artifacts (.zip files) and assessment results. Customers who chose this bucket for a specific purpose (compliance, lifecycle policies) may not expect tooling artifacts to appear there. Consider adding --s3-prefix sam-artifacts/ to the sam deploy commands.

4. Stack update leaves orphaned bucket (confirmed by live test): When updating an existing stack from ExistingBucketName='' to ExistingBucketName='my-bucket', the originally auto-created bucket is removed from CloudFormation tracking (due to Condition: CreateNewBucket flipping) but not deleted (due to DeletionPolicy: Retain). It becomes an orphaned bucket incurring storage costs. Users need to empty and delete it manually.

Suggested addition (somewhere in the section):

Prerequisites for the existing bucket

• The bucket must be in the same AWS Region as your CloudFormation stack
• The bucket must allow s3:PutObject, s3:GetObject, s3:ListBucket, and s3:CreateMultipartUpload from the CodeBuild role
• SAM deployment artifacts (Lambda ZIP packages) will be written to the bucket alongside assessment results. Use --s3-prefix sam-artifacts/ in your sam deploy command if you need to keep them separate.
• Stack update note: Switching an existing stack from auto-created bucket to an existing bucket will orphan the previously created bucket (it is retained but removed from CloudFormation tracking). Empty and delete it manually to avoid unnecessary storage costs.

---

🔵 Low

L-01 — DependsOn: AssessmentBucketPolicy removal has no explanatory comment

Location: deployment/aiml-security-single-account.yaml and deployment/2-aiml-security-codebuild.yaml — CodeBuildStartBuild resource

The removal is correct: CloudFormation cannot reference a conditional resource in DependsOn, so keeping it would cause a stack failure when UseExistingBucket=true. But it's non-obvious to future maintainers.

Fix: Add an inline comment:

  CodeBuildStartBuild:
    Type: Custom::CodeBuildStartBuild
    # DependsOn: AssessmentBucketPolicy removed — AssessmentBucketPolicy is conditional
    # (only exists when CreateNewBucket=true). CloudFormation infers ordering via the
    # !If reference to AssessmentBucket in the BUCKET_REPORT environment variable.
    Properties:

L-02 — aiml-security-* wildcard grant in UploadtoS3 IAM policy is always active regardless of ExistingBucketName

Location: deployment/aiml-security-single-account.yaml → CodeBuildRole/UploadtoS3; deployment/2-aiml-security-codebuild.yaml → MultiAccountCodeBuildRole/UploadtoS3

Resource:
  - !If [UseExistingBucket, ..., !GetAtt AssessmentBucket.Arn]
  - !Sub "arn:${AWS::Partition}:s3:::aiml-security-*"    # ← always present
  - !Sub "arn:${AWS::Partition}:s3:::aiml-security-*/*"  # ← always present

In SCP-restricted environments where this feature is used, no aiml-security-* buckets exist — these grants are dead weight. More importantly, any bucket matching that prefix in the account (e.g. from a prior deployment with DeletionPolicy: Retain) becomes readable/listable by CodeBuild regardless of the parameter.

Fix: Wrap in a condition or document the intent clearly:

- !If
  - CreateNewBucket
  - !Sub "arn:${AWS::Partition}:s3:::aiml-security-*"
  - !Ref AWS::NoValue

L-03 — s3:CreateBucket granted to CodeBuild roles regardless of ExistingBucketName

Location: deployment/aiml-security-single-account.yaml → CodeBuildRole/SAMDeploymentPermissions; deployment/2-aiml-security-codebuild.yaml → MultiAccountCodeBuildRole/SAMPermissions

The PR's primary use case is "environments with SCPs that restrict S3 bucket creation." Yet the CodeBuild roles still have s3:CreateBucket on Resource: '*'. The SCP blocks it anyway, but granting unused broad permissions violates least-privilege and will generate IAM Access Analyzer findings.

Fix: Scope s3:CreateBucket to SAM-managed bucket names (e.g. arn:*:s3:::aws-sam-cli-*) or conditionally remove it when UseExistingBucket=true.

L-04 — CreateNewBucket condition is redundant — it's the exact logical negation of UseExistingBucket

Location: All three templates

UseExistingBucket: !Not [!Equals [!Ref ExistingBucketName, ""]]
CreateNewBucket:   !Equals [!Ref ExistingBucketName, ""]   # exactly !Not [UseExistingBucket]

Two conditions where one is the logical negation of the other adds maintenance overhead — if either is updated without the other they silently diverge.

Fix (optional): Remove CreateNewBucket and replace its usages with !Not [Condition: UseExistingBucket]:

AIMLAssessmentBucket:
  Condition: !Not [Condition: UseExistingBucket]
  # or keep CreateNewBucket but add a comment explaining the relationship

---

ℹ️ Info / Observations

I-01 — Stack update from existing→new bucket confirmed working (but leaves orphaned bucket — see M-03 #4)

Confirmed by live test: Updating a stack from ExistingBucketName='my-bucket' → ExistingBucketName='' succeeds. CloudFormation correctly creates a new AssessmentBucket, updates the stack output, and updates all CodeBuild env vars. The original my-bucket is left untouched (outside CloudFormation control). ✅

I-02 — Versioned bucket cleanup is incomplete in README — live-tested and confirmed

Location: README.md — Cleanup section

Confirmed by live test: Running aws s3 rm s3://<bucket> --recursive does not fully empty a versioned bucket — it only deletes current object versions, leaving delete markers and non-current versions behind. The subsequent aws s3 rb (or CloudFormation stack delete) then fails with The bucket you tried to delete is not empty (confirmed with the AIMLAssessmentBucket resource which has VersioningConfiguration: Enabled).

The README cleanup section already shows the correct aws s3api delete-objects approach using list-object-versions but it's listed as a fallback ("if stack deletion fails"). It should be the primary approach for this solution since versioning is always enabled.

Fix: Update the cleanup section to proactively use the version-aware deletion command before attempting stack deletion, not as a recovery step after failure.

---

Summary table:

ID	Severity	Category	One-liner
C-01	🔴 Critical	Missing Feature	template-multi-account.yaml not updated — ExistingBucketName silently ignored, new bucket always created
H-01	🟠 High	IAM/Security	S3CrudPolicy grants DeleteObject on customer bucket (6 functions)
H-02	🟠 High	Design	Hardcoded role names — two stacks can't coexist in same account
H-03	🟠 High	Bug	Single-account sam deploy missing --region flag
H-04	🟠 High	Bug	SAM_PARAMS unquoted variable in buildspec
M-01	🟡 Medium	Bug	post_build bucket comparison fragile to empty/None ACCOUNT_BUCKET
M-02	🟡 Medium	Docs	CLI examples missing deployment/ path prefix
M-03	🟡 Medium	Docs	New section missing: permissions, region, artifacts, orphan warning
L-01	🔵 Low	Code Quality	DependsOn removal has no comment
L-02	🔵 Low	IAM	aiml-security-* wildcard grant always active
L-03	🔵 Low	IAM	s3:CreateBucket not scoped/conditional
L-04	🔵 Low	Code Quality	CreateNewBucket condition is redundant
I-01	ℹ️ Info	Confirmed	Stack update existing→new works ✅
I-02	ℹ️ Info	Docs	Versioned bucket cleanup incomplete in README