When handling a connection, today we only set a deadline after each
command received.
However, this does not cover our initial greeting, or the initial TLS
handshake (if the socket is TLS), so a connection can hang
indefininitely at that stage.
This patch fixes that by setting a deadline earlier, before we send or
receive anythong on the connection.
Our non-delivery status notifications are quite simple today, but that
makes it much more difficult to support internationalization and
cross-language reporting.
There is a standard for internationalized DSNs, RFC 6533 (which builds
on top of the structured DSNs from RFC 3464).
This patch changes our DSN messages to be based on those standards, so
it is easier for MUAs to display reports according to the users'
languages preferences.
Note we still use message/rfc822 + 8bit to transmit the message, instead
of message/global, for compatibility reasons. This seems to be more
universally compatible, but the decision might be revisited in the
future. See RFC 5335 (section 4.6 in particular).
This patch contains some minor code style improvements, to leave the
linter happier and generally follow best practices in some areas where
things snuck through.
In the upcoming Go release, logging from a finished testing.T triggers a
panic. In the courier tests, this is possible because we don't wait for
completion of fakeServer before ending the test.
This patch makes the tests wait for fakeServer to finish before exiting,
removing the race.
Despite its loose appearance, the "Received" header has a reasonably
standarized format.
We were not following the standard format as closely as we should; this
rarely causes problems in this particular case, but there's no need to
deviate from it.
This patch changes the Received header generation as follows:
- The "from" section now uses the remote address as canonical (for
non-authenticated users) which provides more valuable information
than the user-supplied EHLO address (which is also included).
- The remote authenticated user is now hidden, for additional privacy.
- Use the "with" optional clause.
- Use the standard way of printing TLS cipher suite.
- Use the standard way of printing address literals.
MTA-STS has been published as RFC 8461, with no major changes since the
last draft we updated (-18).
This patch updates the documentation accordingly (no code changes).
Instead of pre-filtering the MX list based on STS policy, just check
if it's allowed before each attempt, and skip it if not.
This simplifies the code.
This patch updates the STS implementation from draft version 02 to 18.
The main changes are:
- Policy is now in an ad-hoc format instead of JSON (😒).
- Minor policy well-known URL change (now ends in ".txt").
- Enforce HTTP media type == text/plain, as with the ad-hoc format this
becomes much more important.
- Simplify wildcard mx matching (same algorithm), extend test cases.
- Valid modes are "enforce" (as before), "testing" (replaces "report"),
and "none" (new).
This commit brings back the experimental MTA-STS (Strict Transport
Security) implementation, removed in commit
7f5bedf4aa.
We will continue development in the "sts" branch, subject to rebase,
until it is ready to be integrated into "next" again.
This patch adds more tests for the dovecot library, in particular:
- Protocol errors (invalid versions, etc.).
- Invalid command (cli-specific test).
- Connection breakups.
This patch adds some test cases for I/O errors, in particular when
reading, writing and listing from files that don't exist or that we
shouldn't have permissions to access.
This patch makes chasquid reload domaininfo periodically, so it notices
any external changes made to it.
It is in line with what we do for aliases and authentication already,
and makes it possible for external removals an additions to the
domaininfo database to be picked up without a restart.
Some transient issues might take more than 12h to resolve, specially if
they happen overnight.
20h gives a bit more margin for retries, while still being short enough
so that users are notified early.
This patch adds a missing docstrings for exported identifiers, and
adjust some of the existing ones to match the standard style.
In some cases, the identifiers were un-exported after noticing they had
no external users.
Besides improving documentation, it also reduces the linter noise
significantly.
In the protoio tests, we were using unkeyed fields in some composite
literals. This can cause confusion and makes the code more brittle wrt.
future changes. go vet also complains about this.
This patch fixes the issue by adding the field names to the struct
initializations.
This patch extends various packages and integration tests, increasing
test coverage. They're small enough that it's not worth splitting them
up, as it would add a lot of noise to the history.
We don't expect the AddHeader function to be given an empty key;
however, if that were to happen, it currently crashes.
This patch fixes the bug, while also adding tests for that and other
similar cases.
Dovecot has options for changing the formatting of usernames; for
example, dropping the domain part, or replacing characters.
chasquid's implementation, however, fails to handle this well, as it
expects the reply to contain the username exactly as requested.
This patch fixes the problem by making chasquid ignoring the returned
username, which is unused anyway. The protocol is unambiguous enough.
Tests are also amended to always exercise this case.
This patch adds dovecot support to the chasquid daemon, using the
internal dovecot library added in previous patches.
Dovecot support is still considered EXPERIMENTAL and may be reverted, or
changed in backwards-incompatible ways.
The patch also adds the corresponding integration test, which brings up
a dovecot server with a custom configuration, and tests chasquid's
authentication against it. If dovecot is not installed, the test is
skipped.
This patch adds a new package which implements two basic primitives for
authenticating against dovecot ("user exists", and "check password").
It is still experimental/work in progress.
This patch implements an Authenticator type, which connections use to
do authentication and user existence checks.
It simplifies the abstractions (the server doesn't need to know about
userdb, or keep track of domain-userdb maps), and lays the foundation
for other types of authentication backends which will come in later
patches.
For direct TLS connections, such as submission-over-TLS, we currently
don't get the TLS information so it appears in the headers as "plain
text", which is misleading.
This patch fixes the problem by getting the connection information
early. Note it explicitly triggers the handshake, which would otherwise
happen transparently on the first read/write, so we can use the hostname
(if any) in our hello message.
One of the SMTP tests was doing an external DNS lookup for a
non-existing host, which is reasonably harmless but makes the test less
hermetic.
This patch changes the non-existing host for an invalid address, which
has the same effect but avoids the network lookup.
This is a hack; ideally we would be able to override the resolver, but
Go does not implement that yet, and changing the code is not worth the
additional complexity.
We have many places in our tests where we create temporary directories,
which we later remove (most of the time). We have at least 3 helpers to
do this, and various places where it's done ad-hoc (and the cleanup is
not always present).
To try to reduce the clutter, and make the tests more uniform and
readable, this patch introduces two helpers in a new "testutil" package:
one for creating and one for removing temporary directories.
These new functions are safer, better tested, and make the tests more
consistent. All the tests are updated to use them.
The right-hand side addresses of an alias should be normalized, to
maintain the internal invariant that we always deal with normalized
addresses.
Otherwise, strange situations may arise, such as the same domain having
two different domaininfo structures depending on case.
The outgoing security level checks are not being performed, because of a
bug: the courier thinks the "to"'s domain is always empty.
This patch fixes the bug by simplifying the logic, as there's no need
for the conditional (there is always a domain in the "to" address if it
got to the SMTP courier).
PasswordMatches calculates the proposed derived key, and then compares
it with the actual derived key. That comparison is done using
bytes.Equal, which is not in constant time.
In theory, users with knowledge of the salt could use timing to extract
information about the actual derived key.
In practice, the salt is not being exposed to users, and the caller of
PasswordMatches will add a delay to password checks, so it should not be
easy to exploit via chasquid.
But just to be safe and more future-proof, this patch changes the
comparison to be in constant time.
This commit removes the experimental MTA-STS (Strict Transport Security)
implementation for now, as it's not up to date with the latest draft.
Development will continue on the "sts" branch, but this way it won't
block releases until it is ready.
Commits reverted:
- cb6500b993
- 0eeb964534
- e66288e4b4
- 216cf47ffa
- d66b06de51
- fe00750e39
- 933ab54cd8
This patch adds support for TLS-wrapped submission connections.
Instead of clients establishing a connection over plain text and then
using STARTTLS to switch over a TLS connection, this new mode allows the
clients to connect directly over TLS, like it's done in HTTPS.
This is not an official standard yet, but it's reasonably common in
practice, and provides some advantages over the traditional submission
port.
The default port is 465, commonly used for this; chasquid defaults to
systemd file descriptor passing as for the other protocols (for now).
The current default is "procmail" for historical reasons, but the
program has been unmaintained for years and its use is not generally
advisable.
This patch changes the default MDA binary to "maildrop", which is a more
modern and reasonable MDA to use.
We keep the courier.Procmail name for now, as that's internal, but it
may be changed later. Its documentation is updated to note that the
name is just for reference but it actually works with almost any binary.
expvar.Int.Value appeared in Go 1.8, but we want to keep compatibility
with Go 1.7 at least until the next release.
So this patch replaces the use of expvar.Int.Value in tests, to make
them compatible with Go 1.7 again.
Currently, if we can't find a mail server for a domain, we consider that
a transient failure (semi-accidentally, as we iterate over the (empty)
list of MXs and fall through the list).
It should be treated as a permanent error, in line with other DNS
issues (which is not ideal but seems to be generally accepted
behaviour).
