Bug 249860 - mail/fetchmail: rc.conf overrides set daemon interval
Summary: mail/fetchmail: rc.conf overrides set daemon interval
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Matthias Andree
URL: https://sourceforge.net/p/fetchmail/m...
Keywords:
Depends on:
Blocks:
 
Reported: 2020-09-24 17:26 UTC by Matthias Andree
Modified: 2020-10-19 11:10 UTC (History)
2 users (show)

See Also:
chalpin: maintainer-feedback+
chalpin: maintainer-feedback+
mandree: merge-quarterly+


Attachments
Proposed patch (1.97 KB, patch)
2020-09-25 21:16 UTC, Corey Halpin
chalpin: maintainer-approval+
Details | Diff
revised patch/overhaul of fetchmail.in (10.22 KB, patch)
2020-09-26 10:48 UTC, Matthias Andree
no flags Details | Diff
Proposed patch (9.73 KB, patch)
2020-10-03 18:23 UTC, Corey Halpin
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Andree freebsd_committer 2020-09-24 17:26:09 UTC
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.
Comment 1 Corey Halpin 2020-09-25 21:16:38 UTC
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.
Comment 2 Matthias Andree freebsd_committer 2020-09-26 10:48:04 UTC
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.
Comment 3 Corey Halpin 2020-10-03 18:23:35 UTC
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.
Comment 4 commit-hook freebsd_committer 2020-10-05 19:09:27 UTC
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
Comment 5 Matthias Andree freebsd_committer 2020-10-09 23:49:26 UTC
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
------------------------------------
Comment 6 commit-hook freebsd_committer 2020-10-19 10:56:10 UTC
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