diff --git a/weed/s3api/policy_conversion.go b/weed/s3api/policy_conversion.go index 5d90bd187..e689f2fed 100644 --- a/weed/s3api/policy_conversion.go +++ b/weed/s3api/policy_conversion.go @@ -99,16 +99,18 @@ func convertPrincipal(principal interface{}) (*policy_engine.StringOrStringSlice return nil, nil } - var strs []string - switch p := principal.(type) { case string: - strs = []string{p} + result := policy_engine.NewStringOrStringSlice(p) + return &result, nil case []string: - strs = p + if len(p) == 0 { + return nil, nil + } + result := policy_engine.NewStringOrStringSlice(p...) + return &result, nil case []interface{}: - // Convert []interface{} to []string - strs = make([]string, 0, len(p)) + strs := make([]string, 0, len(p)) for _, v := range p { if v != nil { str, err := convertToString(v) @@ -118,6 +120,11 @@ func convertPrincipal(principal interface{}) (*policy_engine.StringOrStringSlice 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"} @@ -129,35 +136,15 @@ func convertPrincipal(principal interface{}) (*policy_engine.StringOrStringSlice return nil, fmt.Errorf("unsupported principal map, only a single 'AWS' key is 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) + // 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) } - - if len(strs) > 0 { - result := policy_engine.NewStringOrStringSlice(strs...) - return &result, nil - } - - return nil, nil } // convertCondition converts policy conditions to PolicyConditions diff --git a/weed/s3api/policy_conversion_test.go b/weed/s3api/policy_conversion_test.go index 25b05610e..9bd9c56fb 100644 --- a/weed/s3api/policy_conversion_test.go +++ b/weed/s3api/policy_conversion_test.go @@ -437,3 +437,111 @@ func TestConvertPrincipalUnsupportedTypes(t *testing.T) { } } +func TestConvertStatementWithUnsupportedFields(t *testing.T) { + // Test that warnings are logged for unsupported fields + // These fields are critical for policy semantics and ignoring them is a security risk + + testCases := []struct { + name string + statement *policy.Statement + }{ + { + 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/*"}, + }, + }, + { + 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/*"}, + }, + }, + { + 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"}, + }, + }, + { + name: "All unsupported fields", + statement: &policy.Statement{ + Sid: "TestAllUnsupported", + Effect: "Deny", + Action: []string{"s3:GetObject"}, + NotAction: []string{"s3:PutObject"}, + Resource: []string{"arn:aws:s3:::bucket/*"}, + NotResource: []string{"arn:aws:s3:::bucket/public/*"}, + NotPrincipal: map[string]interface{}{"AWS": "*"}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // The conversion should succeed but log warnings + result, err := convertStatement(tc.statement) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + // Verify the result has the basic fields + if result.Sid != tc.statement.Sid { + t.Errorf("Expected Sid %q, got %q", tc.statement.Sid, result.Sid) + } + if string(result.Effect) != tc.statement.Effect { + t.Errorf("Expected Effect %q, got %q", tc.statement.Effect, result.Effect) + } + + // Note: We can't easily verify the warnings were logged without + // capturing log output, but the conversion should complete successfully + }) + } +} + +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)) + } +} +