s3api: fix AccessDenied by correctly propagating principal ARN in vended tokens (#8330)

* s3api: fix AccessDenied by correctly propagating principal ARN in vended tokens * s3api: update TestLoadS3ApiConfiguration to match standardized ARN format * s3api: address PR review comments (nil-safety and cleanup) * s3api: address second round of PR review comments (cleanups and naming conventions) * s3api: address third round of PR review comments (unify default account ID and duplicate log) * s3api: address fourth round of PR review comments (define defaultAccountID as constant)
2026-02-12 23:11:41 -08:00
parent 1e4f30c56f
commit c433fee36a
4 changed files with 51 additions and 37 deletions
--- a/weed/s3api/auth_credentials.go
+++ b/weed/s3api/auth_credentials.go
@@ -100,6 +100,9 @@ type Account struct {
 	Id string
 }

+// Default account ID for all automated SeaweedFS accounts and fallback
+const defaultAccountID = "000000000000"
+
 // Predefined Accounts
 var (
 	// AccountAdmin is used as the default account for IAM-Credentials access without Account configured
@@ -809,7 +812,6 @@ func (iam *IdentityAccessManagement) MergeS3ApiConfiguration(config *iam_pb.S3Ap
 	iam.nameToIdentity = nameToIdentity
 	iam.accessKeyIdent = accessKeyIdent
 	iam.policies = policies
-	iam.accessKeyIdent = accessKeyIdent
 	// Update authentication state based on whether identities exist
 	// Once enabled, keep it enabled (one-way toggle)
 	authJustEnabled := iam.updateAuthenticationState(len(identities))
@@ -1010,11 +1012,11 @@ func generatePrincipalArn(identityName string) string {
 	// Handle special cases
 	switch identityName {
 	case AccountAnonymous.Id:
-		return "arn:aws:iam::user/anonymous"
+		return "*" // Use universal wildcard for anonymous allowed by bucket policy
 	case AccountAdmin.Id:
-		return "arn:aws:iam::user/admin"
+		return fmt.Sprintf("arn:aws:iam::%s:user/admin", defaultAccountID)
 	default:
-		return fmt.Sprintf("arn:aws:iam::user/%s", identityName)
+		return fmt.Sprintf("arn:aws:iam::%s:user/%s", defaultAccountID, identityName)
 	}
 }

@@ -1406,7 +1408,12 @@ func buildPrincipalARN(identity *Identity, r *http.Request) string {
 		return "*" // Anonymous
 	}

-	// Check if this is the anonymous user identity (authenticated as anonymous)
+	// Priority 1: Use principal ARN if explicitly set (from STS JWT or IAM user)
+	if identity.PrincipalArn != "" {
+		return identity.PrincipalArn
+	}
+
+	// Priority 2: Check if this is the anonymous user identity (authenticated as anonymous)
 	// S3 policies expect Principal: "*" for anonymous access
 	if identity.Name == s3_constants.AccountAnonymousId ||
 		(identity.Account != nil && identity.Account.Id == s3_constants.AccountAnonymousId) {
@@ -1415,9 +1422,9 @@ func buildPrincipalARN(identity *Identity, r *http.Request) string {

 	// Build an AWS-compatible principal ARN
 	// Format: arn:aws:iam::account-id:user/user-name
-	accountId := identity.Account.Id
-	if accountId == "" {
-		accountId = "000000000000" // Default account ID
+	accountID := defaultAccountID // Default account ID
+	if identity.Account != nil && identity.Account.Id != "" {
+		accountID = identity.Account.Id
 	}

 	userName := identity.Name
@@ -1425,7 +1432,7 @@ func buildPrincipalARN(identity *Identity, r *http.Request) string {
 		userName = "unknown"
 	}

-	return fmt.Sprintf("arn:aws:iam::%s:user/%s", accountId, userName)
+	return fmt.Sprintf("arn:aws:iam::%s:user/%s", accountID, userName)
 }

 // GetCredentialManager returns the credential manager instance
@@ -1435,7 +1442,6 @@ func (iam *IdentityAccessManagement) GetCredentialManager() *credential.Credenti

 // LoadS3ApiConfigurationFromCredentialManager loads configuration using the credential manager
 func (iam *IdentityAccessManagement) LoadS3ApiConfigurationFromCredentialManager() error {
-	glog.V(1).Infof("IAM: reloading configuration from credential manager")
 	glog.V(1).Infof("Loading S3 API configuration from credential manager")

 	s3ApiConfiguration, err := iam.credentialManager.LoadConfiguration(context.Background())
--- a/weed/s3api/auth_credentials_test.go
+++ b/weed/s3api/auth_credentials_test.go
@@ -1,6 +1,7 @@
 package s3api

 import (
+	"fmt"
 	"os"
 	"reflect"
 	"sync"
@@ -294,7 +295,7 @@ func TestLoadS3ApiConfiguration(t *testing.T) {
 			expectIdent: &Identity{
 				Name:         "notSpecifyAccountId",
 				Account:      &AccountAdmin,
-				PrincipalArn: "arn:aws:iam::user/notSpecifyAccountId",
+				PrincipalArn: fmt.Sprintf("arn:aws:iam::%s:user/notSpecifyAccountId", defaultAccountID),
 				Actions: []Action{
 					"Read",
 					"Write",
@@ -320,7 +321,7 @@ func TestLoadS3ApiConfiguration(t *testing.T) {
 			expectIdent: &Identity{
 				Name:         "specifiedAccountID",
 				Account:      &specifiedAccount,
-				PrincipalArn: "arn:aws:iam::user/specifiedAccountID",
+				PrincipalArn: fmt.Sprintf("arn:aws:iam::%s:user/specifiedAccountID", defaultAccountID),
 				Actions: []Action{
 					"Read",
 					"Write",
@@ -338,7 +339,7 @@ func TestLoadS3ApiConfiguration(t *testing.T) {
 			expectIdent: &Identity{
 				Name:         "anonymous",
 				Account:      &AccountAnonymous,
-				PrincipalArn: "arn:aws:iam::user/anonymous",
+				PrincipalArn: "*",
 				Actions: []Action{
 					"Read",
 					"Write",
--- a/weed/s3api/s3api_bucket_policy_arn_test.go
+++ b/weed/s3api/s3api_bucket_policy_arn_test.go
@@ -1,6 +1,7 @@
 package s3api

 import (
+	"fmt"
 	"testing"

 	"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
@@ -62,6 +63,14 @@ func TestBuildPrincipalARN(t *testing.T) {
 			identity: nil,
 			expected: "*",
 		},
+		{
+			name: "explicit principal ARN",
+			identity: &Identity{
+				Name:         "test-user",
+				PrincipalArn: "arn:aws:iam::123456789012:role/MyRole",
+			},
+			expected: "arn:aws:iam::123456789012:role/MyRole",
+		},
 		{
 			name: "anonymous user by name",
 			identity: &Identity{
@@ -100,7 +109,7 @@ func TestBuildPrincipalARN(t *testing.T) {
 					Id: "",
 				},
 			},
-			expected: "arn:aws:iam::000000000000:user/test-user",
+			expected: fmt.Sprintf("arn:aws:iam::%s:user/test-user", defaultAccountID),
 		},
 		{
 			name: "identity without name",
--- a/weed/s3api/s3api_sts.go
+++ b/weed/s3api/s3api_sts.go
@@ -44,9 +44,6 @@ const (
 const (
 	minDurationSeconds = int64(900)   // 15 minutes
 	maxDurationSeconds = int64(43200) // 12 hours
-
-	// Default account ID for federated users
-	defaultAccountId = "111122223333"
 )

 // parseDurationSeconds parses and validates the DurationSeconds parameter
@@ -88,6 +85,13 @@ func NewSTSHandlers(stsService *sts.STSService, iam *IdentityAccessManagement) *
 	}
 }

+func (h *STSHandlers) getAccountID() string {
+	if h.stsService != nil && h.stsService.Config != nil && h.stsService.Config.AccountId != "" {
+		return h.stsService.Config.AccountId
+	}
+	return defaultAccountID
+}
+
 // HandleSTSRequest is the main entry point for STS requests
 // It routes requests based on the Action parameter
 func (h *STSHandlers) HandleSTSRequest(w http.ResponseWriter, r *http.Request) {
@@ -287,7 +291,7 @@ func (h *STSHandlers) handleAssumeRole(w http.ResponseWriter, r *http.Request) {
 	}

 	// Generate common STS components
-	stsCreds, assumedUser, err := h.prepareSTSCredentials(roleArn, roleSessionName, identity.PrincipalArn, durationSeconds, nil)
+	stsCreds, assumedUser, err := h.prepareSTSCredentials(roleArn, roleSessionName, durationSeconds, nil)
 	if err != nil {
 		h.writeSTSErrorResponse(w, r, STSErrInternalError, err)
 		return
@@ -396,14 +400,7 @@ func (h *STSHandlers) handleAssumeRoleWithLDAPIdentity(w http.ResponseWriter, r
 	glog.V(2).Infof("AssumeRoleWithLDAPIdentity: user %s authenticated successfully, groups=%v",
 		ldapUsername, identity.Groups)

-	// Verify that the identity is allowed to assume the role
-	// We create a temporary identity to represent the LDAP user for permission checking
-	// The checking logic will verify if the role's trust policy allows this principal
-	// Use configured account ID or default to "111122223333" for federated users
-	accountId := defaultAccountId
-	if h.stsService != nil && h.stsService.Config != nil && h.stsService.Config.AccountId != "" {
-		accountId = h.stsService.Config.AccountId
-	}
+	accountID := h.getAccountID()

 	ldapUserIdentity := &Identity{
 		Name: identity.UserID,
@@ -412,7 +409,7 @@ func (h *STSHandlers) handleAssumeRoleWithLDAPIdentity(w http.ResponseWriter, r
 			EmailAddress: identity.Email,
 			Id:           identity.UserID,
 		},
-		PrincipalArn: fmt.Sprintf("arn:aws:iam::%s:user/%s", accountId, identity.UserID),
+		PrincipalArn: fmt.Sprintf("arn:aws:iam::%s:user/%s", accountID, identity.UserID),
 	}

 	// Verify that the identity is allowed to assume the role by checking the Trust Policy
@@ -428,7 +425,7 @@ func (h *STSHandlers) handleAssumeRoleWithLDAPIdentity(w http.ResponseWriter, r
 		claims.WithIdentityProvider("ldap", identity.UserID, identity.Provider)
 	}

-	stsCreds, assumedUser, err := h.prepareSTSCredentials(roleArn, roleSessionName, ldapUserIdentity.PrincipalArn, durationSeconds, modifyClaims)
+	stsCreds, assumedUser, err := h.prepareSTSCredentials(roleArn, roleSessionName, durationSeconds, modifyClaims)
 	if err != nil {
 		h.writeSTSErrorResponse(w, r, STSErrInternalError, err)
 		return
@@ -447,7 +444,7 @@ func (h *STSHandlers) handleAssumeRoleWithLDAPIdentity(w http.ResponseWriter, r
 }

 // prepareSTSCredentials extracts common shared logic for credential generation
-func (h *STSHandlers) prepareSTSCredentials(roleArn, roleSessionName, principalArn string,
+func (h *STSHandlers) prepareSTSCredentials(roleArn, roleSessionName string,
 	durationSeconds *int64, modifyClaims func(*sts.STSSessionClaims)) (STSCredentials, *AssumedRoleUser, error) {

 	// Calculate duration
@@ -470,10 +467,17 @@ func (h *STSHandlers) prepareSTSCredentials(roleArn, roleSessionName, principalA
 		roleName = roleArn // Fallback to full ARN if extraction fails
 	}

+	accountID := h.getAccountID()
+
+	// Construct AssumedRoleUser ARN - this will be used as the principal for the vended token
+	assumedRoleArn := fmt.Sprintf("arn:aws:sts::%s:assumed-role/%s/%s", accountID, roleName, roleSessionName)
+
 	// Create session claims with role information
+	// SECURITY: Use the assumedRoleArn as the principal in the token.
+	// This ensures that subsequent requests using this token are correctly identified as the assumed role.
 	claims := sts.NewSTSSessionClaims(sessionId, h.stsService.Config.Issuer, expiration).
 		WithSessionName(roleSessionName).
-		WithRoleInfo(roleArn, fmt.Sprintf("%s:%s", roleName, roleSessionName), principalArn)
+		WithRoleInfo(roleArn, fmt.Sprintf("%s:%s", roleName, roleSessionName), assumedRoleArn)

 	// Apply custom claims if provided (e.g., LDAP identity)
 	if modifyClaims != nil {
@@ -495,12 +499,6 @@ func (h *STSHandlers) prepareSTSCredentials(roleArn, roleSessionName, principalA
 	accessKeyId := stsCredsDet.AccessKeyId
 	secretAccessKey := stsCredsDet.SecretAccessKey

-	// Get account ID from STS config or use default
-	accountId := defaultAccountId
-	if h.stsService != nil && h.stsService.Config != nil && h.stsService.Config.AccountId != "" {
-		accountId = h.stsService.Config.AccountId
-	}
-
 	stsCreds := STSCredentials{
 		AccessKeyId:     accessKeyId,
 		SecretAccessKey: secretAccessKey,
@@ -510,7 +508,7 @@ func (h *STSHandlers) prepareSTSCredentials(roleArn, roleSessionName, principalA

 	assumedUser := &AssumedRoleUser{
 		AssumedRoleId: fmt.Sprintf("%s:%s", roleName, roleSessionName),
-		Arn:           fmt.Sprintf("arn:aws:sts::%s:assumed-role/%s/%s", accountId, roleName, roleSessionName),
+		Arn:           assumedRoleArn,
 	}

 	return stsCreds, assumedUser, nil