Browse Source

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.
pull/7944/head
Chris Lu 1 month ago
parent
commit
5331e21f49
  1. 6
      weed/iam/sts/session_claims_test.go
  2. 32
      weed/s3api/auth_credentials.go
  3. 9
      weed/s3api/auth_signature_v4.go
  4. 15
      weed/s3api/auth_sts_v4_test.go

6
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 {

32
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
}

9
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
}

15
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)
}
}
Loading…
Cancel
Save