Fix SigV4 auth failure due to missing req.GetBody on ES/OS writes#8308
Conversation
Signed-off-by: Anthony Marchante <anthony.marchante@doordash.com> Made-with: Cursor
dd-tone
force-pushed
the
fix-sigv4-getbody-roundtripper
branch
from
c09446b to
b529777
Compare
There was a problem hiding this comment.
Pull request overview
This pull request fixes a SigV4 authentication failure issue that affects all PUT/POST requests (bulk writes, template creation) when using the sigv4auth extension with Elasticsearch/OpenSearch backends. The root cause is that the olivere/elastic v7 client sets req.Body directly without setting req.GetBody, which breaks HTTP authenticators that rely on GetBody to hash the request payload for signing.
Changes:
- Introduce a
getBodyFixRoundTrippermiddleware that populatesreq.GetBodywheneverreq.Bodyis set butGetBodyis nil, ensuring authenticators see properly populatedGetBodyfields - The middleware reads the body into a buffer and replaces both
req.Bodyand setsreq.GetBodyto return fresh readers over that buffer - Insert the middleware in
GetHTTPRoundTripper()immediately before the authenticator wrapping, so it sits between the base transport and the authenticator - Add three comprehensive unit tests covering all cases: body set with GetBody nil, preserving existing GetBody, and nil body (no-op)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/storage/elasticsearch/config/config.go |
Added getBodyFixRoundTripper middleware type with RoundTrip method and integrated it into the GetHTTPRoundTripper function |
internal/storage/elasticsearch/config/config_test.go |
Added three new unit tests for the middleware and a helper recordingRoundTripper for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8308 +/- ##
==========================================
+ Coverage 95.80% 95.84% +0.03%
==========================================
Files 319 319
Lines 16819 16829 +10
==========================================
+ Hits 16114 16130 +16
+ Misses 549 545 -4
+ Partials 156 154 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| func (t *getBodyFixRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| if req.Body != nil && req.GetBody == nil { | ||
| body, err := io.ReadAll(req.Body) | ||
| if err != nil { |
There was a problem hiding this comment.
can you add a test for this branch?
There was a problem hiding this comment.
@yurishkuro Added a test for the io.ReadAll error branch, all of the middleware cases are covered now.
Head branch was pushed to by a user without write access
Signed-off-by: dd-tone <anthony.marchante@doordash.com> Made-with: Cursor
dd-tone
force-pushed
the
fix-sigv4-getbody-roundtripper
branch
from
a868fe9 to
e312793
Compare
Which problem is this PR solving?
Resolves #8307
When using the
sigv4authextension with an OpenSearch backend, all PUT/POST requests (bulk writes, template creation) fail withHTTP 403 Forbiddenand the AWS error message:This affects any deployment using SigV4 authentication with Elasticsearch or OpenSearch.
Description of the changes
Root cause: The
olivere/elasticv7 client creates HTTP requests withhttp.NewRequest(method, url, nil)and then setsreq.Bodydirectly viasetBodyReader(), without ever settingreq.GetBody. The OTelsigv4authextension'shashPayloadfunction callsreq.GetBodyto read the payload for SigV4 signing. WhenGetBodyis nil, it falls back to hashing an empty body (e3b0c44...), causing a signature mismatch on every request that carries a body.Fix: Introduce a
getBodyFixRoundTrippermiddleware that wraps the basehttp.RoundTripperand ensuresreq.GetBodyis populated wheneverreq.Bodyis set butGetBodyis nil. The middleware is inserted inGetHTTPRoundTripper()immediately before thehttpAuth.RoundTripper()call, so the authenticator always sees a properly populatedGetBody.The middleware:
req.Bodyinto a byte bufferreq.Bodywith a new reader over the bufferreq.GetBodyto return a fresh reader over the same bufferGetBodyis already set orBodyis nilHow was this change tested?
make fmt-- passedmake lint-- 0 issuesmake test-- all package tests pass, including 3 new unit tests:TestGetBodyFixRoundTripper_SetsGetBody-- Body set, GetBody nil: verifies GetBody is populated and returns correct bytesTestGetBodyFixRoundTripper_PreservesExistingGetBody-- Body and GetBody both set: verifies existing GetBody is not overwrittenTestGetBodyFixRoundTripper_NilBody-- Body nil: verifies no-op passthroughChecklist
make lint testAI Usage in this PR (choose one)
See AI Usage Policy.
Made with Cursor