[4/4,V2] zabbix_agentd: Add IPFire specific userparameters

  Provide IPFire specific items for the Zabbix server to monitor:
- Networking stats:
  - ipfire.net.gateway.pingtime: Internet Line Quality
  - ipfire.net.gateway.ping: Internet connection
  - ipfire.net.fw.hits[*]: Firewall hits
- IPFire service states:
  - ipfire.services: JSON formatted state of all IPFire services
    using new ipfire_services.pl script.

Users can install the IPFire 2 Zabbix template-set provided here:
https://share.zabbix.com/network-appliances/ipfire-2
to monitor these metrics. Or create their own template.

Signed-off-by: Robin Roevens <robin.roevens@disroot.org>
---
 config/rootfiles/packages/zabbix_agentd       |   3 +
 config/zabbix_agentd/ipfire_services.pl       | 221 ++++++++++++++++++
 config/zabbix_agentd/sudoers                  |   2 +-
 .../template_module_ipfire_network_stats.conf |   4 +
 .../template_module_ipfire_services.conf      |   2 +
 lfs/zabbix_agentd                             |   8 +-
 src/paks/zabbix_agentd/install.sh             |   5 +
 src/paks/zabbix_agentd/uninstall.sh           |   2 +
 8 files changed, 245 insertions(+), 2 deletions(-)
 create mode 100755 config/zabbix_agentd/ipfire_services.pl
 create mode 100644 config/zabbix_agentd/template_module_ipfire_network_stats.conf
 create mode 100644 config/zabbix_agentd/template_module_ipfire_services.conf
  

Hello,

> On 7 Apr 2021, at 21:44, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> Provide IPFire specific items for the Zabbix server to monitor:
> - Networking stats:
>  - ipfire.net.gateway.pingtime: Internet Line Quality
>  - ipfire.net.gateway.ping: Internet connection
>  - ipfire.net.fw.hits[*]: Firewall hits
> - IPFire service states:
>  - ipfire.services: JSON formatted state of all IPFire services
>    using new ipfire_services.pl script.
> 
> Users can install the IPFire 2 Zabbix template-set provided here:
> https://share.zabbix.com/network-appliances/ipfire-2
> to monitor these metrics. Or create their own template.
> 
> Signed-off-by: Robin Roevens <robin.roevens@disroot.org>
> ---
> config/rootfiles/packages/zabbix_agentd       |   3 +
> config/zabbix_agentd/ipfire_services.pl       | 221 ++++++++++++++++++
> config/zabbix_agentd/sudoers                  |   2 +-
> .../template_module_ipfire_network_stats.conf |   4 +
> .../template_module_ipfire_services.conf      |   2 +
> lfs/zabbix_agentd                             |   8 +-
> src/paks/zabbix_agentd/install.sh             |   5 +
> src/paks/zabbix_agentd/uninstall.sh           |   2 +
> 8 files changed, 245 insertions(+), 2 deletions(-)
> create mode 100755 config/zabbix_agentd/ipfire_services.pl
> create mode 100644 config/zabbix_agentd/template_module_ipfire_network_stats.conf
> create mode 100644 config/zabbix_agentd/template_module_ipfire_services.conf
> 
> diff --git a/config/rootfiles/packages/zabbix_agentd b/config/rootfiles/packages/zabbix_agentd
> index 6945c5ef7..aa3f1846b 100644
> --- a/config/rootfiles/packages/zabbix_agentd
> +++ b/config/rootfiles/packages/zabbix_agentd
> @@ -3,9 +3,12 @@ etc/rc.d/init.d/zabbix_agentd
> etc/sudoers.d/zabbix.ipfirenew
> #etc/zabbix_agentd
> #etc/zabbix_agentd/scripts
> +etc/zabbix_agentd/scripts/ipfire_services.pl.ipfirenew
> etc/zabbix_agentd/zabbix_agentd.conf.ipfirenew
> #etc/zabbix_agentd/zabbix_agentd.d
> etc/zabbix_agentd/zabbix_agentd.d/template_app_pakfire.conf.ipfirenew
> +etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_network_stats.conf.ipfirenew
> +etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_services.conf.ipfirenew
> usr/bin/zabbix_get
> usr/bin/zabbix_sender
> #usr/lib/modules
> diff --git a/config/zabbix_agentd/ipfire_services.pl b/config/zabbix_agentd/ipfire_services.pl
> new file mode 100755
> index 000000000..dbf8aec56
> --- /dev/null
> +++ b/config/zabbix_agentd/ipfire_services.pl
> @@ -0,0 +1,221 @@
> +#!/usr/bin/perl
> +###############################################################################
> +# ipfire_services.pl - Retrieves available IPFire services information and 
> +#                      return this as a JSON array suitable for easy processing 
> +#                      by Zabbix server
> +#
> +# Author: robin.roevens (at) disroot.org
> +# Version: 1.0
> +#
> +# Based on: services.cgi by IPFire Team
> +# Copyright (C) 2007-2021  IPFire Team  <info@ipfire.org> 
> +# 
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +# 
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the 
> +# GNU General Public License for more details.
> +# 
> +# You should have received a copy of the GNU General Public License 
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +# 
> +###############################################################################
> +
> +use strict;
> +
> +# enable only the following on debugging purpose
> +# use warnings;
> +
> +# Maps a nice printable name to the changing part of the pid file, which
> +# is also the name of the program
> +my %servicenames =(
> +        'DHCP Server' => 'dhcpd',
> +        'Web Server' => 'httpd',
> +        'CRON Server' => 'fcron',
> +        'DNS Proxy Server' => 'unbound',
> +        'Logging Server' => 'syslogd',
> +        'Kernel Logging Server' => 'klogd',
> +        'NTP Server' => 'ntpd',
> +        'Secure Shell Server' => 'sshd',
> +        'VPN' => 'charon',
> +        'Web Proxy' => 'squid',
> +        'Intrusion Detection System' => 'suricata',
> +        'OpenVPN' => 'openvpn'
> +);

You have a hard-coded list with process names here. It technically is missing add-ons but including everything would make this list very long and a lot of work to maintain...

> +
> +# Hash to overwrite the process name of a process if it differs from the launch command.
> +my %overwrite_exename_hash = (
> +        "suricata" => "Suricata-Main"
> +);
> +
> +my $first = 1;
> +
> +print "[";
> +
> +# Built-in services
> +my $key = '';
> +foreach $key (sort keys %servicenames){
> +	print "," if not $first;
> +	$first = 0;
> +
> +	print "{";
> +	print "\"service\":\"$key\",";
> +
> +	my $shortname = $servicenames{$key};
> +	print &servicestats($shortname);
> +	
> +	print "}";
> +}
> +
> +# Generate list of installed addon pak's
> +my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -d"-" -f2`;
> +foreach (@pak){
> +	chomp($_);
> +
> +	# Check which of the paks are services
> +	my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
> +	foreach (@svc){
> +		# blacklist some packages
> +		#
> +		# alsa has trouble with the volume saving and was not really stopped
> +		# mdadm should not stopped with webif because this could crash the system
> +		#
> +		chomp($_);
> +		if ( $_ eq 'squid' ) {
> +			next;
> +		}
> +		if ( ($_ ne "alsa") && ($_ ne "mdadm") ) {

Another hardcoded list due to code duplication. Not your fault, but not pretty.

> +			print ",";
> +			print "{";
> +
> +			print "\"service\":\"Addon: $_\",";
> +			print "\"servicename\":\"$_\",";
> +
> +			my $onboot = isautorun($_);
> +			print "\"onboot\":$onboot,";
> +
> +			print &addonservicestats($_);
> +
> +			print "}";
> +		}
> +	}
> +}	
> +
> +print "]";
> +
> +sub servicestats{
> +	my $cmd = $_[0];
> +	my $status = "\"servicename\":\"$cmd\",\"state\":\"0\"";
> +	my $pid = '';
> +	my $testcmd = '';
> +        my $exename;
> +        my $memory;
> +
> +
> +	$cmd =~ /(^[a-z]+)/;
> +	
> +	# Check if the exename needs to be overwritten.
> +        # This happens if the expected process name string
> +        # differs from the real one. This may happened if
> +        # a service uses multiple processes or threads.
> +        if (exists($overwrite_exename_hash{$cmd})) {
> +                # Grab the string which will be reported by
> +                # the process from the corresponding hash.
> +                $exename = $overwrite_exename_hash{$1};
> +        } else {
> +                # Directly expect the launched command as
> +                # process name.
> +                $exename = $1;
> +        }
> +	
> +	if (open(FILE, "/var/run/${cmd}.pid")){
> +                $pid = <FILE>; chomp $pid;
> +                close FILE;
> +                if (open(FILE, "/proc/${pid}/status")){
> +                        while (<FILE>){
> +                                if (/^Name:\W+(.*)/) {
> +                                        $testcmd = $1;
> +                                }
> +                        }
> +                        close FILE;
> +                }
> +                if (open(FILE, "/proc/${pid}/status")) {
> +                        while (<FILE>) {
> +                                my ($key, $val) = split(":", $_, 2);
> +                                if ($key eq 'VmRSS') {
> +					$val =~ /\s*([0-9]*)\s+kB/;
> +					# Convert kB to B
> +                                        $memory = $1*1024;
> +                                        last;
> +                                }
> +                        }
> +                        close(FILE);
> +                }
> +                if ($testcmd =~ /$exename/){
> +			$status = "\"servicename\":\"$cmd\",\"state\":1,\"pid\":$pid,\"memory\":$memory";
> +		}
> +        }
> +        return $status;
> +}
> +
> +sub isautorun{
> +        my $cmd = $_[0];
> +        my $status = "0";
> +        my $init = `find /etc/rc.d/rc3.d/S??${cmd} 2>/dev/null`;
> +        chomp ($init);
> +        if ($init ne ''){
> +                $status = "1";
> +        }
> +        $init = `find /etc/rc.d/rc3.d/off/S??${cmd} 2>/dev/null`;
> +        chomp ($init);
> +        if ($init ne ''){
> +                $status = "0";
> +        }
> +
> +        return $status;
> +}
> +
> +sub addonservicestats{
> +        my $cmd = $_[0];
> +        my $status = "0";
> +        my $pid = '';
> +        my $testcmd = '';
> +        my $exename;
> +        my @memory = (0);
> +
> +        $testcmd = `sudo /usr/local/bin/addonctrl $_ status 2>/dev/null`;
> +
> +        if ( $testcmd =~ /is\ running/ && $testcmd !~ /is\ not\ running/){
> +                $status = "\"state\":1";
> +
> +                $testcmd =~ s/.* //gi;
> +                $testcmd =~ s/[a-z_]//gi;
> +                $testcmd =~ s/\[[0-1]\;[0-9]+//gi;
> +                $testcmd =~ s/[\(\)\.]//gi;
> +                $testcmd =~ s/  //gi;
> +                $testcmd =~ s///gi;
> +
> +                my @pid = split(/\s/,$testcmd);
> +                $status .=",\"pid\":\"$pid[0]\"";
> +
> +                my $memory = 0;
> +
> +                foreach (@pid){
> +                        chomp($_);
> +                        if (open(FILE, "/proc/$_/statm")){
> +                                my $temp = <FILE>;
> +                                @memory = split(/ /,$temp);
> +                        }
> +                        $memory+=$memory[0];
> +                }
> +		$memory*=1024;
> +                $status .=",\"memory\":$memory";
> +        }else{
> +                $status = "\"state\":0";
> +        }
> +        return $status;
> +}
> diff --git a/config/zabbix_agentd/sudoers b/config/zabbix_agentd/sudoers
> index 1b362a4fd..340bb8e66 100644
> --- a/config/zabbix_agentd/sudoers
> +++ b/config/zabbix_agentd/sudoers
> @@ -14,4 +14,4 @@
> # Append / edit the following list of commands to fit your needs:
> #
> Defaults:zabbix !requiretty
> -zabbix ALL=(ALL) NOPASSWD: /opt/pakfire/pakfire status
> +zabbix ALL=(ALL) NOPASSWD: /opt/pakfire/pakfire status, /usr/local/bin/addonctrl, /sbin/iptables, /usr/sbin/fping

This is absolutely impossible to give the zabbix user control to start/stop any add-on and to read and change the entire iptables ruleset.

Zabbix is listening on the network and does not have any kind of authentication. Any security vulnerabilities in Zabbix would allow to alter the firewall rules and DoS any add-ons. This is dangerous.

Are there any alternative ways to realise this functionality?

> diff --git a/config/zabbix_agentd/template_module_ipfire_network_stats.conf b/config/zabbix_agentd/template_module_ipfire_network_stats.conf
> new file mode 100644
> index 000000000..f1658ed07
> --- /dev/null
> +++ b/config/zabbix_agentd/template_module_ipfire_network_stats.conf
> @@ -0,0 +1,4 @@
> +### Parameters for monitoring IPFire network statistics 
> +UserParameter=ipfire.net.gateway.pingtime,sudo /usr/sbin/fping -c 3 gateway 2>&1 | tail -n 1 | awk '{print $NF}' | cut -d '/' -f2
> +UserParameter=ipfire.net.gateway.ping,sudo /usr/sbin/fping -q -r 3 gateway; [ ! $? ]; echo $?
> +UserParameter=ipfire.net.fw.hits[*],sudo /sbin/iptables -vnxL $1 | grep "\/\* $2 \*\/" | awk '{ print $$2 }';
> diff --git a/config/zabbix_agentd/template_module_ipfire_services.conf b/config/zabbix_agentd/template_module_ipfire_services.conf
> new file mode 100644
> index 000000000..5f95218e3
> --- /dev/null
> +++ b/config/zabbix_agentd/template_module_ipfire_services.conf
> @@ -0,0 +1,2 @@
> +### Parameter for monitoring IPFire services
> +UserParameter=ipfire.services,/etc/zabbix_agentd/scripts/ipfire_services.pl
> diff --git a/lfs/zabbix_agentd b/lfs/zabbix_agentd
> index 73e08d20a..c0d28d51f 100644
> --- a/lfs/zabbix_agentd
> +++ b/lfs/zabbix_agentd
> @@ -33,7 +33,7 @@ DIR_APP    = $(DIR_SRC)/$(THISAPP)
> TARGET     = $(DIR_INFO)/$(THISAPP)
> PROG       = zabbix_agentd
> PAK_VER    = 5
> -DEPS       =
> +DEPS       = "fping"
> 
> ###############################################################################
> # Top-level Rules
> @@ -97,6 +97,12 @@ $(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects))
> 		/etc/zabbix_agentd/zabbix_agentd.conf.ipfirenew
> 	install -v -m 644 $(DIR_SRC)/config/zabbix_agentd/template_app_pakfire.conf \
> 		/etc/zabbix_agentd/zabbix_agentd.d/template_app_pakfire.conf.ipfirenew
> +	install -v -m 644 $(DIR_SRC)/config/zabbix_agentd/template_module_ipfire_network_stats.conf \
> +		/etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_network_stats.conf.ipfirenew
> +	install -v -m 644 $(DIR_SRC)/config/zabbix_agentd/template_module_ipfire_services.conf \
> +		/etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_services.conf.ipfirenew
> +	install -v -m 755 $(DIR_SRC)/config/zabbix_agentd/ipfire_services.pl \
> +		/etc/zabbix_agentd/scripts/ipfire_services.pl.ipfirenew
> 
> 	# Create directory for additional agent modules
> 	-mkdir -pv /usr/lib/zabbix
> diff --git a/src/paks/zabbix_agentd/install.sh b/src/paks/zabbix_agentd/install.sh
> index 4248a7ec1..ced915c81 100644
> --- a/src/paks/zabbix_agentd/install.sh
> +++ b/src/paks/zabbix_agentd/install.sh
> @@ -66,8 +66,13 @@ restore_backup ${NAME}
> # Put zabbix configfiles in place
> setup_configfile /etc/zabbix_agentd/zabbix_agentd.conf
> setup_configfile /etc/zabbix_agentd/zabbix_agentd.d/template_app_pakfire.conf
> +setup_configfile /etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_network_stats.conf
> +setup_configfile /etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_services.conf
> setup_configfile /etc/sudoers.d/zabbix
> 
> +# Overwrite script if it exists as user should not modify it but it is included in backup
> +mv /etc/zabbix_agentd/scripts/ipfire_services.pl.ipfirenew /etc/zabbix_agentd/scripts/ipfire_services.pl
> +
> if $review_required; then
> 	echo "WARNING: New versions of some configfile(s) where provided as .ipfirenew-files."
> 	echo "         They may need manual review in order to take advantage of new features"
> diff --git a/src/paks/zabbix_agentd/uninstall.sh b/src/paks/zabbix_agentd/uninstall.sh
> index 7a13880c5..ccbc8f7cf 100644
> --- a/src/paks/zabbix_agentd/uninstall.sh
> +++ b/src/paks/zabbix_agentd/uninstall.sh
> @@ -26,6 +26,8 @@ stop_service ${NAME}
> 
> # Remove .ipfirenew files in advance so they won't be included in backup
> rm -rfv /etc/zabbix_agentd/*.ipfirenew /etc/zabbix_agentd/*/*.ipfirenew
> +# Remove script-file as it should not have been modified by user
> +rm -fv /etc/zabbix_agentd/scripts/ipfire_services.pl
> 
> make_backup ${NAME}
> remove_files
> -- 
> 2.30.2
> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
> 

-Michael
  

Hi Michael


Michael Tremer schreef op ma 12-04-2021 om 11:36 [+0100]:
> Hello,
> 
> > On 7 Apr 2021, at 21:44, Robin Roevens <robin.roevens@disroot.org>
> > wrote:
> > ...
> > ...
> > diff --git a/config/zabbix_agentd/ipfire_services.pl
> > b/config/zabbix_agentd/ipfire_services.pl
> > new file mode 100755
> > index 000000000..dbf8aec56
> > --- /dev/null
> > +++ b/config/zabbix_agentd/ipfire_services.pl
> > @@ -0,0 +1,221 @@
> > +#!/usr/bin/perl
> > +####################################################################
> > ###########
> > +# ipfire_services.pl - Retrieves available IPFire services
> > information and 
> > +#                      return this as a JSON array suitable for easy
> > processing 
> > +#                      by Zabbix server
> > +#
> > +# Author: robin.roevens (at) disroot.org
> > +# Version: 1.0
> > +#
> > +# Based on: services.cgi by IPFire Team
> > +# Copyright (C) 2007-2021  IPFire Team  <info@ipfire.org> 
> > +# 
> > +# This program is free software: you can redistribute it and/or
> > modify
> > +# it under the terms of the GNU General Public License as published
> > by
> > +# the Free Software Foundation, either version 3 of the License, or
> > +# (at your option) any later version.
> > +# 
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the 
> > +# GNU General Public License for more details.
> > +# 
> > +# You should have received a copy of the GNU General Public License 
> > +# along with this program.  If not, see <
> > http://www.gnu.org/licenses/>.
> > +# 
> > +####################################################################
> > ###########
> > +
> > +use strict;
> > +
> > +# enable only the following on debugging purpose
> > +# use warnings;
> > +
> > +# Maps a nice printable name to the changing part of the pid file,
> > which
> > +# is also the name of the program
> > +my %servicenames =(
> > +        'DHCP Server' => 'dhcpd',
> > +        'Web Server' => 'httpd',
> > +        'CRON Server' => 'fcron',
> > +        'DNS Proxy Server' => 'unbound',
> > +        'Logging Server' => 'syslogd',
> > +        'Kernel Logging Server' => 'klogd',
> > +        'NTP Server' => 'ntpd',
> > +        'Secure Shell Server' => 'sshd',
> > +        'VPN' => 'charon',
> > +        'Web Proxy' => 'squid',
> > +        'Intrusion Detection System' => 'suricata',
> > +        'OpenVPN' => 'openvpn'
> > +);
> 
> You have a hard-coded list with process names here. It technically is
> missing add-ons but including everything would make this list very long
> and a lot of work to maintain...
> 
As explained in my previous mail, I don't currently know of a method to
get a list of mandatory IPFire specific services. So this list is a
straight copy out of services.cgi minus the translation.
The add-on services are looked up below, exactly like it is done in
services.cgi.

> > +
> > +# Hash to overwrite the process name of a process if it differs from
> > the launch command.
> > +my %overwrite_exename_hash = (
> > +        "suricata" => "Suricata-Main"
> > +);
> > +
> > +my $first = 1;
> > +
> > +print "[";
> > +
> > +# Built-in services
> > +my $key = '';
> > +foreach $key (sort keys %servicenames){
> > +       print "," if not $first;
> > +       $first = 0;
> > +
> > +       print "{";
> > +       print "\"service\":\"$key\",";
> > +
> > +       my $shortname = $servicenames{$key};
> > +       print &servicestats($shortname);
> > +       
> > +       print "}";
> > +}
> > +
> > +# Generate list of installed addon pak's
> > +my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -
> > d"-" -f2`;
> > +foreach (@pak){
> > +       chomp($_);
> > +
> > +       # Check which of the paks are services
> > +       my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
> > +       foreach (@svc){
> > +               # blacklist some packages
> > +               #
> > +               # alsa has trouble with the volume saving and was not
> > really stopped
> > +               # mdadm should not stopped with webif because this
> > could crash the system
> > +               #
> > +               chomp($_);
> > +               if ( $_ eq 'squid' ) {
> > +                       next;
> > +               }
> > +               if ( ($_ ne "alsa") && ($_ ne "mdadm") ) {
> 
> Another hardcoded list due to code duplication. Not your fault, but not
> pretty.
> 

Indeed, as is probably clear by now, I too would prefer a more clean
way of generating a list of services.

Currently seeing this piece of code again.. I think this blacklist in
services.cgi only exists so the user can't stop/start the service using
the webgui.. But there is no such functionality in this script.. so I
think I can skip this blacklist, and also provide service state of alsa
and mdadm.. (not sure why squid is here, is there an addon that also
provides squid, maybe squid-accounting?)

> > +                       print ",";
> > +                       print "{";
> > +
> > +                       print "\"service\":\"Addon: $_\",";
> > +                       print "\"servicename\":\"$_\",";
> > +
> > +                       my $onboot = isautorun($_);
> > +                       print "\"onboot\":$onboot,";
> > +
> > +                       print &addonservicestats($_);
> > +
> > +                       print "}";
> > +               }
> > +       }
> > +}      
> > +
> > +print "]";
> > +
> > +sub servicestats{
> > +       my $cmd = $_[0];
> > +       my $status = "\"servicename\":\"$cmd\",\"state\":\"0\"";
> > +       my $pid = '';
> > +       my $testcmd = '';
> > +        my $exename;
> > +        my $memory;
> > +
> > +
> > +       $cmd =~ /(^[a-z]+)/;
> > +       
> > +       # Check if the exename needs to be overwritten.
> > +        # This happens if the expected process name string
> > +        # differs from the real one. This may happened if
> > +        # a service uses multiple processes or threads.
> > +        if (exists($overwrite_exename_hash{$cmd})) {
> > +                # Grab the string which will be reported by
> > +                # the process from the corresponding hash.
> > +                $exename = $overwrite_exename_hash{$1};
> > +        } else {
> > +                # Directly expect the launched command as
> > +                # process name.
> > +                $exename = $1;
> > +        }
> > +       
> > +       if (open(FILE, "/var/run/${cmd}.pid")){
> > +                $pid = <FILE>; chomp $pid;
> > +                close FILE;
> > +                if (open(FILE, "/proc/${pid}/status")){
> > +                        while (<FILE>){
> > +                                if (/^Name:\W+(.*)/) {
> > +                                        $testcmd = $1;
> > +                                }
> > +                        }
> > +                        close FILE;
> > +                }
> > +                if (open(FILE, "/proc/${pid}/status")) {
> > +                        while (<FILE>) {
> > +                                my ($key, $val) = split(":", $_, 2);
> > +                                if ($key eq 'VmRSS') {
> > +                                       $val =~ /\s*([0-9]*)\s+kB/;
> > +                                       # Convert kB to B
> > +                                        $memory = $1*1024;
> > +                                        last;
> > +                                }
> > +                        }
> > +                        close(FILE);
> > +                }
> > +                if ($testcmd =~ /$exename/){
> > +                       $status =
> > "\"servicename\":\"$cmd\",\"state\":1,\"pid\":$pid,\"memory\":$memory
> > ";
> > +               }
> > +        }
> > +        return $status;
> > +}
> > +
> > +sub isautorun{
> > +        my $cmd = $_[0];
> > +        my $status = "0";
> > +        my $init = `find /etc/rc.d/rc3.d/S??${cmd} 2>/dev/null`;
> > +        chomp ($init);
> > +        if ($init ne ''){
> > +                $status = "1";
> > +        }
> > +        $init = `find /etc/rc.d/rc3.d/off/S??${cmd} 2>/dev/null`;
> > +        chomp ($init);
> > +        if ($init ne ''){
> > +                $status = "0";
> > +        }
> > +
> > +        return $status;
> > +}
> > +
> > +sub addonservicestats{
> > +        my $cmd = $_[0];
> > +        my $status = "0";
> > +        my $pid = '';
> > +        my $testcmd = '';
> > +        my $exename;
> > +        my @memory = (0);
> > +
> > +        $testcmd = `sudo /usr/local/bin/addonctrl $_ status
> > 2>/dev/null`;
> > +
> > +        if ( $testcmd =~ /is\ running/ && $testcmd !~ /is\ not\
> > running/){
> > +                $status = "\"state\":1";
> > +
> > +                $testcmd =~ s/.* //gi;
> > +                $testcmd =~ s/[a-z_]//gi;
> > +                $testcmd =~ s/\[[0-1]\;[0-9]+//gi;
> > +                $testcmd =~ s/[\(\)\.]//gi;
> > +                $testcmd =~ s/  //gi;
> > +                $testcmd =~ s///gi;
> > +
> > +                my @pid = split(/\s/,$testcmd);
> > +                $status .=",\"pid\":\"$pid[0]\"";
> > +
> > +                my $memory = 0;
> > +
> > +                foreach (@pid){
> > +                        chomp($_);
> > +                        if (open(FILE, "/proc/$_/statm")){
> > +                                my $temp = <FILE>;
> > +                                @memory = split(/ /,$temp);
> > +                        }
> > +                        $memory+=$memory[0];
> > +                }
> > +               $memory*=1024;
> > +                $status .=",\"memory\":$memory";
> > +        }else{
> > +                $status = "\"state\":0";
> > +        }
> > +        return $status;
> > +} 
> > diff --git a/config/zabbix_agentd/sudoers
> > b/config/zabbix_agentd/sudoers
> > index 1b362a4fd..340bb8e66 100644
> > --- a/config/zabbix_agentd/sudoers
> > +++ b/config/zabbix_agentd/sudoers
> > @@ -14,4 +14,4 @@
> > # Append / edit the following list of commands to fit your needs:
> > #
> > Defaults:zabbix !requiretty
> > -zabbix ALL=(ALL) NOPASSWD: /opt/pakfire/pakfire status
> > +zabbix ALL=(ALL) NOPASSWD: /opt/pakfire/pakfire status,
> > /usr/local/bin/addonctrl, /sbin/iptables, /usr/sbin/fping
> 
> This is absolutely impossible to give the zabbix user control to
> start/stop any add-on and to read and change the entire iptables
> ruleset.
> 
> Zabbix is listening on the network and does not have any kind of
> authentication. Any security vulnerabilities in Zabbix would allow to
> alter the firewall rules and DoS any add-ons. This is dangerous.
There is some level of security: 
- the agent will only respond on requests from the configured Zabbix
server (which the user has to configure in the zabbix agentd conf-file,
and defaults to 127.0.0.1 as we can't possibly know the ip of the
user's server, so by default the agent responds to no one)
- By default remote commands (as in requests to the agent to execute
any command given) are disabled so the only way to make the agent call
those binaries is to request a predefined userparameter that in turn
will execute such a command with predefined params (see the additional
template_...conf-files)
and on top of that, the user can add additional security by configuring
encryption in the agent main configfile

But as you point out, a possible vulnerability of some sort may indeed
still pose risks this way.

> 
> Are there any alternative ways to realise this functionality?
As sudo is not able to allow commands with variable parameters together
with fixed parameters, a popular solution is using wrapper scripts, not
writable by the user, denying the user direct access to the commands in
question.

/usr/local/bin/addonctrl is only called from within the
ipfire_services.pl script I provide, in the form of "addonctrl
<service> status". 
So I could add /etc/zabbix_agentd/scripts/ipfire_services.pl to the
sudo list instead so that the zabbix user won't be able to call
addonctrl directly. (making sure zabbix user has no write permission on
that script)

The same goes for iptables. This is only called by the predefined
userparameter "ipfire.net.fw.hits[*]" defined in
template_module_ipfire_network_stats.conf in the form "/sbin/iptables -
vnxL $1 | grep "\/\* $2 \*\/" | awk '{ print $$2 }';" where $1 (chain)
and $2 (rule) are parameters that have to be supplied to the agent when
requesting that item. 
Here I can also supply a wrapper-script to be called  by sudo
/etc/zabbix_agentd/scripts/firewall_hits.sh that accepts those
parameters, and checks them for validity and against abuse.

Regards
Robin

> 
> > diff --git
> > a/config/zabbix_agentd/template_module_ipfire_network_stats.conf
> > b/config/zabbix_agentd/template_module_ipfire_network_stats.conf
> > new file mode 100644
> > index 000000000..f1658ed07
> > --- /dev/null
> > +++ b/config/zabbix_agentd/template_module_ipfire_network_stats.conf
> > @@ -0,0 +1,4 @@
> > +### Parameters for monitoring IPFire network statistics 
> > +UserParameter=ipfire.net.gateway.pingtime,sudo /usr/sbin/fping -c 3
> > gateway 2>&1 | tail -n 1 | awk '{print $NF}' | cut -d '/' -f2
> > +UserParameter=ipfire.net.gateway.ping,sudo /usr/sbin/fping -q -r 3
> > gateway; [ ! $? ]; echo $?
> > +UserParameter=ipfire.net.fw.hits[*],sudo /sbin/iptables -vnxL $1 |
> > grep "\/\* $2 \*\/" | awk '{ print $$2 }';
> > diff --git
> > a/config/zabbix_agentd/template_module_ipfire_services.conf
> > b/config/zabbix_agentd/template_module_ipfire_services.conf
> > new file mode 100644
> > index 000000000..5f95218e3
> > --- /dev/null
> > +++ b/config/zabbix_agentd/template_module_ipfire_services.conf
> > @@ -0,0 +1,2 @@
> > +### Parameter for monitoring IPFire services
> > +UserParameter=ipfire.services,/etc/zabbix_agentd/scripts/ipfire_serv
> > ices.pl
> > diff --git a/lfs/zabbix_agentd b/lfs/zabbix_agentd
> > index 73e08d20a..c0d28d51f 100644
> > --- a/lfs/zabbix_agentd
> > +++ b/lfs/zabbix_agentd
> > @@ -33,7 +33,7 @@ DIR_APP    = $(DIR_SRC)/$(THISAPP)
> > TARGET     = $(DIR_INFO)/$(THISAPP)
> > PROG       = zabbix_agentd
> > PAK_VER    = 5
> > -DEPS       =
> > +DEPS       = "fping"
> > 
> > #####################################################################
> > ##########
> > # Top-level Rules
> > @@ -97,6 +97,12 @@ $(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects))
> >                 /etc/zabbix_agentd/zabbix_agentd.conf.ipfirenew
> >         install -v -m 644
> > $(DIR_SRC)/config/zabbix_agentd/template_app_pakfire.conf \
> >                 /etc/zabbix_agentd/zabbix_agentd.d/template_app_pakfi
> > re.conf.ipfirenew
> > +       install -v -m 644
> > $(DIR_SRC)/config/zabbix_agentd/template_module_ipfire_network_stats.
> > conf \
> > +               /etc/zabbix_agentd/zabbix_agentd.d/template_module_ip
> > fire_network_stats.conf.ipfirenew
> > +       install -v -m 644
> > $(DIR_SRC)/config/zabbix_agentd/template_module_ipfire_services.conf
> > \
> > +               /etc/zabbix_agentd/zabbix_agentd.d/template_module_ip
> > fire_services.conf.ipfirenew
> > +       install -v -m 755
> > $(DIR_SRC)/config/zabbix_agentd/ipfire_services.pl \
> > +               /etc/zabbix_agentd/scripts/ipfire_services.pl.ipfiren
> > ew
> > 
> >         # Create directory for additional agent modules
> >         -mkdir -pv /usr/lib/zabbix
> > diff --git a/src/paks/zabbix_agentd/install.sh
> > b/src/paks/zabbix_agentd/install.sh
> > index 4248a7ec1..ced915c81 100644
> > --- a/src/paks/zabbix_agentd/install.sh
> > +++ b/src/paks/zabbix_agentd/install.sh
> > @@ -66,8 +66,13 @@ restore_backup ${NAME}
> > # Put zabbix configfiles in place
> > setup_configfile /etc/zabbix_agentd/zabbix_agentd.conf
> > setup_configfile
> > /etc/zabbix_agentd/zabbix_agentd.d/template_app_pakfire.conf
> > +setup_configfile
> > /etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_network_sta
> > ts.conf
> > +setup_configfile
> > /etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_services.co
> > nf
> > setup_configfile /etc/sudoers.d/zabbix
> > 
> > +# Overwrite script if it exists as user should not modify it but it
> > is included in backup
> > +mv /etc/zabbix_agentd/scripts/ipfire_services.pl.ipfirenew
> > /etc/zabbix_agentd/scripts/ipfire_services.pl
> > +
> > if $review_required; then
> >         echo "WARNING: New versions of some configfile(s) where
> > provided as .ipfirenew-files."
> >         echo "         They may need manual review in order to take
> > advantage of new features"
> > diff --git a/src/paks/zabbix_agentd/uninstall.sh
> > b/src/paks/zabbix_agentd/uninstall.sh
> > index 7a13880c5..ccbc8f7cf 100644
> > --- a/src/paks/zabbix_agentd/uninstall.sh
> > +++ b/src/paks/zabbix_agentd/uninstall.sh
> > @@ -26,6 +26,8 @@ stop_service ${NAME}
> > 
> > # Remove .ipfirenew files in advance so they won't be included in
> > backup
> > rm -rfv /etc/zabbix_agentd/*.ipfirenew
> > /etc/zabbix_agentd/*/*.ipfirenew
> > +# Remove script-file as it should not have been modified by user
> > +rm -fv /etc/zabbix_agentd/scripts/ipfire_services.pl
> > 
> > make_backup ${NAME}
> > remove_files
> > -- 
> > 2.30.2
> > 
> > 
> > -- 
> > Dit bericht is gescanned op virussen en andere gevaarlijke
> > inhoud door MailScanner en lijkt schoon te zijn.
> > 
> 
> -Michael
> 
>
  

Hello,

> On 12 Apr 2021, at 23:16, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> Hi Michael
> 
> 
> Michael Tremer schreef op ma 12-04-2021 om 11:36 [+0100]:
>> Hello,
>> 
>>> On 7 Apr 2021, at 21:44, Robin Roevens <robin.roevens@disroot.org>
>>> wrote:
>>> ...
>>> ...
>>> diff --git a/config/zabbix_agentd/ipfire_services.pl
>>> b/config/zabbix_agentd/ipfire_services.pl
>>> new file mode 100755
>>> index 000000000..dbf8aec56
>>> --- /dev/null
>>> +++ b/config/zabbix_agentd/ipfire_services.pl
>>> @@ -0,0 +1,221 @@
>>> +#!/usr/bin/perl
>>> +####################################################################
>>> ###########
>>> +# ipfire_services.pl - Retrieves available IPFire services
>>> information and 
>>> +#                      return this as a JSON array suitable for easy
>>> processing 
>>> +#                      by Zabbix server
>>> +#
>>> +# Author: robin.roevens (at) disroot.org
>>> +# Version: 1.0
>>> +#
>>> +# Based on: services.cgi by IPFire Team
>>> +# Copyright (C) 2007-2021  IPFire Team  <info@ipfire.org> 
>>> +# 
>>> +# This program is free software: you can redistribute it and/or
>>> modify
>>> +# it under the terms of the GNU General Public License as published
>>> by
>>> +# the Free Software Foundation, either version 3 of the License, or
>>> +# (at your option) any later version.
>>> +# 
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the 
>>> +# GNU General Public License for more details.
>>> +# 
>>> +# You should have received a copy of the GNU General Public License 
>>> +# along with this program.  If not, see <
>>> http://www.gnu.org/licenses/>.
>>> +# 
>>> +####################################################################
>>> ###########
>>> +
>>> +use strict;
>>> +
>>> +# enable only the following on debugging purpose
>>> +# use warnings;
>>> +
>>> +# Maps a nice printable name to the changing part of the pid file,
>>> which
>>> +# is also the name of the program
>>> +my %servicenames =(
>>> +        'DHCP Server' => 'dhcpd',
>>> +        'Web Server' => 'httpd',
>>> +        'CRON Server' => 'fcron',
>>> +        'DNS Proxy Server' => 'unbound',
>>> +        'Logging Server' => 'syslogd',
>>> +        'Kernel Logging Server' => 'klogd',
>>> +        'NTP Server' => 'ntpd',
>>> +        'Secure Shell Server' => 'sshd',
>>> +        'VPN' => 'charon',
>>> +        'Web Proxy' => 'squid',
>>> +        'Intrusion Detection System' => 'suricata',
>>> +        'OpenVPN' => 'openvpn'
>>> +);
>> 
>> You have a hard-coded list with process names here. It technically is
>> missing add-ons but including everything would make this list very long
>> and a lot of work to maintain...
>> 
> As explained in my previous mail, I don't currently know of a method to
> get a list of mandatory IPFire specific services. So this list is a
> straight copy out of services.cgi minus the translation.
> The add-on services are looked up below, exactly like it is done in
> services.cgi.

What list are you looking for? Just all processes that are running?

>>> +
>>> +# Hash to overwrite the process name of a process if it differs from
>>> the launch command.
>>> +my %overwrite_exename_hash = (
>>> +        "suricata" => "Suricata-Main"
>>> +);
>>> +
>>> +my $first = 1;
>>> +
>>> +print "[";
>>> +
>>> +# Built-in services
>>> +my $key = '';
>>> +foreach $key (sort keys %servicenames){
>>> +       print "," if not $first;
>>> +       $first = 0;
>>> +
>>> +       print "{";
>>> +       print "\"service\":\"$key\",";
>>> +
>>> +       my $shortname = $servicenames{$key};
>>> +       print &servicestats($shortname);
>>> +       
>>> +       print "}";
>>> +}
>>> +
>>> +# Generate list of installed addon pak's
>>> +my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null | cut -
>>> d"-" -f2`;
>>> +foreach (@pak){
>>> +       chomp($_);
>>> +
>>> +       # Check which of the paks are services
>>> +       my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/" -f4`;
>>> +       foreach (@svc){
>>> +               # blacklist some packages
>>> +               #
>>> +               # alsa has trouble with the volume saving and was not
>>> really stopped
>>> +               # mdadm should not stopped with webif because this
>>> could crash the system
>>> +               #
>>> +               chomp($_);
>>> +               if ( $_ eq 'squid' ) {
>>> +                       next;
>>> +               }
>>> +               if ( ($_ ne "alsa") && ($_ ne "mdadm") ) {
>> 
>> Another hardcoded list due to code duplication. Not your fault, but not
>> pretty.
>> 
> 
> Indeed, as is probably clear by now, I too would prefer a more clean
> way of generating a list of services.
> 
> Currently seeing this piece of code again.. I think this blacklist in
> services.cgi only exists so the user can't stop/start the service using
> the webgui.. But there is no such functionality in this script.. so I
> think I can skip this blacklist, and also provide service state of alsa
> and mdadm.. (not sure why squid is here, is there an addon that also
> provides squid, maybe squid-accounting?)

The code seemed to have some trouble with hyphenated package names, that is why those checks were introduced.

It is pretty bad code and we should rework it some time, but definitely not copy it if we can avoid it.

>>> +                       print ",";
>>> +                       print "{";
>>> +
>>> +                       print "\"service\":\"Addon: $_\",";
>>> +                       print "\"servicename\":\"$_\",";
>>> +
>>> +                       my $onboot = isautorun($_);
>>> +                       print "\"onboot\":$onboot,";
>>> +
>>> +                       print &addonservicestats($_);
>>> +
>>> +                       print "}";
>>> +               }
>>> +       }
>>> +}      
>>> +
>>> +print "]";
>>> +
>>> +sub servicestats{
>>> +       my $cmd = $_[0];
>>> +       my $status = "\"servicename\":\"$cmd\",\"state\":\"0\"";
>>> +       my $pid = '';
>>> +       my $testcmd = '';
>>> +        my $exename;
>>> +        my $memory;
>>> +
>>> +
>>> +       $cmd =~ /(^[a-z]+)/;
>>> +       
>>> +       # Check if the exename needs to be overwritten.
>>> +        # This happens if the expected process name string
>>> +        # differs from the real one. This may happened if
>>> +        # a service uses multiple processes or threads.
>>> +        if (exists($overwrite_exename_hash{$cmd})) {
>>> +                # Grab the string which will be reported by
>>> +                # the process from the corresponding hash.
>>> +                $exename = $overwrite_exename_hash{$1};
>>> +        } else {
>>> +                # Directly expect the launched command as
>>> +                # process name.
>>> +                $exename = $1;
>>> +        }
>>> +       
>>> +       if (open(FILE, "/var/run/${cmd}.pid")){
>>> +                $pid = <FILE>; chomp $pid;
>>> +                close FILE;
>>> +                if (open(FILE, "/proc/${pid}/status")){
>>> +                        while (<FILE>){
>>> +                                if (/^Name:\W+(.*)/) {
>>> +                                        $testcmd = $1;
>>> +                                }
>>> +                        }
>>> +                        close FILE;
>>> +                }
>>> +                if (open(FILE, "/proc/${pid}/status")) {
>>> +                        while (<FILE>) {
>>> +                                my ($key, $val) = split(":", $_, 2);
>>> +                                if ($key eq 'VmRSS') {
>>> +                                       $val =~ /\s*([0-9]*)\s+kB/;
>>> +                                       # Convert kB to B
>>> +                                        $memory = $1*1024;
>>> +                                        last;
>>> +                                }
>>> +                        }
>>> +                        close(FILE);
>>> +                }
>>> +                if ($testcmd =~ /$exename/){
>>> +                       $status =
>>> "\"servicename\":\"$cmd\",\"state\":1,\"pid\":$pid,\"memory\":$memory
>>> ";
>>> +               }
>>> +        }
>>> +        return $status;
>>> +}
>>> +
>>> +sub isautorun{
>>> +        my $cmd = $_[0];
>>> +        my $status = "0";
>>> +        my $init = `find /etc/rc.d/rc3.d/S??${cmd} 2>/dev/null`;
>>> +        chomp ($init);
>>> +        if ($init ne ''){
>>> +                $status = "1";
>>> +        }
>>> +        $init = `find /etc/rc.d/rc3.d/off/S??${cmd} 2>/dev/null`;
>>> +        chomp ($init);
>>> +        if ($init ne ''){
>>> +                $status = "0";
>>> +        }
>>> +
>>> +        return $status;
>>> +}
>>> +
>>> +sub addonservicestats{
>>> +        my $cmd = $_[0];
>>> +        my $status = "0";
>>> +        my $pid = '';
>>> +        my $testcmd = '';
>>> +        my $exename;
>>> +        my @memory = (0);
>>> +
>>> +        $testcmd = `sudo /usr/local/bin/addonctrl $_ status
>>> 2>/dev/null`;
>>> +
>>> +        if ( $testcmd =~ /is\ running/ && $testcmd !~ /is\ not\
>>> running/){
>>> +                $status = "\"state\":1";
>>> +
>>> +                $testcmd =~ s/.* //gi;
>>> +                $testcmd =~ s/[a-z_]//gi;
>>> +                $testcmd =~ s/\[[0-1]\;[0-9]+//gi;
>>> +                $testcmd =~ s/[\(\)\.]//gi;
>>> +                $testcmd =~ s/  //gi;
>>> +                $testcmd =~ s///gi;
>>> +
>>> +                my @pid = split(/\s/,$testcmd);
>>> +                $status .=",\"pid\":\"$pid[0]\"";
>>> +
>>> +                my $memory = 0;
>>> +
>>> +                foreach (@pid){
>>> +                        chomp($_);
>>> +                        if (open(FILE, "/proc/$_/statm")){
>>> +                                my $temp = <FILE>;
>>> +                                @memory = split(/ /,$temp);
>>> +                        }
>>> +                        $memory+=$memory[0];
>>> +                }
>>> +               $memory*=1024;
>>> +                $status .=",\"memory\":$memory";
>>> +        }else{
>>> +                $status = "\"state\":0";
>>> +        }
>>> +        return $status;
>>> +} 
>>> diff --git a/config/zabbix_agentd/sudoers
>>> b/config/zabbix_agentd/sudoers
>>> index 1b362a4fd..340bb8e66 100644
>>> --- a/config/zabbix_agentd/sudoers
>>> +++ b/config/zabbix_agentd/sudoers
>>> @@ -14,4 +14,4 @@
>>> # Append / edit the following list of commands to fit your needs:
>>> #
>>> Defaults:zabbix !requiretty
>>> -zabbix ALL=(ALL) NOPASSWD: /opt/pakfire/pakfire status
>>> +zabbix ALL=(ALL) NOPASSWD: /opt/pakfire/pakfire status,
>>> /usr/local/bin/addonctrl, /sbin/iptables, /usr/sbin/fping
>> 
>> This is absolutely impossible to give the zabbix user control to
>> start/stop any add-on and to read and change the entire iptables
>> ruleset.
>> 
>> Zabbix is listening on the network and does not have any kind of
>> authentication. Any security vulnerabilities in Zabbix would allow to
>> alter the firewall rules and DoS any add-ons. This is dangerous.
> There is some level of security: 

I agree that it is *some*, but is it enough?

> - the agent will only respond on requests from the configured Zabbix
> server (which the user has to configure in the zabbix agentd conf-file,
> and defaults to 127.0.0.1 as we can't possibly know the ip of the
> user's server, so by default the agent responds to no one)

Host-based ACLs are not really a thing. On the local network faking a source IP address is one of the easiest jobs.

> - By default remote commands (as in requests to the agent to execute
> any command given) are disabled so the only way to make the agent call
> those binaries is to request a predefined userparameter that in turn
> will execute such a command with predefined params (see the additional
> template_...conf-files)
> and on top of that, the user can add additional security by configuring
> encryption in the agent main configfile

Encryption is not authentication.

And the predefined parameters might be a thing, but my view is that zabbix can execute quite serious commands if it is being exploited.

The Zabbix Agent had exactly one of these vulnerabilities in 2009 where an attacker could execute arbitrary commands:

  https://www.cvedetails.com/cve/CVE-2009-4502/

> But as you point out, a possible vulnerability of some sort may indeed
> still pose risks this way.

I just find this risk insanely large. I am not going to tell people what software they can use and what not, but I want to avoid that the default configuration is adding major security issues to the entire system. And adding those sudo rules by default does exactly this.

What would a monitoring tool do with a dump of the iptables ruleset? I do not even understand where the use of this is.

>> Are there any alternative ways to realise this functionality?
> As sudo is not able to allow commands with variable parameters together
> with fixed parameters, a popular solution is using wrapper scripts, not
> writable by the user, denying the user direct access to the commands in
> question.
> 
> /usr/local/bin/addonctrl is only called from within the
> ipfire_services.pl script I provide, in the form of "addonctrl
> <service> status". 
> So I could add /etc/zabbix_agentd/scripts/ipfire_services.pl to the
> sudo list instead so that the zabbix user won't be able to call
> addonctrl directly. (making sure zabbix user has no write permission on
> that script)

> The same goes for iptables. This is only called by the predefined
> userparameter "ipfire.net.fw.hits[*]" defined in
> template_module_ipfire_network_stats.conf in the form "/sbin/iptables -
> vnxL $1 | grep "\/\* $2 \*\/" | awk '{ print $$2 }';" where $1 (chain)
> and $2 (rule) are parameters that have to be supplied to the agent when
> requesting that item. 
> Here I can also supply a wrapper-script to be called  by sudo
> /etc/zabbix_agentd/scripts/firewall_hits.sh that accepts those
> parameters, and checks them for validity and against abuse.
> 
> Regards
> Robin

-Michael

>> 
>>> diff --git
>>> a/config/zabbix_agentd/template_module_ipfire_network_stats.conf
>>> b/config/zabbix_agentd/template_module_ipfire_network_stats.conf
>>> new file mode 100644
>>> index 000000000..f1658ed07
>>> --- /dev/null
>>> +++ b/config/zabbix_agentd/template_module_ipfire_network_stats.conf
>>> @@ -0,0 +1,4 @@
>>> +### Parameters for monitoring IPFire network statistics 
>>> +UserParameter=ipfire.net.gateway.pingtime,sudo /usr/sbin/fping -c 3
>>> gateway 2>&1 | tail -n 1 | awk '{print $NF}' | cut -d '/' -f2
>>> +UserParameter=ipfire.net.gateway.ping,sudo /usr/sbin/fping -q -r 3
>>> gateway; [ ! $? ]; echo $?
>>> +UserParameter=ipfire.net.fw.hits[*],sudo /sbin/iptables -vnxL $1 |
>>> grep "\/\* $2 \*\/" | awk '{ print $$2 }';
>>> diff --git
>>> a/config/zabbix_agentd/template_module_ipfire_services.conf
>>> b/config/zabbix_agentd/template_module_ipfire_services.conf
>>> new file mode 100644
>>> index 000000000..5f95218e3
>>> --- /dev/null
>>> +++ b/config/zabbix_agentd/template_module_ipfire_services.conf
>>> @@ -0,0 +1,2 @@
>>> +### Parameter for monitoring IPFire services
>>> +UserParameter=ipfire.services,/etc/zabbix_agentd/scripts/ipfire_serv
>>> ices.pl
>>> diff --git a/lfs/zabbix_agentd b/lfs/zabbix_agentd
>>> index 73e08d20a..c0d28d51f 100644
>>> --- a/lfs/zabbix_agentd
>>> +++ b/lfs/zabbix_agentd
>>> @@ -33,7 +33,7 @@ DIR_APP    = $(DIR_SRC)/$(THISAPP)
>>> TARGET     = $(DIR_INFO)/$(THISAPP)
>>> PROG       = zabbix_agentd
>>> PAK_VER    = 5
>>> -DEPS       =
>>> +DEPS       = "fping"
>>> 
>>> #####################################################################
>>> ##########
>>> # Top-level Rules
>>> @@ -97,6 +97,12 @@ $(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects))
>>>                 /etc/zabbix_agentd/zabbix_agentd.conf.ipfirenew
>>>         install -v -m 644
>>> $(DIR_SRC)/config/zabbix_agentd/template_app_pakfire.conf \
>>>                 /etc/zabbix_agentd/zabbix_agentd.d/template_app_pakfi
>>> re.conf.ipfirenew
>>> +       install -v -m 644
>>> $(DIR_SRC)/config/zabbix_agentd/template_module_ipfire_network_stats.
>>> conf \
>>> +               /etc/zabbix_agentd/zabbix_agentd.d/template_module_ip
>>> fire_network_stats.conf.ipfirenew
>>> +       install -v -m 644
>>> $(DIR_SRC)/config/zabbix_agentd/template_module_ipfire_services.conf
>>> \
>>> +               /etc/zabbix_agentd/zabbix_agentd.d/template_module_ip
>>> fire_services.conf.ipfirenew
>>> +       install -v -m 755
>>> $(DIR_SRC)/config/zabbix_agentd/ipfire_services.pl \
>>> +               /etc/zabbix_agentd/scripts/ipfire_services.pl.ipfiren
>>> ew
>>> 
>>>         # Create directory for additional agent modules
>>>         -mkdir -pv /usr/lib/zabbix
>>> diff --git a/src/paks/zabbix_agentd/install.sh
>>> b/src/paks/zabbix_agentd/install.sh
>>> index 4248a7ec1..ced915c81 100644
>>> --- a/src/paks/zabbix_agentd/install.sh
>>> +++ b/src/paks/zabbix_agentd/install.sh
>>> @@ -66,8 +66,13 @@ restore_backup ${NAME}
>>> # Put zabbix configfiles in place
>>> setup_configfile /etc/zabbix_agentd/zabbix_agentd.conf
>>> setup_configfile
>>> /etc/zabbix_agentd/zabbix_agentd.d/template_app_pakfire.conf
>>> +setup_configfile
>>> /etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_network_sta
>>> ts.conf
>>> +setup_configfile
>>> /etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_services.co
>>> nf
>>> setup_configfile /etc/sudoers.d/zabbix
>>> 
>>> +# Overwrite script if it exists as user should not modify it but it
>>> is included in backup
>>> +mv /etc/zabbix_agentd/scripts/ipfire_services.pl.ipfirenew
>>> /etc/zabbix_agentd/scripts/ipfire_services.pl
>>> +
>>> if $review_required; then
>>>         echo "WARNING: New versions of some configfile(s) where
>>> provided as .ipfirenew-files."
>>>         echo "         They may need manual review in order to take
>>> advantage of new features"
>>> diff --git a/src/paks/zabbix_agentd/uninstall.sh
>>> b/src/paks/zabbix_agentd/uninstall.sh
>>> index 7a13880c5..ccbc8f7cf 100644
>>> --- a/src/paks/zabbix_agentd/uninstall.sh
>>> +++ b/src/paks/zabbix_agentd/uninstall.sh
>>> @@ -26,6 +26,8 @@ stop_service ${NAME}
>>> 
>>> # Remove .ipfirenew files in advance so they won't be included in
>>> backup
>>> rm -rfv /etc/zabbix_agentd/*.ipfirenew
>>> /etc/zabbix_agentd/*/*.ipfirenew
>>> +# Remove script-file as it should not have been modified by user
>>> +rm -fv /etc/zabbix_agentd/scripts/ipfire_services.pl
>>> 
>>> make_backup ${NAME}
>>> remove_files
>>> -- 
>>> 2.30.2
>>> 
>>> 
>>> -- 
>>> Dit bericht is gescanned op virussen en andere gevaarlijke
>>> inhoud door MailScanner en lijkt schoon te zijn.
>>> 
>> 
>> -Michael
>> 
>> 
> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
  

Hi Michael


Michael Tremer schreef op do 15-04-2021 om 12:21 [+0100]:
> Hello,
> 
> > On 12 Apr 2021, at 23:16, Robin Roevens <
> > robin.roevens@disroot.org
> > > wrote:
> > 
> > Hi Michael
> > 
> > 
> > Michael Tremer schreef op ma 12-04-2021 om 11:36 [+0100]:
> > > Hello,
> > > 
> > > > On 7 Apr 2021, at 21:44, Robin Roevens <
> > > > robin.roevens@disroot.org
> > > > >
> > > > wrote:
> > > > ...
> > > > ...
> > > > diff --git a/config/zabbix_agentd/ipfire_services.pl
> > > > b/config/zabbix_agentd/ipfire_services.pl
> > > > new file mode 100755
> > > > index 000000000..dbf8aec56
> > > > --- /dev/null
> > > > +++ b/config/zabbix_agentd/ipfire_services.pl
> > > > @@ -0,0 +1,221 @@
> > > > +#!/usr/bin/perl
> > > > +##############################################################
> > > > ######
> > > > ###########
> > > > +# ipfire_services.pl - Retrieves available IPFire services
> > > > information and 
> > > > +#                      return this as a JSON array suitable
> > > > for easy
> > > > processing 
> > > > +#                      by Zabbix server
> > > > +#
> > > > +# Author: robin.roevens (at) disroot.org
> > > > +# Version: 1.0
> > > > +#
> > > > +# Based on: services.cgi by IPFire Team
> > > > +# Copyright (C) 2007-2021  IPFire Team  <
> > > > info@ipfire.org
> > > > > 
> > > > +# 
> > > > +# This program is free software: you can redistribute it
> > > > and/or
> > > > modify
> > > > +# it under the terms of the GNU General Public License as
> > > > published
> > > > by
> > > > +# the Free Software Foundation, either version 3 of the
> > > > License, or
> > > > +# (at your option) any later version.
> > > > +# 
> > > > +# This program is distributed in the hope that it will be
> > > > useful,
> > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty
> > > > of
> > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> > > > the 
> > > > +# GNU General Public License for more details.
> > > > +# 
> > > > +# You should have received a copy of the GNU General Public
> > > > License 
> > > > +# along with this program.  If not, see <
> > > > http://www.gnu.org/licenses/
> > > > >.
> > > > +# 
> > > > +##############################################################
> > > > ######
> > > > ###########
> > > > +
> > > > +use strict;
> > > > +
> > > > +# enable only the following on debugging purpose
> > > > +# use warnings;
> > > > +
> > > > +# Maps a nice printable name to the changing part of the pid
> > > > file,
> > > > which
> > > > +# is also the name of the program
> > > > +my %servicenames =(
> > > > +        'DHCP Server' => 'dhcpd',
> > > > +        'Web Server' => 'httpd',
> > > > +        'CRON Server' => 'fcron',
> > > > +        'DNS Proxy Server' => 'unbound',
> > > > +        'Logging Server' => 'syslogd',
> > > > +        'Kernel Logging Server' => 'klogd',
> > > > +        'NTP Server' => 'ntpd',
> > > > +        'Secure Shell Server' => 'sshd',
> > > > +        'VPN' => 'charon',
> > > > +        'Web Proxy' => 'squid',
> > > > +        'Intrusion Detection System' => 'suricata',
> > > > +        'OpenVPN' => 'openvpn'
> > > > +);
> > > 
> > > You have a hard-coded list with process names here. It
> > > technically is
> > > missing add-ons but including everything would make this list
> > > very long
> > > and a lot of work to maintain...
> > > 
> > 
> > As explained in my previous mail, I don't currently know of a
> > method to
> > get a list of mandatory IPFire specific services. So this list is a
> > straight copy out of services.cgi minus the translation.
> > The add-on services are looked up below, exactly like it is done in
> > services.cgi.
> 
> What list are you looking for? Just all processes that are running?

The purpose of my extension to the default is that a user can easily
configure Zabbix to monitor "IPFire"-specific functionalities. Generic
Linux metrics can be monitored by default Zabbix templates and agent
built-in functionality eventually extended with user-defined scripts or
modules. 

The IPFire webgui has a nice overview of all vital IPFire services and
their state, as well as for addon-services. 
A user can browse the status pages of the WebGUI to have a view of how
their IPFire machine is running
This is exactly what I want to provide to the Zabbix user, hence my
copy of services.cgi-code.
Alternatively, I could 'parse' the output of the services-page of the
webserver. But that is rather something one would do when trying to
monitor a black-box. 


> > > > +
> > > > +# Hash to overwrite the process name of a process if it
> > > > differs from
> > > > the launch command.
> > > > +my %overwrite_exename_hash = (
> > > > +        "suricata" => "Suricata-Main"
> > > > +);
> > > > +
> > > > +my $first = 1;
> > > > +
> > > > +print "[";
> > > > +
> > > > +# Built-in services
> > > > +my $key = '';
> > > > +foreach $key (sort keys %servicenames){
> > > > +       print "," if not $first;
> > > > +       $first = 0;
> > > > +
> > > > +       print "{";
> > > > +       print "\"service\":\"$key\",";
> > > > +
> > > > +       my $shortname = $servicenames{$key};
> > > > +       print &servicestats($shortname);
> > > > +       
> > > > +       print "}";
> > > > +}
> > > > +
> > > > +# Generate list of installed addon pak's
> > > > +my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null |
> > > > cut -
> > > > d"-" -f2`;
> > > > +foreach (@pak){
> > > > +       chomp($_);
> > > > +
> > > > +       # Check which of the paks are services
> > > > +       my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/"
> > > > -f4`;
> > > > +       foreach (@svc){
> > > > +               # blacklist some packages
> > > > +               #
> > > > +               # alsa has trouble with the volume saving and
> > > > was not
> > > > really stopped
> > > > +               # mdadm should not stopped with webif because
> > > > this
> > > > could crash the system
> > > > +               #
> > > > +               chomp($_);
> > > > +               if ( $_ eq 'squid' ) {
> > > > +                       next;
> > > > +               }
> > > > +               if ( ($_ ne "alsa") && ($_ ne "mdadm") ) {
> > > 
> > > Another hardcoded list due to code duplication. Not your fault,
> > > but not
> > > pretty.
> > > 
> > 
> > Indeed, as is probably clear by now, I too would prefer a more
> > clean
> > way of generating a list of services.
> > 
> > Currently seeing this piece of code again.. I think this blacklist
> > in
> > services.cgi only exists so the user can't stop/start the service
> > using
> > the webgui.. But there is no such functionality in this script.. so
> > I
> > think I can skip this blacklist, and also provide service state of
> > alsa
> > and mdadm.. (not sure why squid is here, is there an addon that
> > also
> > provides squid, maybe squid-accounting?)
> 
> The code seemed to have some trouble with hyphenated package names,
> that is why those checks were introduced.
> 
> It is pretty bad code and we should rework it some time, but
> definitely not copy it if we can avoid it.
> 
> > > > +                       print ",";
> > > > +                       print "{";
> > > > +
> > > > +                       print "\"service\":\"Addon: $_\",";
> > > > +                       print "\"servicename\":\"$_\",";
> > > > +
> > > > +                       my $onboot = isautorun($_);
> > > > +                       print "\"onboot\":$onboot,";
> > > > +
> > > > +                       print &addonservicestats($_);
> > > > +
> > > > +                       print "}";
> > > > +               }
> > > > +       }
> > > > +}      
> > > > +
> > > > +print "]";
> > > > +
> > > > +sub servicestats{
> > > > +       my $cmd = $_[0];
> > > > +       my $status =
> > > > "\"servicename\":\"$cmd\",\"state\":\"0\"";
> > > > +       my $pid = '';
> > > > +       my $testcmd = '';
> > > > +        my $exename;
> > > > +        my $memory;
> > > > +
> > > > +
> > > > +       $cmd =~ /(^[a-z]+)/;
> > > > +       
> > > > +       # Check if the exename needs to be overwritten.
> > > > +        # This happens if the expected process name string
> > > > +        # differs from the real one. This may happened if
> > > > +        # a service uses multiple processes or threads.
> > > > +        if (exists($overwrite_exename_hash{$cmd})) {
> > > > +                # Grab the string which will be reported by
> > > > +                # the process from the corresponding hash.
> > > > +                $exename = $overwrite_exename_hash{$1};
> > > > +        } else {
> > > > +                # Directly expect the launched command as
> > > > +                # process name.
> > > > +                $exename = $1;
> > > > +        }
> > > > +       
> > > > +       if (open(FILE, "/var/run/${cmd}.pid")){
> > > > +                $pid = <FILE>; chomp $pid;
> > > > +                close FILE;
> > > > +                if (open(FILE, "/proc/${pid}/status")){
> > > > +                        while (<FILE>){
> > > > +                                if (/^Name:\W+(.*)/) {
> > > > +                                        $testcmd = $1;
> > > > +                                }
> > > > +                        }
> > > > +                        close FILE;
> > > > +                }
> > > > +                if (open(FILE, "/proc/${pid}/status")) {
> > > > +                        while (<FILE>) {
> > > > +                                my ($key, $val) = split(":",
> > > > $_, 2);
> > > > +                                if ($key eq 'VmRSS') {
> > > > +                                       $val =~ /\s*([0-
> > > > 9]*)\s+kB/;
> > > > +                                       # Convert kB to B
> > > > +                                        $memory = $1*1024;
> > > > +                                        last;
> > > > +                                }
> > > > +                        }
> > > > +                        close(FILE);
> > > > +                }
> > > > +                if ($testcmd =~ /$exename/){
> > > > +                       $status =
> > > > "\"servicename\":\"$cmd\",\"state\":1,\"pid\":$pid,\"memory\":$
> > > > memory
> > > > ";
> > > > +               }
> > > > +        }
> > > > +        return $status;
> > > > +}
> > > > +
> > > > +sub isautorun{
> > > > +        my $cmd = $_[0];
> > > > +        my $status = "0";
> > > > +        my $init = `find /etc/rc.d/rc3.d/S??${cmd}
> > > > 2>/dev/null`;
> > > > +        chomp ($init);
> > > > +        if ($init ne ''){
> > > > +                $status = "1";
> > > > +        }
> > > > +        $init = `find /etc/rc.d/rc3.d/off/S??${cmd}
> > > > 2>/dev/null`;
> > > > +        chomp ($init);
> > > > +        if ($init ne ''){
> > > > +                $status = "0";
> > > > +        }
> > > > +
> > > > +        return $status;
> > > > +}
> > > > +
> > > > +sub addonservicestats{
> > > > +        my $cmd = $_[0];
> > > > +        my $status = "0";
> > > > +        my $pid = '';
> > > > +        my $testcmd = '';
> > > > +        my $exename;
> > > > +        my @memory = (0);
> > > > +
> > > > +        $testcmd = `sudo /usr/local/bin/addonctrl $_ status
> > > > 2>/dev/null`;
> > > > +
> > > > +        if ( $testcmd =~ /is\ running/ && $testcmd !~ /is\
> > > > not\
> > > > running/){
> > > > +                $status = "\"state\":1";
> > > > +
> > > > +                $testcmd =~ s/.* //gi;
> > > > +                $testcmd =~ s/[a-z_]//gi;
> > > > +                $testcmd =~ s/\[[0-1]\;[0-9]+//gi;
> > > > +                $testcmd =~ s/[\(\)\.]//gi;
> > > > +                $testcmd =~ s/  //gi;
> > > > +                $testcmd =~ s///gi;
> > > > +
> > > > +                my @pid = split(/\s/,$testcmd);
> > > > +                $status .=",\"pid\":\"$pid[0]\"";
> > > > +
> > > > +                my $memory = 0;
> > > > +
> > > > +                foreach (@pid){
> > > > +                        chomp($_);
> > > > +                        if (open(FILE, "/proc/$_/statm")){
> > > > +                                my $temp = <FILE>;
> > > > +                                @memory = split(/ /,$temp);
> > > > +                        }
> > > > +                        $memory+=$memory[0];
> > > > +                }
> > > > +               $memory*=1024;
> > > > +                $status .=",\"memory\":$memory";
> > > > +        }else{
> > > > +                $status = "\"state\":0";
> > > > +        }
> > > > +        return $status;
> > > > +} 
> > > > diff --git a/config/zabbix_agentd/sudoers
> > > > b/config/zabbix_agentd/sudoers
> > > > index 1b362a4fd..340bb8e66 100644
> > > > --- a/config/zabbix_agentd/sudoers
> > > > +++ b/config/zabbix_agentd/sudoers
> > > > @@ -14,4 +14,4 @@
> > > > # Append / edit the following list of commands to fit your
> > > > needs:
> > > > #
> > > > Defaults:zabbix !requiretty
> > > > -zabbix ALL=(ALL) NOPASSWD: /opt/pakfire/pakfire status
> > > > +zabbix ALL=(ALL) NOPASSWD: /opt/pakfire/pakfire status,
> > > > /usr/local/bin/addonctrl, /sbin/iptables, /usr/sbin/fping
> > > 
> > > This is absolutely impossible to give the zabbix user control to
> > > start/stop any add-on and to read and change the entire iptables
> > > ruleset.
> > > 
> > > Zabbix is listening on the network and does not have any kind of
> > > authentication. Any security vulnerabilities in Zabbix would
> > > allow to
> > > alter the firewall rules and DoS any add-ons. This is dangerous.
> > 
> > There is some level of security: 
> 
> I agree that it is *some*, but is it enough?
> 
> > - the agent will only respond on requests from the configured
> > Zabbix
> > server (which the user has to configure in the zabbix agentd conf-
> > file,
> > and defaults to 127.0.0.1 as we can't possibly know the ip of the
> > user's server, so by default the agent responds to no one)
> 
> Host-based ACLs are not really a thing. On the local network faking a
> source IP address is one of the easiest jobs.
> 
> > - By default remote commands (as in requests to the agent to
> > execute
> > any command given) are disabled so the only way to make the agent
> > call
> > those binaries is to request a predefined userparameter that in
> > turn
> > will execute such a command with predefined params (see the
> > additional
> > template_...conf-files)
> > and on top of that, the user can add additional security by
> > configuring
> > encryption in the agent main configfile
> 
> Encryption is not authentication.
> 
> And the predefined parameters might be a thing, but my view is that
> zabbix can execute quite serious commands if it is being exploited.
> 
> The Zabbix Agent had exactly one of these vulnerabilities in 2009
> where an attacker could execute arbitrary commands:
> 
>   
> https://www.cvedetails.com/cve/CVE-2009-4502/
> 
> 
> > But as you point out, a possible vulnerability of some sort may
> > indeed
> > still pose risks this way.
> 
> I just find this risk insanely large. I am not going to tell people
> what software they can use and what not, but I want to avoid that the
> default configuration is adding major security issues to the entire
> system. And adding those sudo rules by default does exactly this.

I totally agree, I was only pointing out the *some* security there is,
but that will indeed never be sufficient to allow full access to
iptables or addonctrl by default. 
A user could, on his own risk, do that to maybe have the agent execute
firewall-actions as a reaction to a detected network attack or so.. But
that is not something we should provide by default.

> 
> What would a monitoring tool do with a dump of the iptables ruleset?
> I do not even understand where the use of this is.

The same as IPFire webgui currently does on netother.cgi?fwhits?day
by calling /sbin/iptables -vnxL <chain> | grep "\/\* <rule> \*\/" | awk
'{ print $2 }';: keep history of and eventually react on the number of
firewall-hits.

But you are completely right that access to iptables should be
restricted to just that what we want to get from it, which can be
achieved with a wrapper script as explained below in my previous mail.

Regards

Robin

> 
> > > Are there any alternative ways to realise this functionality?
> > 
> > As sudo is not able to allow commands with variable parameters
> > together
> > with fixed parameters, a popular solution is using wrapper scripts,
> > not
> > writable by the user, denying the user direct access to the
> > commands in
> > question.
> > 
> > /usr/local/bin/addonctrl is only called from within the
> > ipfire_services.pl script I provide, in the form of "addonctrl
> > <service> status". 
> > So I could add /etc/zabbix_agentd/scripts/ipfire_services.pl to the
> > sudo list instead so that the zabbix user won't be able to call
> > addonctrl directly. (making sure zabbix user has no write
> > permission on
> > that script)
> > The same goes for iptables. This is only called by the predefined
> > userparameter "ipfire.net.fw.hits[*]" defined in
> > template_module_ipfire_network_stats.conf in the form
> > "/sbin/iptables -
> > vnxL $1 | grep "\/\* $2 \*\/" | awk '{ print $$2 }';" where $1
> > (chain)
> > and $2 (rule) are parameters that have to be supplied to the agent
> > when
> > requesting that item. 
> > Here I can also supply a wrapper-script to be called  by sudo
> > /etc/zabbix_agentd/scripts/firewall_hits.sh that accepts those
> > parameters, and checks them for validity and against abuse.
> > 
> > Regards
> > Robin
> 
> -Michael
> 
> > > > diff --git
> > > > a/config/zabbix_agentd/template_module_ipfire_network_stats.con
> > > > f
> > > > b/config/zabbix_agentd/template_module_ipfire_network_stats.con
> > > > f
> > > > new file mode 100644
> > > > index 000000000..f1658ed07
> > > > --- /dev/null
> > > > +++
> > > > b/config/zabbix_agentd/template_module_ipfire_network_stats.con
> > > > f
> > > > @@ -0,0 +1,4 @@
> > > > +### Parameters for monitoring IPFire network statistics 
> > > > +UserParameter=ipfire.net.gateway.pingtime,sudo /usr/sbin/fping
> > > > -c 3
> > > > gateway 2>&1 | tail -n 1 | awk '{print $NF}' | cut -d '/' -f2
> > > > +UserParameter=ipfire.net.gateway.ping,sudo /usr/sbin/fping -q
> > > > -r 3
> > > > gateway; [ ! $? ]; echo $?
> > > > +UserParameter=ipfire.net.fw.hits[*],sudo /sbin/iptables -vnxL
> > > > $1 |
> > > > grep "\/\* $2 \*\/" | awk '{ print $$2 }';
> > > > diff --git
> > > > a/config/zabbix_agentd/template_module_ipfire_services.conf
> > > > b/config/zabbix_agentd/template_module_ipfire_services.conf
> > > > new file mode 100644
> > > > index 000000000..5f95218e3
> > > > --- /dev/null
> > > > +++ b/config/zabbix_agentd/template_module_ipfire_services.conf
> > > > @@ -0,0 +1,2 @@
> > > > +### Parameter for monitoring IPFire services
> > > > +UserParameter=ipfire.services,/etc/zabbix_agentd/scripts/ipfir
> > > > e_serv
> > > > ices.pl
> > > > diff --git a/lfs/zabbix_agentd b/lfs/zabbix_agentd
> > > > index 73e08d20a..c0d28d51f 100644
> > > > --- a/lfs/zabbix_agentd
> > > > +++ b/lfs/zabbix_agentd
> > > > @@ -33,7 +33,7 @@ DIR_APP    = $(DIR_SRC)/$(THISAPP)
> > > > TARGET     = $(DIR_INFO)/$(THISAPP)
> > > > PROG       = zabbix_agentd
> > > > PAK_VER    = 5
> > > > -DEPS       =
> > > > +DEPS       = "fping"
> > > > 
> > > > ###############################################################
> > > > ######
> > > > ##########
> > > > # Top-level Rules
> > > > @@ -97,6 +97,12 @@ $(TARGET) : $(patsubst
> > > > %,$(DIR_DL)/%,$(objects))
> > > >                 /etc/zabbix_agentd/zabbix_agentd.conf.ipfirenew
> > > >         install -v -m 644
> > > > $(DIR_SRC)/config/zabbix_agentd/template_app_pakfire.conf \
> > > >                 /etc/zabbix_agentd/zabbix_agentd.d/template_app
> > > > _pakfi
> > > > re.conf.ipfirenew
> > > > +       install -v -m 644
> > > > $(DIR_SRC)/config/zabbix_agentd/template_module_ipfire_network_
> > > > stats.
> > > > conf \
> > > > +               /etc/zabbix_agentd/zabbix_agentd.d/template_mod
> > > > ule_ip
> > > > fire_network_stats.conf.ipfirenew
> > > > +       install -v -m 644
> > > > $(DIR_SRC)/config/zabbix_agentd/template_module_ipfire_services
> > > > .conf
> > > > \
> > > > +               /etc/zabbix_agentd/zabbix_agentd.d/template_mod
> > > > ule_ip
> > > > fire_services.conf.ipfirenew
> > > > +       install -v -m 755
> > > > $(DIR_SRC)/config/zabbix_agentd/ipfire_services.pl \
> > > > +               /etc/zabbix_agentd/scripts/ipfire_services.pl.i
> > > > pfiren
> > > > ew
> > > > 
> > > >         # Create directory for additional agent modules
> > > >         -mkdir -pv /usr/lib/zabbix
> > > > diff --git a/src/paks/zabbix_agentd/install.sh
> > > > b/src/paks/zabbix_agentd/install.sh
> > > > index 4248a7ec1..ced915c81 100644
> > > > --- a/src/paks/zabbix_agentd/install.sh
> > > > +++ b/src/paks/zabbix_agentd/install.sh
> > > > @@ -66,8 +66,13 @@ restore_backup ${NAME}
> > > > # Put zabbix configfiles in place
> > > > setup_configfile /etc/zabbix_agentd/zabbix_agentd.conf
> > > > setup_configfile
> > > > /etc/zabbix_agentd/zabbix_agentd.d/template_app_pakfire.conf
> > > > +setup_configfile
> > > > /etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_netwo
> > > > rk_sta
> > > > ts.conf
> > > > +setup_configfile
> > > > /etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_servi
> > > > ces.co
> > > > nf
> > > > setup_configfile /etc/sudoers.d/zabbix
> > > > 
> > > > +# Overwrite script if it exists as user should not modify it
> > > > but it
> > > > is included in backup
> > > > +mv /etc/zabbix_agentd/scripts/ipfire_services.pl.ipfirenew
> > > > /etc/zabbix_agentd/scripts/ipfire_services.pl
> > > > +
> > > > if $review_required; then
> > > >         echo "WARNING: New versions of some configfile(s) where
> > > > provided as .ipfirenew-files."
> > > >         echo "         They may need manual review in order to
> > > > take
> > > > advantage of new features"
> > > > diff --git a/src/paks/zabbix_agentd/uninstall.sh
> > > > b/src/paks/zabbix_agentd/uninstall.sh
> > > > index 7a13880c5..ccbc8f7cf 100644
> > > > --- a/src/paks/zabbix_agentd/uninstall.sh
> > > > +++ b/src/paks/zabbix_agentd/uninstall.sh
> > > > @@ -26,6 +26,8 @@ stop_service ${NAME}
> > > > 
> > > > # Remove .ipfirenew files in advance so they won't be included
> > > > in
> > > > backup
> > > > rm -rfv /etc/zabbix_agentd/*.ipfirenew
> > > > /etc/zabbix_agentd/*/*.ipfirenew
> > > > +# Remove script-file as it should not have been modified by
> > > > user
> > > > +rm -fv /etc/zabbix_agentd/scripts/ipfire_services.pl
> > > > 
> > > > make_backup ${NAME}
> > > > remove_files
> > > > -- 
> > > > 2.30.2
> > > > 
> > > > 
> > > > -- 
> > > > Dit bericht is gescanned op virussen en andere gevaarlijke
> > > > inhoud door MailScanner en lijkt schoon te zijn.
> > > > 
> > > 
> > > -Michael
> > > 
> > > 
> > 
> > 
> > -- 
> > Dit bericht is gescanned op virussen en andere gevaarlijke
> > inhoud door MailScanner en lijkt schoon te zijn.
> 
>
  

Hi Michael

I looked a bit deeper into this and was thinking, as IPfire already
runs collectd, another possible solution to preventing zabbix agent
from using iptables directly for getting metrics from the firewall, is
to request them from collectd:

One way could be by enabling the collectd unixsock plugin
(https://collectd.org/documentation/manpages/collectd-unixsock.5.shtml)
and to make things 'easier' I could then call collectd-nagios
(https://collectd.org/documentation/manpages/collectd-nagios.1.shtml)
to collect live metrics from collectd as is explained in this example:
https://docs.wallarm.com/admin-en/monitoring/collectd-zabbix/
However the collectd unixsock plugin is currently not enabled and
collectd-nagios binary is not installed.
So I'm not sure if all that is worth the effort for an optional addon
as zabbix_agentd is. Since I assume this would require changes to a
core component.

Another way could be by reading the last recorded value from the rrd-
files generated by collectd using for example:
# rrdtool lastupdate /var/log/rrd/collectd/localhost/iptables-filter-
POLICYFWD/ipt_bytes-DROP_FORWARD.rrd
which gives back the timestamp of the measurement and the metric.
But this would require a different approach on how to send those values
to the Zabbix server since we now also have to send a timestamp
together with the metric and this can only be done by using
zabbix_sender to actively send metrics to Zabbix.
Hence I would have to create a script that collects all required last
values from the RRD files, and then send them to Zabbix using
zabbix_sender. 
The executing of that script can be triggered by a cron job or as a
custom userparameter by the agent, but then the script needs to finish
within the timeout configured in the zabbix agent config (default: 3s,
max 10s).. And in de case of a cron job, we'd only want that enabled if
the Zabbix server is set up to listen for those specific metric-items.
This could possibly integrated in the webgui as a user-configurable
setting somehow.. (Providing a webgui-addon for configuring
zabbix_agentd is also something I can imagine myself submitting in the
future, but I wasn't planning to do that in short terms :-)) 


So as you see, both have different challenges.. And of course, should
collectd silently start to fail, Zabbix also will not get accurate data
anymore. Or if the RRD file(s) would become corrupt (which I had once
after migrating from i586 vm to x86_64 appliance)..
Up to some level, additional checks by Zabbix on collectd and the rrd
files could possibly detect such failures..

But I still prefer for zabbix agent (or any monitoring tool for that
matter) to have the shortest and most trustworthy direct path to the
metrics it wants, in this case iptables statistics, hence the iptables
command.
So those wrapper scripts I brought up before are still my preferred
solution on this.

Your thoughts ?

Regards

Robin

Robin Roevens schreef op do 15-04-2021 om 15:12 [+0200]:
> 
> > 
> > What would a monitoring tool do with a dump of the iptables ruleset?
> > I do not even understand where the use of this is.
> 
> The same as IPFire webgui currently does on netother.cgi?fwhits?day
> by calling /sbin/iptables -vnxL <chain> | grep "\/\* <rule> \*\/" | awk
> '{ print $2 }';: keep history of and eventually react on the number of
> firewall-hits.
> 
> But you are completely right that access to iptables should be
> restricted to just that what we want to get from it, which can be
> achieved with a wrapper script as explained below in my previous mail.
> 
> Regards
> 
> Robin
>
  

Hello,

> On 15 Apr 2021, at 14:12, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> Hi Michael
> 
> 
> Michael Tremer schreef op do 15-04-2021 om 12:21 [+0100]:
>> Hello,
>> 
>>> On 12 Apr 2021, at 23:16, Robin Roevens <
>>> robin.roevens@disroot.org
>>>> wrote:
>>> 
>>> Hi Michael
>>> 
>>> 
>>> Michael Tremer schreef op ma 12-04-2021 om 11:36 [+0100]:
>>>> Hello,
>>>> 
>>>>> On 7 Apr 2021, at 21:44, Robin Roevens <
>>>>> robin.roevens@disroot.org
>>>>>> 
>>>>> wrote:
>>>>> ...
>>>>> ...
>>>>> diff --git a/config/zabbix_agentd/ipfire_services.pl
>>>>> b/config/zabbix_agentd/ipfire_services.pl
>>>>> new file mode 100755
>>>>> index 000000000..dbf8aec56
>>>>> --- /dev/null
>>>>> +++ b/config/zabbix_agentd/ipfire_services.pl
>>>>> @@ -0,0 +1,221 @@
>>>>> +#!/usr/bin/perl
>>>>> +##############################################################
>>>>> ######
>>>>> ###########
>>>>> +# ipfire_services.pl - Retrieves available IPFire services
>>>>> information and 
>>>>> +#                      return this as a JSON array suitable
>>>>> for easy
>>>>> processing 
>>>>> +#                      by Zabbix server
>>>>> +#
>>>>> +# Author: robin.roevens (at) disroot.org
>>>>> +# Version: 1.0
>>>>> +#
>>>>> +# Based on: services.cgi by IPFire Team
>>>>> +# Copyright (C) 2007-2021  IPFire Team  <
>>>>> info@ipfire.org
>>>>>> 
>>>>> +# 
>>>>> +# This program is free software: you can redistribute it
>>>>> and/or
>>>>> modify
>>>>> +# it under the terms of the GNU General Public License as
>>>>> published
>>>>> by
>>>>> +# the Free Software Foundation, either version 3 of the
>>>>> License, or
>>>>> +# (at your option) any later version.
>>>>> +# 
>>>>> +# This program is distributed in the hope that it will be
>>>>> useful,
>>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty
>>>>> of
>>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
>>>>> the 
>>>>> +# GNU General Public License for more details.
>>>>> +# 
>>>>> +# You should have received a copy of the GNU General Public
>>>>> License 
>>>>> +# along with this program.  If not, see <
>>>>> http://www.gnu.org/licenses/
>>>>>> .
>>>>> +# 
>>>>> +##############################################################
>>>>> ######
>>>>> ###########
>>>>> +
>>>>> +use strict;
>>>>> +
>>>>> +# enable only the following on debugging purpose
>>>>> +# use warnings;
>>>>> +
>>>>> +# Maps a nice printable name to the changing part of the pid
>>>>> file,
>>>>> which
>>>>> +# is also the name of the program
>>>>> +my %servicenames =(
>>>>> +        'DHCP Server' => 'dhcpd',
>>>>> +        'Web Server' => 'httpd',
>>>>> +        'CRON Server' => 'fcron',
>>>>> +        'DNS Proxy Server' => 'unbound',
>>>>> +        'Logging Server' => 'syslogd',
>>>>> +        'Kernel Logging Server' => 'klogd',
>>>>> +        'NTP Server' => 'ntpd',
>>>>> +        'Secure Shell Server' => 'sshd',
>>>>> +        'VPN' => 'charon',
>>>>> +        'Web Proxy' => 'squid',
>>>>> +        'Intrusion Detection System' => 'suricata',
>>>>> +        'OpenVPN' => 'openvpn'
>>>>> +);
>>>> 
>>>> You have a hard-coded list with process names here. It
>>>> technically is
>>>> missing add-ons but including everything would make this list
>>>> very long
>>>> and a lot of work to maintain...
>>>> 
>>> 
>>> As explained in my previous mail, I don't currently know of a
>>> method to
>>> get a list of mandatory IPFire specific services. So this list is a
>>> straight copy out of services.cgi minus the translation.
>>> The add-on services are looked up below, exactly like it is done in
>>> services.cgi.
>> 
>> What list are you looking for? Just all processes that are running?
> 
> The purpose of my extension to the default is that a user can easily
> configure Zabbix to monitor "IPFire"-specific functionalities. Generic
> Linux metrics can be monitored by default Zabbix templates and agent
> built-in functionality eventually extended with user-defined scripts or
> modules. 

Yes, and I am absolutely for it. The Zabbix Agent alone is not very useful when it does not understand the special metrics that IPFire has and exports them in some way or another.

That said, there is obviously always the problem that if a third person gains access to the monitoring system, they would have a full map of the entire network and loads of other metrics that are helpful to see whether their attack is effective and/or detected. That is something that is out of scope of this project and a risk that people need to evaluate elsewhere. Obviously no monitoring at all is not a solution either.

> The IPFire webgui has a nice overview of all vital IPFire services and
> their state, as well as for addon-services. 
> A user can browse the status pages of the WebGUI to have a view of how
> their IPFire machine is running
> This is exactly what I want to provide to the Zabbix user, hence my
> copy of services.cgi-code.

Understood. Since the services.cgi code is a bit ugly, we might want to rethink how we solve this.

I would be in favour of a file that every add-on brings with it and that makes it show up on the services.cgi table as well as bringing some more meta information like the name of the initscript (if there is one) which you could then call by using addonctrl status.

> Alternatively, I could 'parse' the output of the services-page of the
> webserver. But that is rather something one would do when trying to
> monitor a black-box. 

That is going to break as soon as we touch any of the markup.

>>>>> +
>>>>> +# Hash to overwrite the process name of a process if it
>>>>> differs from
>>>>> the launch command.
>>>>> +my %overwrite_exename_hash = (
>>>>> +        "suricata" => "Suricata-Main"
>>>>> +);
>>>>> +
>>>>> +my $first = 1;
>>>>> +
>>>>> +print "[";
>>>>> +
>>>>> +# Built-in services
>>>>> +my $key = '';
>>>>> +foreach $key (sort keys %servicenames){
>>>>> +       print "," if not $first;
>>>>> +       $first = 0;
>>>>> +
>>>>> +       print "{";
>>>>> +       print "\"service\":\"$key\",";
>>>>> +
>>>>> +       my $shortname = $servicenames{$key};
>>>>> +       print &servicestats($shortname);
>>>>> +       
>>>>> +       print "}";
>>>>> +}
>>>>> +
>>>>> +# Generate list of installed addon pak's
>>>>> +my @pak = `find /opt/pakfire/db/installed/meta-* 2>/dev/null |
>>>>> cut -
>>>>> d"-" -f2`;
>>>>> +foreach (@pak){
>>>>> +       chomp($_);
>>>>> +
>>>>> +       # Check which of the paks are services
>>>>> +       my @svc = `find /etc/init.d/$_ 2>/dev/null | cut -d"/"
>>>>> -f4`;
>>>>> +       foreach (@svc){
>>>>> +               # blacklist some packages
>>>>> +               #
>>>>> +               # alsa has trouble with the volume saving and
>>>>> was not
>>>>> really stopped
>>>>> +               # mdadm should not stopped with webif because
>>>>> this
>>>>> could crash the system
>>>>> +               #
>>>>> +               chomp($_);
>>>>> +               if ( $_ eq 'squid' ) {
>>>>> +                       next;
>>>>> +               }
>>>>> +               if ( ($_ ne "alsa") && ($_ ne "mdadm") ) {
>>>> 
>>>> Another hardcoded list due to code duplication. Not your fault,
>>>> but not
>>>> pretty.
>>>> 
>>> 
>>> Indeed, as is probably clear by now, I too would prefer a more
>>> clean
>>> way of generating a list of services.
>>> 
>>> Currently seeing this piece of code again.. I think this blacklist
>>> in
>>> services.cgi only exists so the user can't stop/start the service
>>> using
>>> the webgui.. But there is no such functionality in this script.. so
>>> I
>>> think I can skip this blacklist, and also provide service state of
>>> alsa
>>> and mdadm.. (not sure why squid is here, is there an addon that
>>> also
>>> provides squid, maybe squid-accounting?)
>> 
>> The code seemed to have some trouble with hyphenated package names,
>> that is why those checks were introduced.
>> 
>> It is pretty bad code and we should rework it some time, but
>> definitely not copy it if we can avoid it.
>> 
>>>>> +                       print ",";
>>>>> +                       print "{";
>>>>> +
>>>>> +                       print "\"service\":\"Addon: $_\",";
>>>>> +                       print "\"servicename\":\"$_\",";
>>>>> +
>>>>> +                       my $onboot = isautorun($_);
>>>>> +                       print "\"onboot\":$onboot,";
>>>>> +
>>>>> +                       print &addonservicestats($_);
>>>>> +
>>>>> +                       print "}";
>>>>> +               }
>>>>> +       }
>>>>> +}      
>>>>> +
>>>>> +print "]";
>>>>> +
>>>>> +sub servicestats{
>>>>> +       my $cmd = $_[0];
>>>>> +       my $status =
>>>>> "\"servicename\":\"$cmd\",\"state\":\"0\"";
>>>>> +       my $pid = '';
>>>>> +       my $testcmd = '';
>>>>> +        my $exename;
>>>>> +        my $memory;
>>>>> +
>>>>> +
>>>>> +       $cmd =~ /(^[a-z]+)/;
>>>>> +       
>>>>> +       # Check if the exename needs to be overwritten.
>>>>> +        # This happens if the expected process name string
>>>>> +        # differs from the real one. This may happened if
>>>>> +        # a service uses multiple processes or threads.
>>>>> +        if (exists($overwrite_exename_hash{$cmd})) {
>>>>> +                # Grab the string which will be reported by
>>>>> +                # the process from the corresponding hash.
>>>>> +                $exename = $overwrite_exename_hash{$1};
>>>>> +        } else {
>>>>> +                # Directly expect the launched command as
>>>>> +                # process name.
>>>>> +                $exename = $1;
>>>>> +        }
>>>>> +       
>>>>> +       if (open(FILE, "/var/run/${cmd}.pid")){
>>>>> +                $pid = <FILE>; chomp $pid;
>>>>> +                close FILE;
>>>>> +                if (open(FILE, "/proc/${pid}/status")){
>>>>> +                        while (<FILE>){
>>>>> +                                if (/^Name:\W+(.*)/) {
>>>>> +                                        $testcmd = $1;
>>>>> +                                }
>>>>> +                        }
>>>>> +                        close FILE;
>>>>> +                }
>>>>> +                if (open(FILE, "/proc/${pid}/status")) {
>>>>> +                        while (<FILE>) {
>>>>> +                                my ($key, $val) = split(":",
>>>>> $_, 2);
>>>>> +                                if ($key eq 'VmRSS') {
>>>>> +                                       $val =~ /\s*([0-
>>>>> 9]*)\s+kB/;
>>>>> +                                       # Convert kB to B
>>>>> +                                        $memory = $1*1024;
>>>>> +                                        last;
>>>>> +                                }
>>>>> +                        }
>>>>> +                        close(FILE);
>>>>> +                }
>>>>> +                if ($testcmd =~ /$exename/){
>>>>> +                       $status =
>>>>> "\"servicename\":\"$cmd\",\"state\":1,\"pid\":$pid,\"memory\":$
>>>>> memory
>>>>> ";
>>>>> +               }
>>>>> +        }
>>>>> +        return $status;
>>>>> +}
>>>>> +
>>>>> +sub isautorun{
>>>>> +        my $cmd = $_[0];
>>>>> +        my $status = "0";
>>>>> +        my $init = `find /etc/rc.d/rc3.d/S??${cmd}
>>>>> 2>/dev/null`;
>>>>> +        chomp ($init);
>>>>> +        if ($init ne ''){
>>>>> +                $status = "1";
>>>>> +        }
>>>>> +        $init = `find /etc/rc.d/rc3.d/off/S??${cmd}
>>>>> 2>/dev/null`;
>>>>> +        chomp ($init);
>>>>> +        if ($init ne ''){
>>>>> +                $status = "0";
>>>>> +        }
>>>>> +
>>>>> +        return $status;
>>>>> +}
>>>>> +
>>>>> +sub addonservicestats{
>>>>> +        my $cmd = $_[0];
>>>>> +        my $status = "0";
>>>>> +        my $pid = '';
>>>>> +        my $testcmd = '';
>>>>> +        my $exename;
>>>>> +        my @memory = (0);
>>>>> +
>>>>> +        $testcmd = `sudo /usr/local/bin/addonctrl $_ status
>>>>> 2>/dev/null`;
>>>>> +
>>>>> +        if ( $testcmd =~ /is\ running/ && $testcmd !~ /is\
>>>>> not\
>>>>> running/){
>>>>> +                $status = "\"state\":1";
>>>>> +
>>>>> +                $testcmd =~ s/.* //gi;
>>>>> +                $testcmd =~ s/[a-z_]//gi;
>>>>> +                $testcmd =~ s/\[[0-1]\;[0-9]+//gi;
>>>>> +                $testcmd =~ s/[\(\)\.]//gi;
>>>>> +                $testcmd =~ s/  //gi;
>>>>> +                $testcmd =~ s///gi;
>>>>> +
>>>>> +                my @pid = split(/\s/,$testcmd);
>>>>> +                $status .=",\"pid\":\"$pid[0]\"";
>>>>> +
>>>>> +                my $memory = 0;
>>>>> +
>>>>> +                foreach (@pid){
>>>>> +                        chomp($_);
>>>>> +                        if (open(FILE, "/proc/$_/statm")){
>>>>> +                                my $temp = <FILE>;
>>>>> +                                @memory = split(/ /,$temp);
>>>>> +                        }
>>>>> +                        $memory+=$memory[0];
>>>>> +                }
>>>>> +               $memory*=1024;
>>>>> +                $status .=",\"memory\":$memory";
>>>>> +        }else{
>>>>> +                $status = "\"state\":0";
>>>>> +        }
>>>>> +        return $status;
>>>>> +} 
>>>>> diff --git a/config/zabbix_agentd/sudoers
>>>>> b/config/zabbix_agentd/sudoers
>>>>> index 1b362a4fd..340bb8e66 100644
>>>>> --- a/config/zabbix_agentd/sudoers
>>>>> +++ b/config/zabbix_agentd/sudoers
>>>>> @@ -14,4 +14,4 @@
>>>>> # Append / edit the following list of commands to fit your
>>>>> needs:
>>>>> #
>>>>> Defaults:zabbix !requiretty
>>>>> -zabbix ALL=(ALL) NOPASSWD: /opt/pakfire/pakfire status
>>>>> +zabbix ALL=(ALL) NOPASSWD: /opt/pakfire/pakfire status,
>>>>> /usr/local/bin/addonctrl, /sbin/iptables, /usr/sbin/fping
>>>> 
>>>> This is absolutely impossible to give the zabbix user control to
>>>> start/stop any add-on and to read and change the entire iptables
>>>> ruleset.
>>>> 
>>>> Zabbix is listening on the network and does not have any kind of
>>>> authentication. Any security vulnerabilities in Zabbix would
>>>> allow to
>>>> alter the firewall rules and DoS any add-ons. This is dangerous.
>>> 
>>> There is some level of security: 
>> 
>> I agree that it is *some*, but is it enough?
>> 
>>> - the agent will only respond on requests from the configured
>>> Zabbix
>>> server (which the user has to configure in the zabbix agentd conf-
>>> file,
>>> and defaults to 127.0.0.1 as we can't possibly know the ip of the
>>> user's server, so by default the agent responds to no one)
>> 
>> Host-based ACLs are not really a thing. On the local network faking a
>> source IP address is one of the easiest jobs.
>> 
>>> - By default remote commands (as in requests to the agent to
>>> execute
>>> any command given) are disabled so the only way to make the agent
>>> call
>>> those binaries is to request a predefined userparameter that in
>>> turn
>>> will execute such a command with predefined params (see the
>>> additional
>>> template_...conf-files)
>>> and on top of that, the user can add additional security by
>>> configuring
>>> encryption in the agent main configfile
>> 
>> Encryption is not authentication.
>> 
>> And the predefined parameters might be a thing, but my view is that
>> zabbix can execute quite serious commands if it is being exploited.
>> 
>> The Zabbix Agent had exactly one of these vulnerabilities in 2009
>> where an attacker could execute arbitrary commands:
>> 
>> 
>> https://www.cvedetails.com/cve/CVE-2009-4502/
>> 
>> 
>>> But as you point out, a possible vulnerability of some sort may
>>> indeed
>>> still pose risks this way.
>> 
>> I just find this risk insanely large. I am not going to tell people
>> what software they can use and what not, but I want to avoid that the
>> default configuration is adding major security issues to the entire
>> system. And adding those sudo rules by default does exactly this.
> 
> I totally agree, I was only pointing out the *some* security there is,
> but that will indeed never be sufficient to allow full access to
> iptables or addonctrl by default. 

I could live with the information leak (that is pretty much what Zabbix is designed for), but I cannot live with the possibility that breaking in the agent allows changing the iptables ruleset. Definitely not in the default configuration.

> A user could, on his own risk, do that to maybe have the agent execute
> firewall-actions as a reaction to a detected network attack or so.. But
> that is not something we should provide by default.
> 
>> 
>> What would a monitoring tool do with a dump of the iptables ruleset?
>> I do not even understand where the use of this is.
> 
> The same as IPFire webgui currently does on netother.cgi?fwhits?day
> by calling /sbin/iptables -vnxL <chain> | grep "\/\* <rule> \*\/" | awk
> '{ print $2 }';: keep history of and eventually react on the number of
> firewall-hits.

We have the following SUID binary:

  https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=src/misc-progs/getipstat.c;h=c806d54a9b0145aa2f0a11c848bc158f6e70a481;hb=HEAD

It isn’t pretty, but dumps the entire iptables ruleset into files which you can then read.

This allows exposing the ruleset without giving the agent the option to run the iptables command as root.

> But you are completely right that access to iptables should be
> restricted to just that what we want to get from it, which can be
> achieved with a wrapper script as explained below in my previous mail.

-Michael

> 
> Regards
> 
> Robin
> 
>> 
>>>> Are there any alternative ways to realise this functionality?
>>> 
>>> As sudo is not able to allow commands with variable parameters
>>> together
>>> with fixed parameters, a popular solution is using wrapper scripts,
>>> not
>>> writable by the user, denying the user direct access to the
>>> commands in
>>> question.
>>> 
>>> /usr/local/bin/addonctrl is only called from within the
>>> ipfire_services.pl script I provide, in the form of "addonctrl
>>> <service> status". 
>>> So I could add /etc/zabbix_agentd/scripts/ipfire_services.pl to the
>>> sudo list instead so that the zabbix user won't be able to call
>>> addonctrl directly. (making sure zabbix user has no write
>>> permission on
>>> that script)
>>> The same goes for iptables. This is only called by the predefined
>>> userparameter "ipfire.net.fw.hits[*]" defined in
>>> template_module_ipfire_network_stats.conf in the form
>>> "/sbin/iptables -
>>> vnxL $1 | grep "\/\* $2 \*\/" | awk '{ print $$2 }';" where $1
>>> (chain)
>>> and $2 (rule) are parameters that have to be supplied to the agent
>>> when
>>> requesting that item. 
>>> Here I can also supply a wrapper-script to be called  by sudo
>>> /etc/zabbix_agentd/scripts/firewall_hits.sh that accepts those
>>> parameters, and checks them for validity and against abuse.
>>> 
>>> Regards
>>> Robin
>> 
>> -Michael
>> 
>>>>> diff --git
>>>>> a/config/zabbix_agentd/template_module_ipfire_network_stats.con
>>>>> f
>>>>> b/config/zabbix_agentd/template_module_ipfire_network_stats.con
>>>>> f
>>>>> new file mode 100644
>>>>> index 000000000..f1658ed07
>>>>> --- /dev/null
>>>>> +++
>>>>> b/config/zabbix_agentd/template_module_ipfire_network_stats.con
>>>>> f
>>>>> @@ -0,0 +1,4 @@
>>>>> +### Parameters for monitoring IPFire network statistics 
>>>>> +UserParameter=ipfire.net.gateway.pingtime,sudo /usr/sbin/fping
>>>>> -c 3
>>>>> gateway 2>&1 | tail -n 1 | awk '{print $NF}' | cut -d '/' -f2
>>>>> +UserParameter=ipfire.net.gateway.ping,sudo /usr/sbin/fping -q
>>>>> -r 3
>>>>> gateway; [ ! $? ]; echo $?
>>>>> +UserParameter=ipfire.net.fw.hits[*],sudo /sbin/iptables -vnxL
>>>>> $1 |
>>>>> grep "\/\* $2 \*\/" | awk '{ print $$2 }';
>>>>> diff --git
>>>>> a/config/zabbix_agentd/template_module_ipfire_services.conf
>>>>> b/config/zabbix_agentd/template_module_ipfire_services.conf
>>>>> new file mode 100644
>>>>> index 000000000..5f95218e3
>>>>> --- /dev/null
>>>>> +++ b/config/zabbix_agentd/template_module_ipfire_services.conf
>>>>> @@ -0,0 +1,2 @@
>>>>> +### Parameter for monitoring IPFire services
>>>>> +UserParameter=ipfire.services,/etc/zabbix_agentd/scripts/ipfir
>>>>> e_serv
>>>>> ices.pl
>>>>> diff --git a/lfs/zabbix_agentd b/lfs/zabbix_agentd
>>>>> index 73e08d20a..c0d28d51f 100644
>>>>> --- a/lfs/zabbix_agentd
>>>>> +++ b/lfs/zabbix_agentd
>>>>> @@ -33,7 +33,7 @@ DIR_APP    = $(DIR_SRC)/$(THISAPP)
>>>>> TARGET     = $(DIR_INFO)/$(THISAPP)
>>>>> PROG       = zabbix_agentd
>>>>> PAK_VER    = 5
>>>>> -DEPS       =
>>>>> +DEPS       = "fping"
>>>>> 
>>>>> ###############################################################
>>>>> ######
>>>>> ##########
>>>>> # Top-level Rules
>>>>> @@ -97,6 +97,12 @@ $(TARGET) : $(patsubst
>>>>> %,$(DIR_DL)/%,$(objects))
>>>>>                /etc/zabbix_agentd/zabbix_agentd.conf.ipfirenew
>>>>>        install -v -m 644
>>>>> $(DIR_SRC)/config/zabbix_agentd/template_app_pakfire.conf \
>>>>>                /etc/zabbix_agentd/zabbix_agentd.d/template_app
>>>>> _pakfi
>>>>> re.conf.ipfirenew
>>>>> +       install -v -m 644
>>>>> $(DIR_SRC)/config/zabbix_agentd/template_module_ipfire_network_
>>>>> stats.
>>>>> conf \
>>>>> +               /etc/zabbix_agentd/zabbix_agentd.d/template_mod
>>>>> ule_ip
>>>>> fire_network_stats.conf.ipfirenew
>>>>> +       install -v -m 644
>>>>> $(DIR_SRC)/config/zabbix_agentd/template_module_ipfire_services
>>>>> .conf
>>>>> \
>>>>> +               /etc/zabbix_agentd/zabbix_agentd.d/template_mod
>>>>> ule_ip
>>>>> fire_services.conf.ipfirenew
>>>>> +       install -v -m 755
>>>>> $(DIR_SRC)/config/zabbix_agentd/ipfire_services.pl \
>>>>> +               /etc/zabbix_agentd/scripts/ipfire_services.pl.i
>>>>> pfiren
>>>>> ew
>>>>> 
>>>>>        # Create directory for additional agent modules
>>>>>        -mkdir -pv /usr/lib/zabbix
>>>>> diff --git a/src/paks/zabbix_agentd/install.sh
>>>>> b/src/paks/zabbix_agentd/install.sh
>>>>> index 4248a7ec1..ced915c81 100644
>>>>> --- a/src/paks/zabbix_agentd/install.sh
>>>>> +++ b/src/paks/zabbix_agentd/install.sh
>>>>> @@ -66,8 +66,13 @@ restore_backup ${NAME}
>>>>> # Put zabbix configfiles in place
>>>>> setup_configfile /etc/zabbix_agentd/zabbix_agentd.conf
>>>>> setup_configfile
>>>>> /etc/zabbix_agentd/zabbix_agentd.d/template_app_pakfire.conf
>>>>> +setup_configfile
>>>>> /etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_netwo
>>>>> rk_sta
>>>>> ts.conf
>>>>> +setup_configfile
>>>>> /etc/zabbix_agentd/zabbix_agentd.d/template_module_ipfire_servi
>>>>> ces.co
>>>>> nf
>>>>> setup_configfile /etc/sudoers.d/zabbix
>>>>> 
>>>>> +# Overwrite script if it exists as user should not modify it
>>>>> but it
>>>>> is included in backup
>>>>> +mv /etc/zabbix_agentd/scripts/ipfire_services.pl.ipfirenew
>>>>> /etc/zabbix_agentd/scripts/ipfire_services.pl
>>>>> +
>>>>> if $review_required; then
>>>>>        echo "WARNING: New versions of some configfile(s) where
>>>>> provided as .ipfirenew-files."
>>>>>        echo "         They may need manual review in order to
>>>>> take
>>>>> advantage of new features"
>>>>> diff --git a/src/paks/zabbix_agentd/uninstall.sh
>>>>> b/src/paks/zabbix_agentd/uninstall.sh
>>>>> index 7a13880c5..ccbc8f7cf 100644
>>>>> --- a/src/paks/zabbix_agentd/uninstall.sh
>>>>> +++ b/src/paks/zabbix_agentd/uninstall.sh
>>>>> @@ -26,6 +26,8 @@ stop_service ${NAME}
>>>>> 
>>>>> # Remove .ipfirenew files in advance so they won't be included
>>>>> in
>>>>> backup
>>>>> rm -rfv /etc/zabbix_agentd/*.ipfirenew
>>>>> /etc/zabbix_agentd/*/*.ipfirenew
>>>>> +# Remove script-file as it should not have been modified by
>>>>> user
>>>>> +rm -fv /etc/zabbix_agentd/scripts/ipfire_services.pl
>>>>> 
>>>>> make_backup ${NAME}
>>>>> remove_files
>>>>> -- 
>>>>> 2.30.2
>>>>> 
>>>>> 
>>>>> -- 
>>>>> Dit bericht is gescanned op virussen en andere gevaarlijke
>>>>> inhoud door MailScanner en lijkt schoon te zijn.
>>>>> 
>>>> 
>>>> -Michael
>>>> 
>>>> 
>>> 
>>> 
>>> -- 
>>> Dit bericht is gescanned op virussen en andere gevaarlijke
>>> inhoud door MailScanner en lijkt schoon te zijn.
>> 
>> 
> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
  

Hello,

> On 15 Apr 2021, at 21:34, Robin Roevens <robin.roevens@disroot.org> wrote:
> 
> Hi Michael
> 
> I looked a bit deeper into this and was thinking, as IPfire already
> runs collectd, another possible solution to preventing zabbix agent
> from using iptables directly for getting metrics from the firewall, is
> to request them from collectd:
> 
> One way could be by enabling the collectd unixsock plugin
> (https://collectd.org/documentation/manpages/collectd-unixsock.5.shtml)
> and to make things 'easier' I could then call collectd-nagios
> (https://collectd.org/documentation/manpages/collectd-nagios.1.shtml)
> to collect live metrics from collectd as is explained in this example:
> https://docs.wallarm.com/admin-en/monitoring/collectd-zabbix/
> However the collectd unixsock plugin is currently not enabled and
> collectd-nagios binary is not installed.
> So I'm not sure if all that is worth the effort for an optional addon
> as zabbix_agentd is. Since I assume this would require changes to a
> core component.

Hmm, it does not strike me as a very straight-forward solution to involve collectd. We have a replacement for collectd ready which we will roll out at some point and so it will go away eventually.

See my remarks about “getipstat” from my other email.

> Another way could be by reading the last recorded value from the rrd-
> files generated by collectd using for example:
> # rrdtool lastupdate /var/log/rrd/collectd/localhost/iptables-filter-
> POLICYFWD/ipt_bytes-DROP_FORWARD.rrd

If we have to go this road, this would be a better solution in my view, but it still makes the Zabbix Agent dependant on collectd.

> which gives back the timestamp of the measurement and the metric.
> But this would require a different approach on how to send those values
> to the Zabbix server since we now also have to send a timestamp
> together with the metric and this can only be done by using
> zabbix_sender to actively send metrics to Zabbix.
> Hence I would have to create a script that collects all required last
> values from the RRD files, and then send them to Zabbix using
> zabbix_sender. 
> The executing of that script can be triggered by a cron job or as a
> custom userparameter by the agent, but then the script needs to finish
> within the timeout configured in the zabbix agent config (default: 3s,
> max 10s).. And in de case of a cron job, we'd only want that enabled if
> the Zabbix server is set up to listen for those specific metric-items.
> This could possibly integrated in the webgui as a user-configurable
> setting somehow.. (Providing a webgui-addon for configuring
> zabbix_agentd is also something I can imagine myself submitting in the
> future, but I wasn't planning to do that in short terms :-)) 

This sounds overly complicated to me and will require a lot of things being right (i.e. time).

You will end up with corner cases and bad metrics when the system is under load and the execution of the command is delayed.

> So as you see, both have different challenges.. And of course, should
> collectd silently start to fail, Zabbix also will not get accurate data
> anymore. Or if the RRD file(s) would become corrupt (which I had once
> after migrating from i586 vm to x86_64 appliance)..

They are not corrupt, just incompatible between different architectures.

The result is the same :)

> Up to some level, additional checks by Zabbix on collectd and the rrd
> files could possibly detect such failures..
> 
> But I still prefer for zabbix agent (or any monitoring tool for that
> matter) to have the shortest and most trustworthy direct path to the
> metrics it wants, in this case iptables statistics, hence the iptables
> command.
> So those wrapper scripts I brought up before are still my preferred
> solution on this.
> 
> Your thoughts ?

I agree :)

-Michael

> 
> Regards
> 
> Robin
> 
> Robin Roevens schreef op do 15-04-2021 om 15:12 [+0200]:
>> 
>>> 
>>> What would a monitoring tool do with a dump of the iptables ruleset?
>>> I do not even understand where the use of this is.
>> 
>> The same as IPFire webgui currently does on netother.cgi?fwhits?day
>> by calling /sbin/iptables -vnxL <chain> | grep "\/\* <rule> \*\/" | awk
>> '{ print $2 }';: keep history of and eventually react on the number of
>> firewall-hits.
>> 
>> But you are completely right that access to iptables should be
>> restricted to just that what we want to get from it, which can be
>> achieved with a wrapper script as explained below in my previous mail.
>> 
>> Regards
>> 
>> Robin
>> 
> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
>
  

Hi

Michael Tremer schreef op ma 19-04-2021 om 14:37 [+0100]:
> Hello,
> 
> > On 15 Apr 2021, at 14:12, Robin Roevens <robin.roevens@disroot.org>
> > wrote:
> > 
> > Hi Michael
> > 
> > ...
> > 
> > The purpose of my extension to the default is that a user can
> > easily
> > configure Zabbix to monitor "IPFire"-specific functionalities.
> > Generic
> > Linux metrics can be monitored by default Zabbix templates and
> > agent
> > built-in functionality eventually extended with user-defined
> > scripts or
> > modules. 
> 
> Yes, and I am absolutely for it. The Zabbix Agent alone is not very
> useful when it does not understand the special metrics that IPFire
> has and exports them in some way or another.
> 
> That said, there is obviously always the problem that if a third
> person gains access to the monitoring system, they would have a full
> map of the entire network and loads of other metrics that are helpful
> to see whether their attack is effective and/or detected. That is
> something that is out of scope of this project and a risk that people
> need to evaluate elsewhere. Obviously no monitoring at all is not a
> solution either.
> 
True that, a monitoring system an sich is a security risk and should be
appropriately secured on its own. But is indeed not our scope here.
What we can do to secure it a little bit more is adding a big remark to
the wiki page for the user to seriously consider setting up encrypted
agent communication so hackers on the network at least can't intercept
the values coming from IPFire. But by default we can't set that up for
the user as PSK's or certificates are required that only the user can
provide.

> > The IPFire webgui has a nice overview of all vital IPFire services
> > and
> > their state, as well as for addon-services. 
> > A user can browse the status pages of the WebGUI to have a view of
> > how
> > their IPFire machine is running
> > This is exactly what I want to provide to the Zabbix user, hence my
> > copy of services.cgi-code.
> 
> Understood. Since the services.cgi code is a bit ugly, we might want
> to rethink how we solve this.
> 
> I would be in favour of a file that every add-on brings with it and
> that makes it show up on the services.cgi table as well as bringing
> some more meta information like the name of the initscript (if there
> is one) which you could then call by using addonctrl status.
> 
For the add-on services at least, that sounds like a good idea. I see
there is already a bit of metadata in /opt/pakfire/db/installed/meta-*.
This could possibly be extended with an InitScript: entry? But I'm not
sure how to do this as I'm not sure how and where this file is actually
generated. 
A proper place to set the variable that would be used in the final
meta-file if an initscript is present would be in the lfs call
INSTALL_INITSCRIPT but also here I have no idea where this function
INSTALL_INITSCRIPT is actually defined. So I'm just guessing that this
is the ideal place.


> > Alternatively, I could 'parse' the output of the services-page of
> > the
> > webserver. But that is rather something one would do when trying to
> > monitor a black-box. 
> 
> That is going to break as soon as we touch any of the markup.
That's one of the reasons why I would like to avoid that. :-)

> > > > > > 
> > ...
> > I totally agree, I was only pointing out the *some* security there
> > is,
> > but that will indeed never be sufficient to allow full access to
> > iptables or addonctrl by default. 
> 
> I could live with the information leak (that is pretty much what
> Zabbix is designed for), but I cannot live with the possibility that
> breaking in the agent allows changing the iptables ruleset.
> Definitely not in the default configuration.
> 
> > A user could, on his own risk, do that to maybe have the agent
> > execute
> > firewall-actions as a reaction to a detected network attack or so..
> > But
> > that is not something we should provide by default.
> > 
> > > 
> > > What would a monitoring tool do with a dump of the iptables
> > > ruleset?
> > > I do not even understand where the use of this is.
> > 
> > The same as IPFire webgui currently does on netother.cgi?fwhits?day
> > by calling /sbin/iptables -vnxL <chain> | grep "\/\* <rule> \*\/" |
> > awk
> > '{ print $2 }';: keep history of and eventually react on the number
> > of
> > firewall-hits.
> 
> We have the following SUID binary:
> 
>       
> https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=src/misc-progs/getipstat.c;h=c806d54a9b0145aa2f0a11c848bc158f6e70a481;hb=HEAD
> 
> It isn’t pretty, but dumps the entire iptables ruleset into files
> which you can then read.
> 
> This allows exposing the ruleset without giving the agent the option
> to run the iptables command as root.

At first, this sounded like the solution. But I found 2 problems with
it:

- It generates a (few) fixed files with names that can't be customized.
And it is called in guardian.cgi and iptables.cgi. So if it so happens
that a user opens one of those 2 pages at the moment the zabbix agent
is using that command; chances are that the file is removed again by
the time the agent can parse it or incompletely (re)written. Resulting
in the agent failing to collect the metrics accurately. (or one of
those cgi's failing to give a correct output).
A possible fix for this could be to have a command parameter on this
tool that switches output to stdout, or that would let you define the
output-filename.

- It doesn't call iptables with the -x parameter (to show the exact #
of bytes in the output) which I really require. Rounded values up to
K/M or even G are pretty useless for fine grained monitoring.
Also here is a possible fix to accept a command line parameter to make
the command call iptables with the -x parameter added.
(as it is currently used by iptables, we probably don't want -x to be
used by default, hence an optional parameter)

If you see no problem in these changes, I will try to submit a patch
for it shortly.

About addonctrl where zabbix needs to call "addonctrl status" which
requires root:
I propose to add a wrapper script to the zabbix_agentd package that
will call addonctrl status "$1" which then in turn can be added to the
sudo list instead of addonctrl itself.


Regards

Robin
> 
> > But you are completely right that access to iptables should be
> > restricted to just that what we want to get from it, which can be
> > achieved with a wrapper script as explained below in my previous
> > mail.
> 
> -Michael
> 
> > 
> > Regards
> > 
> > Robin