Refactor SSHD config to allow both SSHD options and banner/motd to be set

In https://review.openstack.org/#/c/444622/7 the sshd_options and banner/motd
are mutually exclusive. This patch, and the next patchset of that review,
resolves the conflict.

Related-Bug: 1668543

Change-Id: I1d09530d69e42c0c36311789166554a889e46556

manifests/profile/base/sshd.pp

@@ -27,14 +27,19 @@
 #   The text used within SSH Banner
 #   Defaults to hiera('MOTD')
 #
+# [*options*]
+#   Hash of SSHD options to set. See the puppet-ssh module documentation for
+#   details.
+#   Defaults to {}
+
 class tripleo::profile::base::sshd (
   $bannertext = hiera('BannerText', undef),
   $motd = hiera('MOTD', undef),
+  $options = {}
 ) {
 
-  include ::ssh::server
+  if $bannertext and $bannertext != '' {
-
+    $sshd_options_banner = {'Banner' => '/etc/issue.net'}
-  if $bannertext {
     $filelist = [ '/etc/issue', '/etc/issue.net', ]
     file { $filelist:
       ensure  => file,
@@ -44,9 +49,12 @@ class tripleo::profile::base::sshd (
       group   => 'root',
       mode    => '0644'
     }
+  } else {
+    $sshd_options_banner = {}
   }
 
-  if $motd {
+  if $motd and $motd != '' {
+    $sshd_options_motd = {'PrintMotd' => 'yes'}
     file { '/etc/motd':
       ensure  => file,
       backup  => false,
@@ -55,5 +63,23 @@ class tripleo::profile::base::sshd (
       group   => 'root',
       mode    => '0644'
     }
+  } else {
+    $sshd_options_motd = {}
+  }
+
+  $sshd_options = merge(
+    $options,
+    $sshd_options_banner,
+    $sshd_options_motd
+  )
+
+  # NB (owalsh) in puppet-ssh hiera takes precedence over the class param
+  # we need to control this, so error if it's set in hiera
+  if hiera('ssh:server::options', undef) {
+    err('ssh:server::options must not be set, use tripleo::profile::base::sshd::options')
+  }
+  class { '::ssh::server':
+    storeconfigs_enabled => false,
+    options              => $sshd_options
   }
 }

spec/classes/tripleo_profile_base_sshd_spec.rb

@@ -24,7 +24,23 @@ describe 'tripleo::profile::base::sshd' do
 
     context 'it should do nothing' do
       it do
-        is_expected.to contain_class('ssh::server')
+        is_expected.to contain_class('ssh::server').with({
+          'storeconfigs_enabled' => false,
+          'options' => {}
+        })
+        is_expected.to_not contain_file('/etc/issue')
+        is_expected.to_not contain_file('/etc/issue.net')
+        is_expected.to_not contain_file('/etc/motd')
+      end
+    end
+
+    context 'it should do nothing with empty strings' do
+      let(:params) {{ :bannertext => '', :motd => '' }}
+      it do
+        is_expected.to contain_class('ssh::server').with({
+          'storeconfigs_enabled' => false,
+          'options' => {}
+        })
         is_expected.to_not contain_file('/etc/issue')
         is_expected.to_not contain_file('/etc/issue.net')
         is_expected.to_not contain_file('/etc/motd')
@@ -34,6 +50,12 @@ describe 'tripleo::profile::base::sshd' do
     context 'with issue and issue.net configured' do
       let(:params) {{ :bannertext => 'foo' }}
       it do
+        is_expected.to contain_class('ssh::server').with({
+          'storeconfigs_enabled' => false,
+          'options' => {
+            'Banner' => '/etc/issue.net'
+          }
+        })
         is_expected.to contain_file('/etc/issue').with({
           'content' => 'foo',
           'owner'   => 'root',
@@ -53,6 +75,12 @@ describe 'tripleo::profile::base::sshd' do
     context 'with motd configured' do
       let(:params) {{ :motd => 'foo' }}
       it do
+        is_expected.to contain_class('ssh::server').with({
+          'storeconfigs_enabled' => false,
+          'options' => {
+            'PrintMotd' => 'yes'
+          }
+        })
         is_expected.to contain_file('/etc/motd').with({
           'content' => 'foo',
           'owner'   => 'root',
@@ -63,6 +91,94 @@ describe 'tripleo::profile::base::sshd' do
         is_expected.to_not contain_file('/etc/issue.net')
       end
     end
+
+    context 'with options configured' do
+      let(:params) {{ :options => {'X11Forwarding' => 'no'} }}
+      it do
+        is_expected.to contain_class('ssh::server').with({
+          'storeconfigs_enabled' => false,
+          'options' => {
+            'X11Forwarding' => 'no'
+          }
+        })
+        is_expected.to_not contain_file('/etc/motd')
+        is_expected.to_not contain_file('/etc/issue')
+        is_expected.to_not contain_file('/etc/issue.net')
+      end
+    end
+
+    context 'with motd and issue configured' do
+      let(:params) {{
+        :bannertext => 'foo',
+        :motd => 'foo'
+      }}
+      it do
+        is_expected.to contain_class('ssh::server').with({
+          'storeconfigs_enabled' => false,
+          'options' => {
+            'Banner' => '/etc/issue.net',
+            'PrintMotd' => 'yes'
+          }
+        })
+        is_expected.to contain_file('/etc/motd').with({
+          'content' => 'foo',
+          'owner'   => 'root',
+          'group'   => 'root',
+          'mode'    => '0644',
+          })
+        is_expected.to contain_file('/etc/issue').with({
+          'content' => 'foo',
+          'owner'   => 'root',
+          'group'   => 'root',
+          'mode'    => '0644',
+          })
+        is_expected.to contain_file('/etc/issue.net').with({
+          'content' => 'foo',
+          'owner'   => 'root',
+          'group'   => 'root',
+          'mode'    => '0644',
+          })
+      end
+    end
+
+    context 'with motd and issue and options configured' do
+      let(:params) {{
+        :bannertext => 'foo',
+        :motd => 'foo',
+        :options => {
+          'PrintMotd' => 'no', # this should be overridden
+          'X11Forwarding' => 'no'
+        }
+      }}
+      it do
+        is_expected.to contain_class('ssh::server').with({
+          'storeconfigs_enabled' => false,
+          'options' => {
+            'Banner' => '/etc/issue.net',
+            'PrintMotd' => 'yes',
+            'X11Forwarding' => 'no'
+          }
+        })
+        is_expected.to contain_file('/etc/motd').with({
+          'content' => 'foo',
+          'owner'   => 'root',
+          'group'   => 'root',
+          'mode'    => '0644',
+          })
+        is_expected.to contain_file('/etc/issue').with({
+          'content' => 'foo',
+          'owner'   => 'root',
+          'group'   => 'root',
+          'mode'    => '0644',
+          })
+        is_expected.to contain_file('/etc/issue.net').with({
+          'content' => 'foo',
+          'owner'   => 'root',
+          'group'   => 'root',
+          'mode'    => '0644',
+          })
+      end
+    end
   end
 
   on_supported_os.each do |os, facts|