Page MenuHomeFreeBSD

newsyslog.conf: Restrict included files in default config to [!.]*.conf
ClosedPublic

Authored by woodsb02 on Sep 9 2018, 5:07 AM.

Details

Summary

newsyslog.conf: Restrict included files in default config to [!.]*.conf

The new default config will only include files from the following
directories which end with '.conf' and do not beginning with a '.'
character:

  • /etc/newsyslog.conf.d/
  • /usr/local/etc/newsyslog.conf.d/

This matches the syslog.conf(5) functionality, and also prevents '.sample' or
'.pkgnew' files being included. This is important for ports which install files
in /usr/local/etc/newsyslog.conf.d/ and also for pkgbase.

Test Plan

newsyslog -Nv
Review output to ensure the correct files are being included, and not others.
When testing this, also create the following new files:
/etc/newsyslog.conf.d/testing
/etc/newsyslog.conf.d/testing.conf
/etc/newsyslog.conf.d/testingconf
/etc/newsyslog.conf.d/.testing.conf

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

woodsb02 created this revision.Sep 9 2018, 5:07 AM
eadler accepted this revision.Sep 9 2018, 5:58 AM

This is fine to me, though I could imagine some users getting confused when their config stops working.

Don't forget to bump .Dd

usr.sbin/newsyslog/newsyslog.8
271 ↗(On Diff #47826)

Perhaps
"By default each file..."

This revision is now accepted and ready to land.Sep 9 2018, 5:58 AM
mat added a subscriber: mat.Sep 9 2018, 11:23 AM

This breaks POLA in a big way, if this goes through, it will need a very large warning in the release notes.

cem added a comment.EditedSep 9 2018, 4:00 PM

Hmm. I'm definitely ok rejecting the (hidden) names .sample or .pkgnew, and I think probably rejecting all hidden (dot-prefixed) names would be reasonable.

I don't see a good reason to require files be named something.conf, though. If they aren't configuration, outside of internal pkg machinations, they shouldn't be in newsyslog.conf.d/!

If we are going to require newsyslog conf files be named foo.conf, there needs to be automation that detects files with other names and at least produces a warning syslog message so admins with such files have a prayer of determining why their logs are not rotating. (This could just be a boot-time rc script.)

eadler added a comment.Sep 9 2018, 7:04 PM

To clarify my accept: mechanically it is fine. I assumed that the social aspects were already resolved.

dvl added a subscriber: dvl.Sep 9 2018, 7:27 PM

The proposed change makes newsyslog.conf consistent with syslog.conf

I like it. Looking forward to it.

I think I like cem's idea to only include files if they do not start with a "." or finish with ".pkgnew", ".sample" or ".bak" (any others?).

Can I ask for some globbing assistance please? I can't seem to find a way to match all filenames which do not end with ".sample", ".pkgnew" or ".bak"...

I think I like cem's idea to only include files if they do not start with a "." or finish with ".pkgnew", ".sample" or ".bak" (any others?).

If we do this, we should update other utilities that have .d directories or the like.

bapt added a comment.Sep 10 2018, 4:51 PM

I think there is absolutely no POLA here as this is a configuration, there should be some warning in the release Release note about it and that is all imho.
Considering the option to only blacklist certain patterns: dotfiles, .pkgnew etc the problem is one would need to do more than that:
remove patch leftovers: .orig, .rej, .bak
editors leftovers file~ file.bak, etc

There are too many patterns to consider

dvl added a comment.Sep 10 2018, 5:10 PM
In D17086#364678, @bapt wrote:

I think there is absolutely no POLA here as this is a configuration, there should be some warning in the release Release note about it and that is all imho.
Considering the option to only blacklist certain patterns: dotfiles, .pkgnew etc the problem is one would need to do more than that:
remove patch leftovers: .orig, .rej, .bak
editors leftovers file~ file.bak, etc
There are too many patterns to consider

Given that, should we instead just take *.conf and be done with it? Allow .files to be brought in.

In D17086#364678, @bapt wrote:

I think there is absolutely no POLA here as this is a configuration, there should be some warning in the release Release note about it and that is all imho.
Considering the option to only blacklist certain patterns: dotfiles, .pkgnew etc the problem is one would need to do more than that:
remove patch leftovers: .orig, .rej, .bak
editors leftovers file~ file.bak, etc
There are too many patterns to consider

The worst part is, these types of files are being executed today as valid newsyslog scripts (would this be causing double rotation?).

@mat - as the strongest voice of POLA violations, what about if we proceed with the change as is, and take a few extra steps to notify admins:

  • include checks in rc.d/newsyslog which look for files in newsyslog.conf.d not ending with .conf or .sample etc, and print a warning list to the console explaining the change?
  • when changing the 5 ports which install newsyslog.d files without the .conf suffix, include a pkg-install script which moves the old file to the new location (to bring in local user changes) and also prints an output to the console.

You also raised the concern about if admins were using automatic deployment tools to update newsyslog.d files (ansible, salt, puppet, chef, etc). This type of user would be exposed to both of the above. Firstly, the newsyslog.d file would be moved during pkg install, and then later when their deployment tool installs the script back in the old location, they will see commands printed to the console.

In my opinion, there is no perfect answer here. The fact seems to be the current system has issues (in that newsyslog is ingesting any .sample, .bak and .orig files and actioning them) and this should be fixed. The best we can hope for is to try and ease this transition for most people through migration tools, and detect/warn the others through console error messages.

I agree with @dvl and @bapt, including only *.conf is common across other parts of FreeBSD and the wider conf.d directory idiom elsewhere. There are too many changes for a rouge file to break your configuration otherwise.

bapt accepted this revision.Nov 10 2018, 6:44 AM
woodsb02 marked an inline comment as done.Nov 10 2018, 10:46 AM
This revision was automatically updated to reflect the committed changes.