New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lmtp delivery to subfolder fails #1099
Comments
From: Alberto Sentieri I am trying to deliver emails to subfolders using lmtp and getting strange A regular delivery works fine, as could be seen on the attempt below: $ telnet 192.168.44.70 24 testing A subfolder delivery fails: $ telnet 192.168.44.70 24 testing ... The log file contains this information: File /var/log/maillog after a unsucessful delivery: Apr 6 12:05:46 localhost master[21710]: about to exec The exact information about the version I am using is: Server version information (version inside cyradm): localhost.localdomain> version |
I can confirm this with cyrus imapd 2.4.8 package on Fedora 15 (Beta at this time). My old procmail script tried to deliver the following /usr/lib/cyrus-imapd/deliver -a myaccount -m user.myaccount.ebay After debugging heavily i found that moving folders does not work anymore as before in the old cyrus version of fedora 14. Bug description: The old behaviour of cyrus was to exactly taking the -m argument and moving this into the folder. The behavour of 2.4.8 is that there is already an
----------- Patch 1 Restore behaviour of old cyrus ---------- diff -ur cyrus-imapd-2.4.8/imap/lmtpd.c ../cyrus-imapd-2.4.8.modified/imap/lmtpd.c
@@ -701,14 +705,18 @@
----- Patch 2: Helpful debug messages --------------------------- diff -ur cyrus-imapd-2.4.8/imap/append.c cyrus-imapd-2.4.8.modified/imap/append.c
@@ -171,6 +174,12 @@
@@ -180,6 +189,7 @@
|
Attachment-Id: 1380 Restores old behaviour of cyrus |
Attachment-Id: 1381 Adds Debug Messages for finding problems in CYrus |
From: Jeroen van Meeuwen (Kolab Systems) Updating the milestone to be 2.4.9, the upcoming 2.4-next release. |
Guys, there was an additional bug, i wonder if anyone has ever test the old way of cyrus by using something like /usr/lib/cyrus-imapd/deliver -a myusername -m user.myusername.ebay myusername This doesn't work with the current code, one for the reason i posted above, ---> all attempts to deliver into a subfolder fail. Therefore i cleaned up the code a little bit (i admit i couldn't understand what was oging on which is always a bad sign) and move the auth_newstate(username) to hopefully the right position. Now the check for acls do not fails --> and the move into the mailbox succeeds even with the acl check on. I tested it on my system (with and without -m pathnames) it seems to work now. Please add this asap into your next release, as i cannot go without a working cyrus (and this functionality worked for 7 years now in my old setup) Patch will be attached |
Attachment-Id: 1382 Restores old behaviour of cyrus (and fixes 2 bugs, one acl and one folderpath) |
Attachment-Id: 1383 Restores old behaviour V3.0 (fixes 2bugs) |
Attachment-Id: 1384 V3.1 of patch, further refinement, and fixed typos and missing %s |
From: Bron Gondwana I'm trying to find where it's documented this should work. My apologies for leaving this one so long - I really do want to integrate it into stable Cyrus, but I want to know that it's actually the correct interface and not some historical accident that was never supposed to work. My reading is that the correct way to deliver to a subfolder is plus addressing... |
Hi, after a quick search with Google here an english mail from 2002 which describes the behaviour http://lists.andrew.cmu.edu/pipermail/info-cyrus/2002-September/018660.html From 2007 (sorry german, but there actually a lot of German FAQ documenting this behavior) so i believe this makes the case that it is today an expected behaviour - and people who start to switch from old cyrus 1.4* to 2.* releases will be very disappointed. (Mit Bezug zu comment #9) |
From: Adam Tauno Williams I believe the correct behavior is the use of plussed addressing. We've been using Cyrus for a decade or more and recall that as having always been the documented mechanism for subfolder delivery. |
Please explain "plussed addressing" But please provide also evidence for this. Here another bunch of URLs which document the behaviour i want to restore: http://www.irbs.net/internet/info-cyrus/0504/0167.html (Mit Bezug zu comment #11) |
From: Bron Gondwana (In reply to comment #12) RCPT TO: <username+subfolder> > But please provide also evidence for this. Here another bunch of URLs which They all look like documentation of behaviour of the "deliver" command line tool, I will look and see what I can find on the matter. I guess we could go with an "if the user is pre-authenticated admin then allow a direct mailbox name", which is probably what it used to do. By the way, the example here was "rcpt to abcd.test" - the examples in the links you have given seem to be "rcpt to user.abcd.test". I would be more willing to accept the second format than the first. The first is an unholy abomination. Bron. |
i am absolutly fine with "user.myusername.subfolder" - this is the way i want it. but the implementation (which didnt work ) currently internally prepares "user.myusername" already as an prefix and would - if it would have worked - only accept "subfolder". My patch actually just does. > |
i am not exactly sure what you mean by pre-authenticated admin. The usual usage is to call "procmail" instead of "deliver" from sendmail or postfix (i used both mtas). Remember that sendmail and postfix run with the userid "mail" or "postfix" on a fedora system. With this user deliver gets called nowadays. Btw the patch 3.1 runs now since one month witouth troubles on my system. > "if the user is pre-authenticated admin then allow a direct mailbox name", |
From: Bron Gondwana Wow, just trying to read the patch again. Please, please, PLEASE if you need to do code cleanup first, make it two separate patches. It's really hard figuring out what actually changed in amongst a bunch of meaningless code layout changes. Even better, get yourself a copy of the git repository and make a series of commits in on a branch somewhere that I can pull :) |
;-)
(Mit Bezug zu comment #16) |
From: Bron Gondwana No, you made a memory leak :( I'm cleaning it up to match the coding standards. The general concept of the cleanup was good, but large rewrites that don't fit with the rest of the codebase aren't going to win you confidence that it's a safe thing to apply to a stable release. As for git - you can get it pretty easily, and install it as an ordinary user. It really is worth it. |
From: Bron Gondwana Yeah, OK - this will go in 2.4.9. Thanks for following up on it. |
From: Bron Gondwana The patch broke Sieve deliveries, so I have reverted it on both branches. Here's what deliver actually does: /* just deliver to mailbox 'mailbox' */ so in other words what gets send to the lmtpd is: RCPT TO: <postuser+user.brong.SubFolder> (where postuser is configurable: but most people just put 'anyone p' as an ACL or us a pre-authed admin instance of lmtpd, so it doesn't really matter) NOTE: I'm not certain how all this interacts with altnamespace and unixheirarchysep - it could get pretty ugly. I'm more certain that your solution is not the correct one though. |
From: Bron Gondwana Ok - I'm marking this whole kit and kaboodle invalid. Maybe it used to work because of a bug in altnamespace and unixheirarchysep. It might be easier for you to turn those two off for your local lmtp daemon, lime this: imapd.conf: cyrus.conf: (or whatever) Then deliver -m user.foo.subdir will work (I just tested it on master and stable). You will need either '-a' in cyrus.conf, or "anyone p" ACL on the mailbox. If you leave unixheirarchysep on, you may need to deliver to user/foo/subdir instead. I have also tested that this works. Under the hood, if you don't set a postuser, the network traffic will be: RCPT TO: <+user.foo.subdir> Note the leading plus. If you do set a postuser, it will be RCPT TO: <postuser+user.foo.subdir> Regards, Bron. |
sorry for causing a memory leak. However i'd like to know if it only because of the memory leak it breaks (because this i can fix) Why is the patch invalid? Please provide a test case which proves failure with sieve. I am not sure, but i spotted way too much bugs when i did go I request to reopen is and wait for a memory leak fixed patch. Or a test case which proves that it breaks sieve. But please do not hide bugs by closing it. |
From: Bron Gondwana It's invalid because you're wrong. It's not correct to deliver via LMTP to recipient user.foo.subdir, and it never was - even if somewhere on the net said so, and it perhaps used to work. Which is why deliver sends to either: RCPT TO: <+user.foo.subdir> or if you specify a "postuser", to: RCPT TO: <postuser+user.foo.subdir> when you specify the '-m mailbox' option. As for the sieve case, I can try to recreate a test case, but I can promise you that I rolled this code out on FastMail's production systems and had literally thousands of "could not find folder INBOX.name" errors, for mailboxes that clearly existed. When I checked the sieve scripts, they had rules of the form: fileinto INBOX.name; I rebuilt our packages with the one patch reverted, and the problem went away. I do appreciate that you've put a lot of work into this, but I have spent a lot of time testing this, and it's not a bug. It's supposed to work like this. The target of a "rcpt to" is a username, not a mailbox spec. It might contain a "detail" part, see rfc5233 for description of plus addressing and "detail" - and the detail is interpreted as being a mailbox name in the user's namespace if sieve doesn't do anything else with it. But RCPT TO: <foo.subdir> is clearly wrong, as could be clearly seen in the I have fixed a bunch of actual bugs relating to auth and delivery with the RCPT TO: <+user/foo/subdir@domain> which is pretty strange looking! Bron. |
let me be clear - i do not want to defend my code endlessly - if the facts but at this point in time, i only see you claiming that there is a faulty you are telling sieve rule doesn't work. (2) I also have used a sieve script and tried to reproduce your problem with filtering to inbox and "inbox.ebay". This worked correctly in my environment(*). Therefore i fail to reproduce your problem. I have added now some more debug messages and would be happy to get a test case which reproduces your problem. I do not argue that you don't have a problem. But it seems that we have not a common understanding of the real reason. (*) my environment has 5% more debug messages, which i didn't publish :-) |
From: Bron Gondwana (In reply to comment #24) Well, I reverted just this patch and the problem went away again. That's > you are telling sieve rule doesn't work. It fiddles with the mailbox name inside "deliver_local", which path is used > (2) I also have used a sieve script and tried to reproduce your problem with Interesting. I'll try to make a complete testcase now, but I sure did Can you please give me the values of "altnamespace" and "unixheirarchysep" > Therefore i fail to reproduce your problem. I have added now some more debug I'll see what I can do! > I do not argue that you don't have a problem. But it seems that we have not a Sure. I'd like to get a deeper understanding too. Will do. > (*) my environment has 5% more debug messages, which i didn't publish :-) |
:-) regarding your content you seem to believe that i have added the changing of the mailbox name. let me be clear. I have appended the original modification from 2.4.8 source code. I have added line numbers to the patch, i have made originally. "-" indicates removed code from 2.4.8 and + inidicates added code. Important Info: From my debugging i know that in namebuf, already contains the string "user.myusername" when reaching this part of the code. Line 20,30 are "original 2.4.8" code, they show up again in in line 70,80. So the only modification i claim to be mine is line 40,50,60,90, two of the only contain brackets - so leaves me with 40,50. i currently cannnot access my machiine it would be nice if you would addd on debugging statement at line 15, which would print out the content of namebuf and the content of mailboxname Therefore i am puzzled how i would have to crash the statement in line 40,or 50. 10 if (mailboxname) { |
From: Bron Gondwana Sorry - I can't recreate my crash, but I want to show you this: 'man deliver'
See this: ff any userids are specified, attempts to deliver to user.userid.mailbox for each userid. This documentation dates from the stoneage some time. git blame man/deliver.8 ff90aff (John Gardiner Myers 1994-06-28 21:28:50 +0000 116) .BI -m " mailbox" This is really, REALLY clear that the current behaviour is intentional. You have two clear correct ways to do this: a) use a pre-authenticated connection without specifying userid and give a full mailbox path. b) use a userid and give the bit after user.$username only. If you're sending via LMTP then you can either specify: a) RCPT TO: <username+subdir> As per the RFC on subaddressing. And I'm closing this as invalid again. If you old procmail worked, it was due to a bug, and this behaviour is correct as per the documentation. |
so lets summarize the facts (1) you did fail to recreate the behaviourr At this pointed i am disappointed - i will not fight ignorance, which is probably coming from the will to make a release. I will keep a patched version, please proceed as you want. (Mit Bezug zu comment #27) |
From: Bron Gondwana (In reply to comment #28) I take back my claim that your patch is responsible for the crashes that we saw at FastMail, since I am unable to reproduce them with just your patch in a standalone environment. I strongly suspect it was actually my porting of those patches on top of master interacting badly with our environment. > (2) you are bringing up the documentation argument I have also been unable to find a single Cyrus release in which the behaviour was EVER what you want here. Cyrus 2.2 does not appear to have (in the released version anyway) done mailbox expansion at all when using an authenticated user, and Cyrus 2.3.x all did exactly what Cyrus 2.4.x does - append the mailbox name to the user's INBOX name if you pass a username to deliver. This is also what the documentation says it does. > At this pointed i am disappointed - i will not fight ignorance, which is No - I had already decided that this would be dropped from the release anyway, and I can happily release without it regardless. It's not ignorance, it's disagreeing with your assertion that this was ever a bug. Webpages published by a third party suggesting something that's not how it ever worked are not evidence to the contrary. > I will keep a patched version, please proceed as you want. You're quite welcome to apply whatever patches you like. We apply a small subset of patches at FastMail which provide functionality we want, but which are not appropriate for upstream. The reason I'm annoyed here is that this was pushed as a bugfix, when it's actually a new feature that clashes with the documented behaviour. And I spent a lot of time on it because I believed it was a legitimate bug, when actually it was just a failure to understand how mailboxes are addressed via LMTP with Cyrus. |
From: Alberto Sentieri
Bugzilla-Id: 3163
Version: 2.4.8
Owner: Bron Gondwana
The text was updated successfully, but these errors were encountered: