[LTP] [PATCH 0/4] DHCP tests and AppArmor improvements

[LTP] [PATCH 0/4] DHCP tests and AppArmor improvements

[Petr Vorel]

Hi,

here are some more DHCP tests polishing.

Kind regards,
Petr

Petr Vorel (4):
  net/dhcp: Use paths allowed by AppArmor for dnsmasq
  net/dhcp: Move print_dhcp_log() into dhcp library
  ver_linux: Print AppArmor and SELinux status
  tst_net.sh: Warn about enabled AppArmor

 testcases/lib/tst_net.sh                |  5 +++
 testcases/network/dhcp/dhcp_lib.sh      |  7 ++++
 testcases/network/dhcp/dhcpd_tests.sh   |  7 +---
 testcases/network/dhcp/dnsmasq_tests.sh | 13 +++----
 ver_linux                               | 46 ++++++++++++++++++++++---
 5 files changed, 60 insertions(+), 18 deletions(-)

-- 
2.19.0

[LTP] [PATCH 1/4] net/dhcp: Use paths allowed by AppArmor for dnsmasq

[Petr Vorel]

Fixes for --log-facility and --dhcp-leasefile.

Path for log file expects AppArmor commit
025c7dc6 ("dnsmasq: Add permission to open log files").

NOTE: AppArmor optimization isn't needed for dhcpd.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Changing path to /var/log require root, but we run most of network tests
under root anyway, at least for network  namespaces.
I didn't add TST_NEEDS_ROOT=1, maybe I should.


Kind regards,
Petr
---
 testcases/network/dhcp/dnsmasq_tests.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/testcases/network/dhcp/dnsmasq_tests.sh b/testcases/network/dhcp/dnsmasq_tests.sh
index ad5885c84..43961f85f 100755
--- a/testcases/network/dhcp/dnsmasq_tests.sh
+++ b/testcases/network/dhcp/dnsmasq_tests.sh
@@ -9,9 +9,11 @@ dhcp_name="dnsmasq"
 
 . dhcp_lib.sh
 
+log="/var/log/tst_dnsmasq.log"
+
 common_opt="--no-hosts --no-resolv --dhcp-authoritative \
-	--log-facility=./tst_dnsmasq.log --interface=$iface0 \
-	--dhcp-leasefile=tst_dnsmasq.lease --port=0 --conf-file= "
+	--log-facility=$log --interface=$iface0 \
+	--dhcp-leasefile=/var/lib/misc/dnsmasq.tst.leases --port=0 --conf-file= "
 
 start_dhcp()
 {
@@ -33,12 +35,12 @@ start_dhcp6()
 
 cleanup_dhcp()
 {
-	rm -f tst_dnsmasq.log
+	rm -f $log
 }
 
 print_dhcp_log()
 {
-	cat tst_dnsmasq.log
+	cat $log
 }
 
 print_dhcp_version()
-- 
2.19.0

[LTP] [PATCH 1/4] net/dhcp: Use paths allowed by AppArmor for dnsmasq

[Petr Vorel]

Hi,

> Fixes for --log-facility and --dhcp-leasefile.

> Path for log file expects AppArmor commit
> 025c7dc6 ("dnsmasq: Add permission to open log files").

> NOTE: AppArmor optimization isn't needed for dhcpd.

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
...
> Changing path to /var/log require root, but we run most of network tests
> under root anyway, at least for network  namespaces.
> I didn't add TST_NEEDS_ROOT=1, maybe I should.

...
> +++ b/testcases/network/dhcp/dnsmasq_tests.sh
...


> +log="/var/log/tst_dnsmasq.log"
Another option (instead of writing int /var/log/) is to detect enabled AppArmor
and /etc/apparmor.d/local/.  If enabled and dir exist, then append/create
/etc/apparmor.d/local/usr.sbin.dnsmasq with write permissions to our directory.
But this would require restart AppArmor.

> +
>  common_opt="--no-hosts --no-resolv --dhcp-authoritative \
> -	--log-facility=./tst_dnsmasq.log --interface=$iface0 \
> -	--dhcp-leasefile=tst_dnsmasq.lease --port=0 --conf-file= "
> +	--log-facility=$log --interface=$iface0 \
> +	--dhcp-leasefile=/var/lib/misc/dnsmasq.tst.leases --port=0 --conf-file= "


Kind regards,
Petr

[LTP] [PATCH 1/4] net/dhcp: Use paths allowed by AppArmor for dnsmasq

[Alexey Kodanev]

On 12.10.2018 01:05, Petr Vorel wrote:
> Fixes for --log-facility and --dhcp-leasefile.
> 
> Path for log file expects AppArmor commit
> 025c7dc6 ("dnsmasq: Add permission to open log files").
> 
> NOTE: AppArmor optimization isn't needed for dhcpd.
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Changing path to /var/log require root, but we run most of network tests
> under root anyway, at least for network  namespaces.
> I didn't add TST_NEEDS_ROOT=1, maybe I should.
> 
> 
> Kind regards,
> Petr
> ---
>  testcases/network/dhcp/dnsmasq_tests.sh | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/testcases/network/dhcp/dnsmasq_tests.sh b/testcases/network/dhcp/dnsmasq_tests.sh
> index ad5885c84..43961f85f 100755
> --- a/testcases/network/dhcp/dnsmasq_tests.sh
> +++ b/testcases/network/dhcp/dnsmasq_tests.sh
> @@ -9,9 +9,11 @@ dhcp_name="dnsmasq"
>  
>  . dhcp_lib.sh
>  
> +log="/var/log/tst_dnsmasq.log"
> +
>  common_opt="--no-hosts --no-resolv --dhcp-authoritative \
> -	--log-facility=./tst_dnsmasq.log --interface=$iface0 \
> -	--dhcp-leasefile=tst_dnsmasq.lease --port=0 --conf-file= "
> +	--log-facility=$log --interface=$iface0 \

It could be stderr with writing the output of dnsmasq to the test directory:

  --log-facility=-

> +	--dhcp-leasefile=/var/lib/misc/dnsmasq.tst.leases --port=0 --conf-file= "
>  

What if this directory doesn't exist? Why not to use the standard one for dnsmasq /var/lib/dnsmasq/?

Forgot to remove this file in cleanup? BTW, it's better to have "ltp" instead of "tst" in this path.


>  start_dhcp()
>  {
> @@ -33,12 +35,12 @@ start_dhcp6()
>  
>  cleanup_dhcp()
>  {
> -	rm -f tst_dnsmasq.log
> +	rm -f $log
>  }
>  
>  print_dhcp_log()
>  {
> -	cat tst_dnsmasq.log
> +	cat $log
>  }
>  
>  print_dhcp_version()
> 

[LTP] [PATCH 1/4] net/dhcp: Use paths allowed by AppArmor for dnsmasq

[Petr Vorel]

Hi Alexey,

thanks for you review!

> On 12.10.2018 01:05, Petr Vorel wrote:
> > Fixes for --log-facility and --dhcp-leasefile.

> > Path for log file expects AppArmor commit
> > 025c7dc6 ("dnsmasq: Add permission to open log files").

> > NOTE: AppArmor optimization isn't needed for dhcpd.

> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> > Changing path to /var/log require root, but we run most of network tests
> > under root anyway, at least for network  namespaces.
> > I didn't add TST_NEEDS_ROOT=1, maybe I should.

...
> >  common_opt="--no-hosts --no-resolv --dhcp-authoritative \
> > -	--log-facility=./tst_dnsmasq.log --interface=$iface0 \
> > -	--dhcp-leasefile=tst_dnsmasq.lease --port=0 --conf-file= "
> > +	--log-facility=$log --interface=$iface0 \

> It could be stderr with writing the output of dnsmasq to the test directory:

>   --log-facility=-
Yes, I noticed the possibility to use stderr as well. But it's since 2.53, which
breaks old distros (centos6/rhel6) and would require check for version.
Is it worth of it?
And isn't there anything else requiring root anyway on SSH/RSH based testing?
(default netns testing requires root).

> > +	--dhcp-leasefile=/var/lib/misc/dnsmasq.tst.leases --port=0 --conf-file= "

> What if this directory doesn't exist? Why not to use the standard one for dnsmasq /var/lib/dnsmasq/?
No, default path for linux is /var/lib/misc/dnsmasq.leases [1]:
define LEASEFILE "/var/lib/misc/dnsmasq.leases"

AppArmor also expects it there [2]:
/var/lib/misc/dnsmasq.leases rw, # Required only for DHCP server usage

but also accept different paths:
/var/lib/misc/dnsmasq.*.leases rw,
/var/lib/lxd-bridge/dnsmasq.*.leases rw,
/var/lib/NetworkManager/dnsmasq-*.leases rw,

> Forgot to remove this file in cleanup?
Yes, I should be consistent. But is it really needed to cleanup files, when
temporary directory is being deleted after test?  I was actually thinking to
remove cleanup_dhcp at all from both test scripts.

> BTW, it's better to have "ltp" instead of "tst" in this path.
Yes, but I wanted to be consistent with dhcpd_tests.sh - there is:
tst_dhcpd.conf, tst_hdcpd.lease

BTW: Others possible improvements of DHCP tests (not planning them before
finishing this):
* I was also thinking about passing file location of config file instead of
  changing content of global files in setup_dhcpd_conf().
* Handle situation when dhclient is already running in daemon mode (rare
  situation nowadays, probably started manually).
* Handle situation, when DHCP server is already running (and blocking port)


Kind regards,
Petr

[1] http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=blob;f=src/config.h;h=762c49b586bb26fb05d0eceac87d28f939693a6f;hb=HEAD#l193
[2] https://gitlab.com/apparmor/apparmor/blob/master/profiles/apparmor.d/usr.sbin.dnsmasq#L58

[LTP] [PATCH 1/4] net/dhcp: Use paths allowed by AppArmor for dnsmasq

[Alexey Kodanev]

Hi Petr,
On 24.10.2018 00:57, Petr Vorel wrote:
...
>> What if this directory doesn't exist? Why not to use the standard one for dnsmasq /var/lib/dnsmasq/?
> No, default path for linux is /var/lib/misc/dnsmasq.leases [1]:
> define LEASEFILE "/var/lib/misc/dnsmasq.leases"
> 
> AppArmor also expects it there [2]:
> /var/lib/misc/dnsmasq.leases rw, # Required only for DHCP server usage
> 
> but also accept different paths:
> /var/lib/misc/dnsmasq.*.leases rw,
> /var/lib/lxd-bridge/dnsmasq.*.leases rw,
> /var/lib/NetworkManager/dnsmasq-*.leases rw,
>

May be it is for the newest versions only, I was looking at 2.48/2.76 and it is
/var/lib/dnsmasq/dnsmasq.leases.
 
>> Forgot to remove this file in cleanup?
> Yes, I should be consistent. But is it really needed to cleanup files, when
> temporary directory is being deleted after test?  I was actually thinking to
> remove cleanup_dhcp at all from both test scripts.

But the file now outside of LTP temp directory, in /var/lib/misc/...

[LTP] [PATCH 1/4] net/dhcp: Use paths allowed by AppArmor for dnsmasq

[Petr Vorel]

Hi Alexey,

> Hi Petr,
> On 24.10.2018 00:57, Petr Vorel wrote:
> ...
> >> What if this directory doesn't exist? Why not to use the standard one for dnsmasq /var/lib/dnsmasq/?
> > No, default path for linux is /var/lib/misc/dnsmasq.leases [1]:
> > define LEASEFILE "/var/lib/misc/dnsmasq.leases"

> > AppArmor also expects it there [2]:
> > /var/lib/misc/dnsmasq.leases rw, # Required only for DHCP server usage

> > but also accept different paths:
> > /var/lib/misc/dnsmasq.*.leases rw,
> > /var/lib/lxd-bridge/dnsmasq.*.leases rw,
> > /var/lib/NetworkManager/dnsmasq-*.leases rw,

> May be it is for the newest versions only, I was looking at 2.48/2.76 and it is
> /var/lib/dnsmasq/dnsmasq.leases.
It's not upstream, src/config.h haven't changed for linux since 2.0.
/var/lib/dnsmasq/ is Fedora/RHEL/CentOS/Oracle Linux (RHEL*) specific [1], changed since
2.41 (in 2007) [2] [3]. I checked various other distros and others (SUSE, Debian,
Archlinux, Gentoo, Ubuntu) use default location in /var/lib/misc/.

/var/lib/misc/ also exists on RHEL* (filesystem package, which is on every RHEL*
system), so maybe we could be happy about that.

But RHEL* doesn't use AppArmor and SELinux supports wildcard on /var/lib/dnsmasq/
but in /var/lib/misc/ support just dnsmasq.leases [4]:
	/var/lib/misc/dnsmasq\.leases	--	gen_context(system_u:object_r:dnsmasq_lease_t,s0)
	/var/lib/dnsmasq(/.*)?	gen_context(system_u:object_r:dnsmasq_lease_t,s0)

so for RHEL* it'd be really better to use /var/lib/misc/.

Therefore could use /var/lib/misc/ as default and if directory not exist use
/var/lib/dnsmasq/ (as it's probably RHEL*). Writing into either of them
requires root, so we need to add TST_NEEDS_ROOT=1.
But still paths aren't compatible, either SELinux or AppArmor need to be more
relax (add star for both log and lease file).

Similar situation is for logging file:
SELinux [4]
	/var/log/dnsmasq.*	--	gen_context(system_u:object_r:dnsmasq_var_log_t,s0)
AppArmor [5]:
	/var/log/*dnsmasq.log w,

I'll report it to both projects. In meanwhile we could workaround with adjusting
dnsmasq's policy/profile (AppArmor: create /etc/apparmor.d/local/usr.sbin.dnsmasq,
SELinux: create /etc/selinux/targeted/contexts/files/file_contexts.local).
Or just to temporarily disable AppArmor or SELinux).

Not sure what is a better approach. Unfortunately these tests look to me more
like userspace related and catching AppArmor or SELinux policy/profile bugs than
kernel networking problems.

> >> Forgot to remove this file in cleanup?
> > Yes, I should be consistent. But is it really needed to cleanup files, when
> > temporary directory is being deleted after test?  I was actually thinking to
> > remove cleanup_dhcp at all from both test scripts.

> But the file now outside of LTP temp directory, in /var/lib/misc/...
OK, that needs to be removed.


Kind regards,
Petr

[1] https://src.fedoraproject.org/cgit/rpms/dnsmasq.git/tree/dnsmasq.spec#n67
[2] https://src.fedoraproject.org/cgit/rpms/dnsmasq.git/commit/?id=91d4b30e7b55bbb561547312e83ce4d709e505e2
[3] https://bugzilla.redhat.com/show_bug.cgi?id=407901
[4] https://github.com/SELinuxProject/refpolicy/blob/master/policy/modules/services/dnsmasq.fc
[5] https://gitlab.com/apparmor/apparmor/blob/master/profiles/apparmor.d/usr.sbin.dnsmasq

[LTP] [PATCH 2/4] net/dhcp: Move print_dhcp_log() into dhcp library

[Petr Vorel]

Create default path for log file, but allow test to specify it.
This is needed for AppArmor respected paths, which was already defined
in 3e76b9896 ("net/dhcp: Use paths allowed by AppArmor for dnsmasq").

Also check if file exists. This is needed for dnsmasq --log-facility
switch when AppArmor enabled, because according to dnsmasq(8) errors
whilst reading configuration will still go to syslog. And then error
report of cat of missing file is misleading.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/network/dhcp/dhcp_lib.sh      | 7 +++++++
 testcases/network/dhcp/dhcpd_tests.sh   | 7 +------
 testcases/network/dhcp/dnsmasq_tests.sh | 5 -----
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/testcases/network/dhcp/dhcp_lib.sh b/testcases/network/dhcp/dhcp_lib.sh
index 29b8027de..fb8b8840a 100755
--- a/testcases/network/dhcp/dhcp_lib.sh
+++ b/testcases/network/dhcp/dhcp_lib.sh
@@ -38,6 +38,8 @@ stop_dhcp()
 
 init()
 {
+	[ -z "$log" ] && log="$PWD/$(basename $0 '.sh').log"
+
 	if [ $TST_IPV6 ]; then
 		ip_addr="fd00:1:1:2::12/64"
 		ip_addr_check="fd00:1:1:2::100/64"
@@ -89,6 +91,11 @@ cleanup()
 	[ "$veth_loaded" = "no" ] && lsmod | grep -q '^veth ' && rmmod veth
 }
 
+print_dhcp_log()
+{
+	[ -f "$log" ] && cat $log
+}
+
 test01()
 {
 	local wicked
diff --git a/testcases/network/dhcp/dhcpd_tests.sh b/testcases/network/dhcp/dhcpd_tests.sh
index ff46b825d..e8339932b 100755
--- a/testcases/network/dhcp/dhcpd_tests.sh
+++ b/testcases/network/dhcp/dhcpd_tests.sh
@@ -31,7 +31,7 @@ setup_dhcpd_conf()
 start_dhcpd()
 {
 	touch tst_hdcpd.lease
-	dhcpd -lf tst_hdcpd.lease -$TST_IPVER $iface0 > tst_dhcpd.err 2>&1
+	dhcpd -lf tst_hdcpd.lease -$TST_IPVER $iface0 > $log 2>&1
 }
 
 start_dhcp()
@@ -69,11 +69,6 @@ cleanup_dhcp()
 	[ -f dhcpd.conf ] && mv dhcpd.conf $DHCPD_CONF
 }
 
-print_dhcp_log()
-{
-	cat tst_dhcpd.err
-}
-
 print_dhcp_version()
 {
 	dhcpd --version 2>&1
diff --git a/testcases/network/dhcp/dnsmasq_tests.sh b/testcases/network/dhcp/dnsmasq_tests.sh
index 43961f85f..e034f4f63 100755
--- a/testcases/network/dhcp/dnsmasq_tests.sh
+++ b/testcases/network/dhcp/dnsmasq_tests.sh
@@ -38,11 +38,6 @@ cleanup_dhcp()
 	rm -f $log
 }
 
-print_dhcp_log()
-{
-	cat $log
-}
-
 print_dhcp_version()
 {
 	dnsmasq --version | head -2
-- 
2.19.0

[LTP] [PATCH 3/4] ver_linux: Print AppArmor and SELinux status

[Petr Vorel]

+ add some helper functions

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
If you don't like the functions, I can remove them.
---
 ver_linux | 46 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/ver_linux b/ver_linux
index 897571703..5994c0e0f 100755
--- a/ver_linux
+++ b/ver_linux
@@ -3,8 +3,35 @@
 # typical as you use for compilation/istallation. I use
 # /bin /sbin /usr/bin /usr/sbin /usr/local/bin, but it may
 # differ on your system.
-#
+
 PATH=/sbin:/usr/sbin:/bin:/usr/bin:$PATH
+
+tst_cmd_available()
+{
+	if type command > /dev/null 2>&1; then
+		command -v $1 > /dev/null 2>&1 || return 1
+	else
+		which $1 > /dev/null 2>&1
+		if [ $? -eq 0 ]; then
+			return 0
+		else
+			return 1
+		fi
+	fi
+}
+
+tst_cmd_run()
+{
+	local cmd="$1"
+	shift
+	tst_cmd_available $cmd && eval "$cmd $@"
+}
+
+is_enabled()
+{
+	[ -f "$1" ] && [ "$(cat $1)" = "Y" -o "$(cat $1)" = "1" ]
+}
+
 echo 'If some fields are empty or look unusual you may have an old version.'
 echo 'Compare to the current minimal requirements in Documentation/Changes.'
 
@@ -101,8 +128,19 @@ free
 
 echo
 echo 'cpuinfo:'
-if which lscpu > /dev/null 2>&1; then
-	lscpu
+tst_cmd_run lscpu || cat /proc/cpuinfo
+
+echo
+if is_enabled /sys/module/apparmor/parameters/enabled; then
+	echo 'AppArmor enabled'
+	tst_cmd_run aa-status
 else
-	cat /proc/cpuinfo
+	echo 'AppArmor disabled'
+fi
+
+echo
+
+if ! tst_cmd_run sestatus; then
+	printf 'SELinux mode: '
+	tst_cmd_run getenforce || echo 'unknown'
 fi
-- 
2.19.0

[LTP] [PATCH 4/4] tst_net.sh: Warn about enabled AppArmor

[Petr Vorel]

This is due some false positives because improper usage or bugs
in AppArmor profiles (e.g. traceroute, dnsmasq).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/lib/tst_net.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
index a4467da7c..d60d50820 100644
--- a/testcases/lib/tst_net.sh
+++ b/testcases/lib/tst_net.sh
@@ -713,6 +713,11 @@ if [ -z "$TST_PARSE_VARIABLES" ]; then
 	tst_res_ TINFO "$LHOST_IFACES -- $RHOST_IFACES"
 	tst_res_ TINFO "$IPV4_LHOST/$IPV4_LPREFIX -- $IPV4_RHOST/$IPV4_RPREFIX"
 	tst_res_ TINFO "$IPV6_LHOST/$IPV6_LPREFIX -- $IPV6_RHOST/$IPV6_RPREFIX"
+
+	if [ -f /sys/module/apparmor/parameters/enabled ] && [ "$(cat /sys/module/apparmor/parameters/enabled)" = "Y" ]; then
+		tst_res_ TINFO "AppArmor enabled, this may affect test results"
+	fi
+
 	export TST_PARSE_VARIABLES="yes"
 fi
 
-- 
2.19.0