From 1c4eb0653ed7e72ab6404e98cb35fbd268ebf2c2 Mon Sep 17 00:00:00 2001 From: Pierre-Olivier Mercier Date: Sat, 1 Nov 2025 17:57:57 +0700 Subject: [PATCH] Don't alert on missing -all on included SPF records --- pkg/analyzer/dns_spf.go | 39 +++++++++++--------- pkg/analyzer/dns_spf_test.go | 71 +++++++++++++++++++++++++++++++++++- 2 files changed, 92 insertions(+), 18 deletions(-) diff --git a/pkg/analyzer/dns_spf.go b/pkg/analyzer/dns_spf.go index fa819c1..a6b74c1 100644 --- a/pkg/analyzer/dns_spf.go +++ b/pkg/analyzer/dns_spf.go @@ -33,11 +33,12 @@ import ( // checkSPFRecords looks up and validates SPF records for a domain, including resolving include: directives func (d *DNSAnalyzer) checkSPFRecords(domain string) *[]api.SPFRecord { visited := make(map[string]bool) - return d.resolveSPFRecords(domain, visited, 0) + return d.resolveSPFRecords(domain, visited, 0, true) } // resolveSPFRecords recursively resolves SPF records including include: directives -func (d *DNSAnalyzer) resolveSPFRecords(domain string, visited map[string]bool, depth int) *[]api.SPFRecord { +// isMainRecord indicates if this is the primary domain's record (not an included one) +func (d *DNSAnalyzer) resolveSPFRecords(domain string, visited map[string]bool, depth int, isMainRecord bool) *[]api.SPFRecord { const maxDepth = 10 // Prevent infinite recursion if depth > maxDepth { @@ -103,7 +104,7 @@ func (d *DNSAnalyzer) resolveSPFRecords(domain string, visited map[string]bool, } // Basic validation - validationErr := d.validateSPF(spfRecord) + validationErr := d.validateSPF(spfRecord, isMainRecord) // Extract the "all" mechanism qualifier var allQualifier *api.SPFRecordAllQualifier @@ -140,7 +141,7 @@ func (d *DNSAnalyzer) resolveSPFRecords(domain string, visited map[string]bool, if redirectDomain != "" { // redirect= replaces the current domain's policy entirely // Only follow if no other mechanisms matched (per RFC 7208) - redirectRecords := d.resolveSPFRecords(redirectDomain, visited, depth+1) + redirectRecords := d.resolveSPFRecords(redirectDomain, visited, depth+1, false) if redirectRecords != nil { results = append(results, *redirectRecords...) } @@ -150,7 +151,7 @@ func (d *DNSAnalyzer) resolveSPFRecords(domain string, visited map[string]bool, // Extract and resolve include: directives includes := d.extractSPFIncludes(spfRecord) for _, includeDomain := range includes { - includedRecords := d.resolveSPFRecords(includeDomain, visited, depth+1) + includedRecords := d.resolveSPFRecords(includeDomain, visited, depth+1, false) if includedRecords != nil { results = append(results, *includedRecords...) } @@ -236,7 +237,8 @@ func (d *DNSAnalyzer) isValidSPFMechanism(token string) error { } // validateSPF performs basic SPF record validation -func (d *DNSAnalyzer) validateSPF(record string) error { +// isMainRecord indicates if this is the primary domain's record (not an included one) +func (d *DNSAnalyzer) validateSPF(record string, isMainRecord bool) error { // Must start with v=spf1 if !strings.HasPrefix(record, "v=spf1") { return fmt.Errorf("SPF record must start with 'v=spf1'") @@ -269,19 +271,22 @@ func (d *DNSAnalyzer) validateSPF(record string) error { return nil } - // Check for common syntax issues - // Should have a final mechanism (all, +all, -all, ~all, ?all) - validEndings := []string{" all", " +all", " -all", " ~all", " ?all"} - hasValidEnding := false - for _, ending := range validEndings { - if strings.HasSuffix(record, ending) { - hasValidEnding = true - break + // Only check for 'all' mechanism on the main record, not on included records + if isMainRecord { + // Check for common syntax issues + // Should have a final mechanism (all, +all, -all, ~all, ?all) + validEndings := []string{" all", " +all", " -all", " ~all", " ?all"} + hasValidEnding := false + for _, ending := range validEndings { + if strings.HasSuffix(record, ending) { + hasValidEnding = true + break + } } - } - if !hasValidEnding { - return fmt.Errorf("SPF record should end with an 'all' mechanism (e.g., '-all', '~all') or have a 'redirect=' modifier") + if !hasValidEnding { + return fmt.Errorf("SPF record should end with an 'all' mechanism (e.g., '-all', '~all') or have a 'redirect=' modifier") + } } return nil diff --git a/pkg/analyzer/dns_spf_test.go b/pkg/analyzer/dns_spf_test.go index bc51a6f..b1195cb 100644 --- a/pkg/analyzer/dns_spf_test.go +++ b/pkg/analyzer/dns_spf_test.go @@ -128,7 +128,8 @@ func TestValidateSPF(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := analyzer.validateSPF(tt.record) + // Test as main record (isMainRecord = true) since these tests check overall SPF validity + err := analyzer.validateSPF(tt.record, true) if tt.expectError { if err == nil { t.Errorf("validateSPF(%q) expected error but got nil", tt.record) @@ -144,6 +145,74 @@ func TestValidateSPF(t *testing.T) { } } +func TestValidateSPF_IncludedRecords(t *testing.T) { + tests := []struct { + name string + record string + isMainRecord bool + expectError bool + errorMsg string + }{ + { + name: "Main record without 'all' - should error", + record: "v=spf1 include:_spf.example.com", + isMainRecord: true, + expectError: true, + errorMsg: "should end with an 'all' mechanism", + }, + { + name: "Included record without 'all' - should NOT error", + record: "v=spf1 include:_spf.example.com", + isMainRecord: false, + expectError: false, + }, + { + name: "Included record with only mechanisms - should NOT error", + record: "v=spf1 ip4:192.0.2.0/24 mx", + isMainRecord: false, + expectError: false, + }, + { + name: "Main record with only mechanisms - should error", + record: "v=spf1 ip4:192.0.2.0/24 mx", + isMainRecord: true, + expectError: true, + errorMsg: "should end with an 'all' mechanism", + }, + { + name: "Included record with 'all' - valid", + record: "v=spf1 ip4:192.0.2.0/24 -all", + isMainRecord: false, + expectError: false, + }, + { + name: "Main record with 'all' - valid", + record: "v=spf1 ip4:192.0.2.0/24 -all", + isMainRecord: true, + expectError: false, + }, + } + + analyzer := NewDNSAnalyzer(5 * time.Second) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := analyzer.validateSPF(tt.record, tt.isMainRecord) + if tt.expectError { + if err == nil { + t.Errorf("validateSPF(%q, isMainRecord=%v) expected error but got nil", tt.record, tt.isMainRecord) + } else if tt.errorMsg != "" && !strings.Contains(err.Error(), tt.errorMsg) { + t.Errorf("validateSPF(%q, isMainRecord=%v) error = %q, want error containing %q", tt.record, tt.isMainRecord, err.Error(), tt.errorMsg) + } + } else { + if err != nil { + t.Errorf("validateSPF(%q, isMainRecord=%v) unexpected error: %v", tt.record, tt.isMainRecord, err) + } + } + }) + } +} + func TestExtractSPFRedirect(t *testing.T) { tests := []struct { name string