From fe1d7a404dae9e40a4cfe0feb8ef70a8fb26366b Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 24 Apr 2026 13:08:24 -0700 Subject: [PATCH] fix(iam): substitute dynamic jwt:/saml:/oidc: claim variables in policies (#9217) * fix(iam): expand arbitrary jwt:/saml:/oidc: claim variables in policies The policy engine gated variable substitution on a fixed allowlist (jwt:sub, jwt:iss, jwt:aud, jwt:preferred_username), so patterns like arn:aws:s3:::softs/${jwt:project_path}/* were passed through as literals and never matched the requested resource. Dynamic claims from OIDC providers (e.g. GitLab CI's project_path / namespace_path) could not be used to scope policies. Allow any jwt:/saml:/oidc: prefixed variable to be substituted when the claim is present in RequestContext. These values originate from a cryptographically verified identity token (the STS session JWT or federated assertion), and the claim names are controlled by the trusted identity provider, so the dynamic prefix is safe. Missing claims keep the placeholder intact so the statement still fails to match. Numeric JWT claims (JSON-decoded as float64) are now stringified so patterns like ${jwt:project_id} work the same as string claims. Fixes #9214 * fix(iam): cover all integer widths in claim stringification Address PR review: stringifyClaimValue only handled int/int32/int64 on the signed side and nothing on the unsigned side, so int8, int16, uint, uint8, uint16, uint32, and uint64 claim values fell through to the default branch and the placeholder was left unsubstituted. JSON's generic decoder produces float64/json.Number for numbers, but RequestContext can also be populated from typed sources (custom providers or internal code), so cover all common integer widths - signed and unsigned - explicitly. Extend TestStringifyClaimValue to assert each supported type. --- weed/iam/policy/jwt_claim_variables_test.go | 216 ++++++++++++++++++++ weed/iam/policy/policy_engine.go | 102 ++++++--- 2 files changed, 295 insertions(+), 23 deletions(-) create mode 100644 weed/iam/policy/jwt_claim_variables_test.go diff --git a/weed/iam/policy/jwt_claim_variables_test.go b/weed/iam/policy/jwt_claim_variables_test.go new file mode 100644 index 000000000..b7d293577 --- /dev/null +++ b/weed/iam/policy/jwt_claim_variables_test.go @@ -0,0 +1,216 @@ +package policy + +import ( + "context" + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestStringifyClaimValue covers the scalar types that can appear in +// RequestContext so that both JSON-decoded (float64, json.Number) and typed +// (int8..int64, uint..uint64, bool) values substitute correctly. +func TestStringifyClaimValue(t *testing.T) { + tests := []struct { + name string + value interface{} + want string + wantOk bool + }{ + {name: "string", value: "alice", want: "alice", wantOk: true}, + {name: "bool true", value: true, want: "true", wantOk: true}, + {name: "bool false", value: false, want: "false", wantOk: true}, + {name: "float64 integer", value: float64(301), want: "301", wantOk: true}, + {name: "float64 fraction", value: 1.5, want: "1.5", wantOk: true}, + {name: "float32", value: float32(2.25), want: "2.25", wantOk: true}, + {name: "int", value: int(42), want: "42", wantOk: true}, + {name: "int8", value: int8(-5), want: "-5", wantOk: true}, + {name: "int16", value: int16(32000), want: "32000", wantOk: true}, + {name: "int32", value: int32(-123456), want: "-123456", wantOk: true}, + {name: "int64", value: int64(9999999999), want: "9999999999", wantOk: true}, + {name: "uint", value: uint(7), want: "7", wantOk: true}, + {name: "uint8", value: uint8(255), want: "255", wantOk: true}, + {name: "uint16", value: uint16(65535), want: "65535", wantOk: true}, + {name: "uint32", value: uint32(4000000000), want: "4000000000", wantOk: true}, + {name: "uint64", value: uint64(18000000000000000000), want: "18000000000000000000", wantOk: true}, + {name: "json.Number int", value: json.Number("301"), want: "301", wantOk: true}, + {name: "json.Number float", value: json.Number("1.5"), want: "1.5", wantOk: true}, + {name: "nil", value: nil, want: "", wantOk: false}, + {name: "slice unsupported", value: []string{"a", "b"}, want: "", wantOk: false}, + {name: "map unsupported", value: map[string]string{"a": "b"}, want: "", wantOk: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := stringifyClaimValue(tt.value) + assert.Equal(t, tt.wantOk, ok) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestExpandPolicyVariablesDynamicJWTClaims verifies that arbitrary JWT claims +// (not only the hardcoded jwt:sub/jwt:iss/jwt:aud/jwt:preferred_username) +// can be substituted in policy patterns when present in the request context. +// Reproduces the scenario in https://github.com/seaweedfs/seaweedfs/issues/9214 +// where ${jwt:project_path} was left unsubstituted. +func TestExpandPolicyVariablesDynamicJWTClaims(t *testing.T) { + evalCtx := &EvaluationContext{ + RequestContext: map[string]interface{}{ + "jwt:project_path": "some/namespace/project", + "jwt:namespace_path": "some/namespace", + "jwt:user_login": "mat", + // JSON-decoded numeric claim + "jwt:project_id": float64(301), + // Dynamic SAML/OIDC claims + "saml:department": "engineering", + "oidc:tenant": "acme", + }, + } + + tests := []struct { + name string + pattern string + expected string + }{ + { + name: "dynamic jwt claim substituted in resource arn", + pattern: "arn:aws:s3:::softs/${jwt:project_path}/*", + expected: "arn:aws:s3:::softs/some/namespace/project/*", + }, + { + name: "multiple dynamic jwt claims in one pattern", + pattern: "${jwt:namespace_path}/${jwt:user_login}/data", + expected: "some/namespace/mat/data", + }, + { + name: "dynamic numeric jwt claim is stringified", + pattern: "projects/${jwt:project_id}/uploads", + expected: "projects/301/uploads", + }, + { + name: "dynamic saml claim substituted", + pattern: "dept/${saml:department}/*", + expected: "dept/engineering/*", + }, + { + name: "dynamic oidc claim substituted", + pattern: "tenants/${oidc:tenant}/data", + expected: "tenants/acme/data", + }, + { + name: "missing jwt claim leaves placeholder intact", + pattern: "softs/${jwt:missing_claim}/*", + expected: "softs/${jwt:missing_claim}/*", + }, + { + name: "non-identity prefix is not substituted", + pattern: "foo/${custom:project_path}/*", + expected: "foo/${custom:project_path}/*", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := expandPolicyVariables(tt.pattern, evalCtx) + assert.Equal(t, tt.expected, result) + }) + } +} + +// TestGitLabProjectUploadPolicy reproduces the GitLab OIDC policy from issue +// #9214 end-to-end against the policy engine, proving that jwt:project_path +// (a non-hardcoded JWT claim) is now substituted in Resource patterns so that +// each project only gets access under its own prefix. +func TestGitLabProjectUploadPolicy(t *testing.T) { + engine := NewPolicyEngine() + require.NoError(t, engine.Initialize(&PolicyEngineConfig{ + DefaultEffect: "Deny", + StoreType: "memory", + })) + + policyDoc := &PolicyDocument{ + Version: "2012-10-17", + Statement: []Statement{ + { + Sid: "AllowPutInOwnFolder", + Effect: "Allow", + Action: []string{"s3:PutObject"}, + Resource: []string{"arn:aws:s3:::softs/${jwt:project_path}/*"}, + }, + }, + } + + require.NoError(t, engine.AddPolicy("", "GitlabProjectUploadPolicy", policyDoc)) + + ctx := context.Background() + + mat := map[string]interface{}{ + "jwt:project_path": "some/namespace/project", + } + other := map[string]interface{}{ + "jwt:project_path": "other/namespace/project", + } + + cases := []struct { + name string + action string + resource string + reqCtx map[string]interface{} + want Effect + }{ + { + name: "user writes into own project folder - allowed", + action: "s3:PutObject", + resource: "arn:aws:s3:::softs/some/namespace/project/build.zip", + reqCtx: mat, + want: EffectAllow, + }, + { + name: "user tries to write into another project - falls through to default Deny", + action: "s3:PutObject", + resource: "arn:aws:s3:::softs/other/namespace/project/build.zip", + reqCtx: mat, + want: EffectDeny, + }, + { + name: "different user writes into their own project - allowed", + action: "s3:PutObject", + resource: "arn:aws:s3:::softs/other/namespace/project/build.zip", + reqCtx: other, + want: EffectAllow, + }, + { + name: "user tries to GetObject - only PutObject allowed, default Deny", + action: "s3:GetObject", + resource: "arn:aws:s3:::softs/some/namespace/project/build.zip", + reqCtx: mat, + want: EffectDeny, + }, + { + name: "missing project_path claim - placeholder prevents match, default Deny", + action: "s3:PutObject", + resource: "arn:aws:s3:::softs/some/namespace/project/build.zip", + reqCtx: map[string]interface{}{}, + want: EffectDeny, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + evalCtx := &EvaluationContext{ + Principal: "user", + Action: tc.action, + Resource: tc.resource, + RequestContext: tc.reqCtx, + } + + result, err := engine.Evaluate(ctx, "", evalCtx, []string{"GitlabProjectUploadPolicy"}) + require.NoError(t, err) + assert.Equal(t, tc.want, result.Effect, + "matching statements: %+v", result.MatchingStatements) + }) + } +} diff --git a/weed/iam/policy/policy_engine.go b/weed/iam/policy/policy_engine.go index e51d34ed0..6810d09b3 100644 --- a/weed/iam/policy/policy_engine.go +++ b/weed/iam/policy/policy_engine.go @@ -23,7 +23,11 @@ const ( var ( policyVariablePattern = regexp.MustCompile(`\$\{([^}]+)\}`) - safePolicyVariables = map[string]bool{ + // safePolicyVariables is the explicit allowlist of fixed-name variables. + // Identity claims published under jwt:/saml:/oidc: are additionally accepted + // as dynamic variables (see isSafePolicyVariable) so that any claim from a + // cryptographically verified federated token can be used in policies. + safePolicyVariables = map[string]bool{ // AWS standard identity variables "aws:username": true, "aws:userid": true, @@ -32,20 +36,6 @@ var ( "aws:principaltype": true, "aws:FederatedProvider": true, "aws:PrincipalServiceName": true, - // SAML identity variables - "saml:username": true, - "saml:sub": true, - "saml:aud": true, - "saml:iss": true, - // OIDC/JWT identity variables - "oidc:sub": true, - "oidc:aud": true, - "oidc:iss": true, - // JWT identity variables - "jwt:preferred_username": true, - "jwt:sub": true, - "jwt:iss": true, - "jwt:aud": true, // AWS request context (not from headers) "aws:SourceIp": true, "aws:SecureTransport": true, @@ -56,6 +46,20 @@ var ( } ) +// isSafePolicyVariable reports whether a policy variable may be substituted +// with a value from RequestContext. The fixed allowlist covers AWS-defined +// variables; any jwt:/saml:/oidc: claim is also allowed because those come +// from a validated identity token (the STS session JWT or federated assertion) +// and the claim set is controlled by the trusted identity provider. +func isSafePolicyVariable(variable string) bool { + if safePolicyVariables[variable] { + return true + } + return strings.HasPrefix(variable, "jwt:") || + strings.HasPrefix(variable, "saml:") || + strings.HasPrefix(variable, "oidc:") +} + // PolicyEngine evaluates policies against requests type PolicyEngine struct { config *PolicyEngineConfig @@ -1240,25 +1244,77 @@ func expandPolicyVariables(pattern string, evalCtx *EvaluationContext) string { // Extract variable name from ${variable} variable := match[2 : len(match)-1] - // Only substitute if variable is in the safe allowlist - if !safePolicyVariables[variable] { - return match // Leave unsafe variables as-is + // Only substitute if the variable is in the allowlist or is a dynamic + // identity-claim variable (jwt:/saml:/oidc:) + if !isSafePolicyVariable(variable) { + return match // Leave unrecognised variables as-is } // Get value from request context - if value, exists := evalCtx.RequestContext[variable]; exists { - if str, ok := value.(string); ok { - return str - } + value, exists := evalCtx.RequestContext[variable] + if !exists { + return match // Variable not supplied: leave placeholder so statement won't match } - // Variable not found or not a string, leave as-is + if str, ok := stringifyClaimValue(value); ok { + return str + } + + // Value is a non-scalar (array/object) we can't meaningfully substitute return match }) return result } +// stringifyClaimValue converts a claim value to its string form for policy +// variable substitution. Returns (value, true) for scalar types that a JWT +// claim can produce after JSON decoding, and ("", false) for slices/maps/nil. +// JSON's generic decoder only produces float64 / json.Number for numbers, but +// RequestContext can also be populated from typed sources (e.g., custom +// providers or internal code), so all common integer widths — signed and +// unsigned — are handled explicitly. +func stringifyClaimValue(value interface{}) (string, bool) { + switch v := value.(type) { + case string: + return v, true + case bool: + return strconv.FormatBool(v), true + case float64: + // JSON-decoded numbers are float64; render integers without a decimal point + if v == float64(int64(v)) { + return strconv.FormatInt(int64(v), 10), true + } + return strconv.FormatFloat(v, 'g', -1, 64), true + case float32: + return strconv.FormatFloat(float64(v), 'g', -1, 32), true + case int: + return strconv.FormatInt(int64(v), 10), true + case int8: + return strconv.FormatInt(int64(v), 10), true + case int16: + return strconv.FormatInt(int64(v), 10), true + case int32: + return strconv.FormatInt(int64(v), 10), true + case int64: + return strconv.FormatInt(v, 10), true + case uint: + return strconv.FormatUint(uint64(v), 10), true + case uint8: + return strconv.FormatUint(uint64(v), 10), true + case uint16: + return strconv.FormatUint(uint64(v), 10), true + case uint32: + return strconv.FormatUint(uint64(v), 10), true + case uint64: + return strconv.FormatUint(v, 10), true + case json.Number: + return v.String(), true + default: + return "", false + } +} + // evaluateStringConditionIgnoreCase evaluates string conditions with case insensitivity func (e *PolicyEngine) evaluateStringConditionIgnoreCase(block map[string]interface{}, evalCtx *EvaluationContext, shouldMatch bool, useWildcard bool, forAllValues bool) bool { for key, expectedValues := range block {