diff --git a/weed/iam/integration/iam_manager.go b/weed/iam/integration/iam_manager.go index 6e7ade42cf..0ea4a32b2c 100644 --- a/weed/iam/integration/iam_manager.go +++ b/weed/iam/integration/iam_manager.go @@ -1672,10 +1672,16 @@ func (m *IAMManager) IsActionAllowed(ctx context.Context, request *ActionRequest // Parse principal ARN to extract details for context variables (e.g. ${aws:username}) arnInfo := utils.ParsePrincipalARN(request.Principal) - if arnInfo.RoleName != "" { - // For assumed roles, AWS docs say aws:username IS the role name. - // However, for user isolation in these tests, we typically map the session name (the user who assumed the role) to aws:username. - // arn:aws:sts::account:assumed-role/RoleName/SessionName + if arnInfo.RoleName != "" && strings.HasPrefix(request.Principal, "arn:aws:sts::") { + // An assumed-role ARN ends in the session name the caller chose; the + // session's user is the one it was issued to. + if sessionInfo != nil && sessionInfo.ExternalUserId != "" { + evalCtx.RequestContext["aws:username"] = sessionInfo.ExternalUserId + } else { + delete(evalCtx.RequestContext, "aws:username") + } + evalCtx.RequestContext["aws:userid"] = arnInfo.RoleName + } else if arnInfo.RoleName != "" { awsUsername := arnInfo.RoleName if idx := strings.LastIndex(request.Principal, "/"); idx != -1 && idx < len(request.Principal)-1 { awsUsername = request.Principal[idx+1:] diff --git a/weed/iam/integration/session_username_test.go b/weed/iam/integration/session_username_test.go new file mode 100644 index 0000000000..8bb8441e1e --- /dev/null +++ b/weed/iam/integration/session_username_test.go @@ -0,0 +1,66 @@ +package integration + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/seaweedfs/seaweedfs/weed/iam/policy" + "github.com/seaweedfs/seaweedfs/weed/iam/sts" +) + +// ${aws:username} of a role session is the user the session was issued to, +// not the session name its caller chose or a value forwarded in the request +// context. +func TestAssumedRoleUsernameIsTheSessionUser(t *testing.T) { + ctx := context.Background() + manager := setupIntegratedIAMSystem(t) + require.NoError(t, manager.CreatePolicy(ctx, "", "OwnFolderPolicy", &policy.PolicyDocument{ + Version: "2012-10-17", + Statement: []policy.Statement{{ + Effect: "Allow", + Action: []string{"s3:GetObject"}, + Resource: []string{"arn:aws:s3:::home/${aws:username}/*"}, + }}, + })) + require.NoError(t, manager.CreateRole(ctx, "", "SharedRole", &RoleDefinition{ + RoleName: "SharedRole", + TrustPolicy: &policy.PolicyDocument{ + Version: "2012-10-17", + Statement: []policy.Statement{{ + Effect: "Allow", + Principal: map[string]interface{}{"Federated": "test-oidc"}, + Action: []string{"sts:AssumeRoleWithWebIdentity"}, + }}, + }, + AttachedPolicies: []string{"OwnFolderPolicy"}, + })) + + response, err := manager.AssumeRoleWithWebIdentity(ctx, &sts.AssumeRoleWithWebIdentityRequest{ + RoleArn: "arn:aws:iam::role/SharedRole", + WebIdentityToken: createTestJWT(t, "https://test-issuer.com", "test-user-123", "test-signing-key"), + RoleSessionName: "alice", + }) + require.NoError(t, err) + session, err := manager.GetSTSService().ValidateSessionToken(ctx, response.Credentials.SessionToken) + require.NoError(t, err) + require.NotEmpty(t, session.ExternalUserId) + require.NotEqual(t, "alice", session.ExternalUserId) + + allowed := func(resource string) bool { + t.Helper() + ok, err := manager.IsActionAllowed(ctx, &ActionRequest{ + Principal: response.AssumedRoleUser.Arn, + Action: "s3:GetObject", + Resource: resource, + SessionToken: response.Credentials.SessionToken, + RequestContext: map[string]interface{}{"aws:username": "alice"}, + }) + require.NoError(t, err) + return ok + } + assert.False(t, allowed("arn:aws:s3:::home/alice/notes.txt"), "the session name must not select another user's folder") + assert.True(t, allowed("arn:aws:s3:::home/"+session.ExternalUserId+"/notes.txt"), "the session's user keeps its folder") +} diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index ee9a48698b..ad5a2c0e68 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -2755,7 +2755,7 @@ func (iam *IdentityAccessManagement) evaluateAttachedIAMPolicies(r *http.Request for key, values := range vectorIAMConditions(r) { conditions[key] = values } - for k, v := range policy_engine.ExtractPrincipalVariables(principal) { + for k, v := range principalVariables(principal, identity.Claims) { conditions[k] = v } diff --git a/weed/s3api/auth_signature_v4.go b/weed/s3api/auth_signature_v4.go index b86c14b42c..f18f12f4eb 100644 --- a/weed/s3api/auth_signature_v4.go +++ b/weed/s3api/auth_signature_v4.go @@ -532,6 +532,13 @@ func (iam *IdentityAccessManagement) validateSTSSessionToken(r *http.Request, se for k, v := range sessionInfo.RequestContext { claims[k] = v } + // The session's user is the one it was issued to, never a claim the + // identity provider forwarded. + if sessionInfo.ExternalUserId != "" { + claims["aws:username"] = sessionInfo.ExternalUserId + } else { + delete(claims, "aws:username") + } // Key the principal on the OIDC subject so a session resolves to the same // identity on the SigV4 and JWT paths (see s3_iam_middleware.go); fall back to diff --git a/weed/s3api/policy_engine/engine.go b/weed/s3api/policy_engine/engine.go index e544b227bd..c59b54ffd0 100644 --- a/weed/s3api/policy_engine/engine.go +++ b/weed/s3api/policy_engine/engine.go @@ -404,14 +404,9 @@ func ExtractPrincipalVariables(principal string) map[string][]string { vars["aws:username"] = []string{roleName} } case "assumed-role": + // The last segment is the session name, which the caller chose, so it + // names no user. vars["aws:principaltype"] = []string{"AssumedRole"} - // For assumed roles: assumed-role/RoleName/SessionName or assumed-role/path/to/RoleName/SessionName - // The session name is always the last segment - if len(resourceParts) >= 3 { - sessionName := resourceParts[len(resourceParts)-1] - vars["aws:username"] = []string{sessionName} - vars["aws:userid"] = []string{sessionName} - } } // Note: principaltype is already set correctly in the switch above based on resource type diff --git a/weed/s3api/policy_engine/engine_enhanced_test.go b/weed/s3api/policy_engine/engine_enhanced_test.go index 0a6deae726..4ad4055469 100644 --- a/weed/s3api/policy_engine/engine_enhanced_test.go +++ b/weed/s3api/policy_engine/engine_enhanced_test.go @@ -26,8 +26,6 @@ func TestExtractPrincipalVariables(t *testing.T) { expected: map[string][]string{ "aws:PrincipalAccount": {"123456789012"}, "aws:principaltype": {"AssumedRole"}, - "aws:username": {"session-alice"}, - "aws:userid": {"session-alice"}, }, }, { diff --git a/weed/s3api/policy_engine/engine_paths_test.go b/weed/s3api/policy_engine/engine_paths_test.go index 074e4e4a59..1dc3fddfd4 100644 --- a/weed/s3api/policy_engine/engine_paths_test.go +++ b/weed/s3api/policy_engine/engine_paths_test.go @@ -36,8 +36,6 @@ func TestExtractPrincipalVariablesWithPaths(t *testing.T) { expected: map[string][]string{ "aws:PrincipalAccount": {"123456789012"}, "aws:principaltype": {"AssumedRole"}, - "aws:username": {"session-name"}, - "aws:userid": {"session-name"}, }, }, } diff --git a/weed/s3api/s3api_bucket_policy_engine.go b/weed/s3api/s3api_bucket_policy_engine.go index 8552ccc3ab..6f92dfdb42 100644 --- a/weed/s3api/s3api_bucket_policy_engine.go +++ b/weed/s3api/s3api_bucket_policy_engine.go @@ -149,7 +149,7 @@ func (bpe *BucketPolicyEngine) EvaluatePolicy(bucket, object, action, principal args.Conditions = bpe.engine.ExtractConditionValuesFromRequest(r) // Extract principal-related variables (aws:username, etc.) from principal ARN - principalVars := policy_engine.ExtractPrincipalVariables(principal) + principalVars := principalVariables(principal, claims) for k, v := range principalVars { args.Conditions[k] = v } @@ -191,3 +191,17 @@ func (bpe *BucketPolicyEngine) EvaluatePolicy(bucket, object, action, principal return false, false, fmt.Errorf("unknown policy result: %v", result) } } + +// principalVariables resolves the policy variables of a principal. A role +// session's ARN ends in the session name the caller chose, so its user comes +// from the session itself: the user the session was issued to. +func principalVariables(principal string, claims map[string]interface{}) map[string][]string { + vars := policy_engine.ExtractPrincipalVariables(principal) + if types := vars["aws:principaltype"]; len(types) == 1 && types[0] == "AssumedRole" { + if user, ok := claims["aws:username"].(string); ok && user != "" { + vars["aws:username"] = []string{user} + vars["aws:userid"] = []string{user} + } + } + return vars +} diff --git a/weed/s3api/s3api_sts.go b/weed/s3api/s3api_sts.go index 9daa1cacf7..51e0c237b8 100644 --- a/weed/s3api/s3api_sts.go +++ b/weed/s3api/s3api_sts.go @@ -12,6 +12,7 @@ import ( "net/http" "regexp" "strconv" + "strings" "time" "github.com/seaweedfs/seaweedfs/weed/credential" @@ -454,14 +455,23 @@ func (h *STSHandlers) handleAssumeRole(w http.ResponseWriter, r *http.Request) { // travel into a session the caller assumed for itself — a legacy static admin // carries no IAM policies for such a session to inherit. Assuming a named role // scopes the session to that role's policies, admin caller or not. - var modifyClaims func(claims *sts.STSSessionClaims) - if assumesSelf && identity.isAdmin() { - modifyClaims = func(claims *sts.STSSessionClaims) { - if claims.RequestContext == nil { - claims.RequestContext = make(map[string]interface{}) - } - claims.RequestContext["is_admin"] = true + // The session name is the caller's choice, so the session records the + // caller itself as the user it was issued to; an STS session caller + // passes on its own user, if it has one. + sessionUser := identity.Name + if strings.HasPrefix(identity.PrincipalArn, "arn:aws:sts::") { + sessionUser, _ = identity.Claims["aws:username"].(string) + } + sessionIsAdmin := assumesSelf && identity.isAdmin() + modifyClaims := func(claims *sts.STSSessionClaims) { + claims.ExternalUserId = sessionUser + if !sessionIsAdmin { + return } + if claims.RequestContext == nil { + claims.RequestContext = make(map[string]interface{}) + } + claims.RequestContext["is_admin"] = true } // Generate common STS components diff --git a/weed/s3api/s3api_sts_session_username_test.go b/weed/s3api/s3api_sts_session_username_test.go new file mode 100644 index 0000000000..097d22de5d --- /dev/null +++ b/weed/s3api/s3api_sts_session_username_test.go @@ -0,0 +1,113 @@ +package s3api + +import ( + "context" + "encoding/xml" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/gorilla/mux" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/seaweedfs/seaweedfs/weed/iam/integration" + "github.com/seaweedfs/seaweedfs/weed/iam/policy" + "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" +) + +const sessionUserAccessKey, sessionUserSecretKey = "AKIABOB000000000000", "bobSecret123" + +func newSharedRoleIAM(t *testing.T) (*integration.IAMManager, *IdentityAccessManagement) { + t.Helper() + manager := newTestSTSIntegrationManager(t) + require.NoError(t, manager.CreateRole(context.Background(), "", "SharedRole", &integration.RoleDefinition{ + RoleName: "SharedRole", + TrustPolicy: &policy.PolicyDocument{ + Version: "2012-10-17", + Statement: []policy.Statement{{Effect: "Allow", Principal: "*", Action: []string{"sts:AssumeRole"}}}, + }, + })) + iam := &IdentityAccessManagement{iamIntegration: NewS3IAMIntegration(manager, "")} + require.NoError(t, iam.loadS3ApiConfiguration(&iam_pb.S3ApiConfiguration{ + Identities: []*iam_pb.Identity{{ + Name: "bob", + Credentials: []*iam_pb.Credential{{AccessKey: sessionUserAccessKey, SecretKey: sessionUserSecretKey}}, + Actions: []string{"Read:home"}, + }}, + })) + return manager, iam +} + +func assumeSharedRole(t *testing.T, manager *integration.IAMManager, iam *IdentityAccessManagement, accessKey, secretKey, sessionToken, sessionName string) AssumeRoleResult { + t.Helper() + body := url.Values{ + "Action": {"AssumeRole"}, + "Version": {"2011-06-15"}, + "RoleArn": {"arn:aws:iam::" + defaultAccountID + ":role/SharedRole"}, + "RoleSessionName": {sessionName}, + }.Encode() + req, err := newTestRequest(http.MethodPost, "http://sts.seaweedfs.test/", int64(len(body)), strings.NewReader(body)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + if sessionToken != "" { + req.Header.Set("X-Amz-Security-Token", sessionToken) + } + require.NoError(t, signRequestV4(req, accessKey, secretKey)) + + rec := httptest.NewRecorder() + NewSTSHandlers(manager.GetSTSService(), iam).handleAssumeRole(rec, req) + require.Equal(t, http.StatusOK, rec.Code, rec.Body.String()) + var resp AssumeRoleResponse + require.NoError(t, xml.Unmarshal(rec.Body.Bytes(), &resp)) + return resp.Result +} + +// A role session's ARN ends in the session name its caller chose, so a caller +// that names its session after another user must not get that user's +// ${aws:username}. +func TestAssumeRole_SessionNameIsNotTheUsername(t *testing.T) { + ctx := context.Background() + manager, iam := newSharedRoleIAM(t) + iam.policyEngine = NewBucketPolicyEngine() + require.NoError(t, iam.policyEngine.engine.SetBucketPolicy("home", `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Principal":"*","Action":"s3:GetObject","Resource":"arn:aws:s3:::home/${aws:username}/*"}, + {"Effect":"Deny","Principal":"*","Action":"s3:GetObject","NotResource":"arn:aws:s3:::home/${aws:username}/*"}]}`)) + + result := assumeSharedRole(t, manager, iam, sessionUserAccessKey, sessionUserSecretKey, "", "alice") + creds := result.Credentials + assert.Equal(t, "arn:aws:sts::"+defaultAccountID+":assumed-role/SharedRole/alice", result.AssumedRoleUser.Arn) + + session, err := manager.GetSTSService().ValidateSessionToken(ctx, creds.SessionToken) + require.NoError(t, err) + assert.Equal(t, "bob", session.ExternalUserId, "the session records its caller") + + get := func(key string) s3err.ErrorCode { + t.Helper() + r, err := newTestRequest(http.MethodGet, "http://s3.seaweedfs.test/home/"+key, 0, nil) + require.NoError(t, err) + r.Header.Set("X-Amz-Security-Token", creds.SessionToken) + require.NoError(t, signRequestV4(r, creds.AccessKeyId, creds.SecretAccessKey)) + r = mux.SetURLVars(r, map[string]string{"bucket": "home", "object": key}) + _, errCode := iam.authRequest(r, s3_constants.ACTION_READ) + return errCode + } + assert.Equal(t, s3err.ErrAccessDenied, get("alice/notes.txt"), "the session name must not select another user's folder") + assert.Equal(t, s3err.ErrNone, get("bob/notes.txt"), "the caller keeps its own folder") +} + +// A role session that assumes a role passes on its own user, not its session +// ID or the name it picks for the new session. +func TestAssumeRole_ChainedSessionKeepsTheUser(t *testing.T) { + manager, iam := newSharedRoleIAM(t) + first := assumeSharedRole(t, manager, iam, sessionUserAccessKey, sessionUserSecretKey, "", "bob").Credentials + chained := assumeSharedRole(t, manager, iam, first.AccessKeyId, first.SecretAccessKey, first.SessionToken, "alice").Credentials + + session, err := manager.GetSTSService().ValidateSessionToken(context.Background(), chained.SessionToken) + require.NoError(t, err) + assert.Equal(t, "bob", session.ExternalUserId) +}