diff --git a/weed/s3api/policy_conversion.go b/weed/s3api/policy_conversion.go index 943faca6b..7bda3dda6 100644 --- a/weed/s3api/policy_conversion.go +++ b/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 } diff --git a/weed/s3api/policy_conversion_test.go b/weed/s3api/policy_conversion_test.go index d1e66597f..0b6e12749 100644 --- a/weed/s3api/policy_conversion_test.go +++ b/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") + } + }) + } +} + diff --git a/weed/s3api/s3api_bucket_policy_engine.go b/weed/s3api/s3api_bucket_policy_engine.go index 15da52a6b..c306332fe 100644 --- a/weed/s3api/s3api_bucket_policy_engine.go +++ b/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)