diff --git a/weed/s3api/policy_conversion.go b/weed/s3api/policy_conversion.go index e689f2fed..d47564642 100644 --- a/weed/s3api/policy_conversion.go +++ b/weed/s3api/policy_conversion.go @@ -45,16 +45,16 @@ func ConvertPolicyDocumentToPolicyEngine(src *policy.PolicyDocument) (*policy_en // convertStatement converts a policy.Statement to policy_engine.PolicyStatement func convertStatement(src *policy.Statement) (policy_engine.PolicyStatement, error) { - // Warn about unsupported fields that will be ignored - // These fields invert the logic and are critical for policy semantics + // 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 { - glog.Warningf("statement %q: NotAction is not supported and will be ignored (this may make the policy more permissive than intended)", src.Sid) + 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 { - glog.Warningf("statement %q: NotResource is not supported and will be ignored (this may make the policy more permissive than intended)", src.Sid) + 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 { - glog.Warningf("statement %q: NotPrincipal is not supported and will be ignored (this may make the policy more permissive than intended)", src.Sid) + 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{ @@ -76,7 +76,7 @@ func convertStatement(src *policy.Statement) (policy_engine.PolicyStatement, err if src.Principal != nil { principal, err := convertPrincipal(src.Principal) if err != nil { - return stmt, fmt.Errorf("failed to convert principal: %w", err) + return policy_engine.PolicyStatement{}, fmt.Errorf("failed to convert principal: %w", err) } stmt.Principal = principal } @@ -85,7 +85,7 @@ func convertStatement(src *policy.Statement) (policy_engine.PolicyStatement, err if len(src.Condition) > 0 { condition, err := convertCondition(src.Condition) if err != nil { - return stmt, fmt.Errorf("failed to convert condition: %w", err) + return policy_engine.PolicyStatement{}, fmt.Errorf("failed to convert condition: %w", err) } stmt.Condition = condition } diff --git a/weed/s3api/policy_conversion_test.go b/weed/s3api/policy_conversion_test.go index 9bd9c56fb..90468861d 100644 --- a/weed/s3api/policy_conversion_test.go +++ b/weed/s3api/policy_conversion_test.go @@ -1,6 +1,7 @@ package s3api import ( + "strings" "testing" "github.com/seaweedfs/seaweedfs/weed/iam/policy" @@ -438,12 +439,13 @@ 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 + // 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", @@ -454,6 +456,7 @@ func TestConvertStatementWithUnsupportedFields(t *testing.T) { NotAction: []string{"s3:PutObject", "s3:DeleteObject"}, Resource: []string{"arn:aws:s3:::bucket/*"}, }, + wantError: "NotAction is not supported", }, { name: "NotResource field", @@ -464,6 +467,7 @@ func TestConvertStatementWithUnsupportedFields(t *testing.T) { Resource: []string{"arn:aws:s3:::bucket/*"}, NotResource: []string{"arn:aws:s3:::bucket/secret/*"}, }, + wantError: "NotResource is not supported", }, { name: "NotPrincipal field", @@ -474,43 +478,53 @@ func TestConvertStatementWithUnsupportedFields(t *testing.T) { 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": "*"}, - }, + wantError: "NotPrincipal is not supported", }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - // The conversion should succeed but log warnings + // The conversion should fail with an error for security reasons result, err := convertStatement(tc.statement) - if err != nil { - t.Errorf("Unexpected error: %v", err) + 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 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) + // Verify zero-value struct is returned on error + if result.Sid != "" || result.Effect != "" { + t.Error("Expected zero-value struct on error") } - - // Note: We can't easily verify the warnings were logged without - // capturing log output, but the conversion should complete successfully }) } } +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{