fix(bedrock): retrieve managed file content via presigned S3 GET (#26335) - #31433
fix(bedrock): retrieve managed file content via presigned S3 GET (#26335)#31433kingdoooo wants to merge 13 commits into
Conversation
Greptile SummaryThis PR fixes
Confidence Score: 4/5Safe to merge with minor follow-up work; the core presign-and-validate path is correct and well-tested. The bucket-injection protection is correctly implemented in _get_trusted_credentials (only MappingProxyType is trusted, not a plain dict), but the dedicated test that would catch a regression of that specific check was deleted during the handler-to-config migration and has no replacement in the new test files. Everything else — SigV4 signing, multi-bucket fallthrough, region precedence, and the generic-handler wiring — is directly exercised by the new tests. There is also a dead-code raise at the end of _validate_against_configured_buckets that can safely be removed. tests/test_litellm/llms/bedrock/files/test_bedrock_files_handler.py — the plain-dict _litellm_internal_model_credentials injection test was removed without a replacement.
|
| Filename | Overview |
|---|---|
| litellm/llms/bedrock/files/transformation.py | Implements transform_file_content_request (generates a botocore presigned S3 GET URL) and transform_file_content_response (wraps raw bytes); adds helper methods for trusted-snapshot-only bucket resolution, region resolution, and multi-bucket validation. One unreachable sentinel raise at end of _validate_against_configured_buckets. |
| litellm/llms/bedrock/files/handler.py | Deleted — the standalone BedrockFilesHandler class (which used asyncio.run and direct s3_client.get_object) is removed; its responsibilities now live in BedrockFilesConfig and the shared generic handler. |
| litellm/files/main.py | Removes the Bedrock-specific elif custom_llm_provider == 'bedrock' branch; Bedrock now flows through the generic provider_config = ProviderConfigManager.get_provider_files_config(...) path (already present for Anthropic/Manus). Import and module-level instance also cleaned up. |
| tests/test_litellm/llms/bedrock/files/test_bedrock_files_handler.py | Migrates tests from BedrockFilesHandler to BedrockFilesConfig; removes four security-boundary tests including test_should_not_trust_user_supplied_internal_credentials_dict, which verified that a plain dict passed as _litellm_internal_model_credentials is not trusted. The code still enforces this correctly, but the dedicated test is gone. |
| tests/test_litellm/llms/bedrock/files/test_bedrock_files_transformation.py | Adds comprehensive new test classes covering presigned-URL shape, SigV4 verification, partition-correct hostnames (China), region precedence from trusted snapshot, input/output bucket multi-bucket validation, base64 unified-id decoding, SSRF rejection, and a full end-to-end wiring test through the generic handler. |
Reviews (1): Last reviewed commit: "chore: drop internal SDD scaffolding fro..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def test_should_reject_unified_unmanaged_s3_uri(self): | ||
| file_id = _encode_unified_file_id("s3://safe-bucket/private/output.jsonl") | ||
| s3_uri = self.handler._extract_s3_uri_from_file_id(file_id) | ||
|
|
||
| s3_uri = self.config._extract_s3_uri_from_file_id(file_id) | ||
| with pytest.raises(ValueError, match="LiteLLM-managed"): | ||
| self.handler._parse_s3_uri( | ||
| s3_uri=s3_uri, | ||
| configured_bucket_name="safe-bucket", | ||
| ) | ||
|
|
||
| def test_should_not_trust_request_s3_bucket_name_for_expected_bucket(self): | ||
| with patch.dict(os.environ, {"AWS_S3_BUCKET_NAME": "safe-bucket"}): | ||
| assert ( | ||
| self.handler._get_configured_s3_bucket_name( | ||
| {"s3_bucket_name": "attacker-bucket"} | ||
| ) | ||
| == "safe-bucket" | ||
| ) | ||
|
|
||
| def test_should_trust_proxy_config_s3_bucket_name_for_expected_bucket(self): | ||
| trusted_credentials = MappingProxyType({"s3_bucket_name": "safe-bucket"}) | ||
|
|
||
| with patch.dict(os.environ, {}, clear=True): | ||
| assert ( | ||
| self.handler._get_configured_s3_bucket_name( | ||
| { | ||
| "s3_bucket_name": "attacker-bucket", | ||
| "_litellm_internal_model_credentials": trusted_credentials, | ||
| } | ||
| ) | ||
| == "safe-bucket" | ||
| ) | ||
|
|
||
| def test_should_not_trust_user_supplied_internal_credentials_dict(self): | ||
| with patch.dict(os.environ, {}, clear=True): | ||
| with pytest.raises(ValueError, match="S3 bucket_name is required"): | ||
| self.handler._get_configured_s3_bucket_name( | ||
| { | ||
| "_litellm_internal_model_credentials": { | ||
| "s3_bucket_name": "attacker-bucket" | ||
| } | ||
| } | ||
| ) | ||
|
|
||
| def test_should_require_server_s3_bucket_name(self): | ||
| with patch.dict(os.environ, {}, clear=True): | ||
| with pytest.raises(ValueError, match="S3 bucket_name is required"): | ||
| self.handler._get_configured_s3_bucket_name( | ||
| {"s3_bucket_name": "attacker-bucket"} | ||
| ) | ||
| _parse(s3_uri, "safe-bucket") |
There was a problem hiding this comment.
The old handler test class included test_should_not_trust_user_supplied_internal_credentials_dict, which explicitly verified that passing a plain dict as _litellm_internal_model_credentials is rejected (only a MappingProxyType is trusted). That test was deleted and has no direct equivalent in either the new handler tests or in test_bedrock_files_transformation.py. The _get_trusted_credentials code in transformation.py still enforces this correctly via isinstance(snapshot, type(MappingProxyType({}))), but the test that would catch a future regression of that specific check is now gone.
Rule Used: What: Flag any modifications to existing tests and... (source)
| except ValueError: | ||
| if is_last_bucket: | ||
| raise | ||
| raise ValueError("file_id must reference a LiteLLM-managed storage object") |
There was a problem hiding this comment.
raise ValueError(...) sentinel after the loop is unreachable. _get_configured_s3_buckets guarantees the returned tuple is non-empty (it raises if empty), so the loop always iterates at least once. On the final iteration, if validate_managed_cloud_file_id raises, is_last_bucket is True and the inner raise re-propagates it — execution never reaches the outer raise.
| except ValueError: | |
| if is_last_bucket: | |
| raise | |
| raise ValueError("file_id must reference a LiteLLM-managed storage object") | |
| except ValueError: | |
| if is_last_bucket: | |
| raise |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| object_key=object_key, | ||
| litellm_params=litellm_params, | ||
| ) | ||
| return url, {} |
There was a problem hiding this comment.
Medium: Presigned URL leak through logging
transform_file_content_request() returns the full presigned S3 URL, and retrieve_file_content() immediately passes that value to logging_obj.pre_call() as api_base. A user or integration with access to LiteLLM request logs can copy the X-Amz-Signature URL and download the Bedrock-managed file for the next 5 minutes; keep the signed query out of logged fields, for example by redacting X-Amz-* query parameters before logging or by splitting the presigned query into params while logging only the unsigned URL.
PR overviewThis pull request updates the Bedrock files integration so managed file content can be retrieved through a presigned S3 GET URL. The touched code handles transforming file-content requests and invoking the retrieval path for Bedrock-managed files. One security issue remains open: the presigned S3 URL used for file retrieval is passed into request logging, which can expose a short-lived download link to anyone with access to those logs. This creates a concrete data-access risk for Bedrock-managed file content during the URL validity window. No issues have been addressed yet, so the PR still needs a logging redaction or restructuring fix before the exposure is resolved. Open issues (1)
Fixed/addressed: 0 · PR risk: 7/10 |
|
Closing in favor of a focused follow-up. litellm_internal_staging already implements Bedrock file-content retrieval (via header-signed SigV4 GetObject, OSS staging sync #30745), which supersedes this PR's presigned-URL approach for the same fix. The one gap that remains is that the merged retrieval path validates file ids only against the configured input bucket (s3_bucket_name) and ignores s3_output_bucket_name, so a deployment whose Bedrock batch outputs land in a separate output bucket cannot retrieve them. I will open a small follow-up that adds output-bucket support on top of the existing implementation rather than replacing it. |
1 participant
Relevant issues
Fixes #26335 - "File Retrieval always fail for bedrock".
GET /v1/files/{id}/contentreturned a 500 for every Bedrock-backed managed file.Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
make test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Screenshots / Proof of Fix
Live proxy on localhost against real AWS (us-east-1), real S3. The decisive contrast is retrieval (T5-R): the same
GET /v1/files/{id}/contentreturns 500 on the code before this PR and the object bytes after. The output retrieved in T5 is a genuine Bedrock batch output produced end to end.T5 - retrieve a real completed Bedrock batch output through the proxy; returns 200 with the batch output JSONL (the req-1 line carries the PROOF-OF-FIX-26335 marker)
T5-R - the decisive before/after on the same command. PRE (base commit, without this PR) returns 500 "BedrockFilesConfig does not support file content retrieval"; POST (this PR) returns 200 with the object bytes
T6 - the bytes returned by the proxy match the raw S3 object exactly (identical SHA-256, 110/110 records), proving the content is the real object and not transformed or truncated
T7 - a file_id for a bucket the proxy is not configured for is rejected, not presigned or fetched; the response is 500 "file_id bucket does not match the configured storage bucket" (SSRF / bucket-confusion guard)
Type
🐛 Bug Fix
Changes
BedrockFilesConfig.transform_file_content_requestandtransform_file_content_responsepreviously raisedNotImplementedError, so the generic retrieval handler short-circuited to a 500 for every Bedrock managed file. This implements the two transforms so Bedrock flows through the same genericretrieve_file_contentpath as Vertex AI, Anthropic and Manus.The request transform extracts the
s3://URI from the file_id (decoding a base64 unified id or accepting a direct uri), validates it against the configured input and output buckets, and returns a botocore presigned S3 GET url. A presigned url is the only shape that fits a handler which computes the url and headers independently: the authentication lives entirely in the query string. Presigning is pure-local botocore withConfig(signature_version="s3v4", s3={"addressing_style": "path"})and no explicitendpoint_url, so botocore derives the correct per-partition host (standard, China, GovCloud) while staying regional, and STS credentials flow through asX-Amz-Security-Token.ExpiresInis 300s to bound replay of a leaked url. The response transform wraps the raw bytes.All bucket and region configuration is read from the trusted
_litellm_internal_model_credentialssnapshot the proxy forwards, never from request-supplied keys, which is what prevents a caller from redirecting the presign at an arbitrary bucket. Validation accepts the file_id if it matches either the configured input bucket (honoring its optional prefix) or the output bucket, because Bedrock writes batch results tos3://{output_bucket}/litellm-batch-outputs/<job>/at bucket root.The now-superseded
BedrockFilesHandlerand its unreachable branch infiles/main.pyare deleted; its SSRF and path-traversal coverage moves onto the shared validator and the config. Known limitations are documented in code: the shared handler does not forwardtimeoutorraise_for_status(generic, pre-existing), and web-identity (OIDC) auth uses an STS session-policy ceiling that does not yet includes3:GetObject.Tests in
tests/test_litellm/llms/bedrock/files/exercise the real transform logic with only the AWS/S3 boundary mocked: presigned-url shape and partition correctness, region precedence from the trusted snapshot, base64 unified-id decoding, the input/output bucket validation including batch-output-at-root,aws_external_idforwarding, raw-bytes round-trip, SSRF rejection, and a wiring test that drives the real generic handler so the presigned signature is shown surviving the handler's query-param reconstruction.