Bug 246943 - [patch] calendar(1): Do not send reports for uids < 1000, except root
Summary: [patch] calendar(1): Do not send reports for uids < 1000, except root
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Josh Paetzel
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-02 18:37 UTC by wcarson.bugzilla
Modified: 2020-06-16 21:16 UTC (History)
5 users (show)

See Also:


Attachments
Patch to calendar(1) to skip reserved UIDs (436 bytes, patch)
2020-06-02 18:37 UTC, wcarson.bugzilla
no flags Details | Diff
check for valid shell (2.12 KB, text/plain)
2020-06-08 21:13 UTC, Warner Losh
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description wcarson.bugzilla 2020-06-02 18:37:58 UTC
Created attachment 215176 [details]
Patch to calendar(1) to skip reserved UIDs

Overview:

    When using calendar(1) with periodic and a config for the root user, multiple reports will be sent due to system user accounts using /root as their homedir (e.g. toor & daemon). 

Steps to Reproduce: 

    0) Prerequisite: working local mail delivery

    1) Create calendar(1) config as the root user

        # mkdir ~root/.calendar
        # echo '#include <calendar.all>' > ~root/.calendar/calendar

    2) Enable calendar(1) periodic script

        # sysrc -f /etc/periodic.conf daily_calendar_enable="YES"

    3) Wait for daily periodic mail

Actual Results:

    Receive 3 e-mails from the reminder service (one for root, one for toor, and one for daemon).

Expected Results: 

    Should only receive 1 reminder e-mail, for root.

Build Date & Hardware: 

    FreeBSD 13.0-CURRENT #5 r360989: Sun May 17 06:56:59 UTC 2020

Additional Information:

    This patch makes it so the `-a` argument to calendar(1) will exclude UIDs less than 1000 (with an exception for the root user only).
Comment 1 Rodney W. Grimes freebsd_committer 2020-06-02 20:46:48 UTC
This makes a poor hard coded assumption that uid's 1 to 999 are special, and that is not always the case, just look at freefall, uid's start at 500, I am uid 501.

It makes further assumptions that user toor and user root are infact desired to be one and the same.  /etc/aliases is the reason that mail for toor went to root and if someone modified that this again would be a hard coded invisible place that the desired results may not occur.

Based on those 2 facts I reject this patch as submitted.
Comment 2 wcarson.bugzilla 2020-06-02 21:06:34 UTC
(In reply to Rodney W. Grimes from comment #1)

I agree with your first point and am happy to consider an alternative to a hardcoded value. Is there a system defined value I can reference instead?

Regarding your second point, I'm not sure. The way calendar(1) currently works is by looping through each entry in /etc/passwd and looking for a .calendar/ directory in the user's homedir. The problem comes from the default configuration of the toor & daemon users having their homedir set to /root. Changing their homedirs to /nonexistent like other system/reserved UIDs is maybe a viable alternative (not sure of any impact, particularly with the daemon user), but the mail alias seems completely irrelevant to me.

How about this as a workaround/fix -- comparing the owner of the directory to pw->pw_name? It eliminates both of the points you raise because it wouldn't need to look at the UID at all. Thoughts?
Comment 3 wcarson.bugzilla 2020-06-02 21:24:01 UTC
(In reply to wcarson.bugzilla from comment #2)

Well actually, now that I've suggested it, I still don't know how to solve the root/toor problem without hardcoding a comparison. Would that be acceptable? Or perhaps you have a better idea?
Comment 4 Rodney W. Grimes freebsd_committer 2020-06-03 05:00:20 UTC
Stop putting a .calendar file in /root would be my first suggestion, sounds like your using root as a regular user and that should not be done.  Even a die hard
like me that has been logging in as root for 40 years does not do things like
create /root/.calendar to get reminder notices, that goes in ~rgrimes/ or some
other place.

You *could* keep a list of the paths to .calendar files that have been
processed, and only send to those that you have not put on the list yet, then add them to the list.

As far as I can see this is not a bug in calendar, it is a system administration/configuration issue.
Comment 5 Warner Losh freebsd_committer 2020-06-08 20:54:04 UTC
I think this is a good bug and we should fix it, though I have a slightly different suggestion as to how.

By default we ship with daemon with a home directory of "/root" as the home directory. This makes them special. It's a FreeBSD only thing, as far as I can tell, but it's our defaults and we shouldn't preclude root from having a .calendar file. That's an impedance mismatch as it imposes a policy rule on root when we try not to do that without a good reason.

Rather than hard coding the UID range, though, would it be a better idea to only send to users whose shell is in /etc/shells? This is the universal signal to do something special with this account, and it seems  that if you don't have an interactive shell, you don't want mail from calendar. This would also suppress it for people that can't otherwise login.

Anyway, the patch should be simple to tweak to do this given getusershell().
Comment 6 Warner Losh freebsd_committer 2020-06-08 20:57:20 UTC
btw, requiring a non-null shell in addition to it being in /etc/shells would solve the toor problem as well...
Comment 7 Warner Losh freebsd_committer 2020-06-08 21:13:04 UTC
Created attachment 215374 [details]
check for valid shell

Here's the alternative I'm suggesting, including man page fix.
Comment 8 Warner Losh freebsd_committer 2020-06-08 21:13:45 UTC
I've uploaded a patch so the OP can see if my theory that this will work for him is true or not.
Comment 9 wcarson.bugzilla 2020-06-08 22:14:42 UTC
Thanks Warner, I've applied your patch and it should run in a few hours -- I'll update the thread once it does. Am I understanding correctly that if you've set a shell for the toor user, that you'd run into the same problem? (I only care because I used to run that configuration quite often, setting a shell in /usr/local/ for the toor user and one in /bin for root for emergencies). 

What if we combined your patch with Rodney's idea to keep track of paths that have been processed already? Or instead, we could combine your patch and tracking of UIDs, which would avoid dealing with/comparing any strings?
Comment 10 wcarson.bugzilla 2020-06-09 14:26:37 UTC
I was getting segmentation faults, so I changed some things around based on examples I found in usr.bin/chpass/util.c. It runs as expected now, and it will run twice if the toor user has a shell. I'll see if I can find some examples of keeping a hash of already-processed UIDs I can steal, since I'm not sure I'm clever enough for that.

Also, I think perhaps a new bug is in order for the getusershell(3) man page, as it seems to suggest the first call should be (void)getusershell(), but that apparently is not the case. The first call should be setusershell(), which the manpage says merely rewinds the pointer.



--- calendar.c	2020-06-09 14:19:11.301156000 +0000
+++ calendar.c.new	2020-06-09 14:18:58.982708000 +0000
@@ -215,7 +215,6 @@
 	}

 	if (doall) {
-		(void)getusershell();
 		while ((pw = getpwent()) != NULL) {
 			pid_t pid;

@@ -237,7 +236,6 @@
 				exit(0);
 			}
 		}
-		endusershell();
 	} else {
 #ifdef WITH_ICONV
 		/* Save the information about the encoding used in the terminal. */
@@ -253,12 +251,15 @@
 static bool
 bad_user_shell(const char *sh)
 {
-	const char *p;
-
-	while ((sh = getusershell()) != NULL) {
-		if (strcmp(sh, p) == 0)
-			return true;
+	char *p;
+	setusershell();
+	while ((p = getusershell()) != NULL) {
+		if (strcmp(sh, p) == 0) {
+			endusershell();
+			return false;
+		}
 	}
+	endusershell();
 	return true;
 }
Comment 11 Greg Lehey freebsd_committer 2020-06-11 05:18:37 UTC
I agree that the limit to uid < 1000 is wrong, but I'm not sure that
the current approach is right.  How about a situation where there's a
calendar service with its own user id and no other purpose than to
send calendar messages?  Arguably it wouldn't need a shell.  Or how
about it having a "shell" specifically designed to filter and forward
the calendar contents?

The real issue here is that there are multiple entries in /etc/passwd
with the same home directory.  Isn't there an alternative?  Keep track
of which directories have been processed?
Comment 12 Warner Losh freebsd_committer 2020-06-11 16:47:14 UTC
(In reply to Greg Lehey from comment #11)

Yea, duplicate emails is the real problem. However, when you have multiple folks with the same home directory, which one should win? Eg, how do you know that it's duplicate email?

You're right that valid shell is an imperfect substitute for is the a legit account to process. And adding a shell for your hypothetical case to shells has other issues.

Thinking through things, I think only do a path once, though harder to implement, might be less imperfect. Then you have 'which user wins' question. First win is fine for in-order processing of /etc/passwd. Not so good for YP or LDAP situations.

Another possibility is a new flag, but that seems even worse than either shell or path filtering.
Comment 13 Greg Lehey freebsd_committer 2020-06-12 00:34:54 UTC
This problem currently occurs only for the three users sharing /root as the home directory.  By default root occurs as the first entry in /etc/passwd, and we're assuming that this is the user that should get the mail.  But is that the case?  Yes, you could rearrange the entries in /etc/passwd, but that would be tacky.

Taking a step back, there are a number of ways of looking at this problem.  So far we have assumed that duplicate emails is wrong.  But is it?  Again it might be possible that it would be correct, for example, exactly in the situation that I described where there's a separate service to filter calendar contents.  Any of the solutions we're discussing would cause this service to fail.

The idea of a flag is not as bad as it sounds, except that it would be hidden inside the nightly cron jobs.  It might make more sense to add a knob in the calendar file itself to indicate who should get the output, something like

  Mailto=root,toor,grog

As that example shows, that would also give the option of sending to others than the users sharing the home directory.

In passing, toor exists to be an alternate root with a different shell.  Yes, by default it doesn't have a shell, but if people use it, then they could well set a shell for it, which would break the fix currently under discussion.
Comment 14 Rodney W. Grimes freebsd_committer 2020-06-12 02:46:09 UTC
(In reply to Greg Lehey from comment #13)
I continue to assert that this is not a bug.  The program is doing what it has been told to do, send email to all users that share that calendar file location, and that is root, toor and daemon.

Any administrator can and may alter these home directories and or /etc/aliases entries and anything one might do in calendar to address this reported problem very likely would cause probems with that.

Finally, this has been this way for a very long time and no one has complained or reported it which means it is very likely a none problem.
Comment 15 Greg Lehey freebsd_committer 2020-06-15 04:57:00 UTC
I've done a lot of thinking about this issue, and I tend to agree with rgrimes.  Arguably the -a option should be removed.  The big question is: what use is it?  It assumes that everybody on a system wants this feature, and that they don't have any special parameters.  I've been sending myself my calendar by mail with a cron job for decades now, and I have specific options that calendar -a couldn't address.

Clearly we're not going to remove it, but we should address POLA.  How about this in the man page?

     -a      Process the ``calendar'' files for users found in /etc/passwd and
             mail the results to them.  This can result in multiple messages
             for specific files, since /etc/passwd does not require home
             directories to be unique.  In particular, by default root, toor
             and daemon share the same home directory.  If it contains calen-
             dar information, calendar will process the file three times.
             This requires super-user privileges.
Comment 16 Rodney W. Grimes freebsd_committer 2020-06-15 12:38:15 UTC
(In reply to Greg Lehey from comment #15)
Documenting the behavior would be good.  Also one can not that most "modernish" email clients have the ability to de-duplicate mail, which is probably the better place to solve this particular corner case.
Comment 17 Warner Losh freebsd_committer 2020-06-15 16:01:32 UTC
After noodling around with this for a few hours, I find all the proposed ways to cope hacks in different ways. The shell is almost right. The path filtering is almost right, the UID thing is almost right. But each misses the mark in different ways, or introduces too much complexity to the program. I thought this would be easy and simple, but the deeper I've gone into this, I wonder if the right solution is to have an alias for the 'duplicate' accounts in /etc/aliases. That seems the least wrong approach.

For root/toor, the toor->root alias is historical.. It seems least problematic to alter that on the installed systems.

I've come around to Rod's thinking this isn't a bug in calendar, per se, but the confluence of several other factors leading to undesirable behavior.
Comment 18 Greg Lehey freebsd_committer 2020-06-16 05:57:17 UTC
(In reply to Warner Losh from comment #17)

Arguably the shell thing is the least "almost right".  If anybody actually uses toor, it's for an alternative shell, and so they will have to add it with chsh to use it at all.  Bang goes the check for a valid shell.

And a "duplicate account" mail?  I suppose it could work, but somehow it seems to be too much effort for something that shouldn't need fixing.  The real issue that I see is that there's a simpler way to solve the problem: put it in a crontab and forget the calendar -a altogether.  I wonder how many people use it.

Currently it seems that rgrimes and I are for leaving things as-is, and imp still wants to fix things.  I'd suggest two options:

1.  Take the majority view, commit the man page patch and leave it at that.
2.  Take it as another bikeshed for -arch.

Thoughts?
Comment 19 Rodney W. Grimes freebsd_committer 2020-06-16 14:12:21 UTC
(In reply to Warner Losh from comment #17)
I think it boils down to pretty much that.  Though I well note there is a kind of bug in calendar -a with respect to nologin set on daemon.  But even in that view I note that it would be valid for a person to remove the /etc/aliases entry for daemon and expect it to get a copy of the mail in /var/mail/daemon.

We could modify the shipped system in a way that these duplicates did not occur, that would mean creating seperate home dirs for daemon and toor and removing the aliases for them.  It would stop the bug and make it clearer what is going on, but it would also create additional, and probably for the most part, unneeded and anwanted directories.

(In reply to Greg Lehey from comment #18)
For me it seems the most productive path forward is some documentation changes, and a Blue bikeshed discussion on -arch is probably not going to lead to much more than what has been discussed here.

Can we get some feedback from the original poster on their thoughts now that we have kicked this ball around the park a few times?
Comment 20 wcarson.bugzilla 2020-06-16 21:16:41 UTC
(In reply to Rodney W. Grimes from comment #19)

I think it's reasonable for the root user in a default configuration to be able to utilize the ~/.calendar/ & calendar -a functionality enabled by periodic, and not expect duplicate mails (again, due to the default configuration). I don't think changing the homedir of toor is really a suitable solution since it seems to defeat much of the point of the toor user. (I don't know the consequences of changing the homedir of the daemon user. On my minimal system the daemon user is running no processes and only owns 3 directories (and no files) on the system: /var/at/jobs, /var/at/spool, and /var/msgs.) Changing the mail aliases seems to address the symptom rather than the cause, in my opinion.

I think a combination of (1) tracking which calendar files have already been processed and (2) a configuration value in the calendar file to override which users are mailed as suggested by Greg might be a nice solution and I think would address most or all of the concerns raised so far (although, perhaps this is what Warner meant by "too much complexity").

That said, it's of course easy for me to spend another person's time, as I don't feel very qualified to implement such functionality to the rigorous standards of the FreeBSD project. I tried to learn the best way to create a searchable list of strings in C, but came up short. Furthermore, I have been using FreeBSD since the late 90s and only just learned of calendar this past month when I ran into this bug. (I was browsing /etc/defaults/periodic.conf to see what other things I didn't know existed.) So, therefore, given there are available workarounds (e.g. not having root run it) and the fact I am but a single user and not some large business with a true need, I'm ok knowing this was considered, even if it ends up rejected.