cyrusimap / cyrus-imapd

Cyrus IMAP is an email, contacts and calendar server
http://cyrusimap.org
Other
544 stars 149 forks source link

lmtpd autocreate inbox fails on murder frontend #1983

Open elliefm opened 7 years ago

elliefm commented 7 years ago

I'm still struggling to work out whether this is a problem with Cassandane's murder setup or a real bug, but so far it's looking more and more like a real bug.

Cassandane is set up such that the frontend server is also the mupdate master. The docs say this is a legit configuration, but I suspect this is part of the issue.

The call chain gets down to autocreate_user() in imap/autocreate.c, which then calls down to mboxlist_createmailbox() to do the work. It does so with 0 for mbtype, so (eventually) mboxlist_createmailbox_full() creates it locally (since it's not MBTYPE_REMOTE).

So -- I've got a patch on my branch that fixes that up, and lets autocreate_user create a remote mailbox when lmtpd has isproxy=1. Cool.

But then, in mboxlist_createmailbox_full(), it only uses the isremote flag to determine whether or not to do a mailbox_create for the local file system; thereafter it doesn't care. It doesn't set newmbentry->server before calling mboxlist_update_entry().

It then goes to try to update mupdate and,

The mupdate daemon winds up in cmd_set(). It looks up its mailboxes.db and finds the entry (since frontend and mupdate master are the same server). The entry has a blank "server" field so it considers it to be local (even though it's a frontend with no mailboxes). The destination server is the frontend (because of the frontend's config_servername being built into the location string), so it considers it to already exist.

The whole creation then fails because it "already exists" -- the mupdate operations fail, so the whole thing is backed out.

elliefm commented 7 years ago

Looking at imapd's cmd_create, it uses the serverlist setting to choose a destination for remote creations.

I think mboxlist_createmailbox_full() needs to also do this when isremote == 1, use that server to build the location string rather than config_servername, and set it properly in the mbentry it creates.

elliefm commented 7 years ago

The mboxlist_createmailbox() family look like they're meant to be able to do local or remote creations, but cmd_create() in imapd.c treats it as if it's local-only and then does a lot of work to find a suitable backend and then proxies a CREATE to it. This is a mess.

nlindq-maei commented 2 years ago

The autocreate code is still broken in 3.4.4 when dealing with a murder. In my case it's "autocreate on login" which is misbehaving, but I expect the "on post" lmtpd autocreate functionality is the same.

The mailbox is (incorrectly) created locally on the frontend server, and the mupdate server is updated accordingly. Subsequent attempts to access the mailbox fail because the frontend server isn't configured to allow proxyservers users access.

Has any attempt been made to address autocreate functionality in a murder configuration?

nlindq-maei commented 2 years ago

FWIW, I tested with lmtp too, and it behaves similarly; whichever frontend processed the lmtp request auto-creates the mailbox locally, then the lmtp process crashes when the message append is attempted.

Jul 5 14:22:45 edm-cmfe01 cyrus/lmtpunix[18624]: autocreateinbox: User autotest@example.ca, INBOX was successfully created Jul 5 14:22:45 edm-cmfe01 cyrus/lmtpunix[18624]: skiplist: recovered /var/lib/imap/domain/a/example.ca/user/a/autotest.conversations (0 records, 144 bytes) in 0 seconds Jul 5 14:22:45 edm-cmfe01 cyrus/lmtpunix[18624]: skiplist: checkpointed /var/lib/imap/domain/a/example.ca/user/a/autotest.conversations (0 records, 144 bytes) in 0.000 sec Jul 5 14:22:45 edm-cmfe01 cyrus/lmtpunix[18624]: FATAL: Internal error: assertion failed: imap/append.c: 897: stage != NULL && stage->parts.count Jul 5 14:22:45 edm-cmfe01 cyrus/master[18620]: process type:SERVICE name:lmtpunix path:/usr/libexec/cyrus-imapd/lmtpproxyd age:258.638s pid:18624 exited, status 70 Jul 5 14:22:45 edm-cmfe01 sendmail[18734]: 265JwvwX017816: to=autotest@example.ca, delay=00:23:25, xdelay=00:00:00, mailer=cyrusv3d, pri=570057, relay=localhost, dsn=4.3.0, stat=Deferred: 421 4.3.0 lmtpd: Internal error: assertion failed: imap/append.c: 897: stage != NULL && stage->parts.count

elliefm commented 2 years ago

I stopped chasing this because we (Fastmail) don't use murder or autocreate ourselves, so we don't have good experience or insight as to how to make it work in a way that doesn't just break some other thing without us noticing.

You might be able to work around it by only enabling the autocreate imapd.conf settings on your backends, but not your frontends? Though I don't know, I'm guessing. If this works, that would be useful to know, but it might not work.

I mentioned in my original report that I had a "patch on my branch" that partially fixes things... but I no longer know which branch I was talking about, and looking through my branch names, I don't see anything that looks likely. Since it was 2017, it would have been developed on my previous laptop. I'm sure I would have pushed it up to my fork at the time, but since I wouldn't have had a local copy anymore once I moved to my current laptop, maybe the remote copy got pruned... :(

Are you a long-time murder user who is trying to enable autocreate? Or a long-time autocreate user who is trying to switch to a murder deployment? Or something else?

If you have the resources to fix this yourself, I'd be very happy to help shepherd through a pull request. Everything I know about the issue and possible solutions is already written in my earlier comments, though it's a pity I lost that patch!

nlindq-maei commented 2 years ago

I tried your suggestion (enabling autocreate on the backends but not the frontends) but unfortunately it didn't work. Authentication (at the frontend) just fails instead of triggering an autocreate event.

I'm a long-time autocreate user trying to switch to murder rather than the other way round.

I'm not really a coder, but I did muddle my way through the source a bit. Please correct my interpretation! :-) It appears that all of the logic surrounding murder server/partition selection is only present in cmd_create() in imapd.c, which in turn is only called from cmdloop() which seems to be the command parsing engine handling interactions with clients connected via one of the enabled imapd protocols. Once cmd_create() is called from a backend (or non-murder server) and the localcreate: section executes, mboxlist_createmailbox() is called with parameters determined earlier.

The autocreate_ code independently calls mboxlist_createmailbox() with static parameters.

So, do you think it would be better to have the autocreate_ functions call cmd_create(), or to replicate the server/partition discovery logic in autocreate_user()? (or something else entirely?)

elliefm commented 2 years ago

So, do you think it would be better to have the autocreate_ functions call cmd_create(), or to replicate the server/partition discovery logic in autocreate_user()? (or something else entirely?)

None of these options are great, unfortunately, but let's call them 1, 2, and 3:

  1. The autocreate_ functions can't call cmd_create(), because it is static within imapd.c (and therefore functions in autocreate.c don't know it exists and cannot call it). cmd_create could hypothetically be promoted, but it would still only exist within the imapd service. The lmtpd and pop3d services also need the autocreate functionality, but won't be able to call into imapd's cmd_create. By the time all those issues were solved, you'd have wound up implementing 3 instead
  2. If the server/partition discovery logic is duplicated into autocreate_user() (or similar), there's a high chance it will fall out of sync with the original copy, because the primary maintainers of cyrus-imapd don't use either murder or autocreate, and are unlikely to notice breakages that only affect the interaction between those features, unless there are also extensive regression tests in Cassandane, which is a another implementation burden -- how's your perl? ;)
  3. Something else entirely: probably the best long-term approach would be to move the server/partition discovery logic out of cmd_create() and into some new API in libcyrus, which cmd_create() and autocreate and anything else can call into. Off the top of my head, I don't have any specific design ideas of what this function should look like or where it should live. Designing a new API might be an intimidating task if you're "not really a coder"... though on the other hand, it's a great learning experience, if you're motivated to grow in this direction.

I think we can rule out 1 as just unworkable, which leaves 2 and 3.

2 would be your easiest option if you wanted to build and maintain this yourself (as a local patch that you re-apply each time you build a new version), but we might not accept a patch like this into upstream

3 is the option we'd be most likely to eventually merge into upstream and take some ownership of, saving you the burden of long term maintenance. But for someone new to Cyrus development it's probably the harder path

I guess an option 4 is to stop using Cyrus's autocreate feature. Presumably you already have tools and/or procedures for setting up new accounts in your auth system and adding email addresses to your mx server(s). You could extend these tools or procedures to also provision Cyrus mailboxes at the same time, perhaps by making the required IMAP calls directly, or by using a wrapper such as cyradm. If I were doing this myself, I'd probably look at what autocreate does (i.e. which mailboxes it creates, which acl's it sets, etc), and start by replicating that behaviour, but you could also make it do whatever else you wanted to.