Add Owner ID check for bucket with path when prefix is provided #5146
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # src/sagemaker/serve/model_server/multi_model_server/inference.py # src/sagemaker/serve/model_server/torchserve/inference.py # src/sagemaker/serve/model_server/torchserve/xgboost_inference.py
**Description** Previously we called the head_bucket call to ensure the owner ID check, but this doesnt take into consideration cases where the s3 path is provided through the prefix. This change makes sure that director level permissions are supported. **Testing Done** Tested through unit tests, integ tests and manual testing through the installation file. Yes
benieric
reviewed
Apr 25, 2025
src/sagemaker/session.py
Outdated
| s3.meta.client.list_objects_v2( | ||
| Bucket=bucket_name, | ||
| Prefix=self.default_bucket_prefix, | ||
| ExpectedBucketOwner=expected_bucket_owner_id |
There was a problem hiding this comment.
I believe this check is a bit different in this method. Looks like it just checks for general permission to the bucket rather than checking for ExpectedBucketOwner. For this case, probably need to remove the ExpectedBucketOwner=expected_bucket_owner_id to match the behavior of the second block which just does a regular head bucket check - s3.meta.client.head_bucket(Bucket=bucket_name)
There was a problem hiding this comment.
good callout. Updated the PR.
benieric
previously approved these changes
Apr 25, 2025
src/sagemaker/session.py
Outdated
| @@ -668,7 +678,7 @@ def expected_bucket_owner_id_bucket_check(self, bucket_name, s3, expected_bucket | |||
| raise | |||
|
|
|||
| def general_bucket_check_if_user_has_permission( | |||
| self, bucket_name, s3, bucket, region, bucket_creation_date_none | |||
| self, bucket_name, s3, bucket, region, bucket_creation_date_none, expected_bucket_owner_id | |||
There was a problem hiding this comment.
nit: this extra parameter is unused
benieric
approved these changes
Apr 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Previously we called the head_bucket call to ensure the owner ID check, but this doesnt take into consideration cases where the s3 path is provided through the prefix.
This change makes sure that director level permissions are supported.
Testing Done
Tested through unit tests, integ tests and manual testing through the installation file.
General
Tests
unique_name_from_baseto create resource names in integ tests (if appropriate)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.