Add option to restrict API access to specific hosts

[teluq-pbrideau]

Pull Request (PR) description

By default, the api is open to everyone. I would like to restrict it to only the zabbix server.

I’m not sure if it should be included in this module like I suggest, or if I should manage my own apache config by disabling this module apache management manage_vhost => false in web.pp, but I think the changes are simple enough to include in this module

My suggestion in this PR allow to add restriction to the api like so:

class { 'zabbix::web' :
  [...]
  zabbix_api_access => [$facts['networking']['fqdn']],
}

This creates this location entry in apache config (or equivalent in apache 2.2):

<Location "/api_jsonrpc.php" >
  Require host zabbix.example.com
</Location>

This Pull Request (PR) fixes the following issues

[teluq-pbrideau]

I’ve banged my head to try to write a test but I cannot find the right syntax… Would someone have a pointer about it?

        describe 'with restriction to api access' do
          let :params do
            super().merge(
              zabbix_api_access: [ '127.0.0.1' ],
            )
          end

          it { is_expected.to contain_apache__vhost('zabbix.example.com').with_directories(
            including(
              {"path"=>"/api_jsonrpc.php", "provider"=>"location", "require"=>["host 127.0.0.1"]}
            )
          )}
        end

I’ve tried include, including, a_hash_including…

The closer I got it to run is by defining the entire array:

          it { is_expected.to contain_apache__vhost('zabbix.example.com').with_directories([
            {"path"=>"/usr/share/zabbix", "provider"=>"directory", "require"=>"all granted"},
            {"path"=>"/usr/share/zabbix/conf", "provider"=>"directory", "require"=>"all denied"},
            {"path"=>"/usr/share/zabbix/api", "provider"=>"directory", "require"=>"all denied"},
            {"path"=>"/usr/share/zabbix/include", "provider"=>"directory", "require"=>"all denied"},
            {"path"=>"/usr/share/zabbix/include/classes", "provider"=>"directory", "require"=>"all denied"},
            {"path"=>"/api_jsonrpc.php", "provider"=>"location", "require"=>["host 127.0.0.1"]}
          ])}

But this does not work in every tests because there is conditions :

    if $facts['os']['family'] == 'RedHat' and versioncmp($facts['os']['release']['major'], '7') >= 0 and versioncmp($zabbix_version, '5') >= 0 {
      if versioncmp($facts['os']['release']['major'], '7') == 0 {
        $fpm_service = 'rh-php72-php-fpm'
        # PHP parameters are moved to /etc/opt/rh/rh-php72/php-fpm.d/zabbix.conf per package zabbix-web-deps-scl
        $fpm_scl_prefix = '/opt/rh/rh-php72'
      } else {
        $fpm_service = 'php-fpm'
        $fpm_scl_prefix = ''
      }
      $fcgi_filematch = {
        path     => '/usr/share/zabbix',
        provider => 'directory',
        addhandlers => [
          {
            extensions => [
              'php',
              'phar',
            ],
            handler => "proxy:unix:/var${fpm_scl_prefix}/run/php-fpm/zabbix.sock|fcgi://localhost",
          },
        ],
      }

    apache::vhost { $zabbix_url:
      directories     => [
        merge( {
            path     => '/usr/share/zabbix',
            provider => 'directory',
            require  => 'all granted',
          }, $fcgi_filematch
        ),
    [...]

It feels wrong to me to duplicate all this logic inside the test, is it the way to go?

[teluq-pbrideau]

Or should I make a less precise test like what is done in apache module:

              it {
                is_expected.to contain_concat__fragment('zabbix.example.com-directories').with(
                  content: %r{^\s+Require host 127.0.0.1$},
                )
              }

[root-expert]

Or should I make a less precise test like what is done in apache module:

              it {
                is_expected.to contain_concat__fragment('zabbix.example.com-directories').with(
                  content: %r{^\s+Require host 127.0.0.1$},
                )
              }

Although the first option is better that's also okay for me. Probably you need something like this though:

               it {
                 is_expected.to contain_concat__fragment('zabbix.example.com-directories').with(
                   content: %r{^\s*Require host 127\.0\.0.\1$},
                 )
               }

Dot matches everything except new lines, so you need to escape it 😄

[teluq-pbrideau]

Note: The debian11 test has crashed out of my scope

  Error: Could not set 'present' on ensure: Connection reset by peer - SSL_connect (file: /etc/puppetlabs/code/environments/production/modules/apt/manifests/key.pp, line: 54)

[root-expert]

@teluq-pbrideau Can you please rebase with our master branch one last time before merging this?