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 {