From be29f44d87448f88d842f6810a877421832dd81d Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 27 Sep 2026 07:03:01 +0800 Subject: [PATCH] s3: record requester identity before the authz verdict (#11479) * s3: record requester identity before the authz verdict for audit Identity was only stored in request context on the success branch, so denied requests reached WriteErrorResponse without requester attribution and audit entries had empty requester/requester_arn/requester_identity. Authentication failures still resolve no identity, so unauthenticated denials stay unattributed. Fixes #11474 Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: keep the resolved identity through authz denial in Auth Review follow-up on #11479 (devin): authRequest discarded the identity on every error, so a request that authenticated fine but failed the action check still reached handleAuthResult with no identity and the deny path could not audit a requester. Auth now calls authRequestWithAuthType directly, the same entry AuthPostPolicy uses, so the resolved identity reaches the error writer; a failed authN still resolves no identity and stays unattributed. The regression test now signs a denied request end to end through iam.Auth. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- weed/s3api/auth_audit_requester_test.go | 37 +++++++++++++++++++++++++ weed/s3api/auth_credentials.go | 14 ++++++---- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/weed/s3api/auth_audit_requester_test.go b/weed/s3api/auth_audit_requester_test.go index df92fd353..189f328e2 100644 --- a/weed/s3api/auth_audit_requester_test.go +++ b/weed/s3api/auth_audit_requester_test.go @@ -144,6 +144,43 @@ func TestAuditRequesterIdentityEmptyForNonFederatedSession(t *testing.T) { assert.Empty(t, log.RequesterIdentity, "non-federated session must not surface the session id as a requester_identity") } +// Authentication resolves the identity before the policy verdict, so a denied +// request still has a requester and the audit entry must name it. See issue +// #11474. +func TestAuditRequesterForDeniedRequest(t *testing.T) { + iam := &IdentityAccessManagement{} + require.NoError(t, iam.loadS3ApiConfiguration(&iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{ + Name: "read_only_user", + Credentials: []*iam_pb.Credential{{AccessKey: "readonly_access_key", SecretKey: "readonly_secret_key"}}, + Actions: []string{"Read", "List"}, + }}, + })) + + denied, err := newTestRequest(http.MethodDelete, "http://s3/bucket/obj", 0, nil) + require.NoError(t, err) + require.NoError(t, signRequestV4(denied, "readonly_access_key", "readonly_secret_key")) + denied = s3_constants.EnsureIdentityHolder(denied) + + rec := httptest.NewRecorder() + iam.Auth(func(http.ResponseWriter, *http.Request) { + t.Error("denied request must not reach the handler") + }, s3_constants.ACTION_WRITE)(rec, denied) + + require.Equal(t, http.StatusForbidden, rec.Code) + log := s3err.GetAccessLog(denied, rec.Code, s3err.ErrAccessDenied) + assert.Equal(t, "read_only_user", log.Requester, "denied request must still audit the requester") + assert.Equal(t, "arn:aws:iam::"+defaultAccountID+":user/read_only_user", log.RequesterArn) + + anonymous := s3_constants.EnsureIdentityHolder(httptest.NewRequest(http.MethodDelete, "http://s3/bucket/obj", nil)) + rec = httptest.NewRecorder() + iam.handleAuthResult(rec, anonymous, nil, s3err.ErrAccessDenied, func(http.ResponseWriter, *http.Request) { + t.Error("denied request must not reach the handler") + }) + log = s3err.GetAccessLog(anonymous, rec.Code, s3err.ErrAccessDenied) + assert.Empty(t, log.Requester, "unauthenticated denial must not attribute a requester") +} + // A JWT-authenticated identity carries no PrincipalArn of its own — the auth // layer hands the principal over in a request header — so the audit entry has to // resolve the ARN the same way policy evaluation does. diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index 387f51275..7b8b491d1 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -1565,7 +1565,10 @@ func (iam *IdentityAccessManagement) Auth(f http.HandlerFunc, action Action) htt return } - identity, errCode := iam.authRequest(r, action) + // authRequestWithAuthType keeps the resolved identity when authN + // succeeds but authZ denies, so the denied request still audits its + // requester; a failed authN resolves no identity at all. + identity, errCode, _ := iam.authRequestWithAuthType(r, action) if errCode != s3err.ErrNone { glog.V(3).Infof("auth error: %v", errCode) } @@ -1631,11 +1634,12 @@ func recordIdentityInContext(r *http.Request, identity *Identity) context.Contex } func (iam *IdentityAccessManagement) handleAuthResult(w http.ResponseWriter, r *http.Request, identity *Identity, errCode s3err.ErrorCode, f http.HandlerFunc) { + // Store the authenticated identity in request context (secure, cannot be spoofed) + // even on the deny path so audit records for rejected requests keep requester attribution + if identity != nil && identity.Name != "" { + r = r.WithContext(recordIdentityInContext(r, identity)) + } if errCode == s3err.ErrNone { - // Store the authenticated identity in request context (secure, cannot be spoofed) - if identity != nil && identity.Name != "" { - r = r.WithContext(recordIdentityInContext(r, identity)) - } f(w, r) return }