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

Message ID 20181011220525.24628-2-pvorel@suse.cz
State Changes Requested
Delegated to: Petr Vorel
Headers show
Series
  • DHCP tests and AppArmor improvements
Related show

Commit Message

Petr Vorel Oct. 11, 2018, 10:05 p.m.
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(-)

Comments

Petr Vorel Oct. 11, 2018, 10:15 p.m. | #1
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
Alexey Kodanev Oct. 23, 2018, 2:03 p.m. | #2
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()
>
Petr Vorel Oct. 23, 2018, 9:57 p.m. | #3
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
Alexey Kodanev Oct. 24, 2018, 10:40 a.m. | #4
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/...
Petr Vorel Oct. 24, 2018, 3:53 p.m. | #5
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

Patch

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()