Open jimmystewpot opened 2 years ago
jimmystewpot
commented
2 years ago 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
commented
2 years ago 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
commented
2 years ago Thank you and get well soon.
albertito
commented
2 years ago 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.
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.
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.
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.
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
commented
2 years ago 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
commented
2 years ago 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
The RFC suggests SPF records have a record size. The default suggested limit is 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.