From 2a9d4d1e23a99ddbdd4b99d3ddc3ff78cdfdf7ae Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 12 Nov 2025 23:46:52 -0800 Subject: [PATCH] Refactor data structure (#7472) * refactor to avoids circular dependency * converts a policy.PolicyDocument to policy_engine.PolicyDocument * convert numeric types to strings * Update weed/s3api/policy_conversion.go Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * refactoring * not skipping numeric and boolean values in arrays * avoid nil * edge cases * handling conversion failure The handling of unsupported types in convertToString could lead to silent policy alterations. The conversion of map-based principals in convertPrincipal is too generic and could misinterpret policies. * concise * fix doc * adjust warning * recursion * return errors * reject empty principals * better error message --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- weed/s3api/auth_credentials.go | 8 +- weed/s3api/policy_conversion.go | 239 +++++++++ weed/s3api/policy_conversion_test.go | 614 +++++++++++++++++++++++ weed/s3api/s3api_bucket_policy_engine.go | 21 +- weed/s3api/s3api_server.go | 14 +- 5 files changed, 877 insertions(+), 19 deletions(-) create mode 100644 weed/s3api/policy_conversion.go create mode 100644 weed/s3api/policy_conversion_test.go diff --git a/weed/s3api/auth_credentials.go b/weed/s3api/auth_credentials.go index 7a6a706ff..85002377b 100644 --- a/weed/s3api/auth_credentials.go +++ b/weed/s3api/auth_credentials.go @@ -54,8 +54,8 @@ type IdentityAccessManagement struct { // IAM Integration for advanced features iamIntegration *S3IAMIntegration - // Link to S3ApiServer for bucket policy evaluation - s3ApiServer *S3ApiServer + // Bucket policy engine for evaluating bucket policies + policyEngine *BucketPolicyEngine } type Identity struct { @@ -511,9 +511,9 @@ func (iam *IdentityAccessManagement) authRequest(r *http.Request, action Action) // - Explicit DENY in bucket policy → immediate rejection // - Explicit ALLOW in bucket policy → grant access (bypass IAM checks) // - No policy or indeterminate → fall through to IAM checks - if iam.s3ApiServer != nil && iam.s3ApiServer.policyEngine != nil && bucket != "" { + if iam.policyEngine != nil && bucket != "" { principal := buildPrincipalARN(identity) - allowed, evaluated, err := iam.s3ApiServer.policyEngine.EvaluatePolicy(bucket, object, string(action), principal) + allowed, evaluated, err := iam.policyEngine.EvaluatePolicy(bucket, object, string(action), principal) if err != nil { // SECURITY: Fail-close on policy evaluation errors diff --git a/weed/s3api/policy_conversion.go b/weed/s3api/policy_conversion.go new file mode 100644 index 000000000..27a8d7560 --- /dev/null +++ b/weed/s3api/policy_conversion.go @@ -0,0 +1,239 @@ +package s3api + +import ( + "fmt" + + "github.com/seaweedfs/seaweedfs/weed/glog" + "github.com/seaweedfs/seaweedfs/weed/iam/policy" + "github.com/seaweedfs/seaweedfs/weed/s3api/policy_engine" +) + +// ConvertPolicyDocumentToPolicyEngine converts a policy.PolicyDocument to policy_engine.PolicyDocument +// This function provides type-safe conversion with explicit field mapping and error handling. +// It handles the differences between the two types: +// - Converts []string fields to StringOrStringSlice +// - Maps Condition types with type validation +// - Converts Principal fields with support for AWS principals only +// - Handles optional fields (Id, NotPrincipal, NotAction, NotResource are ignored in policy_engine) +// +// Returns an error if the policy contains unsupported types or malformed data. +func ConvertPolicyDocumentToPolicyEngine(src *policy.PolicyDocument) (*policy_engine.PolicyDocument, error) { + if src == nil { + return nil, nil + } + + // Warn if the policy document Id is being dropped + if src.Id != "" { + glog.Warningf("policy document Id %q is not supported and will be ignored", src.Id) + } + + dest := &policy_engine.PolicyDocument{ + Version: src.Version, + Statement: make([]policy_engine.PolicyStatement, len(src.Statement)), + } + + for i := range src.Statement { + stmt, err := convertStatement(&src.Statement[i]) + if err != nil { + return nil, fmt.Errorf("failed to convert statement %d: %w", i, err) + } + dest.Statement[i] = stmt + } + + return dest, nil +} + +// convertStatement converts a policy.Statement to policy_engine.PolicyStatement +func convertStatement(src *policy.Statement) (policy_engine.PolicyStatement, error) { + // Check for unsupported fields that would fundamentally change policy semantics + // These fields invert the logic and ignoring them could create security holes + if len(src.NotAction) > 0 { + return policy_engine.PolicyStatement{}, fmt.Errorf("statement %q: NotAction is not supported (would invert action logic, creating potential security risk)", src.Sid) + } + if len(src.NotResource) > 0 { + return policy_engine.PolicyStatement{}, fmt.Errorf("statement %q: NotResource is not supported (would invert resource logic, creating potential security risk)", src.Sid) + } + if src.NotPrincipal != nil { + return policy_engine.PolicyStatement{}, fmt.Errorf("statement %q: NotPrincipal is not supported (would invert principal logic, creating potential security risk)", src.Sid) + } + + stmt := policy_engine.PolicyStatement{ + Sid: src.Sid, + Effect: policy_engine.PolicyEffect(src.Effect), + } + + // Convert Action ([]string to StringOrStringSlice) + if len(src.Action) > 0 { + stmt.Action = policy_engine.NewStringOrStringSlice(src.Action...) + } + + // Convert Resource ([]string to StringOrStringSlice) + if len(src.Resource) > 0 { + stmt.Resource = policy_engine.NewStringOrStringSlice(src.Resource...) + } + + // Convert Principal (interface{} to *StringOrStringSlice) + if src.Principal != nil { + principal, err := convertPrincipal(src.Principal) + if err != nil { + return policy_engine.PolicyStatement{}, fmt.Errorf("statement %q: failed to convert principal: %w", src.Sid, err) + } + stmt.Principal = principal + } + + // Convert Condition (map[string]map[string]interface{} to PolicyConditions) + if len(src.Condition) > 0 { + condition, err := convertCondition(src.Condition) + if err != nil { + return policy_engine.PolicyStatement{}, fmt.Errorf("statement %q: failed to convert condition: %w", src.Sid, err) + } + stmt.Condition = condition + } + + return stmt, nil +} + +// convertPrincipal converts a Principal field to *StringOrStringSlice +func convertPrincipal(principal interface{}) (*policy_engine.StringOrStringSlice, error) { + if principal == nil { + return nil, nil + } + + switch p := principal.(type) { + case string: + if p == "" { + return nil, fmt.Errorf("principal string cannot be empty") + } + result := policy_engine.NewStringOrStringSlice(p) + return &result, nil + case []string: + if len(p) == 0 { + return nil, nil + } + for _, s := range p { + if s == "" { + return nil, fmt.Errorf("principal string in slice cannot be empty") + } + } + result := policy_engine.NewStringOrStringSlice(p...) + return &result, nil + case []interface{}: + strs := make([]string, 0, len(p)) + for _, v := range p { + if v != nil { + str, err := convertToString(v) + if err != nil { + return nil, fmt.Errorf("failed to convert principal array item: %w", err) + } + if str == "" { + return nil, fmt.Errorf("principal string in slice cannot be empty") + } + strs = append(strs, str) + } + } + if len(strs) == 0 { + return nil, nil + } + result := policy_engine.NewStringOrStringSlice(strs...) + return &result, nil + case map[string]interface{}: + // Handle AWS-style principal with service/user keys + // Example: {"AWS": "arn:aws:iam::123456789012:user/Alice"} + // Only AWS principals are supported for now. Other types like Service or Federated need special handling. + + awsPrincipals, ok := p["AWS"] + if !ok || len(p) != 1 { + glog.Warningf("unsupported principal map, only a single 'AWS' key is supported: %v", p) + return nil, fmt.Errorf("unsupported principal map, only a single 'AWS' key is supported, got keys: %v", getMapKeys(p)) + } + + // Recursively convert the AWS principal value + res, err := convertPrincipal(awsPrincipals) + if err != nil { + return nil, fmt.Errorf("invalid 'AWS' principal value: %w", err) + } + return res, nil + default: + return nil, fmt.Errorf("unsupported principal type: %T", p) + } +} + +// convertCondition converts policy conditions to PolicyConditions +func convertCondition(src map[string]map[string]interface{}) (policy_engine.PolicyConditions, error) { + if len(src) == 0 { + return nil, nil + } + + dest := make(policy_engine.PolicyConditions) + for condType, condBlock := range src { + destBlock := make(map[string]policy_engine.StringOrStringSlice) + for key, value := range condBlock { + condValue, err := convertConditionValue(value) + if err != nil { + return nil, fmt.Errorf("failed to convert condition %s[%s]: %w", condType, key, err) + } + destBlock[key] = condValue + } + dest[condType] = destBlock + } + + return dest, nil +} + +// convertConditionValue converts a condition value to StringOrStringSlice +func convertConditionValue(value interface{}) (policy_engine.StringOrStringSlice, error) { + switch v := value.(type) { + case string: + return policy_engine.NewStringOrStringSlice(v), nil + case []string: + return policy_engine.NewStringOrStringSlice(v...), nil + case []interface{}: + strs := make([]string, 0, len(v)) + for _, item := range v { + if item != nil { + str, err := convertToString(item) + if err != nil { + return policy_engine.StringOrStringSlice{}, fmt.Errorf("failed to convert condition array item: %w", err) + } + strs = append(strs, str) + } + } + return policy_engine.NewStringOrStringSlice(strs...), nil + default: + // For non-string types, convert to string + // This handles numbers, booleans, etc. + str, err := convertToString(v) + if err != nil { + return policy_engine.StringOrStringSlice{}, err + } + return policy_engine.NewStringOrStringSlice(str), nil + } +} + +// convertToString converts any value to string representation +// Returns an error for unsupported types to prevent silent data corruption +func convertToString(value interface{}) (string, error) { + switch v := value.(type) { + case string: + return v, nil + case bool, + int, int8, int16, int32, int64, + uint, uint8, uint16, uint32, uint64, + float32, float64: + // Use fmt.Sprint for supported primitive types + return fmt.Sprint(v), nil + default: + glog.Warningf("unsupported type in policy conversion: %T", v) + return "", fmt.Errorf("unsupported type in policy conversion: %T", v) + } +} + +// getMapKeys returns the keys of a map as a slice (helper for error messages) +func getMapKeys(m map[string]interface{}) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + return keys +} + diff --git a/weed/s3api/policy_conversion_test.go b/weed/s3api/policy_conversion_test.go new file mode 100644 index 000000000..e7a77126f --- /dev/null +++ b/weed/s3api/policy_conversion_test.go @@ -0,0 +1,614 @@ +package s3api + +import ( + "strings" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/iam/policy" +) + +func TestConvertPolicyDocumentWithMixedTypes(t *testing.T) { + // Test that numeric and boolean values in arrays are properly converted + src := &policy.PolicyDocument{ + Version: "2012-10-17", + Statement: []policy.Statement{ + { + Sid: "TestMixedTypes", + Effect: "Allow", + Action: []string{"s3:GetObject"}, + Resource: []string{"arn:aws:s3:::bucket/*"}, + Principal: []interface{}{"user1", 123, true}, // Mixed types + Condition: map[string]map[string]interface{}{ + "NumericEquals": { + "s3:max-keys": []interface{}{100, 200, "300"}, // Mixed types + }, + "StringEquals": { + "s3:prefix": []interface{}{"test", 123, false}, // Mixed types + }, + }, + }, + }, + } + + // Convert + dest, err := ConvertPolicyDocumentToPolicyEngine(src) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + // Verify document structure + if dest == nil { + t.Fatal("Expected non-nil result") + } + if dest.Version != "2012-10-17" { + t.Errorf("Expected version '2012-10-17', got '%s'", dest.Version) + } + if len(dest.Statement) != 1 { + t.Fatalf("Expected 1 statement, got %d", len(dest.Statement)) + } + + stmt := dest.Statement[0] + + // Verify Principal conversion (should have 3 items converted to strings) + if stmt.Principal == nil { + t.Fatal("Expected non-nil Principal") + } + principals := stmt.Principal.Strings() + if len(principals) != 3 { + t.Errorf("Expected 3 principals, got %d", len(principals)) + } + // Check that numeric and boolean were converted + expectedPrincipals := []string{"user1", "123", "true"} + for i, expected := range expectedPrincipals { + if principals[i] != expected { + t.Errorf("Principal[%d]: expected '%s', got '%s'", i, expected, principals[i]) + } + } + + // Verify Condition conversion + if len(stmt.Condition) != 2 { + t.Errorf("Expected 2 condition blocks, got %d", len(stmt.Condition)) + } + + // Check NumericEquals condition + numericCond, ok := stmt.Condition["NumericEquals"] + if !ok { + t.Fatal("Expected NumericEquals condition") + } + maxKeys, ok := numericCond["s3:max-keys"] + if !ok { + t.Fatal("Expected s3:max-keys in NumericEquals") + } + maxKeysStrs := maxKeys.Strings() + expectedMaxKeys := []string{"100", "200", "300"} + if len(maxKeysStrs) != len(expectedMaxKeys) { + t.Errorf("Expected %d max-keys values, got %d", len(expectedMaxKeys), len(maxKeysStrs)) + } + for i, expected := range expectedMaxKeys { + if maxKeysStrs[i] != expected { + t.Errorf("max-keys[%d]: expected '%s', got '%s'", i, expected, maxKeysStrs[i]) + } + } + + // Check StringEquals condition + stringCond, ok := stmt.Condition["StringEquals"] + if !ok { + t.Fatal("Expected StringEquals condition") + } + prefix, ok := stringCond["s3:prefix"] + if !ok { + t.Fatal("Expected s3:prefix in StringEquals") + } + prefixStrs := prefix.Strings() + expectedPrefix := []string{"test", "123", "false"} + if len(prefixStrs) != len(expectedPrefix) { + t.Errorf("Expected %d prefix values, got %d", len(expectedPrefix), len(prefixStrs)) + } + for i, expected := range expectedPrefix { + if prefixStrs[i] != expected { + t.Errorf("prefix[%d]: expected '%s', got '%s'", i, expected, prefixStrs[i]) + } + } +} + +func TestConvertPrincipalWithMapAndMixedTypes(t *testing.T) { + // Test AWS-style principal map with mixed types + principalMap := map[string]interface{}{ + "AWS": []interface{}{ + "arn:aws:iam::123456789012:user/Alice", + 456, // User ID as number + true, // Some boolean value + }, + } + + result, err := convertPrincipal(principalMap) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if result == nil { + t.Fatal("Expected non-nil result") + } + + strs := result.Strings() + if len(strs) != 3 { + t.Errorf("Expected 3 values, got %d", len(strs)) + } + + expectedValues := []string{ + "arn:aws:iam::123456789012:user/Alice", + "456", + "true", + } + + for i, expected := range expectedValues { + if strs[i] != expected { + t.Errorf("Value[%d]: expected '%s', got '%s'", i, expected, strs[i]) + } + } +} + +func TestConvertConditionValueWithMixedTypes(t *testing.T) { + // Test []interface{} with mixed types + mixedValues := []interface{}{ + "string", + 123, + true, + 456.78, + } + + result, err := convertConditionValue(mixedValues) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + strs := result.Strings() + + expectedValues := []string{"string", "123", "true", "456.78"} + if len(strs) != len(expectedValues) { + t.Errorf("Expected %d values, got %d", len(expectedValues), len(strs)) + } + + for i, expected := range expectedValues { + if strs[i] != expected { + t.Errorf("Value[%d]: expected '%s', got '%s'", i, expected, strs[i]) + } + } +} + +func TestConvertPolicyDocumentNil(t *testing.T) { + result, err := ConvertPolicyDocumentToPolicyEngine(nil) + if err != nil { + t.Errorf("Unexpected error for nil input: %v", err) + } + if result != nil { + t.Error("Expected nil result for nil input") + } +} + +func TestConvertPrincipalNil(t *testing.T) { + result, err := convertPrincipal(nil) + if err != nil { + t.Errorf("Unexpected error for nil input: %v", err) + } + if result != nil { + t.Error("Expected nil result for nil input") + } +} + +func TestConvertPrincipalEmptyArray(t *testing.T) { + // Empty array should return nil + result, err := convertPrincipal([]interface{}{}) + if err != nil { + t.Errorf("Unexpected error for empty array: %v", err) + } + if result != nil { + t.Error("Expected nil result for empty array") + } +} + +func TestConvertPrincipalUnknownType(t *testing.T) { + // Unknown types should return an error + result, err := convertPrincipal(12345) // Just a number, not valid principal + if err == nil { + t.Error("Expected error for unknown type") + } + if result != nil { + t.Error("Expected nil result for unknown type") + } +} + +func TestConvertPrincipalWithNilValues(t *testing.T) { + // Test that nil values in arrays are skipped for security + principalArray := []interface{}{ + "arn:aws:iam::123456789012:user/Alice", + nil, // Should be skipped + "arn:aws:iam::123456789012:user/Bob", + nil, // Should be skipped + } + + result, err := convertPrincipal(principalArray) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if result == nil { + t.Fatal("Expected non-nil result") + } + + strs := result.Strings() + // Should only have 2 values (nil values skipped) + if len(strs) != 2 { + t.Errorf("Expected 2 values (nil values skipped), got %d", len(strs)) + } + + expectedValues := []string{ + "arn:aws:iam::123456789012:user/Alice", + "arn:aws:iam::123456789012:user/Bob", + } + + for i, expected := range expectedValues { + if strs[i] != expected { + t.Errorf("Value[%d]: expected '%s', got '%s'", i, expected, strs[i]) + } + } +} + +func TestConvertConditionValueWithNilValues(t *testing.T) { + // Test that nil values in condition arrays are skipped + mixedValues := []interface{}{ + "string", + nil, // Should be skipped + 123, + nil, // Should be skipped + true, + } + + result, err := convertConditionValue(mixedValues) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + strs := result.Strings() + + // Should only have 3 values (nil values skipped) + expectedValues := []string{"string", "123", "true"} + if len(strs) != len(expectedValues) { + t.Errorf("Expected %d values (nil values skipped), got %d", len(expectedValues), len(strs)) + } + + for i, expected := range expectedValues { + if strs[i] != expected { + t.Errorf("Value[%d]: expected '%s', got '%s'", i, expected, strs[i]) + } + } +} + +func TestConvertPrincipalMapWithNilValues(t *testing.T) { + // Test AWS-style principal map with nil values + principalMap := map[string]interface{}{ + "AWS": []interface{}{ + "arn:aws:iam::123456789012:user/Alice", + nil, // Should be skipped + "arn:aws:iam::123456789012:user/Bob", + }, + } + + result, err := convertPrincipal(principalMap) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if result == nil { + t.Fatal("Expected non-nil result") + } + + strs := result.Strings() + // Should only have 2 values (nil value skipped) + if len(strs) != 2 { + t.Errorf("Expected 2 values (nil value skipped), got %d", len(strs)) + } + + expectedValues := []string{ + "arn:aws:iam::123456789012:user/Alice", + "arn:aws:iam::123456789012:user/Bob", + } + + for i, expected := range expectedValues { + if strs[i] != expected { + t.Errorf("Value[%d]: expected '%s', got '%s'", i, expected, strs[i]) + } + } +} + +func TestConvertToStringUnsupportedType(t *testing.T) { + // Test that unsupported types (e.g., nested maps/slices) return empty string + // This should trigger a warning log and return an error + + type customStruct struct { + Field string + } + + testCases := []struct { + name string + input interface{} + expected string + }{ + { + name: "nested map", + input: map[string]interface{}{"key": "value"}, + expected: "", // Unsupported, returns empty string + }, + { + name: "nested slice", + input: []int{1, 2, 3}, + expected: "", // Unsupported, returns empty string + }, + { + name: "custom struct", + input: customStruct{Field: "test"}, + expected: "", // Unsupported, returns empty string + }, + { + name: "function", + input: func() {}, + expected: "", // Unsupported, returns empty string + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result, err := convertToString(tc.input) + // For unsupported types, we expect an error + if err == nil { + t.Error("Expected error for unsupported type") + } + if result != tc.expected { + t.Errorf("Expected '%s', got '%s'", tc.expected, result) + } + }) + } +} + +func TestConvertToStringSupportedTypes(t *testing.T) { + // Test that all supported types convert correctly + testCases := []struct { + name string + input interface{} + expected string + }{ + {"string", "test", "test"}, + {"bool true", true, "true"}, + {"bool false", false, "false"}, + {"int", 123, "123"}, + {"int8", int8(127), "127"}, + {"int16", int16(32767), "32767"}, + {"int32", int32(2147483647), "2147483647"}, + {"int64", int64(9223372036854775807), "9223372036854775807"}, + {"uint", uint(123), "123"}, + {"uint8", uint8(255), "255"}, + {"uint16", uint16(65535), "65535"}, + {"uint32", uint32(4294967295), "4294967295"}, + {"uint64", uint64(18446744073709551615), "18446744073709551615"}, + {"float32", float32(3.14), "3.14"}, + {"float64", float64(3.14159265359), "3.14159265359"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result, err := convertToString(tc.input) + if err != nil { + t.Errorf("Unexpected error for supported type %s: %v", tc.name, err) + } + if result != tc.expected { + t.Errorf("Expected '%s', got '%s'", tc.expected, result) + } + }) + } +} + +func TestConvertPrincipalUnsupportedTypes(t *testing.T) { + // Test that unsupported principal types return errors + testCases := []struct { + name string + principal interface{} + }{ + { + name: "Service principal", + principal: map[string]interface{}{"Service": "s3.amazonaws.com"}, + }, + { + name: "Federated principal", + principal: map[string]interface{}{"Federated": "arn:aws:iam::123456789012:saml-provider/ExampleProvider"}, + }, + { + name: "Multiple keys", + principal: map[string]interface{}{"AWS": "arn:...", "Service": "s3.amazonaws.com"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result, err := convertPrincipal(tc.principal) + if err == nil { + t.Error("Expected error for unsupported principal type") + } + if result != nil { + t.Error("Expected nil result for unsupported principal type") + } + }) + } +} + +func TestConvertPrincipalEmptyStrings(t *testing.T) { + // Test that empty string principals are rejected for security + testCases := []struct { + name string + principal interface{} + wantError string + }{ + { + name: "Empty string principal", + principal: "", + wantError: "principal string cannot be empty", + }, + { + name: "Empty string in array", + principal: []string{"arn:aws:iam::123456789012:user/Alice", "", "arn:aws:iam::123456789012:user/Bob"}, + wantError: "principal string in slice cannot be empty", + }, + { + name: "Empty string in interface array", + principal: []interface{}{"arn:aws:iam::123456789012:user/Alice", ""}, + wantError: "principal string in slice cannot be empty", + }, + { + name: "Empty string in AWS map", + principal: map[string]interface{}{ + "AWS": "", + }, + wantError: "principal string cannot be empty", + }, + { + name: "Empty string in AWS map array", + principal: map[string]interface{}{ + "AWS": []string{"arn:aws:iam::123456789012:user/Alice", ""}, + }, + wantError: "principal string in slice cannot be empty", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result, err := convertPrincipal(tc.principal) + if err == nil { + t.Error("Expected error for empty principal string") + } else if !strings.Contains(err.Error(), tc.wantError) { + t.Errorf("Expected error containing %q, got: %v", tc.wantError, err) + } + if result != nil { + t.Error("Expected nil result for empty principal string") + } + }) + } +} + +func TestConvertStatementWithUnsupportedFields(t *testing.T) { + // Test that errors are returned for unsupported fields + // These fields are critical for policy semantics and ignoring them would be a security risk + + testCases := []struct { + name string + statement *policy.Statement + wantError string + }{ + { + name: "NotAction field", + statement: &policy.Statement{ + Sid: "TestNotAction", + Effect: "Deny", + Action: []string{"s3:GetObject"}, + NotAction: []string{"s3:PutObject", "s3:DeleteObject"}, + Resource: []string{"arn:aws:s3:::bucket/*"}, + }, + wantError: "NotAction is not supported", + }, + { + name: "NotResource field", + statement: &policy.Statement{ + Sid: "TestNotResource", + Effect: "Allow", + Action: []string{"s3:*"}, + Resource: []string{"arn:aws:s3:::bucket/*"}, + NotResource: []string{"arn:aws:s3:::bucket/secret/*"}, + }, + wantError: "NotResource is not supported", + }, + { + name: "NotPrincipal field", + statement: &policy.Statement{ + Sid: "TestNotPrincipal", + Effect: "Deny", + Action: []string{"s3:*"}, + Resource: []string{"arn:aws:s3:::bucket/*"}, + NotPrincipal: map[string]interface{}{"AWS": "arn:aws:iam::123456789012:user/Admin"}, + }, + wantError: "NotPrincipal is not supported", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // The conversion should fail with an error for security reasons + result, err := convertStatement(tc.statement) + if err == nil { + t.Error("Expected error for unsupported field, got nil") + } else if !strings.Contains(err.Error(), tc.wantError) { + t.Errorf("Expected error containing %q, got: %v", tc.wantError, err) + } + + // Verify zero-value struct is returned on error + if result.Sid != "" || result.Effect != "" { + t.Error("Expected zero-value struct on error") + } + }) + } +} + +func TestConvertStatementSuccess(t *testing.T) { + // Test successful conversion without unsupported fields + statement := &policy.Statement{ + Sid: "AllowGetObject", + Effect: "Allow", + Action: []string{"s3:GetObject"}, + Resource: []string{"arn:aws:s3:::bucket/*"}, + Principal: map[string]interface{}{ + "AWS": "arn:aws:iam::123456789012:user/Alice", + }, + } + + result, err := convertStatement(statement) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if result.Sid != statement.Sid { + t.Errorf("Expected Sid %q, got %q", statement.Sid, result.Sid) + } + if string(result.Effect) != statement.Effect { + t.Errorf("Expected Effect %q, got %q", statement.Effect, result.Effect) + } +} + +func TestConvertPolicyDocumentWithId(t *testing.T) { + // Test that policy document Id field triggers a warning + src := &policy.PolicyDocument{ + Version: "2012-10-17", + Id: "MyPolicyId", + Statement: []policy.Statement{ + { + Sid: "AllowGetObject", + Effect: "Allow", + Action: []string{"s3:GetObject"}, + Resource: []string{"arn:aws:s3:::bucket/*"}, + }, + }, + } + + // The conversion should succeed but log a warning about Id + dest, err := ConvertPolicyDocumentToPolicyEngine(src) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if dest == nil { + t.Fatal("Expected non-nil result") + } + + // Verify basic conversion worked + if dest.Version != src.Version { + t.Errorf("Expected Version %q, got %q", src.Version, dest.Version) + } + if len(dest.Statement) != 1 { + t.Errorf("Expected 1 statement, got %d", len(dest.Statement)) + } +} + diff --git a/weed/s3api/s3api_bucket_policy_engine.go b/weed/s3api/s3api_bucket_policy_engine.go index 9e77f407c..54b43223e 100644 --- a/weed/s3api/s3api_bucket_policy_engine.go +++ b/weed/s3api/s3api_bucket_policy_engine.go @@ -49,11 +49,8 @@ func (bpe *BucketPolicyEngine) LoadBucketPolicy(bucket string, entry *filer_pb.E // LoadBucketPolicyFromCache loads a bucket policy from a cached BucketConfig // -// NOTE: This function uses JSON marshaling/unmarshaling to convert between -// policy.PolicyDocument and policy_engine.PolicyDocument. This is inefficient -// but necessary because the two types are defined in different packages and -// have subtle differences. A future improvement would be to unify these types -// or create a direct conversion function for better performance and type safety. +// This function uses a type-safe conversion function to convert between +// policy.PolicyDocument and policy_engine.PolicyDocument with explicit field mapping and error handling. func (bpe *BucketPolicyEngine) LoadBucketPolicyFromCache(bucket string, policyDoc *policy.PolicyDocument) error { if policyDoc == nil { // No policy for this bucket - remove it if it exists @@ -61,10 +58,16 @@ func (bpe *BucketPolicyEngine) LoadBucketPolicyFromCache(bucket string, policyDo return nil } - // Convert policy.PolicyDocument to policy_engine.PolicyDocument - // We use JSON marshaling as an intermediate format since both types - // follow the same AWS S3 policy structure - policyJSON, err := json.Marshal(policyDoc) + // Convert policy.PolicyDocument to policy_engine.PolicyDocument using direct conversion + // This is more efficient than JSON marshaling and provides better type safety + enginePolicyDoc, err := ConvertPolicyDocumentToPolicyEngine(policyDoc) + if err != nil { + glog.Errorf("Failed to convert bucket policy for %s: %v", bucket, err) + return fmt.Errorf("failed to convert bucket policy: %w", err) + } + + // Marshal the converted policy to JSON for storage in the engine + policyJSON, err := json.Marshal(enginePolicyDoc) if err != nil { glog.Errorf("Failed to marshal bucket policy for %s: %v", bucket, err) return err diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index 5a06be720..053d4f56a 100644 --- a/weed/s3api/s3api_server.go +++ b/weed/s3api/s3api_server.go @@ -86,10 +86,11 @@ func NewS3ApiServerWithStore(router *mux.Router, option *S3ApiServerOption, expl option.AllowedOrigins = domains } - var iam *IdentityAccessManagement - - iam = NewIdentityAccessManagementWithStore(option, explicitStore) + iam := NewIdentityAccessManagementWithStore(option, explicitStore) + // Initialize bucket policy engine first + policyEngine := NewBucketPolicyEngine() + s3ApiServer = &S3ApiServer{ option: option, iam: iam, @@ -98,11 +99,12 @@ func NewS3ApiServerWithStore(router *mux.Router, option *S3ApiServerOption, expl cb: NewCircuitBreaker(option), credentialManager: iam.credentialManager, bucketConfigCache: NewBucketConfigCache(60 * time.Minute), // Increased TTL since cache is now event-driven - policyEngine: NewBucketPolicyEngine(), // Initialize bucket policy engine + policyEngine: policyEngine, // Initialize bucket policy engine } - // Link IAM back to server for bucket policy evaluation - iam.s3ApiServer = s3ApiServer + // Pass policy engine to IAM for bucket policy evaluation + // This avoids circular dependency by not passing the entire S3ApiServer + iam.policyEngine = policyEngine // Initialize advanced IAM system if config is provided if option.IamConfig != "" {