Skip to content

Added expectedBucketOwner #196

Merged
Cole-Greer merged 2 commits intoaws:developfrom
vaibhavm99:add-expected-bucket-owner-validation
Mar 11, 2026
Merged

Added expectedBucketOwner #196
Cole-Greer merged 2 commits intoaws:developfrom
vaibhavm99:add-expected-bucket-owner-validation

Conversation

@vaibhavm99
Copy link
Contributor

Add S3 Bucket Owner Verification Support

This PR adds optional S3 bucket owner verification to neptune-export, enhancing security by allowing clients to verify that S3 buckets are owned by expected AWS accounts.

Changes

New Parameter: expectedBucketOwner

  • Added as an optional parameter across all S3 operations (GET, PUT, LIST)
  • Can be provided via environment variable EXPECTED_BUCKET_OWNER or JSON field expectedBucketOwner
  • When not provided, automatically defaults to the account ID from the credential provider
  • Applies to all S3 interactions: exports, config files, completion files, and Neptune ML training configs

Security Benefits

This feature helps prevent unauthorized access to S3 buckets by verifying bucket ownership before performing operations, adding an extra layer of security for cross-account scenarios.

…S3 requests, if they don't pass in the value, use credential provider for default accountID.
@vaibhavm99 vaibhavm99 force-pushed the add-expected-bucket-owner-validation branch from 97b953a to 8e3260c Compare February 26, 2026 02:24
@Cole-Greer
Copy link
Collaborator

Thanks for all of the changes. Everything looks good to me. I'm noticing 2 of the tests are failing in the CI, but those when checking out the branch and running them locally they seem fine. I think that's likely a bug in the CI environment and unrelated to these changes. I will look into that closer tomorrow.

Copy link
Collaborator

@Cole-Greer Cole-Greer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit with 1 small change (burying the call to s3CredentialsProvider.resolveCredentials() inside a try-catch), as that call was leading to some unit tests failing in environments with no credentials set. I feel it's best to simply swallow and log that exception as I think it's best to avoid interfering with any existing working use cases (especially ones where an S3 upload is skipped, and these changes have no relevancy).

I believe the PR is good to merge now. Any thoughts regarding this change @vaibhavm99?

@Cole-Greer Cole-Greer merged commit 1455dfa into aws:develop Mar 11, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants