While auditing Answer::as_txt usage as a follow up from the recent
SPF fix, I noticed a TODO in the dkim code (which we forked from
another implementation) to support processing multiple TXT
records.
This commit implements the necessary tweaks to extract multiple
signatures and attempt to verify them against the incoming message.
The issue here was essentially a data fidelity issue around
TXT record representation.
A DNS TXT record can be composed from multiple strings, and a domain can
return multiple TXT records, so there is some nesting.
The SPF RFC says:
```
3.3. Multiple Strings in a Single DNS Record
As defined in [RFC1035], Sections 3.3 and 3.3.14, a single text DNS
record can be composed of more than one string. If a published
record contains multiple character-strings, then the record MUST be
treated as if those strings are concatenated together without adding
spaces. For example:
IN TXT "v=spf1 .... first" "second string..."
is equivalent to:
IN TXT "v=spf1 .... firstsecond string..."
TXT records containing multiple strings are useful in constructing
records that would exceed the 255-octet maximum length of a
character-string within a single TXT record.
```
so the SPF logic was dutifully joining records together around the
empty string.
Howerver, if you look at `dig yahoo.com txt` you'll see a bunch
of non-SPF records:
```
yahoo.com. 1800 IN TXT "google-site-verification=Z3-Vh6zqUMgybVH4wQl1GxKSKN7JE13kyCyeZ3TZZ-I"
yahoo.com. 1800 IN TXT "v=spf1 redirect=_spf.mail.yahoo.com"
yahoo.com. 1800 IN TXT "Zoom=13284637"
yahoo.com. 1800 IN TXT "edb3bff2c0d64622a9b2250438277a59"
```
these were getting joined together and producing a bogus input.
Obviously we should not join the results from the txt lookup
together like that, but then why would the RFC make a point
of talking about joining stuff together, and where should
that logic live?
Our `Answer::as_txt` implementation was doing some joining
of its own and it turned out that it was concatenating across
the outer layer of the aforementioned TXT record nesting.
This commit fixes that up and adds some test coverage.
I worry that 1 minute might be too short to be universally good,
particularly as a change in behavior from an earlier build: going from
no timeout to 1 minute could be super surprising and un-welcome.
Let's bump this up to 5 minutes which more closely matches the
default for DATA in the base SMTP RFC.
This is why slow DNS often expanded to 3-4x as long as the expected
timeout value; we make a handful of calls in succession, assuming
that the cache was effective in an earlier stage.
We now cache errors for 5 minutes by default, and it can be configured
as desired.
refs: https://github.com/KumoCorp/kumomta/issues/325
The queue logic assumes that if we get Some(duration) back from a
throttle check that it must be a non-zero duration.
We found a configuration that used a custom lua delivery handler
together with a max_message_rate throttle. When running with local
(non-redis) throttles it is possible that the in-memory throttle
implementation can return a 0ns delay.
This results in the queue subsystem choosing to requeue the message
using the throttle duration as the delay, which results in the message
being due immediately, which causes recursion into the insert-ready
flow, which performs the same throttle check with the same 0ns result,
and repeat until a stack overflow occurs on the spool in thread.
The circumstances to trigger this are a bit niche and racy: if you turn
up debug logging you can perturb the timing so that the stack overflow
is not 100% repeatable. It is unlikely to cause a stack overflow on the
inbound processing side because the threads doing that processing are
multiplexing many other events that can also perturb the timing.
This commit addresses this edge case in a very simple way: if the
duration is 0, then we return `None` for the duration. This is actually
how we handle this for one of the redis throttle implementations
already. This commit makes the other two cases consistent with that.
previously, we assumed that only the parent log_dir might be the thing
that would need to be created on demand.
If you configure per-record log files with subdirectories in them, and
the indicated directory didn't already exist, then we'd error out.
This commit will catch the error in that case and try to fill out the
directory structure on demand, which makes things a bit more convenient.
If you have a lot of RAM, using only default percentages could
potentially leave some RAM unused. These lua functions
allow more precise control over the limits.
The signer cache maps the signer parameters to a pre-made
signing context.
It is essentially a map of (domain, key) -> signer.
Assuming that the (domain,key) tuple is unique, then this is
a good, effective use of that cache.
However, if multiple domains can share the same key then we
can end up re-parsing the same key data for each of them,
which is moderately expensive and a waste of CPU.
This commit introduces an additional cache for the key source to
the resultant compiled key.
This allows sharing of the same compiled key across signing
parameters that otherwise vary, and should help to shave off
some latency.
We only use the blocking threads portion of the thread pool,
so avoid the default of creating number-of-cores threads
for the IO portion. We have to create one even though we
don't use it, otherwise tokio is unhappy.
Wez Furlong
Ryan Bonnell
MHillyer