the synchronous writing to the socket would have typically invoked the
write callback, which would flush further commands, thus recursing.
we take the easy way out and make it fully asynchronous, i.e., no data
is sent before (re-)entering the event loop.
this also has the effect that socket_write() cannot fail any more, and
any errors will be reported asynchronously. this is consistent with
socket_read(), and produces cleaner code.
this introduces a marginal performance regression: the maildir driver is
synchronous, so all messages (which fit into memory) will be read before
any data is sent. this is not considered relevant.
in the case of imap stores, the failure is bound to the server config,
not just the store config.
that means that the storage of the failure state needs to be private to
the driver, accessible only through a function.
USER (the authorization identity) specifies whom to act for.
AUTHNAME (the authentication identity) specifies who is acting (and
thus whose PASS is being used).
USER is derived from AUTHNAME if omitted, but apparently the
GSS-API module automatically adds the REALM, which is not helpful.
it appears to be common to set both USER and AUTHNAME to the same value,
so let's just do it as well.
REFMAIL: 20150407194807.GA1714@leeloo.kyriasis.com
the PassCmd will be typically non-interactive (or it will use a gui
password agent), so starting a new line just makes the progress counter
uglier. so make it configurable and default to no line break.
- the old meaning of -V[V] was moved to -D{n|N}, as these are really
debugging options.
- don't print the info messages by default; this can be re-enabled with
the -V switch, and is implied by most debug options (it was really
kind of stupid that verbose/debug operation disabled these).
- the sync algo/state debugging can be separately enabled with -Ds now.
it helps if the code actually does what the comment above it claims.
clarify it a bit, so i don't get stupid ideas again.
This reverts commit cf6a7b4d18.
propagating many messages from a fast store (typically maildir or a
local IMAP server) to a slow asynchronous store could cause gigabytes of
data being buffered. avoid this by throttling fetches if the target
context reports memory usage above a configurable limit.
REFMAIL: 9737edb14457c71af4ed156c1be0ae59@mpcjanssen.nl
don't retry dead Stores for every Channel.
this also introduces a state for transient errors (specifically, connect
failures), but this is currently unused.
when LITERAL+ is used, the server has no chance for early rejection of
messages. this means that the client can upload megabytes for nothing.
so simply don't use LITERAL+ for big messages. of course this adds
server roundtrips, but that's tough luck.
the limit could be arguably higher than 100k (or even configurable).
i set it to ~2 sec with my fairly average DSL line.
the seznam.cz IMAP server seems very eager to send UIDNEXT responses
despite not supporting UIDPLUS. this doesn't appear to be a particularly
sensible combination, but it's valid nonetheless.
however, that means that we need to save the UIDNEXT value before we
start storing messages, lest imap_find_new_msgs() will simply overlook
them. we do that outside the driver, in an already present field - this
actually makes the main path more consistent with the journal recovery
path.
analysis by Tomas Tintera <trosos@seznam.cz>.
REFMAIL: 20141220215032.GA10115@kyvadlo.trosos.seznam.cz
this re-introduces 6741bc94 (just a bit differently), thus effectively
reverting fbfcfea5. i suppose this extra CRLF is needed by a broken
CRAM-MD5 implementation of some server, which is why it was there in the
original implementation as well. however, it breaks more pedantic
non-broken servers. if somebody complains, we'll need to add a much
more sophisticated hack.
patch initially by Jack Stone <jwjstone@fastmail.fm>,
cleaned up by Jan Synacek <jsynacek@redhat.com>,
... and then almost completely rewritten by me. ^^
RequireCRAM (another fairly stupid "use if available" option) is now
deprecated. instead, the AuthMech option can be used to give a precise
list of acceptable authentication mechanisms (which is currently "a bit"
short). in particular, this allows *not* using CRAM-MD5 even if it's
available.
the combinations of the various options made quite a mess. additionally,
'RequireSSL no' is inherently insecure - "use SSL if available" is plain
stupid.
the old options are still accepted, but will elicit a warning.
it doesn't belong there - it's a property of imap_server_conf_t.
the port setup is now done while reading the config.
this makes socket.[hc] imap-agnostic.
memcmp() is unfortunately not guaranteed to read forward byte-by-byte,
which means that the clever use as a strncmp() without the pointless
strlen()s is not permitted, and can actually misbehave with
SSE-optimized string functions.
so implement proper equals() and starts_with() functions. as a bonus,
the calls are less cryptic.
such connections don't support STARTTLS. that is reasonable, as whatever
makes the connection preauthenticated (typically a Tunnel used to launch
imapd via a shell login) must already rely on the connection's security.
consequently, we would not try to use STARTTLS with such connections.
unfortunately, we'd also skip the RequireSSL check as a side effect.
this means that a rogue server (via a MITM attack) could simply offer a
preauthenticated connection to make us not use SSL, and thus bypass
server authentication. as a result, we could send potentially sensitive
data to the attacker:
- with Patterns used, we would send a LIST command which reveals the
remote Path setting. this isn't very useful to an attacker. also, IMAP
Accounts usually rely on the server-provided NAMESPACE to start with.
- with Create enabled for the remote Store, we would upload messages
from newly appeared local folders. this isn't a very likely situation,
unless the attacker manages to convince the victim to move/copy
interesting mails to a new folder right before the attack.
- with Expunge enabled for the local Store, previously synchronized
folders would be wiped. however, this would require the attacker to
know the correct UIDVALIDITY of each remote folder, which would
require incredible luck or convincing the victim to disclose them.
the first mismatch would likely tip off the victim.
in practice, someone with the level of technical and social engineering
skills required for this attack would very likely find more attractive
attack vectors. therefore, i don't consider this a particularly serious
issue.
configurations with UseIMAPS enabled or using a secure Tunnel were not
affected to start with.
a side effect of this fix is that most users of Tunnel will now need to
explicitly set RequireSSL to false.
an alternative approach would be defaulting all SSL-related settings to
off when Tunnel is used. this would be too invasive for a patch release,
but i'll consider it for 1.2.
see also CVE-2014-2567 for the Trojita MUA.
this would happen in the absurd corner case that the response code is
properly terminated with a closing bracket, but the atom itself is an
unterminated double-quoted string.
NOT found by coverity.
the BODY[] item in the FETCH response corresponds to what we requested,
and its presence doesn't imply that it actually contains anything useful
- new messages may appear in the mailbox in addition to those we stored
ourselves, and these will obviously have no TUID.
the global timezone variable is glibc-specific.
so use timegm() instead of mktime() for the conversion.
as that is specific to the BSDs and glibc, provide a fallback.
amends 62a6099.
the warning suppression pragma within function scope is apparently a new
thing.
as i don't want to disable the check for the entire function (even if
this currently would make no difference), just use a wrapper function
to suppress the format string check.
this would happen if we were trying to find newly pushed messages, but
none actually arrived.
as imap's ranges are not ordered, this would actually fetch one message.
in fact, UIDNEXT (and UIDVALIDITY) null is *not* allowed (see RFC3501
section 9). them popping up nonetheless was a dovecot bug (which would
also confuse dovecot itself).
having it in as a workaround was no good either, as quite some other
code in mbsync assumes that UIDs are not null.
This reverts commit e1fa867 and most of 39006d7.
-REFMAIL: 4CA62BA1.4020104@lemma.co.uk
instead of SEARCHing every single message (which is slow and happens to
be unreliabe with M$ Exchange 2010), just FETCH the new messages from
the mailbox - the ones we just appended will be amongst them.
- introduce sys_error() and use it instead of perror() and
error(strerror()) in all expected error conditions
- perror() is used only for "something's really wrong with the system"
kind of errors
- file names, etc. are quoted if they are not validated yet, so e.g. an
empty string becomes immediately obvious
- improve and unify language
- add missing newlines
- asynchronous sockets using an event loop
- connect & starttls have completion callback parameters
- callbacks for notification about filled input buffer and emptied
output buffer
- unsent imap command queue
- used when
- socket output buffer is non-empty
- number of commands in flight exceeds limit
- last sent command requires round-trip
- command has a dependency on completion of previous command
- trashnc is tri-state so only a single "scout" trash APPEND/COPY is
sent at first. a possibly resulting CREATE is injected in front of
the remaining trash commands, so they can succeed (or be cancel()d
if it fails).
- queue's presence necessitates imap_cancel implementation
this prepares the code for being called from a callback.
notably, this makes the imap list parser have a "soft stack", so the
recursion can be suspended at any time.
instead of returning a write()-like result, return only a binary status
code - write errors are handled internally anyway, so user code doesn't
have to check the write length.
this makes the IMAP command submission interface asynchronous.
the functions still have synchronous return codes as well - this enables
clean error return paths. only when we invoke callbacks we resort to
refcounting.
as a "side effect", properly sequence commands after CREATE resulting
from [TRYCREATE].
synchronous error codes which are passed through callbacks aren't a
particularly good idea, after all: latest when the callback does stuff
which does not concern the caller, the return code becomes ambiguous.
instead, protect the sync_vars object with a refcount when invoking
driver functions from loops, as the callbacks they call could invalidate
the object and we would have no way of knowing that the loop should be
aborted prematurely. the upcoming async imap driver will also need a
refcount to protect the cancelation marker of the imap socket dispatcher
loop.
that way we don't have to piggy-back (possibly asynchronous) fatal
errors to particular commands.
internally, the drivers still use synchronous return values as well,
so they don't try to access the invalidated store after calling back.
just use the presence of an SSL object as an indicator. if something
goes wrong during the ssl handshake or certificate validation, the
socket must be immediately closed anyway.
don't pretend that the server has no literal+ for the time of the
first relevant command's synchronous execution. instead, enable the
lower layer to do the processing by telling it for which commands
trashnc ("trash's existence not confirmed") is relevant.
we always actually open the mailbox before appending to it, so we
obviously know that it exists - that's why the code was already
commented out. changing this assumption would significantly complicate
matters for little gain, so let's just assume it won't happen.
consequently, also don't set param.create when appending to regular
mailboxes.
- don't silently fail in release mode (expression with side effects
inside assert())
- save some redundand strlen()s by not throwing away known lengths
- reorganize the code for legibility
we'd send a LOGOUT command in plain text while the server was already
expecting an encrypted command, which would typically lead to waiting
for more data and thus an indefinite hang.
so close the socket immediately instead of letting the normal shutdown
path take care of it.
inspired by a patch by Steven Flintham.
-REFMAIL: 4C9AB98E.3000400@lemma.co.uk
this is basically a security fix for nonsensical configurations:
if the specified CertificateFile did not contain any certificates,
we *might* have accepted an arbitrary server certificate.
