munin-cgi-graph: handle html cache dir creation more flexible

[feiner.tom]

Forwarded from: ​http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=598737

Hi,

I just stumbled over the following nasty inconvenience:

• cgi-graphing enabled
• www-data is member of group munin, so for cgi graphing to work the cache dirs need to be group writeable
• added new node to be monitored
• on first run of munin-cron (and munin-html) the cache dir for the new node was created with the hard-coded mode of 755 wich subsequently lead to empty graphs in munin-cgi-graph

So instead of having to change the dir modes on every node addition, I wrote
the attached patch, which makes the mode of directories created by munin-html
configurable by an entry in /etc/munin/munin-node.conf, e.g.

htmldir_mode 775

If that line is missing, the $htmldir_mode variable will be initialized with
the former default value of 755. Additionally, the keyword htmldir_mode is
added as a legal config keyword in Common/Config?.pm

Cheers,

Daniel

[feiner.tom]

munin-custom-htmldir_mode.patch

[feiner.tom]

Trac does not view the patch apprioriately, so please view it in its raw mode (it patches 2 files, but trac only shows the first file patched).

Steve,I see that trunk still creates this directory hard coded as 755 (link). How did you handle the problem raised in this bug (which is for 1.4.5) , in munin 2.0.0 alpha?

Thanks,
Tom

[snide]

Replying to feiner.tom:

Steve,I see that trunk still creates this directory hard coded as 755 (link). How did you handle the problem raised in this bug (which is for 1.4.5) , in munin 2.0.0 alpha?

I cheated : In 2.0, there is a whole new directory tree for CGI purposes (usually based off /tmp), that is configured via cgitmpdir as currently implemented.

The cgi script then uses the new --output-file option to tell GraphOld that it should create the png there.

[snide]

Replying to snide:

The cgi script then uses the new --output-file option to tell GraphOld that it should create the png there.

Btw, similar story for the log file.

[feiner.tom]

In that case , a good solution for 1.4.x branch would be to default to 775 (as suggested by Holger in the debian bug), and as both owner and group = munin, security wise this is not a degradation, and will allow cgi to work nicely out of the box. (At least in regards to htmldir - the logdir will still need to be set manually.

[dhr]

Now I'm taking this thread hostage but since you mentioned it:

nicely out of the box. (At least in regards to htmldir - the logdir will
still need to be set manually.

For cgi graphing my /etc/logorate.d/munin looks like this:
==========================================
/var/log/munin/munin-update.log {

daily
missingok
rotate 7
compress
notifempty
create 640 munin adm

}

/var/log/munin/munin-cgi-graph.log {

daily
missingok
rotate 7
compress
#dhr: munin-cgi-graph
#notifempty
#create 660 munin adm
create 660 munin munin

}

/var/log/munin/munin-graph.log {

daily
missingok
rotate 7
compress
#dhr: munin-cgi-graph
#notifempty
#create 660 munin adm
create 660 munin munin

}

/var/log/munin/munin-html.log {

daily
missingok
rotate 7
compress
notifempty
create 640 munin adm

}

/var/log/munin/munin-limits.log {

daily
missingok
rotate 7
compress
notifempty
create 640 munin adm

}
==========================================

You you see any negative, security-relevant implications here?

Daniel

[dhr]

Replying to feiner.tom:

In that case , a good solution for 1.4.x branch would be to default to 775 (as suggested by Holger in the debian bug), and as both owner and group = munin, security wise this is not a degradation, and will allow cgi to work nicely out of the box. (At least in regards to htmldir - the logdir will still need to be set manually.

Well, attached you'll find a revised patch. I totally overlooked the fact that perl's mkdir takes the umask into account which I'd temporarily set to 0 during patch creation.

--- /usr/share/perl5/Munin/Master/HTMLOld.pm 2010-10-01 19:13:29.810405452 +0200
+++ /usr/share/perl5/Munin/Master/HTMLOld.pm.myutils-new 2010-10-01 19:16:32.192734600 +0200
@@ -359,7 +359,10 @@

$dirname =~ s/\/[^\/]*$;

DEBUG "[DEBUG] Creating service page $filename";

• munin_mkdir_p($dirname, oct(755));

+ my $oldMask = umask();
+ umask 0;
+ munin_mkdir_p($dirname, oct(775));
+ umask $oldMask;

open(my $FILE, '>', $filename)

or die "Cannot open '$filename' for writing: $!";

[wferi]

Why not simply let the umask mechanism work as intended?
I propose simply doing

--- /usr/share/perl5/Munin/Master/HTMLOld.pm.distrib	2010-10-19 23:17:20.000000000 +0200
+++ /usr/share/perl5/Munin/Master/HTMLOld.pm	2011-11-21 16:40:04.078041487 +0100
@@ -359,7 +359,7 @@
     $dirname =~ s/\/[^\/]*$//;
 
     DEBUG "[DEBUG] Creating service page $filename";
-    munin_mkdir_p($dirname, oct(755));
+    munin_mkdir_p($dirname, oct(777));
 
     open(my $FILE, '>', $filename)
         or die "Cannot open '$filename' for writing: $!";

Or the equivalent thing in ensure_dir_exists() in the trunk version.
Actually, does munin_mkdir_p() (from Master::Utils) have to expose the $mode argument of mkpath() at all (misnamed as umask)? If not, why not get rid of this helper function altogether? That would make the code more conscise, more transparent and more correct at the same time. Umask is here to help us, let's use its power!

[snide]

It has been fixed in 2.0. And CGI in 1.4 is broken in so many other ways that I won't fix it there.