From 04db75783624ba52185e34fcff3959dc8d8f24ce Mon Sep 17 00:00:00 2001 From: Oliver Walsh Date: Fri, 5 May 2017 01:30:21 +0100 Subject: [PATCH] Handle duplicate/invalid entries in migration SSH inbound addresses An error (e.g a typo) in a custom tripleo-heat-templates environment file could lead to an invalid match block in /etc/ssh/sshd_config. SSH fails-safe and refuses all logins in this case. This change validates the migration_ssh_localaddrs parameter is an array of IP addresses and removes and duplicate entries. Ica3f79d6d0cfae446e276172146f3a9407f2971f requires this to remove duplicates. Change-Id: Ibcf144d960fe52f0eab0d5015bd30cf7c1e37e25 Closes-Bug: #1688308 (cherry picked from commit 05e696c62d02ef64180d611413ae10f0418c002a) (cherry picked from commit 3d36307bcb2a75933945f5ab5d4241d0e6051cce) --- .../parser/functions/validate_array_of_ips.rb | 32 ++++++ manifests/profile/base/nova.pp | 10 +- .../classes/tripleo_profile_base_nova_spec.rb | 98 +++++++++++++++++++ 3 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 lib/puppet/parser/functions/validate_array_of_ips.rb diff --git a/lib/puppet/parser/functions/validate_array_of_ips.rb b/lib/puppet/parser/functions/validate_array_of_ips.rb new file mode 100644 index 000000000..014a75707 --- /dev/null +++ b/lib/puppet/parser/functions/validate_array_of_ips.rb @@ -0,0 +1,32 @@ +# Custom function to validate an array of ips +# Based on validate_ip_address() in stdlib +module Puppet::Parser::Functions + + newfunction(:validate_array_of_ips) do |argv| + + args = argv[0] + + require "ipaddr" + rescuable_exceptions = [ ArgumentError ] + + if defined?(IPAddr::InvalidAddressError) + rescuable_exceptions << IPAddr::InvalidAddressError + end + + args.each do |arg| + unless arg.is_a?(String) + raise Puppet::ParseError, "#{arg.inspect} is not a string." + end + + begin + unless IPAddr.new(arg).ipv4? or IPAddr.new(arg).ipv6? + raise Puppet::ParseError, "#{arg.inspect} is not a valid IP address." + end + rescue *rescuable_exceptions + raise Puppet::ParseError, "#{arg.inspect} is not a valid IP address." + end + end + + end + +end \ No newline at end of file diff --git a/manifests/profile/base/nova.pp b/manifests/profile/base/nova.pp index 0b4570ee1..84ba604fc 100644 --- a/manifests/profile/base/nova.pp +++ b/manifests/profile/base/nova.pp @@ -84,6 +84,10 @@ class tripleo::profile::base::nova ( $memcache_servers = suffix(hiera('memcached_node_ips'), ':11211') } + validate_array($migration_ssh_localaddrs) + validate_array_of_ips($migration_ssh_localaddrs) + $migration_ssh_localaddrs_real = unique($migration_ssh_localaddrs) + if $step >= 4 or ($step >= 3 and $sync_db) { $rabbit_endpoints = suffix(any2array(normalize_ip_for_uri($rabbit_hosts)), ":${rabbit_port}") class { '::nova' : @@ -124,10 +128,10 @@ class tripleo::profile::base::nova ( # Nova SSH tunnel setup (cold-migration) # Server side - if !empty($migration_ssh_localaddrs) { - $allow_type = sprintf('LocalAddress %s User', join($migration_ssh_localaddrs,',')) + if !empty($migration_ssh_localaddrs_real) { + $allow_type = sprintf('LocalAddress %s User', join($migration_ssh_localaddrs_real,',')) $deny_type = 'LocalAddress' - $deny_name = sprintf('!%s', join($migration_ssh_localaddrs,',!')) + $deny_name = sprintf('!%s', join($migration_ssh_localaddrs_real,',!')) ssh::server::match_block { 'nova_migration deny': name => $deny_name, diff --git a/spec/classes/tripleo_profile_base_nova_spec.rb b/spec/classes/tripleo_profile_base_nova_spec.rb index abeb0ef3e..15b82eb48 100644 --- a/spec/classes/tripleo_profile_base_nova_spec.rb +++ b/spec/classes/tripleo_profile_base_nova_spec.rb @@ -330,6 +330,104 @@ describe 'tripleo::profile::base::nova' do } end + context 'with step 4 with libvirt and migration ssh key and invalid migration_ssh_localaddrs' do + let(:pre_condition) do + <<-eof + include ::nova::compute::libvirt::services + class { '::ssh::server': + storeconfigs_enabled => false, + options => {} + } + eof + end + let(:params) { { + :step => 4, + :libvirt_enabled => true, + :manage_migration => true, + :nova_compute_enabled => true, + :bootstrap_node => 'node.example.com', + :rabbit_hosts => [ '127.0.0.1' ], + :migration_ssh_key => { 'private_key' => 'foo', 'public_key' => 'ssh-rsa bar'}, + :migration_ssh_localaddrs => ['127.0.0.1', ''] + } } + + it { is_expected.to_not compile } + end + + context 'with step 4 with libvirt and migration ssh key and duplicate migration_ssh_localaddrs' do + let(:pre_condition) do + <<-eof + include ::nova::compute::libvirt::services + class { '::ssh::server': + storeconfigs_enabled => false, + options => {} + } + eof + end + let(:params) { { + :step => 4, + :libvirt_enabled => true, + :manage_migration => true, + :nova_compute_enabled => true, + :bootstrap_node => 'node.example.com', + :rabbit_hosts => [ '127.0.0.1' ], + :migration_ssh_key => { 'private_key' => 'foo', 'public_key' => 'ssh-rsa bar'}, + :migration_ssh_localaddrs => ['127.0.0.1', '127.0.0.1'] + } } + + it { + is_expected.to contain_class('tripleo::profile::base::nova') + is_expected.to contain_class('nova').with( + :rabbit_hosts => /.+/, + :nova_public_key => nil, + :nova_private_key => nil, + ) + is_expected.to contain_class('nova::config') + is_expected.to contain_class('nova::cache') + is_expected.to contain_class('nova::migration::libvirt').with( + :transport => 'ssh', + :configure_libvirt => params[:libvirt_enabled], + :configure_nova => params[:nova_compute_enabled] + ) + is_expected.to contain_ssh__server__match_block('nova_migration allow').with( + :type => 'LocalAddress 127.0.0.1 User', + :name => 'nova_migration', + :options => { + 'ForceCommand' => '/bin/nova-migration-wrapper', + 'PasswordAuthentication' => 'no', + 'AllowTcpForwarding' => 'no', + 'X11Forwarding' => 'no', + 'AuthorizedKeysFile' => '/etc/nova/migration/authorized_keys' + } + ) + is_expected.to contain_ssh__server__match_block('nova_migration deny').with( + :type => 'LocalAddress', + :name => '!127.0.0.1', + :options => { + 'DenyUsers' => 'nova_migration' + } + ) + is_expected.to contain_package('openstack-nova-migration').with( + :ensure => 'present' + ) + is_expected.to contain_file('/etc/nova/migration/authorized_keys').with( + :content => 'ssh-rsa bar', + :mode => '0640', + :owner => 'root', + :group => 'nova_migration', + ) + is_expected.to contain_file('/etc/nova/migration/identity').with( + :content => 'foo', + :mode => '0600', + :owner => 'nova', + :group => 'nova', + ) + is_expected.to contain_user('nova_migration').with( + :shell => '/bin/bash' + ) + } + end + context 'with step 4 with libvirt TLS and migration ssh key' do let(:pre_condition) do <<-eof