Skip to content

feat(s3_v2): send Content-MD5 on PUT and optional server-side encryption - #31928

Merged
mubashir1osmani merged 3 commits into
litellm_internal_stagingfrom
litellm_s3_v2_content_md5
Jul 2, 2026
Merged

feat(s3_v2): send Content-MD5 on PUT and optional server-side encryption#31928
mubashir1osmani merged 3 commits into
litellm_internal_stagingfrom
litellm_s3_v2_content_md5

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Relevant issues

s3_v2 PUTs only sent x-amz-content-sha256, so buckets with Object Lock enabled reject the upload because AWS requires a Content-MD5 header on those PUTs per the S3 spec

Linear ticket

Pre-Submission checklist

  • I have added meaningful tests
  • My PR passes all CI/CD checks (e.g., lint, format, unit tests)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have requested a Greptile review by commenting @greptileai and received a Confidence Score of at least 4/5 before requesting a maintainer review

Screenshots / Proof of Fix

Ran a live proxy (litellm/proxy/proxy_cli.py) with s3_v2 logging pointed at a local S3-compatible sink via s3_endpoint_url, made a real Anthropic chat completion (real tokens, real $), and inspected the actual PUT the logger emitted. The sink recomputes the md5 of the body it received and compares it to the header

Real LLM call through the proxy:

$ curl -s -X POST http://127.0.0.1:4000/v1/chat/completions \
    -H "Content-Type: application/json" -H "Authorization: Bearer sk-1234" \
    -d '{"model":"claude","messages":[{"role":"user","content":"Say hello in 3 words"}]}'
{"id":"chatcmpl-...","model":"claude","object":"chat.completion","choices":[{"finish_reason":"stop","index":0,"message":{"content":"Hello, how are you?","role":"assistant"}}],"usage":{"completion_tokens":9,"prompt_tokens":14,"total_tokens":23}}

The PUT captured by the sink (real request the logger sent):

{
  "path": "/litellm-proof-bucket/2026-07-02/time-00-58-50-283780_chatcmpl-....json",
  "headers": {
    "content-type": "application/json",
    "content-md5": "FJNbl8YV7yqoOg5rAftWJQ==",
    "x-amz-content-sha256": "3dced6c57ddca77f3c16029b557811c56ba58568a5b5f0f32c9fa75fc4a6af93",
    "x-amz-server-side-encryption": "aws:kms",
    "authorization": "AWS4-HMAC-SHA256 Credential=.../s3/aws4_request, SignedHeaders=cache-control;content-disposition;content-language;content-length;content-md5;content-type;host;x-amz-content-sha256;x-amz-date;x-amz-server-side-encryption, Signature=..."
  },
  "recomputed_content_md5_of_body": "FJNbl8YV7yqoOg5rAftWJQ==",
  "content_md5_matches_body": true
}

content_md5_matches_body is true, and both content-md5 and x-amz-server-side-encryption appear in the SigV4 SignedHeaders, so Object Lock buckets will accept the upload rather than rejecting it as unsigned/missing

Type

🆕 New Feature
🐛 Bug Fix

Changes

Both PUT paths in s3_v2.py (async_upload_data_to_s3 and upload_data_to_s3) now compute and send Content-MD5

content_md5 = base64.b64encode(hashlib.md5(json_string.encode("utf-8")).digest()).decode()
headers = {
    ...,
    "Content-MD5": content_md5,
    ...,
}

The md5 is over the exact json_string that becomes the request body, and because the header is set before SigV4Auth().add_auth() it ends up in the signed header set

Also adds an optional s3_server_side_encryption param (configurable directly or via s3_callback_params) that, when set, sends x-amz-server-side-encryption (e.g. aws:kms) for buckets with a KMS default-encryption policy. It is injected into the header dict only when configured, so behavior is unchanged when it is unset

Tests in tests/test_litellm/integrations/test_s3_v2.py assert the Content-MD5 header matches base64(md5(body)) on both the async and sync PUT paths, that x-amz-server-side-encryption is absent by default and present with the correct value when configured, and that the param resolves from s3_callback_params

Link to Devin session: https://app.devin.ai/sessions/3fddf4390d7948ba879615b49e6ef468
Requested by: @mubashir1osmani


Note

Medium Risk
Changes the signed S3 PUT header set for all s3_v2 log uploads; miscomputed MD5 would break uploads, though behavior is covered by tests and SSE remains opt-in.

Overview
s3_v2 logging PUTs now include a Content-MD5 header (base64 MD5 of the exact JSON body) on both async_upload_data_to_s3 and upload_data_to_s3, so Object Lock–enabled buckets accept uploads instead of rejecting them for missing MD5.

Adds optional s3_server_side_encryption (constructor arg or s3_callback_params). When set, PUTs also send x-amz-server-side-encryption (e.g. aws:kms); when unset, headers are unchanged.

New tests assert MD5 on async/sync paths, SSE header when configured, and loading SSE from global callback params.

Reviewed by Cursor Bugbot for commit 68a8fc2. Bugbot is set up for automated code reviews on this repo. Configure here.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@mubashir1osmani mubashir1osmani self-assigned this Jul 2, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@CLAassistant

CLAassistant commented Jul 2, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ cursoragent
❌ Mubashir Osmani


Mubashir Osmani seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes S3 PUT uploads for Object Lock–enabled buckets by adding the required Content-MD5 header (base64-encoded MD5 of the exact JSON body) to both the async and sync upload paths in s3_v2.py. It also adds an optional s3_server_side_encryption parameter for KMS-encrypted buckets.

  • Content-MD5 header: Computed from the identical json_string that becomes the request body, added to the header dict before SigV4Auth.add_auth(), so it is included in the signed header set. usedforsecurity=False is passed on both paths, which is correct for FIPS-mode environments and valid on Python 3.10+ (litellm's minimum).
  • s3_server_side_encryption: Optional; accepted as a constructor argument or via s3_callback_params. When set, x-amz-server-side-encryption is injected into the header dict before signing; when unset, behavior is identical to before.
  • Tests: Four new mocked unit tests cover async/sync MD5 correctness, SSE opt-in, and s3_callback_params resolution. A FIPS stub validates that usedforsecurity=False is threaded through correctly.

Confidence Score: 5/5

Safe to merge — both upload paths correctly compute and sign Content-MD5, the SSE param is fully opt-in, and all new tests are properly mocked.

The MD5 is computed over the exact bytes sent as the request body and is placed in the header dict before SigV4 signing, matching what AWS requires. usedforsecurity=False is present on both paths, so FIPS environments are handled. The SSE parameter defaults to None and is conditionally applied, leaving existing deployments unaffected. Tests are isolated (no network calls) and faithfully simulate both the normal and FIPS-restricted code paths.

No files require special attention.

Important Files Changed

Filename Overview
litellm/integrations/s3_v2.py Adds Content-MD5 header (base64 MD5, computed before SigV4 signing) to both async and sync PUT paths, and optionally injects x-amz-server-side-encryption; usedforsecurity=False is correctly used on both paths; s3_server_side_encryption is properly threaded through constructor and _configure_s3_logger
tests/test_litellm/integrations/test_s3_v2.py Four new unit tests added: async/sync Content-MD5 correctness, SSE header presence when configured, and SSE loading from s3_callback_params; all use mocked HTTP clients (no real network calls), and the FIPS-mode stub correctly validates usedforsecurity=False is passed

Reviews (2): Last reviewed commit: "fix: allow S3 Content-MD5 on FIPS hosts" | Re-trigger Greptile

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@yucheng-berri

Copy link
Copy Markdown
Contributor

are greptile comments legit and in scope?

@yucheng-berri

Copy link
Copy Markdown
Contributor

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: FIPS MD5 call may fail
    • MD5 Content-MD5 generation now passes usedforsecurity=False and has regression coverage for both S3 upload paths.

You can send follow-ups to the cloud agent here.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 68a8fc2. Configure here.

Comment thread litellm/integrations/s3_v2.py Outdated

# Calculate SHA256 hash of the content
content_hash = hashlib.sha256(json_string.encode("utf-8")).hexdigest()
content_md5 = base64.b64encode(hashlib.md5(json_string.encode("utf-8")).digest()).decode()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FIPS MD5 call may fail

Low Severity

New Content-MD5 computation uses hashlib.md5() without usedforsecurity=False. On FIPS-enabled OpenSSL setups that block MD5 for security, that call can raise before the PUT runs, so s3_v2 uploads fail even though the payload SHA-256 signing path still works.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 68a8fc2. Configure here.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

@yucheng-berri the Greptile finding is legit and in scope. Both hashlib.md5(...) calls lacked usedforsecurity=False, so on FIPS-hardened deployments (GovCloud/enterprise, which is exactly the Object Lock + KMS audience this PR targets) hashlib.md5() raises ValueError, and since both upload paths wrap the body in except Exception, every S3 write would silently fail instead of surfacing an error

Fixed in e542be1 by passing usedforsecurity=False at both call sites (async ~line 326, sync ~line 499) and mirroring it in the test helper. The md5 value is unchanged, so the existing Content-MD5 assertions still hold; this only prevents the FIPS crash

The rest of the Greptile summary is descriptive and accurate, no other actionable items

@yucheng-berri

Copy link
Copy Markdown
Contributor

can you reping greptile to verify again?

@mubashir1osmani

Copy link
Copy Markdown
Contributor

@greptile-apps

@mubashir1osmani
mubashir1osmani merged commit 9b6d0b0 into litellm_internal_staging Jul 2, 2026
121 checks passed
@mubashir1osmani
mubashir1osmani deleted the litellm_s3_v2_content_md5 branch July 2, 2026 23:17
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.

4 participants