From 44758ac786c713c4b31067e1b2d7d03c354a95d8 Mon Sep 17 00:00:00 2001
From: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
Date: Wed, 7 Oct 2020 11:51:17 +0200
Subject: [PATCH] Do not set recursive ACLs on dhcp

The cause for this change is the correct chaining. Previously there was
no guarantee when the ACL was applied. In practice it was often done as:
File[/etc/dhcp] -> Exec[setfacl] -> File[/etc/dhcp/dhcpd.conf]

This meant the dhcpd.conf file didn't receive the ACL anyway. After
chaining to Class['dhcp'] (to guarantee all files existed) it turns out
that /etc/dhcp/dhcpd.conf would become executable. That resulted in
idempotency problems.

The Proxy needs to read dhcpd.conf, but that's guaranteed to be mode
0644 by theforeman/dhcp. Only the DHCP dir itself can have mode 0750
(owned by root:root) which is why the ACL is needed. By making it
non-recursive and done after Class[dhcp] these problems are avoided.
---
 manifests/proxydhcp.pp                         | 4 ++--
 spec/classes/foreman_proxy__proxydhcp__spec.rb | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/manifests/proxydhcp.pp b/manifests/proxydhcp.pp
index cea65e00..5bfc817b 100644
--- a/manifests/proxydhcp.pp
+++ b/manifests/proxydhcp.pp
@@ -74,10 +74,10 @@
 
     [$dhcp::dhcp_dir, dirname($foreman_proxy::dhcp_leases)].each |$path| {
       exec { "Allow ${foreman_proxy::user} to read ${path}":
-        command => "setfacl -R -m u:${foreman_proxy::user}:rx ${path}",
+        command => "setfacl -m u:${foreman_proxy::user}:rx ${path}",
         path    => ['/bin', '/usr/bin'],
         unless  => "getfacl -p ${path} | grep user:${foreman_proxy::user}:r-x",
-        require => Package['acl'],
+        require => [Class['dhcp'], Package['acl']],
       }
     }
 
diff --git a/spec/classes/foreman_proxy__proxydhcp__spec.rb b/spec/classes/foreman_proxy__proxydhcp__spec.rb
index ff2d2747..ceab6153 100644
--- a/spec/classes/foreman_proxy__proxydhcp__spec.rb
+++ b/spec/classes/foreman_proxy__proxydhcp__spec.rb
@@ -67,11 +67,11 @@
           it { is_expected.to contain_class('dhcp').with_conf_dir_mode('0750') }
 
           it do should contain_exec('Allow foreman-proxy to read /etc/dhcp').
-            with_command("setfacl -R -m u:foreman-proxy:rx /etc/dhcp")
+            with_command("setfacl -m u:foreman-proxy:rx /etc/dhcp")
           end
 
           it do should contain_exec("Allow foreman-proxy to read #{leases_dir}").
-            with_command("setfacl -R -m u:foreman-proxy:rx #{leases_dir}")
+            with_command("setfacl -m u:foreman-proxy:rx #{leases_dir}")
           end
         end
 
@@ -79,7 +79,7 @@
           case facts[:osfamily]
           when 'RedHat', 'Debian'
             it do should contain_exec('Allow foreman-proxy to read /etc/dhcp').
-              with_command('setfacl -R -m u:foreman-proxy:rx /etc/dhcp').
+              with_command('setfacl -m u:foreman-proxy:rx /etc/dhcp').
               with_unless('getfacl -p /etc/dhcp | grep user:foreman-proxy:r-x')
             end
           else
@@ -89,7 +89,7 @@
           case facts[:osfamily]
           when 'RedHat', 'Debian'
             it do should contain_exec("Allow foreman-proxy to read #{leases_dir}").
-              with_command("setfacl -R -m u:foreman-proxy:rx #{leases_dir}").
+              with_command("setfacl -m u:foreman-proxy:rx #{leases_dir}").
               with_unless("getfacl -p #{leases_dir} | grep user:foreman-proxy:r-x")
             end
           else