Ensure the puppet group exists

In puppet 4 AIO packaging, it's the `puppetserver` package that creates
a `puppet` user and group.

The `puppet-agent` package doesn't create the group and unless
`puppetserver` is also installed the ssl keys and certs are owned by
`root:root` and are not readable by the foreman proxy. (It's the
installation of `puppetserver` that chowns the ssldir to
`puppet:puppet`)

With this commit, the module ensures that the `puppet_group` group
exists even on puppet 4.  It also makes sure the ssl_key/cert/ca files
and parent directories are group owned by the `puppet_group`

The change is hopefully quite conservative.  Only if both `$puppet` and
`$puppetca` are false and `$ssl` is true will it have any effect.
By default, it also only applies to puppet 4 and can be turned off
completely by setting `manage_puppet_group` to `false`.

Users who already manage the creation of the puppet group, (for instance
to workaround https://tickets.puppetlabs.com/browse/SERVER-1381) are
further protected by the `if !defined`.

manifests/config.pp

@@ -120,5 +120,26 @@
         changes => template('foreman_proxy/sudo_augeas.erb'),
       }
     }
+  } else {
+    # The puppet-agent (puppet 4 AIO package) doesn't create a puppet user and group
+    # but the foreman proxy still needs to be able to read the agent's private key
+    if $foreman_proxy::manage_puppet_group and $foreman_proxy::ssl {
+      if !defined(Group[$foreman_proxy::puppet_group]) {
+        group { $foreman_proxy::puppet_group:
+          ensure => 'present',
+          before => User[$foreman_proxy::user],
+        }
+      }
+      $ssl_dirs_and_files = [
+        $foreman_proxy::ssldir,
+        "${foreman_proxy::ssldir}/private_keys",
+        $foreman_proxy::ssl_ca,
+        $foreman_proxy::ssl_key,
+        $foreman_proxy::ssl_cert,
+      ]
+      file { $ssl_dirs_and_files:
+        group => $foreman_proxy::puppet_group,
+      }
+    }
   }
 }

manifests/init.pp

@@ -90,6 +90,10 @@
 #
 # $puppet_group::               Groups of Foreman proxy user
 #
+# $manage_puppet_group::        Whether to ensure the $puppet_group exists.  Also ensures group owner of ssl keys and certs is $puppet_group
+#                               Not applicable when ssl is false.
+#                               type:boolean
+#
 # $puppet::                     Enable Puppet module for environment imports and Puppet runs
 #                               type:boolean
 #
@@ -313,6 +317,7 @@
   $puppetdir                  = $foreman_proxy::params::puppetdir,
   $puppetca_cmd               = $foreman_proxy::params::puppetca_cmd,
   $puppet_group               = $foreman_proxy::params::puppet_group,
+  $manage_puppet_group        = $foreman_proxy::params::manage_puppet_group,
   $puppet                     = $foreman_proxy::params::puppet,
   $puppet_listen_on           = $foreman_proxy::params::puppet_listen_on,
   $puppetrun_cmd              = $foreman_proxy::params::puppetrun_cmd,
@@ -398,7 +403,7 @@
 
   # Validate misc params
   validate_string($bind_host)
-  validate_bool($ssl, $manage_sudoersd, $use_sudoersd, $register_in_foreman)
+  validate_bool($ssl, $manage_sudoersd, $use_sudoersd, $register_in_foreman, $manage_puppet_group)
   validate_array($trusted_hosts, $ssl_disabled_ciphers)
   validate_re($log_level, '^(UNKNOWN|FATAL|ERROR|WARN|INFO|DEBUG)$')
   validate_re($plugin_version, '^(installed|present|latest|absent)$')

manifests/params.pp

@@ -157,6 +157,9 @@
   $puppet_group       = 'puppet'
   $puppetdir          = $puppet::params::dir
 
+  # The puppet-agent package, (puppet 4 AIO) doesn't create a puppet group
+  $manage_puppet_group = versioncmp($::puppetversion, '4.0') > 0
+
   # puppetrun settings
   $puppet = true
   $puppet_listen_on = 'https'

spec/classes/foreman_proxy__config__spec.rb

@@ -338,6 +338,10 @@
         it "should not manage #{etc_dir}/sudoers.d" do
           should contain_file("#{etc_dir}/sudoers.d").with_ensure('directory')
         end
+
+        it "should not manage puppet group" do
+          should_not contain_group('puppet')
+        end
       end
 
       context 'with custom foreman_ssl params' do
@@ -971,6 +975,59 @@
           ])
         end
       end
+      describe 'manage_puppet_group' do
+        context 'when puppet and puppetca are false' do
+          context 'when manage_puppet_group = true and ssl = true' do
+            let :pre_condition do
+              'class {"foreman_proxy":
+                puppet              => false,
+                puppetca            => false,
+                manage_puppet_group => true,
+                ssl                 => true,
+              }'
+            end
+
+            it 'manages puppet group' do
+              should contain_group('puppet').with_ensure('present').that_comes_before("User[#{proxy_user_name}]")
+            end
+
+            it 'sets group ownership to puppet on ssl files' do
+              should contain_file("#{var_dir}/ssl").with_group('puppet')
+              should contain_file("#{var_dir}/ssl/private_keys").with_group('puppet')
+              should contain_file("#{var_dir}/ssl/certs/ca.pem").with_group('puppet')
+              should contain_file("#{var_dir}/ssl/certs/#{facts[:fqdn]}.pem").with_group('puppet')
+              should contain_file("#{var_dir}/ssl/private_keys/#{facts[:fqdn]}.pem").with_group('puppet')
+            end
+            context 'when puppet group is already being managed' do
+              let :pre_condition do
+                'group {"puppet": ensure => present}
+                 class {"foreman_proxy":
+                  puppet              => false,
+                  puppetca            => false,
+                  manage_puppet_group => true,
+                  ssl                 => true,
+                 }'
+            end
+              it 'does not manage puppet group' do
+                should_not contain_group('puppet').with_before("User[#{proxy_user_name}]")
+              end
+            end
+          end
+          context 'when manage_puppet_group = true and ssl = false' do
+            let :pre_condition do
+              'class {"foreman_proxy":
+                puppet              => false,
+                puppetca            => false,
+                manage_puppet_group => true,
+                ssl                 => false,
+              }'
+            end
+            it 'does not manage puppet group' do
+              should_not contain_group('puppet')
+            end
+          end
+        end
+      end
     end
   end
 end