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>
2025-11-24 08:46:54 +08:00 · 2025-11-12 23:46:52 -08:00
parent 508d06d9a5
commit 2a9d4d1e23
5 changed files with 877 additions and 19 deletions
--- 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
--- a/weed/s3api/policy_conversion.go
+++ 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
+}
+
--- a/weed/s3api/policy_conversion_test.go
+++ 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))
+	}
+}
+
--- 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
--- 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 != "" {