| Message ID | 20181011220525.24628-2-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | DHCP tests and AppArmor improvements | expand |
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
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() >
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
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/...
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
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()
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(-)