From 5331e21f49219ccbfe800a60e27a8cbf16b72c47 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 2 Jan 2026 22:49:22 -0800 Subject: [PATCH] refactor(s3api): improve code quality and performance - Rename authorization path constants to avoid conflict with existing authType enum - Replace nested if/else with clean switch statement in authorizeWithIAM() - Add determineIAMAuthPath() helper for clearer intent and testability - Optimize key counting in auth_signature_v4.go: remove unnecessary slice allocation - Fix timing assertion in session_claims_test.go: use WithinDuration for symmetric tolerance These changes improve code readability, maintainability, and performance while maintaining full backward compatibility and test coverage. --- weed/iam/sts/session_claims_test.go | 6 +++--- weed/s3api/auth_credentials.go | 32 ++++++++++++++++++++++++----- weed/s3api/auth_signature_v4.go | 9 +++----- weed/s3api/auth_sts_v4_test.go | 15 +++++++------- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/weed/iam/sts/session_claims_test.go b/weed/iam/sts/session_claims_test.go index 7408ef5dd..5cd87d726 100644 --- a/weed/iam/sts/session_claims_test.go +++ b/weed/iam/sts/session_claims_test.go @@ -177,9 +177,9 @@ func TestSTSSessionClaimsToSessionInfoCredentialExpiration(t *testing.T) { sessionInfo := claims.ToSessionInfo() assert.NotNil(t, sessionInfo.Credentials) - // Check expiration within 1 second due to timing precision - assert.True(t, sessionInfo.Credentials.Expiration.Sub(tc.expiresAt) < time.Second) - + // Check expiration within 1 second due to timing precision (symmetric tolerance) + assert.WithinDuration(t, tc.expiresAt, sessionInfo.Credentials.Expiration, time.Second, + "credential expiration should be within 1 second of session expiration") // We set tc.expiresAt to past/future values to exercise expiration handling. // Assert the credentials' expiration relative to now to exercise code behavior if tc.expectNotExpired { diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index 83dc6e91c..89bd1d216 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -951,6 +951,26 @@ func (iam *IdentityAccessManagement) authenticateJWTWithIAM(r *http.Request) (*I return identity, s3err.ErrNone } +// IAM authorization path type constants +const ( + iamAuthPathJWT = "jwt" + iamAuthPathSTS_V4 = "sts_v4" + iamAuthPathStatic_V4 = "static_v4" + iamAuthPathNone = "none" +) + +// determineIAMAuthPath determines the IAM authorization path based on available tokens and principals +func determineIAMAuthPath(sessionToken, principal, principalArn string) string { + if sessionToken != "" && principal != "" { + return iamAuthPathJWT + } else if sessionToken != "" && principalArn != "" { + return iamAuthPathSTS_V4 + } else if principalArn != "" { + return iamAuthPathStatic_V4 + } + return iamAuthPathNone +} + // authorizeWithIAM authorizes requests using the IAM integration policy engine func (iam *IdentityAccessManagement) authorizeWithIAM(r *http.Request, identity *Identity, action Action, bucket string, object string) s3err.ErrorCode { ctx := r.Context() @@ -977,23 +997,25 @@ func (iam *IdentityAccessManagement) authorizeWithIAM(r *http.Request, identity Account: identity.Account, } - // Handle both session-based (JWT and STS) and static-key-based (V4 signature) principals - if sessionToken != "" && principal != "" { + // Determine authorization path and configure identity + authPath := determineIAMAuthPath(sessionToken, principal, identity.PrincipalArn) + switch authPath { + case iamAuthPathJWT: // JWT-based authentication - use session token and principal from headers iamIdentity.Principal = principal iamIdentity.SessionToken = sessionToken glog.V(3).Infof("Using JWT-based IAM authorization for principal: %s", principal) - } else if sessionToken != "" && identity.PrincipalArn != "" { + case iamAuthPathSTS_V4: // STS V4 signature authentication - use session token (from X-Amz-Security-Token) with principal ARN iamIdentity.Principal = identity.PrincipalArn iamIdentity.SessionToken = sessionToken glog.V(3).Infof("Using STS V4 signature IAM authorization for principal: %s with session token", identity.PrincipalArn) - } else if identity.PrincipalArn != "" { + case iamAuthPathStatic_V4: // Static V4 signature authentication - use principal ARN without session token iamIdentity.Principal = identity.PrincipalArn iamIdentity.SessionToken = "" glog.V(3).Infof("Using static V4 signature IAM authorization for principal: %s", identity.PrincipalArn) - } else { + default: glog.V(3).Info("No valid principal information for IAM authorization") return s3err.ErrAccessDenied } diff --git a/weed/s3api/auth_signature_v4.go b/weed/s3api/auth_signature_v4.go index 36c070e7e..3a54f037b 100644 --- a/weed/s3api/auth_signature_v4.go +++ b/weed/s3api/auth_signature_v4.go @@ -223,16 +223,13 @@ func (iam *IdentityAccessManagement) verifyV4Signature(r *http.Request, shouldCh var found bool identity, cred, found = iam.lookupByAccessKey(authInfo.AccessKey) if !found { - // Log detailed error information for InvalidAccessKeyId + // Log detailed error information for InvalidAccessKeyId (avoid slice allocation for performance) iam.m.RLock() - availableKeys := make([]string, 0, len(iam.accessKeyIdent)) - for key := range iam.accessKeyIdent { - availableKeys = append(availableKeys, key) - } + keyCount := len(iam.accessKeyIdent) iam.m.RUnlock() glog.Warningf("InvalidAccessKeyId: attempted key '%s' not found. Available keys: %d, Auth enabled: %v", - authInfo.AccessKey, len(availableKeys), iam.isAuthEnabled) + authInfo.AccessKey, keyCount, iam.isAuthEnabled) return nil, nil, "", nil, s3err.ErrInvalidAccessKeyID } diff --git a/weed/s3api/auth_sts_v4_test.go b/weed/s3api/auth_sts_v4_test.go index dd4b2cafc..792221305 100644 --- a/weed/s3api/auth_sts_v4_test.go +++ b/weed/s3api/auth_sts_v4_test.go @@ -134,17 +134,16 @@ func TestSTSSessionTokenIntoCredentials(t *testing.T) { func TestActionConstantsForV4Auth(t *testing.T) { // Verify that S3 action constants are available actions := map[string]string{ - "READ": s3_constants.ACTION_READ, - "WRITE": s3_constants.ACTION_WRITE, - "READ_ACP": s3_constants.ACTION_READ_ACP, - "WRITE_ACP": s3_constants.ACTION_WRITE_ACP, - "LIST": s3_constants.ACTION_LIST, - "TAGGING": s3_constants.ACTION_TAGGING, - "ADMIN": s3_constants.ACTION_ADMIN, + "READ": s3_constants.ACTION_READ, + "WRITE": s3_constants.ACTION_WRITE, + "READ_ACP": s3_constants.ACTION_READ_ACP, + "WRITE_ACP": s3_constants.ACTION_WRITE_ACP, + "LIST": s3_constants.ACTION_LIST, + "TAGGING": s3_constants.ACTION_TAGGING, + "ADMIN": s3_constants.ACTION_ADMIN, } for name, action := range actions { assert.NotEmpty(t, action, "Action %s should not be empty", name) } } -