Corey, https://sourceforge.net/p/fetchmail/mailman/message/37112612/ reported a non-obvious issue that our rc.d/fetchmail file essentially overrides a daemon interval from the fetchmail configuration. I have some ideas, but not thought this through, how to attack this. - we might use fetchmail --dump and grep the Python output for the interval, and give that precedence if set and other than rc.conf - we might use syntax such as ${fetchmail_polling_interval+-d} to avoid passing the -d parameter altogether, and remove the default (attention, can be astonishing) There may be other attack vectors vs. this bug.
Created attachment 218297 [details] Proposed patch I like your first suggestion. The attached patch implements it. With the second suggestion, I'm not following how that would work in the case where a user doesn't have a 'set daemon' in their config file.
Created attachment 218317 [details] revised patch/overhaul of fetchmail.in I've added some documentation to the comments of files/fetchmail.in and while writing that, I've figured that we have a similar situation for the logging facility (--syslog vs. --logfile) where we need to make a similar change. I've also found that the root/$fetchmail_user split can skew our results because a fetchmail --configdump run as root from the script may refuse output with this error message (I am running the script as root and fetchmailrc is owned by fetchmail): File /usr/local/etc/fetchmailrc must be owned by you. Consequence: we need to add su -m $user -c sh to our fetchmail --configdump runs. Then, the script does not change the "name" variable to fetchmail_$user but isn't written that way, so we manually need to pull up the typical rc.subr variables, for instance, ${name}_env from ${name}_${user}_env. Added. I've pondered the fetchmail_home_prefix, too - we can safely query the actual home directory (rather than second-guess and make something up) with getent passwd $fetchmail_user | cut -f6 -d: Added. Also, the global run will lose the user name because the user variable is unset, and I've replaced it with $name, but not tested the output cosmetically. In case passwords are missing, the rcfile will stall the boot interactively, so I've added exec </dev/null. If it is acceptable to you, this may need to be added to UPDATING, too, so that people know to put the passwords into .netrc or their config files. I've found that we clobber existing fetchmail_flags in rc.conf, so we'd negate effects of a fetchmail_flags=-v, for instance. Finally, the script isn't exactly logical to read and the two huge outer if/else blocks are both reversed from the natural reading direction where I'd like to get the general idea first and then the nitty-gritty details, and I question that we really need to recurse the script into itself when fetchmail_users is blank and it does something like $fetchmail_script start root GLOBALCONFIG. Haven't thought this through... Essentially I've run out of time for now but would like to propose my current working state for your review and comments.
Created attachment 218495 [details] Proposed patch Thank you for the numerous fixes! Stalling the boot when passwords are missing is definitely not a good failure mode. I entirely agree with adding `exec </dev/null` and a note in UPDATING. In the attached, I've worked to simplify the script and to resolve the illogical ordering. * I moved the --configdump call into a function to make the `if`s that use it easier to read * Since the 'umbrella run' always ends with an `exit`, the recursive portion of the script doesn't actually need to be buried an `else` block. * As the `$success` variable didn't seem to actually be used anywhere, I removed it. * In the previous version, after the `for user in ${fetchmail_users}` that was on line 120, the script unconditionally did `exit 0` on line 130 regardless of the exit status of the per-user calls; I've revised the `exit` to check the presence of `$failures` such that it will exit 1 if any of the per-user calls failed.
A commit references this bug: Author: mandree Date: Mon Oct 5 19:09:19 UTC 2020 New revision: 551537 URL: https://svnweb.freebsd.org/changeset/ports/551537 Log: mail/fetchmail: avoid rc.conf overriding daemon interval, many other fixes Authors: CH = Corey Halpin, MA = Matthias Andree - fetchmail's rc script now queries the daemon interval from the configuration, and falls back to the rc.conf value if given. [CH] - Similarly, the logging facility will be taken from the configuration [MA] - Add documentation to the rcfile's header comments. [MA] - Drop support for fetchmail_home_prefix in rc.conf, and query the respective users' home directories with getent instead. [MA] - In the rc scripts, redirect input from /dev/null so it will not ask for passwords. [MA] - Add support for the typical 12.1 rc.conf ${name}_... keywords. [MA] - Make script execution easier to follow by simplifying if...else logic. [CH] - Fix rcscript's exit code to be 1 if one of the per-user calls fails. [CH] - Add relevant notes to UPDATING. [MA] PR: 249860 Submitted by: Corey Halpin (maintainer) Reported by: Chris James (on fetchmail-users mailing list) Approved by: Corey Halpin (maintainer) Changes: head/UPDATING head/mail/fetchmail/Makefile head/mail/fetchmail/files/fetchmail.in head/mail/fetchmailconf/Makefile
FTR, r551607 fixes a regression introduced by this commit. https://svnweb.freebsd.org/changeset/ports/551607 ------------------------------------ mail/fetchmail: fix rcscript regression from _1 that broke global mode In a situation where fetchmail is to be started globally with the configuration in $LOCALBASE/etc, the rc.d file would try to run fetchmail for the wrong user. Simplify script more, avoiding recursive call in single-user mode. Submitted by: Corey Halpin (maintainer, direct mail to mandree@) Reported by: Armin Tuting ------------------------------------
A commit references this bug: Author: mandree Date: Mon Oct 19 10:55:10 UTC 2020 New revision: 552730 URL: https://svnweb.freebsd.org/changeset/ports/552730 Log: MFH: r551537 r551607 mail/fetchmail: avoid rc.conf overriding daemon interval, many other fixes Authors: CH = Corey Halpin, MA = Matthias Andree - fetchmail's rc script now queries the daemon interval from the configuration, and falls back to the rc.conf value if given. [CH] - Similarly, the logging facility will be taken from the configuration [MA] - Add documentation to the rcfile's header comments. [MA] - Drop support for fetchmail_home_prefix in rc.conf, and query the respective users' home directories with getent instead. [MA] - In the rc scripts, redirect input from /dev/null so it will not ask for passwords. [MA] - Add support for the typical 12.1 rc.conf ${name}_... keywords. [MA] - Make script execution easier to follow by simplifying if...else logic. [CH] - Fix rcscript's exit code to be 1 if one of the per-user calls fails. [CH] - Add relevant notes to UPDATING. [MA] PR: 249860 Submitted by: Corey Halpin (maintainer) Reported by: Chris James (on fetchmail-users mailing list) Approved by: Corey Halpin (maintainer) mail/fetchmail: fix rcscript regression from _1 that broke global mode In a situation where fetchmail is to be started globally with the configuration in $LOCALBASE/etc, the rc.d file would try to run fetchmail for the wrong user. Simplify script more, avoiding recursive call in single-user mode. Submitted by: Corey Halpin (maintainer, direct mail to mandree@) Reported by: Armin T?ting Approved by: Corey Halpin (maintainer on MFH, direct mail to mandree@) Approved by: ports-secteam@ (fluffy@) Changes: _U branches/2020Q4/ branches/2020Q4/UPDATING branches/2020Q4/mail/fetchmail/Makefile branches/2020Q4/mail/fetchmail/files/fetchmail.in branches/2020Q4/mail/fetchmailconf/Makefile