Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional strict checking of SPF response size #4

Open
wants to merge 2 commits into
base: next
Choose a base branch
from
Open

Optional strict checking of SPF response size #4

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22f2e30
The RFC suggests SPF records have a record size.
jimmystewpot Jun 29, 2022
c417c78
tidy up response length variable names to be more descriptive
jimmystewpot Jul 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 50 additions & 23 deletions spf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ var (
ErrNoResult = errors.New("no DNS record found")
ErrLookupLimitReached = errors.New("lookup limit reached")
ErrVoidLookupLimitReached = errors.New("void lookup limit reached")
ErrOctetLimitReached = errors.New("response size exceeds 450 octets")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd name this a bit different, since an octet limit can be pretty much anything. What do you think about:

ErrRecordTooLong = errors.New("DNS record is too long")

?

ErrTooManyMXRecords = errors.New("too many MX records")
ErrMultipleRecords = errors.New("multiple matching DNS records")

Expand All @@ -118,6 +119,11 @@ const (
// with a configurable default of 2.
// https://tools.ietf.org/html/rfc7208#section-4.6.4
defaultMaxVoidLookups = 2

// Default value for maximum octet size of the SPF response. The
// RFC suggests the limit should be 450.
// https://tools.ietf.org/html/rfc7208#section-3.4
defaultMaxResponseOctets = 450
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in the functions below: just as the above, I'd name this defaultMaxRecordLength and adjust the comment to match.

)

// TraceFunc is the type of tracing functions.
Expand Down Expand Up @@ -147,14 +153,15 @@ type Option func(*resolution)
// Deprecated: use CheckHostWithSender instead.
func CheckHost(ip net.IP, domain string) (Result, error) {
r := &resolution{
ip: ip,
maxcount: defaultMaxLookups,
maxvoidcount: defaultMaxVoidLookups,
helo: domain,
sender: "@" + domain,
ctx: context.TODO(),
resolver: defaultResolver,
trace: defaultTrace,
ip: ip,
maxcount: defaultMaxLookups,
maxvoidcount: defaultMaxVoidLookups,
maxresponseoctets: defaultMaxResponseOctets,
helo: domain,
sender: "@" + domain,
ctx: context.TODO(),
resolver: defaultResolver,
trace: defaultTrace,
}
return r.Check(domain)
}
Expand All @@ -178,14 +185,15 @@ func CheckHostWithSender(ip net.IP, helo, sender string, opts ...Option) (Result
}

r := &resolution{
ip: ip,
maxcount: defaultMaxLookups,
maxvoidcount: defaultMaxVoidLookups,
helo: helo,
sender: sender,
ctx: context.TODO(),
resolver: defaultResolver,
trace: defaultTrace,
ip: ip,
maxcount: defaultMaxLookups,
maxvoidcount: defaultMaxVoidLookups,
maxresponseoctets: defaultMaxResponseOctets,
helo: helo,
sender: sender,
ctx: context.TODO(),
resolver: defaultResolver,
trace: defaultTrace,
}

for _, opt := range opts {
Expand Down Expand Up @@ -219,6 +227,18 @@ func OverrideVoidLookupLimit(limit uint) Option {
}
}

// OverrideOctetLimit overrides the maximum octet size allowed
// during SPF evaluation.
// Note that as per RFC, the default value of 450 SHOULD
// be used. Please use with care.
//
// This is EXPERIMENTAL for now, and the API is subject to change.
func OverrideOctetLimit(limit int) Option {
return func(r *resolution) {
r.maxresponseoctets = limit
}
}

// WithContext is an option to set the context for this operation, which will
// be passed along to the resolver functions and other external calls if
// needed.
Expand Down Expand Up @@ -246,8 +266,7 @@ var defaultResolver DNSResolver = net.DefaultResolver
//
// The default is to use net.DefaultResolver, which should be appropriate for
// most users.
//
// This is EXPERIMENTAL for now, and the API is subject to change.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular change is okay with me now, but it's better to do it in a separate patch.

I don't mind if you want to do a separate PR, or just two patches in this one, or just leave it to me to split this out, I am happy with any of those options :)

//https://tools.ietf.org/html/rfc7208#section-3.4
func WithResolver(resolver DNSResolver) Option {
return func(r *resolution) {
r.resolver = resolver
Expand Down Expand Up @@ -278,11 +297,12 @@ func split(addr string) (string, string) {
}

type resolution struct {
ip net.IP
count uint
maxcount uint
voidcount uint
maxvoidcount uint
ip net.IP
count uint
maxcount uint
voidcount uint
maxvoidcount uint
maxresponseoctets int

helo string
sender string
Expand Down Expand Up @@ -329,6 +349,13 @@ func (r *resolution) Check(domain string) (Result, error) {
return None, ErrNoResult
}

if len(txt) > r.maxresponseoctets {
// SPF response greater than 450 octets
// https://tools.ietf.org/html/rfc7208#section-3.4
r.trace("dns perm error: %s response size %d", ErrOctetLimitReached, len(txt))
return PermError, ErrOctetLimitReached
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind moving this check to getDNSRecord? Purely to avoid adding more complexity into Check which is already quite large.

You'll need to extend the if condition in line 335 but it's not that bad I think.

fields := strings.Split(txt, " ")

// redirects must be handled after the rest; instead of having two loops,
Expand Down
22 changes: 22 additions & 0 deletions spf_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,3 +671,25 @@ func TestWithTraceFunc(t *testing.T) {
t.Errorf("expected >0 trace function calls, got 0")
}
}

func TestOctetSizeLimit(t *testing.T) {
dns := NewDefaultResolver()
dns.Txt["domain"] = []string{"v=spf1 ip4:192.168.168.0/24 ip4:192.168.169.0/24 ip4:192.168.170.0/24 ip4:192.168.171.0/24 ip4:192.168.172.0/24 ip4:192.168.173.0/24 ip4:192.168.174.0/24 ip4:192.168.175.0/24 ip4:192.168.176.0/24 ip4:192.168.177.0/24 ip4:192.168.178.0/24 ip4:192.168.179.0/24 ip4:192.168.180.0/24 ip4:192.168.181.0/24 ip4:192.168.182.0/24 ip4:192.168.183.0/24 ip4:192.168.184.0/24 ip4:192.168.185.0/24 ip4:192.168.186.0/24 ip4:192.168.187.0/24 ip4:192.168.188.0/24 ip4:192.168.189.0/24 ip4:192.168.190.0/24 ~all"}
defaultTrace = t.Logf

res, err := CheckHost(ip1111, "domain")
if res != PermError && err != ErrOctetLimitReached {
t.Errorf("expected permerror, got %v (%v)", res, err)
}
}

func TestOverrideOctetSizeLimit(t *testing.T) {
dns := NewDefaultResolver()
dns.Txt["domain"] = []string{"v=spf1 ip4:1.1.1.1 ip4:192.168.169.0/24 ip4:192.168.170.0/24 ip4:192.168.171.0/24 ip4:192.168.172.0/24 ip4:192.168.173.0/24 ip4:192.168.174.0/24 ip4:192.168.175.0/24 ip4:192.168.176.0/24 ip4:192.168.177.0/24 ip4:192.168.178.0/24 ip4:192.168.179.0/24 ip4:192.168.180.0/24 ip4:192.168.181.0/24 ip4:192.168.182.0/24 ip4:192.168.183.0/24 ip4:192.168.184.0/24 ip4:192.168.185.0/24 ip4:192.168.186.0/24 ip4:192.168.187.0/24 ip4:192.168.188.0/24 ip4:192.168.189.0/24 ip4:192.168.190.0/24 ~all"}
defaultTrace = t.Logf

res, err := CheckHostWithSender(ip1111, "helo", "user@domain", OverrideOctetLimit(500))
if res != Pass {
t.Errorf("expected permerror, got %v (%v)", res, err)
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these two can be combined in a single test, for simplicity.

}