Browse Source
s3/iam: reuse one request id per request (#8538)
s3/iam: reuse one request id per request (#8538)
* request_id: add shared request middleware
* s3err: preserve request ids in responses and logs
* iam: reuse request ids in XML responses
* sts: reuse request ids in XML responses
* request_id: drop legacy header fallback
* request_id: use AWS-style request id format
* iam: fix AWS-compatible XML format for ErrorResponse and field ordering
- ErrorResponse uses bare <RequestId> at root level instead of
<ResponseMetadata> wrapper, matching the AWS IAM error response spec
- Move CommonResponse to last field in success response structs so
<ResponseMetadata> serializes after result elements
- Add randomness to request ID generation to avoid collisions
- Add tests for XML ordering and ErrorResponse format
* iam: remove duplicate error_response_test.go
Test is already covered by responses_test.go.
* address PR review comments
- Guard against typed nil pointers in SetResponseRequestID before
interface assertion (CodeRabbit)
- Use regexp instead of strings.Index in test helpers for extracting
request IDs (Gemini)
* request_id: prevent spoofing, fix nil-error branch, thread reqID to error writers
- Ensure() now always generates a server-side ID, ignoring client-sent
x-amz-request-id headers to prevent request ID spoofing. Uses a
private context key (contextKey{}) instead of the header string.
- writeIamErrorResponse in both iamapi and embedded IAM now accepts
reqID as a parameter instead of calling Ensure() internally, ensuring
a single request ID per request lifecycle.
- The nil-iamError branch in writeIamErrorResponse now writes a 500
Internal Server Error response instead of returning silently.
- Updated tests to set request IDs via context (not headers) and added
tests for spoofing prevention and context reuse.
* sts: add request-id consistency assertions to ActionInBody tests
* test: update admin test to expect server-generated request IDs
The test previously sent a client x-amz-request-id header and expected
it echoed back. Since Ensure() now ignores client headers to prevent
spoofing, update the test to verify the server returns a non-empty
server-generated request ID instead.
* iam: add generic WithRequestID helper alongside reflection-based fallback
Add WithRequestID[T] that uses generics to take the address of a value
type, satisfying the pointer receiver on SetRequestId without reflection.
The existing SetResponseRequestID is kept for the two call sites that
operate on interface{} (from large action switches where the concrete
type varies at runtime). Generics cannot replace reflection there since
Go cannot infer type parameters from interface{}.
* Remove reflection and generics from request ID setting
Call SetRequestId directly on concrete response types in each switch
branch before boxing into interface{}, eliminating the need for
WithRequestID (generics) and SetResponseRequestID (reflection).
* iam: return pointer responses in action dispatch
* Fix IAM error handling consistency and ensure request IDs on all responses
- UpdateUser/CreatePolicy error branches: use writeIamErrorResponse instead
of s3err.WriteErrorResponse to preserve IAM formatting and request ID
- ExecuteAction: accept reqID parameter and generate one if empty, ensuring
every response carries a RequestId regardless of caller
* Clean up inline policies on DeleteUser and UpdateUser rename
DeleteUser: remove InlinePolicies[userName] from policy storage before
removing the identity, so policies are not orphaned.
UpdateUser: move InlinePolicies[userName] to InlinePolicies[newUserName]
when renaming, so GetUserPolicy/DeleteUserPolicy work under the new name.
Both operations persist the updated policies and return an error if
the storage write fails, preventing partial state.
pull/8541/head
committed by
GitHub
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
20 changed files with 523 additions and 244 deletions
-
11test/volume_server/http/admin_test.go
-
34weed/iam/error_response_test.go
-
24weed/iam/responses.go
-
30weed/iam/responses_test.go
-
16weed/iamapi/iamapi_handlers.go
-
219weed/iamapi/iamapi_management_handlers.go
-
2weed/iamapi/iamapi_server.go
-
26weed/iamapi/iamapi_test.go
-
170weed/s3api/s3api_embedded_iam.go
-
18weed/s3api/s3api_embedded_iam_test.go
-
2weed/s3api/s3api_server.go
-
10weed/s3api/s3api_sts.go
-
3weed/s3api/s3err/audit_fluent.go
-
19weed/s3api/s3err/audit_fluent_test.go
-
13weed/s3api/s3err/error_handler.go
-
36weed/s3api/s3err/error_handler_test.go
-
14weed/s3api/sts_params_test.go
-
22weed/server/common.go
-
48weed/util/request_id/request_id.go
-
50weed/util/request_id/request_id_test.go
@ -1,34 +0,0 @@ |
|||
package iam |
|||
|
|||
import ( |
|||
"encoding/xml" |
|||
"strings" |
|||
"testing" |
|||
|
|||
"github.com/stretchr/testify/assert" |
|||
"github.com/stretchr/testify/require" |
|||
) |
|||
|
|||
func TestErrorResponseXMLUsesTopLevelRequestId(t *testing.T) { |
|||
errCode := "NoSuchEntity" |
|||
errMsg := "the requested IAM entity does not exist" |
|||
|
|||
resp := ErrorResponse{ |
|||
RequestId: "request-123", |
|||
} |
|||
resp.Error.Type = "Sender" |
|||
resp.Error.Code = &errCode |
|||
resp.Error.Message = &errMsg |
|||
|
|||
output, err := xml.Marshal(resp) |
|||
require.NoError(t, err) |
|||
|
|||
xmlString := string(output) |
|||
errorIndex := strings.Index(xmlString, "<Error>") |
|||
requestIDIndex := strings.Index(xmlString, "<RequestId>request-123</RequestId>") |
|||
|
|||
assert.NotEqual(t, -1, errorIndex, "Error should be present") |
|||
assert.NotEqual(t, -1, requestIDIndex, "RequestId should be present") |
|||
assert.NotContains(t, xmlString, "<ResponseMetadata>") |
|||
assert.Less(t, errorIndex, requestIDIndex, "RequestId should appear after Error at the root level") |
|||
} |
|||
@ -0,0 +1,19 @@ |
|||
package s3err |
|||
|
|||
import ( |
|||
"net/http" |
|||
"net/http/httptest" |
|||
"testing" |
|||
|
|||
"github.com/seaweedfs/seaweedfs/weed/util/request_id" |
|||
"github.com/stretchr/testify/assert" |
|||
) |
|||
|
|||
func TestGetAccessLogUsesAmzRequestID(t *testing.T) { |
|||
req := httptest.NewRequest(http.MethodGet, "/bucket/object", nil) |
|||
req = req.WithContext(request_id.Set(req.Context(), "req-123")) |
|||
|
|||
log := GetAccessLog(req, http.StatusOK, ErrNone) |
|||
|
|||
assert.Equal(t, "req-123", log.RequestID) |
|||
} |
|||
@ -0,0 +1,36 @@ |
|||
package s3err |
|||
|
|||
import ( |
|||
"net/http" |
|||
"net/http/httptest" |
|||
"regexp" |
|||
"testing" |
|||
|
|||
"github.com/gorilla/mux" |
|||
"github.com/seaweedfs/seaweedfs/weed/util/request_id" |
|||
"github.com/stretchr/testify/assert" |
|||
) |
|||
|
|||
func TestWriteErrorResponseReusesRequestID(t *testing.T) { |
|||
req := httptest.NewRequest(http.MethodGet, "/bucket/object", nil) |
|||
req = mux.SetURLVars(req, map[string]string{ |
|||
"bucket": "bucket", |
|||
"object": "object", |
|||
}) |
|||
req = req.WithContext(request_id.Set(req.Context(), "req-123")) |
|||
|
|||
rr := httptest.NewRecorder() |
|||
WriteErrorResponse(rr, req, ErrNoSuchKey) |
|||
|
|||
assert.Equal(t, "req-123", rr.Header().Get(request_id.AmzRequestIDHeader)) |
|||
assert.Equal(t, "req-123", extractRequestIDFromBody(rr.Body.String())) |
|||
} |
|||
|
|||
func extractRequestIDFromBody(body string) string { |
|||
re := regexp.MustCompile(`<RequestId>([^<]+)</RequestId>`) |
|||
matches := re.FindStringSubmatch(body) |
|||
if len(matches) < 2 { |
|||
return "" |
|||
} |
|||
return matches[1] |
|||
} |
|||
@ -0,0 +1,50 @@ |
|||
package request_id |
|||
|
|||
import ( |
|||
"net/http/httptest" |
|||
"regexp" |
|||
"testing" |
|||
) |
|||
|
|||
var requestIDPattern = regexp.MustCompile(`^[0-9A-F]+$`) |
|||
|
|||
func TestNewUsesUppercaseHexFormat(t *testing.T) { |
|||
id := New() |
|||
if !requestIDPattern.MatchString(id) { |
|||
t.Fatalf("expected uppercase hex request id, got %q", id) |
|||
} |
|||
if len(id) < 24 { |
|||
t.Fatalf("expected request id to be at least 24 characters, got %q (len=%d)", id, len(id)) |
|||
} |
|||
} |
|||
|
|||
func TestNewIsUnique(t *testing.T) { |
|||
a := New() |
|||
b := New() |
|||
if a == b { |
|||
t.Fatalf("expected unique request ids, got %q twice", a) |
|||
} |
|||
} |
|||
|
|||
func TestEnsureIgnoresClientHeader(t *testing.T) { |
|||
req := httptest.NewRequest("GET", "/", nil) |
|||
req.Header.Set(AmzRequestIDHeader, "spoofed-id") |
|||
|
|||
req, id := Ensure(req) |
|||
if id == "spoofed-id" { |
|||
t.Fatal("Ensure should not trust client-sent x-amz-request-id header") |
|||
} |
|||
if !requestIDPattern.MatchString(id) { |
|||
t.Fatalf("expected server-generated hex id, got %q", id) |
|||
} |
|||
} |
|||
|
|||
func TestEnsureReusesContextID(t *testing.T) { |
|||
req := httptest.NewRequest("GET", "/", nil) |
|||
req = req.WithContext(Set(req.Context(), "ctx-id-123")) |
|||
|
|||
req, id := Ensure(req) |
|||
if id != "ctx-id-123" { |
|||
t.Fatalf("expected context id ctx-id-123, got %q", id) |
|||
} |
|||
} |
|||
Write
Preview
Loading…
Cancel
Save
Reference in new issue