In a few places, we call Printf-like functions, but for the format we
use either non-format messages (which is not tidy, but okay), or
variable messages (which can be problematic if they contain %-format
directives).
The patch fixes the calls by either moving to Print-like functions, or
using `Printf("%s", message)` instead.
These were found by a combination of `go vet` (which complains about
"non-constant format string in call"), and manual inspection.
staticcheck found a couple of minor code cleanup improvements, like
unused variables or an out-of-order defer, mostly in tests.
This patch fixes those problems by making the necessary adjustments.
They're all fairly small, and should not change the logic in any
significant way.
When constructing the "Received" header, in some cases we want to
include the remote IP address in addition to the EHLO domain.
The way we did that is not fully compliant with RFC 5321 (section 4.4),
and this has the potential to confuse some tools that parse the header.
This patch fixes this problem by adjusting the order of the two pieces
of data, which makes it comply with the RFC.
Before:
Received: from [1.2.3.4] (ehlo.domain.example.com)
After:
Received: from ehlo.domain.example.com ([1.2.3.4])
Thanks to nolanl@github for reporting this problem in
https://github.com/albertito/chasquid/issues/76.
Today, the maximum number of items in the queue, as well as how long we
keep attempting to send each item, is hard-coded and not changed by end
users.
While they are totally adequate for chasquid's main use cases, it can
still be useful for some users to change them.
So this patch adds two new configuration options for those settings.
They're marked experimental for now, so we can adjust them if needed
after they get more exposure.
Thanks to Lewis Ross-Jones <lewis_r_j@hotmail.com> for suggesting this
improvement, and help with testing it.
This patch implements "via" aliases, which let us explicitly select a
server to use for delivery.
This feature is useful in different scenarios, such as a secondary MX
server that forwards all incoming email to a primary.
For now, it is experimental and the syntax and semantics are subject to
change.
Microsoft SMTP servers have a bug that prevents them from successfully
establishing a TLS connection against modern Go TLS servers, and some
OpenSSL versions. It also doesn't fall back to plain-text, so this has
been causing deliverablity issues.
The problem started by the end of 2024 and it's still not fixed.
Unfortunately, because they're quite a big provider and are not fixing
their problem, it is worth to do a server-side workaround.
This patch implements that workaround: it disables TLS session tickets.
There is no security impact for doing so, and there is a small
performance penalty which is likely to be insignificant for chasquid's
main use cases.
This workaround should be removed once Microsoft fixes their problem.
We are going to make a 1.15.1 release for this, which this patch also
documents.
Thanks to Michael (l6d-dev@github) for reporting this issue and
suggesting this workaround!
See https://github.com/albertito/chasquid/issues/64 and
https://github.com/golang/go/issues/70232 for more details.
This commit updates the uses of math/rand to math/rand/v2, which was
released in Go 1.22 (2024-02).
The new package is generally safer, see https://go.dev/blog/randv2 for
the details.
There are no user-visible changes, it is only adjusting the name of
functions, simplify some code thanks to v2 having a better API, etc.
This patch makes chasquid log how many users, aliases and DKIM keys were
loaded for each domain.
This makes it easier to confirm changes, and troubleshoot problems
related to these per-domain configuration files.
Today, when starting up, if there's an error reading the users or
aliases files, we only log but do not exit. And then those files will
not be attempted to be read on the periodic reload.
We also treat "file does not exist" as an error for users file, but not
aliases file, resulting in inconsistent behaviour between the two.
All of this makes some classes of problems (like permission errors) more
difficult to spot and troubleshoot. For example,
https://github.com/albertito/chasquid/issues/55.
So this patch makes errors reading users/aliases files on startup a
fatal error, and also unifies the "file does not exist" behaviour to
make it not an error in both cases.
Note that the behaviour on the periodic reload is unchanged: treat these
errors as fatal too. This may be changed in future patches.
The RFCs are very clear that in DATA contents:
> CR and LF MUST only occur together as CRLF; they MUST NOT appear
> independently in the body.
https://www.rfc-editor.org/rfc/rfc5322#section-2.3https://www.rfc-editor.org/rfc/rfc5321#section-2.3.8
Allowing "independent" CR and LF can cause a number of problems.
In particular, there is a new "SMTP smuggling attack" published recently
that involves the server incorrectly parsing the end of DATA marker
`\r\n.\r\n`, which an attacker can exploit to impersonate a server when
email is transmitted server-to-server.
https://www.postfix.org/smtp-smuggling.htmlhttps://sec-consult.com/blog/detail/smtp-smuggling-spoofing-e-mails-worldwide/
Currently, chasquid is vulnerable to this attack, because Go's standard
libraries net/textproto and net/mail do not enforce CRLF strictly.
This patch fixes the problem by introducing a new "dot reader" function
that strictly enforces CRLF when reading dot-terminated data, used in
the DATA input processing.
When an invalid newline terminator is found, the connection is aborted
immediately because we cannot safely recover from that state.
We still keep the internal representation as LF-terminated for
convenience and simplicity.
However, the MDA courier is changed to pass CRLF-terminated lines, since
that is an external program which could be strict when receiving email
messages.
See https://github.com/albertito/chasquid/issues/47 for more details and
discussion.
The aliases.Resolver.Exists function currently returns the "clean"
address (with the drop characters and suffixes removed), which is relied
upon in its only caller.
That, however, makes the logic more difficult to follow, hiding some
of the address manipulation behind what should be a read-only check.
So this patch reorganizes that code a little bit, removing the
"cleaning" of the address as part of Exists, and making it explicit when
needed instead.
This patch does not have any user-visible change in behaviour, it is
just internal reorganization.
This is in preparation for further patches which will improve the
handling of some aliases corner cases.
This patch makes chasquid run a localrpc server, exporting two methods:
alias resolve, and domaininfo clear.
They will be used by chasquid-util in later patches.
It is not expected that the user modifies the domaininfo database behind
chasquid's back, and reloading it can be somewhat expensive.
So this patch removes the periodic reload, and instead makes it triggered
by SIGHUP so the user can trigger a reload manually if needed.
This patch changes several internal packages to receive and pass tracing
annotations, making use of the new tracing library, so we can have
better debugging information.
ioutil package was deprecated in Go 1.16, replace all uses with their
respective replacements.
This patch was generated with a combination of `gofmt -r`, `eg`, and
manually (for `ioutil.ReadDir`).
We've accumulated a few linter issues around comments and a couple of
variable names.
While none of them is major, this patch cleans them up so it's easier to
go through the linter output, and we can start being more strict about
it.
This patch implements support for catch-all aliases, where users can add
a `*: destination` alias. Mails sent to unknown users (or other aliases)
will not be rejected, but sent to the indicated destination instead.
Please see https://github.com/albertito/chasquid/issues/23 and
https://github.com/albertito/chasquid/pull/24 for more discussion and
background.
Thanks to Alex Ellwein (aellwein@github) for the alternative patch and
help with testing; and to ThinkChaos (ThinkChaos@github) for help with
testing.
This patch simplifies the internal alias lookup logic, unifying it
across Resolve and Exists.
As part of this, the `alias-exists` hook is removed. It was redundant to
begin with, although it enabled a potential optimization, it isn't worth
the complexity. The timeout for execution of both was the same.
This change should be backwards-compatible because `alias-resolve` is
still used, and the semantics haven't changed.
tls.BuildNameToCertificate has been deprecated, and calling it is no
longer necessary since Go 1.14.
Now that our minimum supported Go version is 1.15, we can remove it.
Today, we close the connection after 10 errors. While this is fine for
normal use, it is unnecessarily large.
Lowering it to 3 helps with defense-in-depth for cross-protocol attacks
(e.g. https://alpaca-attack.com/), while still being large enough for
useful troubleshooting and normal operation.
As part of this change, we also remove the AUTH-specific failures limit,
because they're covered by the connection limit.
When we receive unknown commands, we use the first 6 bytes for
troubleshooting (e.g. put them in traces and exported metrics).
While this is safe, since the different places know how to quote them
properly, it makes things more difficult to analyse, since it's not
uncommon to see be binary blobs.
This patch makes us use the ascii-quoted version instead, to make things
easier to analyze.
When we fail to check if a user exists, we currently return a permanent
error, which can be misleading and also make things more difficult to
troubleshoot.
This patch makes chasquid return a temporary error in that case.
Thanks to Thor77 (thor77@thor77.org) for suggesting this change.
This patch implements support for incoming connections wrapped in the
HAProxy protocol v1.
This is useful when running chasquid behind a HAProxy server, as it
needs the original source IP to perform SPF checks.
This patch is a reimplementation of one originally provided by Denys
Vitali in pull request #15, except the logic for the protocol handling
is moved to a new package, and the smtpsrv.Conn handling of the source
IP is simplified.
It is marked as experimental for now, since we want to give it a bit
more exposure just in case the option/api needs adjustment.
Thanks a lot to Denys Vitali (@denysvitali in github) for sending the
original patch for this, and helping test it!
This patch renames courier.Procmail to courier.MDA, to make it more
obvious that the functionality is not tied to that particular MDA.
It's just for readability, there are no functional changes.
Some utilities might want to access the EHLO/HELO domain in the
post-data hook (for example, to do additional SPF validations).
This patch implements that support, including sanitizing the EHLO domain
on the environment variable to reduce the risk of problems.
The EHLO parameter is generally referred to as "domain", even though it
can take either a domain or an address.
For clarity, rename the variable and comments to match.
This is stylistic only, there are no functional changes.
This patch makes chasquid's monitoring server expose an OpenMetrics
metrics endpoint.
It adds a new package "expvarom" which implements an HTTP handler that
exports expvar variables in the OpenMetrics text format.
Then, the handler is registered by the monitoring server at /metrics
(where most things expect it to be).
The existing exported variables are also extended with descriptions,
which is optional, but improves the readability of the metrics.
When we can't authenticate due to a transient issue, for example if we
rely on Dovecot and it is not responding, we should use a differentiated
error code to avoid confusing users.
However, today we return the same error code as when the user enters the
wrong password, which could confuse users as their MUA might think their
credentials are no longer valid.
This patch fixes the issue by returning a differentiated error code in
that case, as per RFC 4954.
Thanks to Max Mazurov (fox.cpp@disroot.org) for reporting this problem.
tls.Config.BuildNameToCertificate was deprecated in Go 1.14, and is no
longer necessary.
However, we support down to 1.11, so we will keep it for now.
This patch adds a TODO to remove it in the future once the minimum
supported version is 1.14; and adjust the CI linter accordingly.
When creating a new Queue instance, we os.MkdirAll the queue directory.
Currently we don't check if it fails, which will cause us to find out
about problems when the queue is first used, where it is more annoying
to troubleshoot.
This patch adjusts the code so that we check and propagate the error.
That way, problems with the queue directory will be more evident and
easier to handle.
The linter complains that we're not checking for errors, but on some
cases it's on code paths were it is reasonable to do so (e.g. we're
closing the connection and it's a best-effort write).
This patch adjusts the code to make those cases explicit.
When receiving a message on a TLS socket, we currently don't check the
Handshake result, so connections often fail in a way that is not easy to
troubleshoot.
This patch fixes that by checking the result and emitting a nicer error
message before closing the connection.
The smtpsrv fuzzer doesn't handle DATA commands particularly well:
it will continue to read but will skip lines that have STARTTLS as
content, and only really care for the first line due to a bug.
This patch fixes the handling, and moves the logic to a separate
function for readability.
When the client closes the connection, which is very common, chasquid
logs it as an error ("exiting with error: EOF").
That can be confusing and mislead users, and also makes a lot of
traces be marked as errored, when nothing wrong occurred.
So this patch changes the log to not treat it as an error.
When the DATA input is too large, we should keep on reading through it
until we reach the end marker, otherwise there is a security problem:
the remaining data will be interpreted as SMTP commands, so for example
a forwarded message that is too long might end up executing SMTP
commands under an authenticated user.
This patch implements this behaviour, while being careful not to consume
extra memory to avoid opening up the possibility of a DoS.
Note the equivalent logic for single long lines is already implemented.
Reloading during tests will cause the testing aliases to be removed,
which makes test runs that extend beyond 30s to be flaky.
This patch fixes the bug by disabling reloads during these tests.
The testing couriers are currently only used in the queue tests, but we
also want to use them in smtpsrv tests so we can make them more robusts
by checking the emails got delivered.
This patch moves the testing couriers to testlib, and makes both queue
and smtpsrv use them.
Currently, there is no limit to incoming line length, so an evil client
could cause a memory exhaustion DoS by issuing very long lines.
This patch fixes the bug by limiting the size of the lines.
To do that, we replace the textproto.Conn with a pair of buffered reader
and writer, which simplify the code and allow for better and cleaner
control.
Thanks to Max Mazurov (fox.cpp@disroot.org) for finding and reporting
this issue.
