From bbaed91c97d1a9624a5511f08b1b23462c1e90a4 Mon Sep 17 00:00:00 2001 From: texastony <5892063+texastony@users.noreply.github.com> Date: Fri, 6 Mar 2026 10:29:50 -0600 Subject: [PATCH 1/3] fix(error-handling): improve error messages for missing S3 objects and instruction files Match Java S3EC behavior when S3 objects or instruction files do not exist: - Add early return in event handler when Body is None (failed S3 response) - Catch ClientError separately in get_object with "Unable to retrieve object" message - Catch ClientError in fetch_instruction_file with instruction-file-specific message - Check for None body in process_instruction_file before reading Tests: - Unit: NoSuchKey, AccessDenied, and missing instruction file error wrapping - Integration: non-existent object and plain object with missing instruction file --- src/s3_encryption/__init__.py | 21 +++++- src/s3_encryption/instruction_file.py | 8 +++ .../test_i_s3_encryption_instruction_file.py | 47 +++++++++++++ test/test_s3_encryption_client.py | 68 +++++++++++++++++++ 4 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 test/test_s3_encryption_client.py diff --git a/src/s3_encryption/__init__.py b/src/s3_encryption/__init__.py index a3558195..d18114be 100644 --- a/src/s3_encryption/__init__.py +++ b/src/s3_encryption/__init__.py @@ -6,6 +6,7 @@ import threading from attrs import define, field +from botocore.exceptions import ClientError from botocore.response import StreamingBody from .exceptions import S3EncryptionClientError @@ -125,6 +126,11 @@ def on_get_object_after_call(self, parsed, **kwargs): # Get encryption context from thread-local storage (set by get_object wrapper) encryption_context = getattr(self._context, "encryption_context", None) + # If Body is None, the S3 request failed (e.g., NoSuchKey). + # Return early and let boto3 raise the original error. + if parsed.get("Body") is None: + return + # The parsed response already has the Body as a StreamingBody # We need to read it, decrypt it, and replace it @@ -165,9 +171,17 @@ def process_instruction_file(self, parsed): """ instruction_key = getattr(self._context, "key", None) + body = parsed.get("Body") + if body is None: + raise S3EncryptionClientError( + "Exception encountered while fetching Instruction File." + " Ensure the object you are attempting to decrypt has been encrypted" + " using the S3 Encryption Client and instruction files are enabled." + ) + # In plaintext mode, parse instruction file and append to metadata - existing_metadata = parsed.get("Metadata", {}) - instruction_data = parsed.get("Body").read() + existing_metadata = parsed.get("Metadata", {}) or {} + instruction_data = body.read() instruction_metadata = parse_instruction_file(instruction_data, instruction_key) # Append parsed instruction file content to existing metadata @@ -278,6 +292,9 @@ def get_object(self, **kwargs): except S3EncryptionClientError: # Re-raise our own exceptions without wrapping raise + except ClientError as e: + # Wrap S3 service errors (e.g., NoSuchKey) with context + raise S3EncryptionClientError(f"Unable to retrieve object: {str(e)}") from e except Exception as e: # Wrap any unexpected errors during decryption raise S3EncryptionClientError(f"Failed to decrypt object: {str(e)}") from e diff --git a/src/s3_encryption/instruction_file.py b/src/s3_encryption/instruction_file.py index 351c4b15..a23252f8 100644 --- a/src/s3_encryption/instruction_file.py +++ b/src/s3_encryption/instruction_file.py @@ -9,6 +9,8 @@ import json from typing import Any +from botocore.exceptions import ClientError + from .exceptions import S3EncryptionClientError from .metadata import VALID_S3EC_METADATA_KEYS @@ -95,6 +97,12 @@ def fetch_instruction_file(s3_client, bucket: str, key: str) -> dict[str, Any]: try: response = s3_client.get_object(Bucket=bucket, Key=key) + except ClientError as e: + raise S3EncryptionClientError( + "Exception encountered while fetching Instruction File." + " Ensure the object you are attempting to decrypt has been encrypted" + " using the S3 Encryption Client and instruction files are enabled." + ) from e finally: # Clear the flags after the call if hasattr(s3_client, "_s3ec_plugin_context"): diff --git a/test/integration/test_i_s3_encryption_instruction_file.py b/test/integration/test_i_s3_encryption_instruction_file.py index 467ddc7a..62390cb0 100644 --- a/test/integration/test_i_s3_encryption_instruction_file.py +++ b/test/integration/test_i_s3_encryption_instruction_file.py @@ -149,3 +149,50 @@ def test_decrypt_v2_instruction_file_custom_suffix(): assert output == "static-v2-instruction-file-from-java-v4" print("Success! V2 custom suffix instruction file decryption completed.") + + +def test_get_nonexistent_object_raises_s3_encryption_client_error(): + """Test that getting a non-existent object raises S3EncryptionClientError. + + Matches Java S3EC behavior: NoSuchKeyException is wrapped in + S3EncryptionClientException with the original as the cause. + """ + from botocore.exceptions import ClientError + + from s3_encryption.exceptions import S3EncryptionClientError + + kms_client = boto3.client("kms", region_name=region) + keyring = KmsKeyring(kms_client, kms_key_id) + wrapped_client = boto3.client("s3") + config = S3EncryptionClientConfig(keyring) + s3ec = S3EncryptionClient(wrapped_client, config) + + with pytest.raises(S3EncryptionClientError, match="Unable to retrieve object") as exc_info: + s3ec.get_object(Bucket=bucket, Key="this-object-does-not-exist") + + assert isinstance(exc_info.value.__cause__, ClientError) + + +def test_get_object_with_missing_instruction_file_raises_s3_encryption_client_error(): + """Test that a missing instruction file raises S3EncryptionClientError. + + When an object has no encryption metadata and the instruction file + also doesn't exist, the error should indicate the instruction file issue. + """ + from s3_encryption.exceptions import S3EncryptionClientError + + kms_client = boto3.client("kms", region_name=region) + keyring = KmsKeyring(kms_client, kms_key_id) + wrapped_client = boto3.client("s3") + config = S3EncryptionClientConfig(keyring) + s3ec = S3EncryptionClient(wrapped_client, config) + + # Use a separate plain S3 client to put an unencrypted object + plain_s3 = boto3.client("s3") + plain_s3.put_object(Bucket=bucket, Key="plain-object-no-instruction-file", Body=b"hello") + + try: + with pytest.raises(S3EncryptionClientError, match="fetching Instruction File"): + s3ec.get_object(Bucket=bucket, Key="plain-object-no-instruction-file") + finally: + plain_s3.delete_object(Bucket=bucket, Key="plain-object-no-instruction-file") diff --git a/test/test_s3_encryption_client.py b/test/test_s3_encryption_client.py new file mode 100644 index 00000000..f0c0063a --- /dev/null +++ b/test/test_s3_encryption_client.py @@ -0,0 +1,68 @@ +# Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Unit tests for S3EncryptionClient get_object error handling.""" + +from unittest.mock import Mock + +import pytest +from botocore.exceptions import ClientError + +from s3_encryption import S3EncryptionClient, S3EncryptionClientConfig +from s3_encryption.exceptions import S3EncryptionClientError +from s3_encryption.materials.keyring import S3Keyring + + +class TestGetObjectNonExistentObject: + """S3EncryptionClient wraps S3 errors with context, preserving the original cause.""" + + def _build_client(self): + mock_s3 = Mock() + mock_s3.meta.events = Mock() + mock_s3.meta.events.register = Mock() + mock_keyring = Mock(spec=S3Keyring) + config = S3EncryptionClientConfig(keyring=mock_keyring) + return S3EncryptionClient(wrapped_s3_client=mock_s3, config=config), mock_s3 + + def test_no_such_key_raises_s3_encryption_client_error(self): + client, mock_s3 = self._build_client() + error_response = { + "Error": {"Code": "NoSuchKey", "Message": "The specified key does not exist."} + } + mock_s3.get_object.side_effect = ClientError(error_response, "GetObject") + + with pytest.raises(S3EncryptionClientError, match="Unable to retrieve object") as exc_info: + client.get_object(Bucket="test-bucket", Key="nonexistent-key") + + assert isinstance(exc_info.value.__cause__, ClientError) + assert exc_info.value.__cause__.response["Error"]["Code"] == "NoSuchKey" + + def test_access_denied_raises_s3_encryption_client_error(self): + client, mock_s3 = self._build_client() + error_response = {"Error": {"Code": "AccessDenied", "Message": "Access Denied"}} + mock_s3.get_object.side_effect = ClientError(error_response, "GetObject") + + with pytest.raises(S3EncryptionClientError, match="Unable to retrieve object") as exc_info: + client.get_object(Bucket="test-bucket", Key="forbidden-key") + + assert isinstance(exc_info.value.__cause__, ClientError) + assert exc_info.value.__cause__.response["Error"]["Code"] == "AccessDenied" + + +class TestFetchMissingInstructionFile: + """fetch_instruction_file wraps NoSuchKey with instruction-file-specific message.""" + + def test_missing_instruction_file_raises_s3_encryption_client_error(self): + mock_s3 = Mock() + mock_s3._s3ec_plugin_context = Mock() + error_response = { + "Error": {"Code": "NoSuchKey", "Message": "The specified key does not exist."} + } + mock_s3.get_object.side_effect = ClientError(error_response, "GetObject") + + from s3_encryption.instruction_file import fetch_instruction_file + + with pytest.raises(S3EncryptionClientError, match="fetching Instruction File") as exc_info: + fetch_instruction_file(mock_s3, "test-bucket", "test-key.instruction") + + assert isinstance(exc_info.value.__cause__, ClientError) + assert exc_info.value.__cause__.response["Error"]["Code"] == "NoSuchKey" From 99a707fc178636563a692a5db60172e3da888a83 Mon Sep 17 00:00:00 2001 From: texastony <5892063+texastony@users.noreply.github.com> Date: Tue, 17 Mar 2026 13:14:13 -0700 Subject: [PATCH 2/3] fix(instruction-file): address PR #149 review comments from kessplas and texastony --- src/s3_encryption/__init__.py | 19 +++++++++++-------- src/s3_encryption/instruction_file.py | 2 -- .../test_i_s3_encryption_instruction_file.py | 14 +++++++++----- test/test_s3_encryption_client.py | 8 ++++++-- 4 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/s3_encryption/__init__.py b/src/s3_encryption/__init__.py index d18114be..9a1e855d 100644 --- a/src/s3_encryption/__init__.py +++ b/src/s3_encryption/__init__.py @@ -118,7 +118,7 @@ def on_get_object_after_call(self, parsed, **kwargs): parsed: Dictionary containing the parsed response **kwargs: Additional event arguments (includes 'params' with request parameters) """ - # Check if plaintext mode is enabled via thread-local flag + # Check if instruction_file_mode is enabled via thread-local flag if getattr(self._context, "instruction_file_mode", False): self.process_instruction_file(parsed) return @@ -128,7 +128,7 @@ def on_get_object_after_call(self, parsed, **kwargs): # If Body is None, the S3 request failed (e.g., NoSuchKey). # Return early and let boto3 raise the original error. - if parsed.get("Body") is None: + if parsed.get("Body", None) is None: return # The parsed response already has the Body as a StreamingBody @@ -171,15 +171,14 @@ def process_instruction_file(self, parsed): """ instruction_key = getattr(self._context, "key", None) - body = parsed.get("Body") + body = parsed.get("Body", None) if body is None: raise S3EncryptionClientError( - "Exception encountered while fetching Instruction File." - " Ensure the object you are attempting to decrypt has been encrypted" - " using the S3 Encryption Client and instruction files are enabled." + f"Instruction file body is empty for key: {instruction_key}" ) # In plaintext mode, parse instruction file and append to metadata + # Metadata may be present but None, so `or {}` handles that case existing_metadata = parsed.get("Metadata", {}) or {} instruction_data = body.read() instruction_metadata = parse_instruction_file(instruction_data, instruction_key) @@ -294,10 +293,14 @@ def get_object(self, **kwargs): raise except ClientError as e: # Wrap S3 service errors (e.g., NoSuchKey) with context - raise S3EncryptionClientError(f"Unable to retrieve object: {str(e)}") from e + raise S3EncryptionClientError( + f"Failed to retrieve and/or decrypt object: {str(e)}" + ) from e except Exception as e: # Wrap any unexpected errors during decryption - raise S3EncryptionClientError(f"Failed to decrypt object: {str(e)}") from e + raise S3EncryptionClientError( + f"Failed to retrieve and/or decrypt object: {str(e)}" + ) from e finally: # Clean up thread-local storage; # do not clean up the client as it is not thread local only diff --git a/src/s3_encryption/instruction_file.py b/src/s3_encryption/instruction_file.py index a23252f8..f3a19b70 100644 --- a/src/s3_encryption/instruction_file.py +++ b/src/s3_encryption/instruction_file.py @@ -100,8 +100,6 @@ def fetch_instruction_file(s3_client, bucket: str, key: str) -> dict[str, Any]: except ClientError as e: raise S3EncryptionClientError( "Exception encountered while fetching Instruction File." - " Ensure the object you are attempting to decrypt has been encrypted" - " using the S3 Encryption Client and instruction files are enabled." ) from e finally: # Clear the flags after the call diff --git a/test/integration/test_i_s3_encryption_instruction_file.py b/test/integration/test_i_s3_encryption_instruction_file.py index 62390cb0..45571c97 100644 --- a/test/integration/test_i_s3_encryption_instruction_file.py +++ b/test/integration/test_i_s3_encryption_instruction_file.py @@ -1,6 +1,7 @@ # Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 import os +import uuid import boto3 import pytest @@ -167,7 +168,9 @@ def test_get_nonexistent_object_raises_s3_encryption_client_error(): config = S3EncryptionClientConfig(keyring) s3ec = S3EncryptionClient(wrapped_client, config) - with pytest.raises(S3EncryptionClientError, match="Unable to retrieve object") as exc_info: + with pytest.raises( + S3EncryptionClientError, match="Failed to retrieve and/or decrypt object" + ) as exc_info: s3ec.get_object(Bucket=bucket, Key="this-object-does-not-exist") assert isinstance(exc_info.value.__cause__, ClientError) @@ -189,10 +192,11 @@ def test_get_object_with_missing_instruction_file_raises_s3_encryption_client_er # Use a separate plain S3 client to put an unencrypted object plain_s3 = boto3.client("s3") - plain_s3.put_object(Bucket=bucket, Key="plain-object-no-instruction-file", Body=b"hello") + test_key = f"plain-object-no-instruction-file-{uuid.uuid4()}" + plain_s3.put_object(Bucket=bucket, Key=test_key, Body=b"hello") try: - with pytest.raises(S3EncryptionClientError, match="fetching Instruction File"): - s3ec.get_object(Bucket=bucket, Key="plain-object-no-instruction-file") + with pytest.raises(S3EncryptionClientError, match="Instruction file body is empty"): + s3ec.get_object(Bucket=bucket, Key=test_key) finally: - plain_s3.delete_object(Bucket=bucket, Key="plain-object-no-instruction-file") + plain_s3.delete_object(Bucket=bucket, Key=test_key) diff --git a/test/test_s3_encryption_client.py b/test/test_s3_encryption_client.py index f0c0063a..2b164fe8 100644 --- a/test/test_s3_encryption_client.py +++ b/test/test_s3_encryption_client.py @@ -30,7 +30,9 @@ def test_no_such_key_raises_s3_encryption_client_error(self): } mock_s3.get_object.side_effect = ClientError(error_response, "GetObject") - with pytest.raises(S3EncryptionClientError, match="Unable to retrieve object") as exc_info: + with pytest.raises( + S3EncryptionClientError, match="Failed to retrieve and/or decrypt object" + ) as exc_info: client.get_object(Bucket="test-bucket", Key="nonexistent-key") assert isinstance(exc_info.value.__cause__, ClientError) @@ -41,7 +43,9 @@ def test_access_denied_raises_s3_encryption_client_error(self): error_response = {"Error": {"Code": "AccessDenied", "Message": "Access Denied"}} mock_s3.get_object.side_effect = ClientError(error_response, "GetObject") - with pytest.raises(S3EncryptionClientError, match="Unable to retrieve object") as exc_info: + with pytest.raises( + S3EncryptionClientError, match="Failed to retrieve and/or decrypt object" + ) as exc_info: client.get_object(Bucket="test-bucket", Key="forbidden-key") assert isinstance(exc_info.value.__cause__, ClientError) From ea4603935b62d88855ade60e0afc7108db1f7b33 Mon Sep 17 00:00:00 2001 From: texastony <5892063+texastony@users.noreply.github.com> Date: Wed, 25 Mar 2026 15:54:45 -0700 Subject: [PATCH 3/3] chore: fix error message --- src/s3_encryption/instruction_file.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/s3_encryption/instruction_file.py b/src/s3_encryption/instruction_file.py index f3a19b70..60305d17 100644 --- a/src/s3_encryption/instruction_file.py +++ b/src/s3_encryption/instruction_file.py @@ -99,7 +99,9 @@ def fetch_instruction_file(s3_client, bucket: str, key: str) -> dict[str, Any]: response = s3_client.get_object(Bucket=bucket, Key=key) except ClientError as e: raise S3EncryptionClientError( - "Exception encountered while fetching Instruction File." + f"Exception encountered while fetching Instruction File. " + f"Ensure the object you are attempting to decrypt has been encrypted using the S3 Encryption Client. " + f"Instruction key: {key}" ) from e finally: # Clear the flags after the call