Optional strict checking of SPF response size

[jimmystewpot]

The RFC suggests SPF records have a record size. The default suggested limit is 450 octets.

Similarly, the sizes for replies to all queries related
to SPF have to be evaluated to fit in a single 512-octet UDP
packet (i.e., DNS message size limited to 450 octets).

This proposed change introduces a 450 octet limit check
with an optional override.

Reference RFC : RFC 7208 Section 3.4

I am using len() on the TXT record as the RFC states that the response packet MUST be ASCII. If future versions of SPF move to unicode support the length calculation will need to be updated.

[jimmystewpot]

Another point, based on the previous PR's that I have submitted to your project. I have this going to the next branch. (and I finally got multi-line commits working). If you want me to change this to master instead happy to do that in future.

[albertito]

Thanks for sending this! On a quick look, this seems totally reasonable.

I am probably going to do a quick pass on the top-1000 domains just to check this isn't going to cause any unexpected failures, and also check what other libraries do, in case common practice has diverged from RFC, but that's just out of an abundance of caution and I don't expect any issues.

Also, doing this on the next branch is perfect, thank you!

I'm out sick so will be a bit slow to do a proper review, but hopefully I'll get to this within a week or so.

Thanks again!

[jimmystewpot]

Thank you and get well soon.

[albertito]

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")

?

[albertito]

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

[albertito]

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 :)

[albertito]

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.

[albertito]

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

[albertito]

I did a review pass and left a few minor comments, but overall structure and approach seems totally fine to me. I'm not that familiar with github reviews so let me know if I messed something up, or ignored any popular convention!

I also spent some time reading the RFC a bit more carefully, looking at perl, C, and Python spf libraries, and analyzing the records of Alexa's top 1000 domains.

RFC interpretation

I think the RFC recommendation is very generic, more like "remember they can't be too long" rather than a strong signal to enforce this at 450. A lot of factors come into play, and the recommendation is from a time where DNS over TCP was much less common than today, AFAIK.

Most interestingly though, the 450 recommendation is about the whole TXT answer, and not just the SPF part. So I don't think this checks exactly what the RFC is recommending.

Note most popular domains' TXT records exceed this recommendation, more on this below.

Popular implementations

Looking at perl's Mail::SPF, C's libspf2, and Python's pyspf, I can't find any limit to the size of the SPF entry.

There are general DNS lookup limits but those are at the DNS library level, not at the SPF checking level.

Alexa top-1000 domains

Looking purely at the SPF top level records of the Alexa top-1000 domains, it seems none would be over the 450 limit.
But that seems to be driven by keeping each individual SPF/TXT string (not the whole answer) to fit within 255 characters.

If we look at the whole TXT answer, exceeding 1000 bytes is very common.

But enforcing each SPF record to be < 450 in length is probably safe.

Suggestion

Based on the above, I am not convinced it's a good idea to have a limit, including enforcing it by default. It doesn't seem to be well backed by RFC (assuming my interpretation is correct, I could definitely be wrong), and it's not done by any other popular implementation as far as I could see.

But if you have a good use case for it, I wouldn't mind it being an option (e.g. WithRecordSizeLimit(limit int)).

I'd be very curious what the use case is. If it's for a type of "SPF record checker", then evaluating at overall TXT record answer size at the DNS level may be a better fit for what the RFC is suggesting?

Please let me know what you think!

Thanks a lot,
Alberto

[jimmystewpot]

I updated the commits to include the recommended variable names before I had read the full PR comment at the bottom.. My brain works chronologically (oops).

In regards to the RFC interpretation, there are two parts to the RFC section and this patch focus's on the second. The first paragraph of section 3.4 talks about the overall TXT record size and links to several other RFC's for reference. The second paragraph is where this pull request is focused.

Note that when computing the sizes for replies to queries of the TXT
format, one has to take into account any other TXT records published
at the domain name. Similarly, the sizes for replies to all queries
related to SPF have to be evaluated to fit in a single 512-octet UDP
packet (i.e., DNS message size limited to 450 octets).

The stand out comment in this is

replies to all queries related to SPF have to be evaluated to fit in a single 512-octet UDP packet (i.e., DNS message size limited to 450 octets).

I have not looked at other SPF implementations as a comparison, I will take the time to review these today. ( I have been unwell which is why this response has taken so long to happen).

To provide more context on the use cases. We have a customer support team that is regularly dealing with email delivery related problems, We have been working on a collection of tools which allow the customer support team to get a single page report on all of the email related setup for the customer and their message logs. One of the checks is to validate that their SPF, DMARC, DKIM, MX etc records are all correctly formatted, within the limits etc. When the customer support team advise customers of an issue the customers will often check themselves using various internet tools (like mxtoolbox, https://vamsoft.com/support/tools/spf-policy-tester etc), many of these tools highlight if the record size is greater than 450. Once this was discovered we found that some of our corporate domains also fell into this problem space and need to be corrected.

I'll take some time to update to make this optional as you suggest. It'll take me a few days.

[albertito]

The stand out comment in this is

replies to all queries related to SPF have to be evaluated to fit in a single 512-octet UDP packet (i.e., DNS message size limited to 450 octets).

I have not looked at other SPF implementations as a comparison, I will take the time to review these today.

Thanks, I see where this is coming from. Let me address this below.

( I have been unwell which is why this response has taken so long to happen).

Sorry to hear this! I hope you are better now :)

And don't worry about the delays! This is all voluntary so of course it's not a problem at all.

I will also be with limited connectivity for the next couple of weeks, but will do my best to follow up and don't block you.

To provide more context on the use cases. We have a customer support team that is regularly dealing with email delivery related problems, We have been working on a collection of tools which allow the customer support team to get a single page report on all of the email related setup for the customer and their message logs. One of the checks is to validate that their SPF, DMARC, DKIM, MX etc records are all correctly formatted, within the limits etc. When the customer support team advise customers of an issue the customers will often check themselves using various internet tools (like mxtoolbox, https://vamsoft.com/support/tools/spf-policy-tester etc), many of these tools highlight if the record size is greater than 450. Once this was discovered we found that some of our corporate domains also fell into this problem space and need to be corrected.

I think such a check is totally reasonable to have in a validator, but I am not convinced that it's a good fit to enforce the limit in the SPF library as such.

The DNS libraries deal with DNS limits, and if the queries succeed (and these days fallback to TCP is probably way more common than when the SPF RPC was written), then I think answers should be accepted.

However, I think it is reasonable to also support the "validator" use case, since the library is already doing the parsing and recursion work which you'd otherwise have to re-implement.

But I think you can use the existing WithResolver option for that: your custom DNS resolver can check the size of TXT responses, and fail the query with a permanent error if it's too large. That error will be propagated and returned to the caller of CheckHostWithSender, and then you can use that in the validator.

And since it's a custom error, and done at DNS lookup time, you can potentially add a lot of information to it based on your needs, which the SPF library will just pass along.

What do you think?

I'm happy to elaborate with an example if what I'm proposing is not clear enough, just let me know.

I'll take some time to update to make this optional as you suggest. It'll take me a few days.

Again, no rush on my part. Much appreciate your thorough analysis and patches to make the library better!

Thank you!
Alberto