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.

weed/s3api/policy_conversion.go

@@ -14,9 +14,11 @@ import (
 // - Converts []string fields to StringOrStringSlice
 // - Maps Condition types
 // - Handles optional fields (Id, NotPrincipal, NotAction, NotResource are ignored in policy_engine)
-func ConvertPolicyDocumentToPolicyEngine(src *policy.PolicyDocument) *policy_engine.PolicyDocument {
+//
+// 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
+		return nil, nil
 	}
 
 	dest := &policy_engine.PolicyDocument{
@@ -25,14 +27,18 @@ func ConvertPolicyDocumentToPolicyEngine(src *policy.PolicyDocument) *policy_eng
 	}
 
 	for i := range src.Statement {
-		dest.Statement[i] = convertStatement(&src.Statement[i])
+		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
+	return dest, nil
 }
 
 // convertStatement converts a policy.Statement to policy_engine.PolicyStatement
-func convertStatement(src *policy.Statement) policy_engine.PolicyStatement {
+func convertStatement(src *policy.Statement) (policy_engine.PolicyStatement, error) {
 	stmt := policy_engine.PolicyStatement{
 		Sid:    src.Sid,
 		Effect: policy_engine.PolicyEffect(src.Effect),
@@ -50,25 +56,32 @@ func convertStatement(src *policy.Statement) policy_engine.PolicyStatement {
 
 	// Convert Principal (interface{} to *StringOrStringSlice)
 	if src.Principal != nil {
-		stmt.Principal = convertPrincipal(src.Principal)
+		principal, err := convertPrincipal(src.Principal)
+		if err != nil {
+			return stmt, fmt.Errorf("failed to convert principal: %w", err)
+		}
+		stmt.Principal = principal
 	}
 
 	// Convert Condition (map[string]map[string]interface{} to PolicyConditions)
 	if len(src.Condition) > 0 {
-		stmt.Condition = convertCondition(src.Condition)
+		condition, err := convertCondition(src.Condition)
+		if err != nil {
+			return stmt, fmt.Errorf("failed to convert condition: %w", err)
+		}
+		stmt.Condition = condition
 	}
 
-	return stmt
+	return stmt, nil
 }
 
 // convertPrincipal converts a Principal field to *StringOrStringSlice
-func convertPrincipal(principal interface{}) *policy_engine.StringOrStringSlice {
+func convertPrincipal(principal interface{}) (*policy_engine.StringOrStringSlice, error) {
 	if principal == nil {
-		return nil
+		return nil, nil
 	}
 
 	var strs []string
-	processed := true
 
 	switch p := principal.(type) {
 	case string:
@@ -80,92 +93,137 @@ func convertPrincipal(principal interface{}) *policy_engine.StringOrStringSlice
 		strs = make([]string, 0, len(p))
 		for _, v := range p {
 			if v != nil {
-				strs = append(strs, convertToString(v))
+				str, err := convertToString(v)
+				if err != nil {
+					return nil, fmt.Errorf("failed to convert principal array item: %w", err)
+				}
+				strs = append(strs, str)
 			}
 		}
 	case map[string]interface{}:
 		// Handle AWS-style principal with service/user keys
 		// Example: {"AWS": "arn:aws:iam::123456789012:user/Alice"}
-		for _, v := range p {
-			switch val := v.(type) {
-			case string:
-				strs = append(strs, val)
-			case []string:
-				strs = append(strs, val...)
-			case []interface{}:
-				for _, item := range val {
-					if item != nil {
-						strs = append(strs, convertToString(item))
+		// Only AWS principals are supported for now. Other types like Service or Federated need special handling.
+		
+		// Check that ONLY the "AWS" key is present
+		if len(p) != 1 {
+			glog.Warningf("unsupported principal map, only single 'AWS' key is supported: %v", p)
+			return nil, fmt.Errorf("unsupported principal map, only single 'AWS' key is supported, got keys: %v", getMapKeys(p))
+		}
+		
+		awsPrincipals, ok := p["AWS"]
+		if !ok {
+			glog.Warningf("unsupported principal map, only 'AWS' key is supported: %v", p)
+			return nil, fmt.Errorf("unsupported principal type, only 'AWS' principals are supported, got keys: %v", getMapKeys(p))
+		}
+		
+		switch val := awsPrincipals.(type) {
+		case string:
+			strs = append(strs, val)
+		case []string:
+			strs = append(strs, val...)
+		case []interface{}:
+			for _, item := range val {
+				if item != nil {
+					str, err := convertToString(item)
+					if err != nil {
+						return nil, fmt.Errorf("failed to convert AWS principal item: %w", err)
 					}
+					strs = append(strs, str)
 				}
 			}
+		default:
+			glog.Warningf("unsupported type for 'AWS' principal value: %T", val)
+			return nil, fmt.Errorf("unsupported type for 'AWS' principal value: %T", val)
 		}
 	default:
-		processed = false
+		return nil, fmt.Errorf("unsupported principal type: %T", p)
 	}
 
-	if processed && len(strs) > 0 {
+	if len(strs) > 0 {
 		result := policy_engine.NewStringOrStringSlice(strs...)
-		return &result
+		return &result, nil
 	}
 
-	return nil
+	return nil, nil
 }
 
 // convertCondition converts policy conditions to PolicyConditions
-func convertCondition(src map[string]map[string]interface{}) policy_engine.PolicyConditions {
+func convertCondition(src map[string]map[string]interface{}) (policy_engine.PolicyConditions, error) {
 	if len(src) == 0 {
-		return nil
+		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 {
-			destBlock[key] = convertConditionValue(value)
+			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
+	return dest, nil
 }
 
 // convertConditionValue converts a condition value to StringOrStringSlice
-func convertConditionValue(value interface{}) policy_engine.StringOrStringSlice {
+func convertConditionValue(value interface{}) (policy_engine.StringOrStringSlice, error) {
 	switch v := value.(type) {
 	case string:
-		return policy_engine.NewStringOrStringSlice(v)
+		return policy_engine.NewStringOrStringSlice(v), nil
 	case []string:
-		return policy_engine.NewStringOrStringSlice(v...)
+		return policy_engine.NewStringOrStringSlice(v...), nil
 	case []interface{}:
 		strs := make([]string, 0, len(v))
 		for _, item := range v {
 			if item != nil {
-				strs = append(strs, convertToString(item))
+				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...)
+		return policy_engine.NewStringOrStringSlice(strs...), nil
 	default:
 		// For non-string types, convert to string
 		// This handles numbers, booleans, etc.
-		return policy_engine.NewStringOrStringSlice(convertToString(v))
+		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
-func convertToString(value interface{}) string {
+// 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
+		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)
+		return fmt.Sprint(v), nil
 	default:
-		glog.Warningf("unsupported type in policy conversion: %T, converting to empty string", v)
-		return ""
+		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
 }
 

weed/s3api/policy_conversion_test.go

@@ -30,7 +30,10 @@ func TestConvertPolicyDocumentWithMixedTypes(t *testing.T) {
 	}
 
 	// Convert
-	dest := ConvertPolicyDocumentToPolicyEngine(src)
+	dest, err := ConvertPolicyDocumentToPolicyEngine(src)
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
 
 	// Verify document structure
 	if dest == nil {
@@ -117,7 +120,10 @@ func TestConvertPrincipalWithMapAndMixedTypes(t *testing.T) {
 		},
 	}
 
-	result := convertPrincipal(principalMap)
+	result, err := convertPrincipal(principalMap)
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
 	
 	if result == nil {
 		t.Fatal("Expected non-nil result")
@@ -150,7 +156,10 @@ func TestConvertConditionValueWithMixedTypes(t *testing.T) {
 		456.78,
 	}
 
-	result := convertConditionValue(mixedValues)
+	result, err := convertConditionValue(mixedValues)
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
 	strs := result.Strings()
 
 	expectedValues := []string{"string", "123", "true", "456.78"}
@@ -166,14 +175,20 @@ func TestConvertConditionValueWithMixedTypes(t *testing.T) {
 }
 
 func TestConvertPolicyDocumentNil(t *testing.T) {
-	result := ConvertPolicyDocumentToPolicyEngine(nil)
+	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 := convertPrincipal(nil)
+	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")
 	}
@@ -181,15 +196,21 @@ func TestConvertPrincipalNil(t *testing.T) {
 
 func TestConvertPrincipalEmptyArray(t *testing.T) {
 	// Empty array should return nil
-	result := convertPrincipal([]interface{}{})
+	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 nil
-	result := convertPrincipal(12345) // Just a number, not valid principal
+	// 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")
 	}
@@ -204,7 +225,10 @@ func TestConvertPrincipalWithNilValues(t *testing.T) {
 		nil, // Should be skipped
 	}
 
-	result := convertPrincipal(principalArray)
+	result, err := convertPrincipal(principalArray)
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
 	
 	if result == nil {
 		t.Fatal("Expected non-nil result")
@@ -238,7 +262,10 @@ func TestConvertConditionValueWithNilValues(t *testing.T) {
 		true,
 	}
 
-	result := convertConditionValue(mixedValues)
+	result, err := convertConditionValue(mixedValues)
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
 	strs := result.Strings()
 
 	// Should only have 3 values (nil values skipped)
@@ -264,7 +291,10 @@ func TestConvertPrincipalMapWithNilValues(t *testing.T) {
 		},
 	}
 
-	result := convertPrincipal(principalMap)
+	result, err := convertPrincipal(principalMap)
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
 	
 	if result == nil {
 		t.Fatal("Expected non-nil result")
@@ -325,12 +355,14 @@ func TestConvertToStringUnsupportedType(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			result := convertToString(tc.input)
+			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)
 			}
-			// Note: In a real test, you might want to capture log output
-			// to verify the warning was actually logged
 		})
 	}
 }
@@ -361,7 +393,10 @@ func TestConvertToStringSupportedTypes(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			result := convertToString(tc.input)
+			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)
 			}
@@ -369,3 +404,36 @@ func TestConvertToStringSupportedTypes(t *testing.T) {
 	}
 }
 
+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")
+			}
+		})
+	}
+}
+

weed/s3api/s3api_bucket_policy_engine.go

@@ -60,7 +60,11 @@ func (bpe *BucketPolicyEngine) LoadBucketPolicyFromCache(bucket string, policyDo
 
 	// Convert policy.PolicyDocument to policy_engine.PolicyDocument using direct conversion
 	// This is more efficient than JSON marshaling and provides better type safety
-	enginePolicyDoc := ConvertPolicyDocumentToPolicyEngine(policyDoc)
+	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)