Skip to content

Commit 5285726

Browse files
kartik2112claude
andauthored
Fix s3 sse kms validate path (#5931)
* fix: use SSE-KMS in _validate_s3_path_exists when kms_key_id is set When a target S3 bucket enforces SSE-KMS, _validate_s3_path_exists previously failed with access denied because its put_object call to create a missing prefix did not specify encryption. Thread the SFTTrainer kms_key_id (already available in _create_output_config) into _validate_s3_path_exists and include ServerSideEncryption=aws:kms and SSEKMSKeyId when it is provided. Fixes #5930 * Removed duplicate * style: black-format SSE-KMS test in test_finetune_utils Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 4c07a98 commit 5285726

2 files changed

Lines changed: 37 additions & 5 deletions

File tree

‎sagemaker-train/src/sagemaker/train/common_utils/finetune_utils.py‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1385,7 +1385,7 @@ def _create_output_config(
13851385
s3_output_path = _get_default_s3_output_path(sagemaker_session)
13861386

13871387
# Validate S3 path exists
1388-
_validate_s3_path_exists(s3_output_path, sagemaker_session)
1388+
_validate_s3_path_exists(s3_output_path, sagemaker_session, kms_key_id=kms_key_id)
13891389

13901390
config_kwargs = {
13911391
"s3_output_path": s3_output_path,
@@ -1486,8 +1486,16 @@ def _validate_eula_for_gated_model(model, accept_eula, is_gated_model):
14861486
return accept_eula
14871487

14881488

1489-
def _validate_s3_path_exists(s3_path: str, sagemaker_session):
1490-
"""Validate S3 path and create bucket/prefix if they don't exist."""
1489+
def _validate_s3_path_exists(s3_path: str, sagemaker_session, kms_key_id: Optional[str] = None):
1490+
"""Validate S3 path and create bucket/prefix if they don't exist.
1491+
1492+
Args:
1493+
s3_path: S3 path to validate.
1494+
sagemaker_session: SageMaker session used to create the S3 client.
1495+
kms_key_id: Optional KMS key ID. When provided, objects created to
1496+
initialize a missing prefix are written with SSE-KMS encryption so
1497+
that validation does not fail on buckets enforcing KMS encryption.
1498+
"""
14911499
if not s3_path.startswith("s3://"):
14921500
raise ValueError(f"Invalid S3 path format: {s3_path}")
14931501

@@ -1535,7 +1543,11 @@ def _validate_s3_path_exists(s3_path: str, sagemaker_session):
15351543
# Create the prefix by putting an empty object
15361544
if not prefix.endswith("/"):
15371545
prefix += "/"
1538-
s3_client.put_object(Bucket=bucket_name, Key=prefix, Body=b"")
1546+
put_object_kwargs = {"Bucket": bucket_name, "Key": prefix, "Body": b""}
1547+
if kms_key_id:
1548+
put_object_kwargs["ServerSideEncryption"] = "aws:kms"
1549+
put_object_kwargs["SSEKMSKeyId"] = kms_key_id
1550+
s3_client.put_object(**put_object_kwargs)
15391551

15401552
except Exception as e:
15411553
raise ValueError(f"Failed to validate/create S3 path '{s3_path}': {str(e)}")

‎sagemaker-train/tests/unit/train/common_utils/test_finetune_utils.py‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -674,7 +674,9 @@ def test__create_output_config(self, mock_validate_s3):
674674

675675
assert config.s3_output_path == "s3://bucket/output"
676676
assert config.kms_key_id == "kms-key"
677-
mock_validate_s3.assert_called_once_with("s3://bucket/output", mock_session)
677+
mock_validate_s3.assert_called_once_with(
678+
"s3://bucket/output", mock_session, kms_key_id="kms-key"
679+
)
678680

679681
def test__convert_input_data_to_channels(self):
680682

@@ -879,6 +881,24 @@ def test__validate_s3_path_exists_with_prefix_not_exists(self, mock_boto_client)
879881
Bucket="test-bucket", Key="prefix/", Body=b""
880882
)
881883

884+
@patch("boto3.client")
885+
def test__validate_s3_path_exists_with_prefix_not_exists_kms(self, mock_boto_client):
886+
"""Test S3 path validation uses SSE-KMS when kms_key_id is provided"""
887+
mock_session = Mock()
888+
mock_s3_client = Mock()
889+
mock_session.boto_session.client.return_value = mock_s3_client
890+
mock_s3_client.list_objects_v2.return_value = {} # No contents
891+
892+
_validate_s3_path_exists("s3://test-bucket/prefix", mock_session, kms_key_id="kms-key")
893+
894+
mock_s3_client.put_object.assert_called_once_with(
895+
Bucket="test-bucket",
896+
Key="prefix/",
897+
Body=b"",
898+
ServerSideEncryption="aws:kms",
899+
SSEKMSKeyId="kms-key",
900+
)
901+
882902

883903
class TestMlflowVersionMeetsMinimum:
884904
def test_meets_minimum(self):

0 commit comments

Comments
 (0)