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>
This commit is contained in:
Chris Lu
2026-09-27 07:03:01 +08:00
committed by GitHub
co-authored by Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
parent 2864bc0fe8
commit be29f44d87
2 changed files with 46 additions and 5 deletions
+37
View File
@@ -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.
+9 -5
View File
@@ -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
}