This patch groups a series of commands that redirect to a file into a
single grouped redirect, for readability reasons.
Suggested by shellcheck SC2129.
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.
There are a couple of places in the tests when we attempt to build and
run simultaneously. Here, if the build is slow, there is a race where
"text file busy" can appear.
To fix this, build to a temporary file with a random name, then
atomically rename it to the final binary name.
When we put something in the queue and respond "250 ok" to the client,
that is taken as accepting the email.
As part of putting something in the queue, we write it to disk, but
today we don't do an fsync on that file.
That leaves a gap where a badly timed crash on some systems could lead
to the file being empty, causing us to lose an email that we accepted.
To elliminate (or drastically reduce on some filesystems) the chances of
that situation, we call fsync on the file that gets written when we put
something in the queue.
Thanks to nolanl@github for reporting this in
https://github.com/albertito/chasquid/issues/78.
The current fail2ban regexp catches all SMTP connection errors.
This works fine, but includes connection errors, that can be caused by
transient external causes, and accidentally delay email delivery.
This patch changes the regexp to be more targeted towards specific SMTP
errors that are likely to be caused by deliberate actions.
The expression was cross-checked with a few month of errors to confirm
it should not have false positives, and that it correctly left
connection errors alone.
Thanks to pepperbob@github for reporting this in
https://github.com/albertito/chasquid/issues/77.
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, we launch dovecot in the background and chasquid in the
foreground using sudo.
This means that dovecot failures won't propagate, and signals to the
container (e.g. to stop it) also don't get propagated to dovecot
(because it's in the background) or chasquid (because they don't go
beyond the sudo process).
Thanks to [Guiorgy@github](https://github.com/Guiorgy) for identifying
the problem, proposing alternatives, help debugging, and discussing this
in https://github.com/albertito/chasquid/pull/70.
When we get SIGINT or SIGTERM, today chasquid exits with code 1. This
can confuse some of the supervision tools.
In particular, Docker and Kubernetes expect exit 0 upon an intentional
stop.
And systemd, the Restart= semantics make a difference with 0 and non-0,
and exiting with 1 prevents users from making that distinction.
This patch changes the SIGINT/SIGTERM exit code to 0, to make it easier
for users to set up things as desired in those environments.
Thanks to [Guiorgy@github](https://github.com/Guiorgy) for reporting
this problem in https://github.com/albertito/chasquid/pull/70.
Dovecot 2.4 has a new configuration format, which is unfortunately
backwards-incompatible with Dovecot 2.3.
This patch adds a 2.4-compatible config, and selects which one to use
based on the Dovecot version in the environment.
In the future, once 2.4 becomes more common, we will drop the 2.3 config
from the test.
Note that we don't change the config used in the Docker image, because
that is based on Debian **stable** which is still on 2.3.
The manpages are generated from pod files using pod2man, but it has been
a long time since we last re-generated them, and the new versions of
pod2man generate significantly different (and simpler) pages.
So this patch just regenerates the man pages, to make future changes
easier and more self contained to review.
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.
We have a few Python scripts which over the years ended up with a
variety of formatting.
This patch auto-formats them using `black -l 79` to make them more
uniform, and easier to read and write.
minidns supports MX records, but today it hard-codes priority=10.
This is limiting when creating test scenarios that depend on having
different MX priorities.
This patch adds support for specifying the priority in MX records.
In the tests, we create a lot of Recipient{}s, and that ends up being
very verbose and sometimes cumbersome.
Also, if we ever want to extend it, it would result in a lot of
unnecessary refactoring.
So this patch replaces the Recipient{} instantiations with helper
functions, to help readability and extendability.
This only affects the tests, and there are no changes to them either, it
is purely a refactor.
Today, aliases parsing is too lax, silently ignoring most kinds of invalid
lines.
That behaviour can cause a lot of confusion when users think the aliases
are being parsed, and also cause problems when extending the syntax.
This patch fixes that problem by making aliases parsing return errors on
the invalid lines.
Unfortunately this will cause some previously-accepted files to now be
rejected, but it should be quite visible.
This patch implements support for aliases that contain '*' as the
destination user.
In that case, we replace it with the original user.
For example, `*: *@pond` will redirect `lilly@domain` to `lilly@pond`.
This is experimental for now, and marked as such in the documentation.
The semantics can be subtle, so we may need to adjust them later.
This patch fixes some minor typos in comments and strings found by
codespell.
While at it, also expand some variable names that were not typos, but
caused false positives, and end up being more readable anyway.
Today, we only require that Docker integration tests use a Go version
>= 1.20. In practice, it almost always picks the latest version.
For consistency, this patch requests it to run the latest Go version.
Today, the step to build the Docker public image depends on the coverage
run. This dependency isn't necessary, as the coverage could be failing
for a variety of reasons (e.g. codecov being down) and doesn't signal
any problem with chasquid itself.
So this patch fixes that: if the integration tests pass, then that is
good enough for building the public image.
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.
Authenticated users are intentionally allowed to send email as other users or
domains. This is a design choice made to balance simplicity of operation and
use.
However, it can be surprising and it's not obvious, so this patch adds a
note to the documentation about it.
Thanks to Matěj Volf for suggesting this improvement!
Fixes: https://github.com/albertito/chasquid/issues/62
This patch adds a document with guidelines for contributing to chasquid.
It includes suggestions for how to ask questions, how to send patches
(and the expectations around them), and documents how the different
branches are used.
Thanks to raspbeguy (https://github.com/raspbeguy) for suggesting this
improvement.
`chasquid-util user-add` is meant to create the domain directory if it
doesn't exist; however there's a bug that makes this not happen, and
instead the command fails with:
Error writing database: open <path>: no such file or directory
This patch fixes the issue and adds a test to ensure we don't have any
regressions on this behaviour.
Thanks to raspbeguy (https://github.com/raspbeguy) for reporting this
issue (on IRC).
This patch regenerates the auto-generated files. There are no
significant changes.
- Protobuf files updated the comment formatting to match recent changes
in Go libraries.
- IANA assignment for a AEGIS (currently an IETF draft) has been
updated.
- The link to the human-readable IANA assignment tables from the
generator was manually updated.
This patch adds a fail2ban filter configuration example for chasquid.
It can be used to configure fail2ban to detect IPs causing connection
churn or high rate of errors.
The ToCRLF/StringToCRLF functions are not very performance critical, but
we call it for each mail, and the current implementation is very
inefficient (mainly because it goes one byte at a time).
This patch replaces it with a better implementation that goes line by line.
The new implementation of ToCRLF is ~40% faster, and StringToCRLF is ~60%
faster.
```
$ benchstat old.txt new.txt
goos: linux
goarch: amd64
pkg: blitiri.com.ar/go/chasquid/internal/normalize
cpu: 13th Gen Intel(R) Core(TM) i9-13900T
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
ToCRLF-32 162.96µ ± 6% 95.42µ ± 12% -41.44% (p=0.000 n=10)
StringToCRLF-32 190.70µ ± 14% 76.51µ ± 6% -59.88% (p=0.000 n=10)
geomean 176.3µ 85.44µ -51.53%
```
ssl.wrap_socket() has been deprecated and is no longer functional in
Python 3.12: https://docs.python.org/3/whatsnew/3.12.html#ssl.
This patch replaces it with the equivalent (in this context)
ssl.SSLContext.
The timestamp string in the t= and x= headers is an "unsigned decimal
integer", but time.Unix takes an int64. Today we parse it as uint64 and
then cast it, but this can cause issues with overflow and type
conversion.
This patch fixes that by parsing the timestamps as signed integers, and
then checking they're positive.
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.
Unfortunately, `go get` rejects repos that have files with ':':
https://github.com/golang/go/issues/28001.
We have one such file in the tests.
This prevents some of the Go tooling from working on the latest release,
including pkg.go.dev.
So, as a workaround we use a compatible file name in the repository, and
rename it when running the test. This is very hacky, but it's okay for a
single test.
The `latest` tag is meant to track the `main` branch, but I just noticed
it hasn't been pushed out in a while. This is because the conditional
gating the push on the branch being `main` is incorrect.
This patch fixes the problem by using the correct conditional on the
branch name.
The current .gitignore pattern doesn't work when the private test cases
are a symlink, which can be convenient.
This patch fixes it by changing the pattern to match symlinks as well as
directories.
This patch adds a cross-tool integration check that uses
driusan/dkim's dkimverify to confirm it can verify our own DKIM signatures.
It is optional, since the tool may not be present.
This patch removes the integration tests that covered using driusan/dkim
and dkimpy's tools in the example hook.
Now that we have internal DKIM support, the example hook doesn't attempt
to use them, so we can remove the tests that cover it.
Those tools, and other DKIM implementations, can still be used in the
post-data hook just as before.
To send mails, today some tests use msmtp and others our internal smtpc.py.
This works, but msmtp slows down the tests significantly, and smtpc.py
is also not particularly fast, and also has some limitations.
This patch introduces a new SMTP client tool written in Go, and makes
almost all the tests use it.
Some tests still remain on msmtp, mainly for client-check compatibility.
It's likely that this will be moved in later patches to a separate
special-purpose test.
With this patch, integration tests take ~20% less time than before.
Dovecot applies an authentication penalty, where it delays failed attempts.
Because we intentionally do bad authentications for testing, this slows
downs the tests significantly. So this patch disables it.
Our tests invoke a variety of helpers, some of them are written in Go.
Today, we call "go build" (directly or indirectly via "go run"), which is
a bit wasteful and slows down the tests.
This patch makes the tests only build our Go helpers once every 10s at
most.
The solution is a bit hacky but in the context of these tests, it's
practical.
The generate_cert cache has a bug because it uses the directory's age,
which won't necessarily change, and it was always re-generating
certificates after 10m.
This patch fixes the bug by checking the age of the private key file
instead of the directory.
This patch adds tests to verify how safeio behaves when *os.File
operations return various errors.
To do this, we allow the possibility of wrapping os.CreateTemp, so we
can simulate the errors during testing.
This is not pretty, but the code is small enough that the readability
overhead is minimal, and we get a lot of tests from it.
We've had a couple of reported issues about the difficulty of setting up
new clients, or confusion due to using broken clients:
- https://github.com/albertito/chasquid/pull/46
- https://github.com/albertito/chasquid/issues/52
This patch adds the first version of a "Clients" document that includes
requirements for all clients, configuration examples, and a list of
known-problematic client software.
The goal is to help reduce friction and confusion when setting up
clients.
The document needs more polishing and examples, which hopefully will be
added later.
Fixes https://github.com/albertito/chasquid/issues/48.
Nearly all the github actions we rely on have increased their major
version, at least to update the Node.js version they run on, since the
previous one is being deprecated and will eventually become unsupported.
So this patch updates all the versions of the github actions we use.
Unfortunately, but understandably, Cirrus CI no longer offers enough
free compute credits to run our tests reliably.
They're currently only used to run the Go tests on FreeBSD.
In the future, this might be replaced with something else; but until a
proper replacement can be found and tested, remove it to avoid
introducing noise in the CI results.
Since we moved the Docker workflows to Github (after v1.10), they have
not been running on tags, so there are no tagged docker images for
v1.11, v1.12 and v1.13.
This is (hopefully) because we're not explicitly asking for the workflow
to be run on tag pushes.
This patch (hopefully) fixes that by adding an explicit section in the
config to make it run on tag pushes.
Thanks to Christoph Mewes (xrstf@github) for reporting this in
https://github.com/albertito/chasquid/issues/51.
The SMTP smuggling vulnerability fixed in 1.13 (and 1.11.1) has been
given a CVE number: CVE-2023-52354
(https://nvd.nist.gov/vuln/detail/CVE-2023-52354).
This patch adds a link to it in the release notes, for ease of reference.
chasquid v1.11.1 was released on 2023-12-26 with a backport of the
security fixes from 1.13.
This was requested by users of Debian stable, who are on 1.11.
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.
This patch makes mail_diff more strict in its parsing, to ensure we
catch any encoding issues that may otherwise be masked by the default
compatibility policy.
The minor dialogs test covers some very specific SMTP exchanges, and
some of those include delivering email.
Today, we don't verify the final mailbox, we just check the SMTP
exchange. However, it can be very useful for some of the tests to do
end-to-end checking of the final mailbox.
This patch implements that ability in the test itself, and on the
(currently only) email delivering dialog.
Subsequent patches that introduce new tests will make use of this
feature.
Some use cases, like receive-only MTAs, need domain users for receiving
emails, but have no real need for passwords since they will never use
submission.
Today, that is not supported, and those use-cases require the
administrator to come up with a password unnecessarily, adding
complexity and possibly risk.
This patch implements "receive-only users", which don't have a valid
password, thus exist for the purposes of delivering mail, but always
fail authentication.
See https://github.com/albertito/chasquid/issues/44 for more details and
rationale.
Thanks to xavierg who suggested this feature on IRC.
Using an empty listening address will result in chasquid listening on a
random port, which is a dangerous misconfiguration.
That is most likely done to prevent it from listening at all.
To prevent this misconfiguration, explicitly reject empty listening
addresses early and with a warning, so there is no ambiguity.
Users can still prevent chasquid from listening by just commenting out
the entry in the config (and not passing any systemd file descriptors).
See https://github.com/albertito/chasquid/issues/45 for more details and
discussion, including alternatives considered.
Thanks to xavierg who reported this via IRC.
When logging the configuration, we currently don't quote the string
values, which can make whitespace-induced problems difficult to identify
and troubleshoot.
This patch changes the formatting to always quote string values when
logging the configuration.
This patch adds the embedded aliases file to the fuzz corpus, because it
is trivial to do so, and is a reasonable seed which will be naturally
adjusted over time as the package evolves (as it happened in recent
commits).
The testing package does not allow t.Fatal to be called from a different
goroutine; however, we do that if the testing server fails listen or
accept connections.
Since that is unexpected and rare, this patch turns those calls into
panics.
Today, when a user sets an alias with drop characters and/or suffixes,
those go unused, since we always "clean" addresses before alias
resolution.
This results in unexpected and surprising behaviour, and it's not
properly documented either.
This patch resolves this unexpected behaviour as follows:
- Drop characters are ignored, both at parsing time and at lookup time.
- Lookups are done including the suffixes first, and if that results in
no matches, they are retried without suffixes.
This results in aliases working more intuitively for the most common use
cases: of users wanting to have different aliases for specific suffixes,
and not having to care for drop characters.
Hooks can be used to get different behaviour if needed, since the first
lookup is done with the address as-is.
Thanks to znerol@ (lo+github@znerol.ch) for reporting this, and the
discussion on how to fix it, in
https://github.com/albertito/chasquid/issues/41.
Today, the parsing functions are stand-alone since they don't need
anything from the resolver.
But in future patches, that will change.
In anticipation of that, move those functions to be methods of the
resolver.
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.
Since commit e6c6df45, `chasquid-util alias-resolve` talks to the server
and returns authoritative answers.
This patch updates docs/aliases.md to reflect that, which was missed in
the original set of changes.
Today we check the aliases deliver mail to the expected locations, but
we don't fail if there are unexpected deliveries.
Doing so can help catch bugs (including test bugs), so this patch
implements that.
In addition, fix two of the tests that were printing on error, but not
causing the tests to fail (which was the original intention).
This patch moves the top-level certificate loading logic out of main and
into a separate function.
This is only for readability and consistency with how we handle domains
(which have a similar structure already). There are no logic changes.
When starting up, for each domain we parse aliases and users files (if
they exist).
Today we print a line for each, which gets quite verbose and doesn't
offer much useful information.
This patch adjusts that logic so that we only print errors (unless a
file does not exist, which is a normal case).
This also improves the scenario where chasquid does not have permissions
to access the users file: before this patch, that would fail silently,
but with this patch we show the correct error.
While at it, make the "Loading" messages consistent with each other, for
readability.
The t-20-bad_configs test can sometimes have false positives because
the last line recorded in the log isn't always the one causing the
failure, due to asynchronous logging and in particular in combination
with coverage tests (which alter the os.Exit behaviour subtly).
This patch fixes that by having the tests look at the last 4 lines of
output instead. This is not super pretty, but it should be good enough
to cover for any timing issues that arise in these particular tests.
Alberto Bertogli