Harden PPA defined type

Prior to this commit there was a possibility that malformed strings
could be passed as the resources name. This could lead to unsafe
executions on a remote system.

This was also a possibility for the options parameter as it was
constrained to a string.

In addition, commands were not properly broken out in to arrays of
arguments when passed to the exec resource.

This commit fixes the above by adding validation to the resource name
ensuring that the given ppa name conforms to expectation. Also, commands
are now broken down in to arrays of arguments appropriately. This ensures
safer execution on the remote system.

Given that the options parameter, passed as a raw string, could lead to
unsafe code execution it was reasonable to change the accepted type to
an `Optional[Array[String]]. This means that an array of options can now
be passed to the exec resource inside the original command.

diff --git a/examples/ppa.pp b/examples/ppa.pp
index 1dea2b4f207531c9d44463de98fe93a33bc56276..4f4432a6fc520f45a6b08e180190ead4769f4960 100644 (file)
--- a/examples/ppa.pp
+++ b/examples/ppa.pp
@@ -1,4 +1,4 @@
 class { 'apt': }
 
 # Example declaration of an Apt PPA
-apt::ppa { 'ppa:openstack-ppa/bleeding-edge': }
+apt::ppa { 'ppa:ubuntuhandbook1/apps': }

diff --git a/lib/facter/apt_sources.rb b/lib/facter/apt_sources.rb
new file mode 100644 (file)
index 0000000..c985802
--- /dev/null
+++ b/lib/facter/apt_sources.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+# This fact lists the .list filenames that are used by apt.
+Facter.add(:apt_sources) do
+  confine osfamily: 'Debian'
+  setcode do
+    sources = ['sources.list']
+    Dir.glob('/etc/apt/sources.list.d/*.list').each do |file|
+      sources.push(File.basename(file))
+    end
+    sources
+  end
+end

diff --git a/manifests/init.pp b/manifests/init.pp
index 704311061b6df0e7f6d649c70721554e29d84ad1..2de585c22c669727111f8089f1c966a69bf41b77 100644 (file)
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -37,10 +37,14 @@
 #   Configures various update settings. Valid options: a hash made up from the following keys:
 #
 # @option update [String] :frequency
-#   Specifies how often to run `apt-get update`. If the exec resource `apt_update` is notified, `apt-get update` runs regardless of this value.
-#   Valid options: 'always' (at every Puppet run); 'daily' (if the value of `apt_update_last_success` is less than current epoch time minus 86400);
-#   'weekly' (if the value of `apt_update_last_success` is less than current epoch time minus 604800); and 'reluctantly' (only if the exec resource
-#   `apt_update` is notified). Default: 'reluctantly'.
+#   Specifies how often to run `apt-get update`. If the exec resource `apt_update` is notified,
+#   `apt-get update` runs regardless of this value.
+#   Valid options:
+#     'always' (at every Puppet run); 
+#      daily' (if the value of `apt_update_last_success` is less than current epoch time minus 86400);
+#     'weekly' (if the value of `apt_update_last_success` is less than current epoch time minus 604800);
+#     'reluctantly' (only if the exec resource `apt_update` is notified).
+#   Default: 'reluctantly'.
 #
 # @option update [Integer] :loglevel
 #   Specifies the log level of logs outputted to the console. Default: undef.
@@ -122,38 +126,37 @@
 #   Specifies whether to perform force purge or delete. Default false.
 #
 class apt (
-  Hash $update_defaults         = $apt::params::update_defaults,
-  Hash $purge_defaults          = $apt::params::purge_defaults,
-  Hash $proxy_defaults          = $apt::params::proxy_defaults,
-  Hash $include_defaults        = $apt::params::include_defaults,
-  String $provider              = $apt::params::provider,
-  String $keyserver             = $apt::params::keyserver,
-  Optional[String] $key_options = $apt::params::key_options,
-  Optional[String] $ppa_options = $apt::params::ppa_options,
-  Optional[String] $ppa_package = $apt::params::ppa_package,
-  Optional[Hash] $backports     = $apt::params::backports,
-  Hash $confs                   = $apt::params::confs,
-  Hash $update                  = $apt::params::update,
-  Hash $purge                   = $apt::params::purge,
-  Apt::Proxy $proxy             = $apt::params::proxy,
-  Hash $sources                 = $apt::params::sources,
-  Hash $keys                    = $apt::params::keys,
-  Hash $ppas                    = $apt::params::ppas,
-  Hash $pins                    = $apt::params::pins,
-  Hash $settings                = $apt::params::settings,
-  Boolean $manage_auth_conf     = $apt::params::manage_auth_conf,
-  Array[Apt::Auth_conf_entry]
-  $auth_conf_entries            = $apt::params::auth_conf_entries,
-  String $auth_conf_owner       = $apt::params::auth_conf_owner,
-  String $root                  = $apt::params::root,
-  String $sources_list          = $apt::params::sources_list,
-  String $sources_list_d        = $apt::params::sources_list_d,
-  String $conf_d                = $apt::params::conf_d,
-  String $preferences           = $apt::params::preferences,
-  String $preferences_d         = $apt::params::preferences_d,
-  String $apt_conf_d            = $apt::params::apt_conf_d,
-  Hash $config_files            = $apt::params::config_files,
-  Boolean $sources_list_force   = $apt::params::sources_list_force,
+  Hash $update_defaults                           = $apt::params::update_defaults,
+  Hash $purge_defaults                            = $apt::params::purge_defaults,
+  Hash $proxy_defaults                            = $apt::params::proxy_defaults,
+  Hash $include_defaults                          = $apt::params::include_defaults,
+  String $provider                                = $apt::params::provider,
+  String $keyserver                               = $apt::params::keyserver,
+  Optional[String] $key_options                   = $apt::params::key_options,
+  Optional[Array[String]] $ppa_options            = $apt::params::ppa_options,
+  Optional[String] $ppa_package                   = $apt::params::ppa_package,
+  Optional[Hash] $backports                       = $apt::params::backports,
+  Hash $confs                                     = $apt::params::confs,
+  Hash $update                                    = $apt::params::update,
+  Hash $purge                                     = $apt::params::purge,
+  Apt::Proxy $proxy                               = $apt::params::proxy,
+  Hash $sources                                   = $apt::params::sources,
+  Hash $keys                                      = $apt::params::keys,
+  Hash $ppas                                      = $apt::params::ppas,
+  Hash $pins                                      = $apt::params::pins,
+  Hash $settings                                  = $apt::params::settings,
+  Boolean $manage_auth_conf                       = $apt::params::manage_auth_conf,
+  Array[Apt::Auth_conf_entry] $auth_conf_entries  = $apt::params::auth_conf_entries,
+  String $auth_conf_owner                         = $apt::params::auth_conf_owner,
+  String $root                                    = $apt::params::root,
+  String $sources_list                            = $apt::params::sources_list,
+  String $sources_list_d                          = $apt::params::sources_list_d,
+  String $conf_d                                  = $apt::params::conf_d,
+  String $preferences                             = $apt::params::preferences,
+  String $preferences_d                           = $apt::params::preferences_d,
+  String $apt_conf_d                              = $apt::params::apt_conf_d,
+  Hash $config_files                              = $apt::params::config_files,
+  Boolean $sources_list_force                     = $apt::params::sources_list_force,
 
   Hash $source_key_defaults = {
     'server'  => $keyserver,

diff --git a/manifests/params.pp b/manifests/params.pp
index 5ef175b713d46b5f7caca0e9102b8aa0958160a4..f045491acf1e673307f74405a06a2aceddc0feac 100644 (file)
--- a/manifests/params.pp
+++ b/manifests/params.pp
@@ -91,7 +91,7 @@ class apt::params {
         'key'      => '630239CC130E1A7FD81A27B140976EAF437D05B5',
         'repos'    => 'main universe multiverse restricted',
       }
-      $ppa_options        = '-y'
+      $ppa_options        = ['-y']
       $ppa_package        = 'software-properties-common'
       $auth_conf_owner = '_apt'
     }

diff --git a/manifests/ppa.pp b/manifests/ppa.pp
index bab7b337e3dbc4918982517e35f520b9b7bcd02d..39852d1b025d1d0af0a89bff45e0fb07b60cfc94 100644 (file)
--- a/manifests/ppa.pp
+++ b/manifests/ppa.pp
@@ -24,12 +24,12 @@
 #   Specifies whether Puppet should manage the package that provides `apt-add-repository`.
 #
 define apt::ppa (
-  String $ensure                 = 'present',
-  Optional[String] $options      = $::apt::ppa_options,
-  Optional[String] $release      = fact('os.distro.codename'),
-  Optional[String] $dist         = $facts['os']['name'],
-  Optional[String] $package_name = $::apt::ppa_package,
-  Boolean $package_manage        = false,
+  String $ensure                        = 'present',
+  Optional[Array[String]] $options      = $::apt::ppa_options,
+  Optional[String] $release             = fact('os.distro.codename'),
+  Optional[String] $dist                = $facts['os']['name'],
+  Optional[String] $package_name        = $::apt::ppa_package,
+  Boolean $package_manage               = false,
 ) {
   unless $release {
     fail('os.distro.codename fact not available: release parameter required')
@@ -39,6 +39,11 @@ define apt::ppa (
     fail('apt::ppa is not currently supported on Debian.')
   }
 
+  # Validate the resource name
+  if $name !~ /^ppa:([a-zA-Z0-9\-_]+)\/([a-zA-z0-9\-_]+)$/ {
+    fail("Invalid PPA name: ${name}")
+  }
+
   if versioncmp($facts['os']['release']['full'], '14.10') >= 0 {
     $distid = downcase($dist)
     $dash_filename = regsubst($name, '^ppa:([^/]+)/(.+)$', "\\1-${distid}-\\2")
@@ -62,6 +67,9 @@ define apt::ppa (
     $trusted_gpg_d_filename = "${dash_filename_no_specialchars}.gpg"
   }
 
+  # This is the location of our main exec script  
+  $script_path = "/opt/puppetlabs/puppet/cache/add-apt-repository-${dash_filename_no_specialchars}-${release}.sh"
+
   if $ensure == 'present' {
     if $package_manage {
       ensure_packages($package_name)
@@ -81,24 +89,36 @@ define apt::ppa (
       $_proxy_env = []
     }
 
-    exec { "add-apt-repository-${name}":
-      environment => $_proxy_env,
-      command     => "/usr/bin/add-apt-repository ${options} ${name} || (rm ${::apt::sources_list_d}/${sources_list_d_filename} && false)",
-      unless      => "/usr/bin/test -f ${::apt::sources_list_d}/${sources_list_d_filename} && /usr/bin/test -f ${::apt::trusted_gpg_d}/${trusted_gpg_d_filename}",
-      user        => 'root',
-      logoutput   => 'on_failure',
-      notify      => Class['apt::update'],
-      require     => $_require,
-    }
+    unless $sources_list_d_filename in $facts['apt_sources'] {
+      $script_content = epp('apt/add-apt-repository.sh.epp', {
+        command                 => ['/usr/bin/add-apt-repository', shell_join($options), $name],
+        sources_list_d_path     => $::apt::sources_list_d,
+        sources_list_d_filename => $sources_list_d_filename,
+      })
 
-    file { "${::apt::sources_list_d}/${sources_list_d_filename}":
-      ensure  => file,
-      require => Exec["add-apt-repository-${name}"],
+      file { "add-apt-repository-script-${name}":
+        ensure  => 'file',
+        path    => $script_path,
+        content => $script_content,
+        mode    => '0755',
+      }
+
+      exec { "add-apt-repository-${name}":
+        environment => $_proxy_env,
+        command     => $script_path,
+        logoutput   => 'on_failure',
+        notify      => Class['apt::update'],
+        require     => $_require,
+      }
     }
   }
   else {
-    file { "${::apt::sources_list_d}/${sources_list_d_filename}":
-      ensure => 'absent',
+    tidy { "remove-apt-repository-script-${name}":
+      path => $script_path,
+    }
+
+    tidy { "remove-apt-repository-${name}":
+      path   => "${::apt::sources_list_d}/${sources_list_d_filename}",
       notify => Class['apt::update'],
     }
   }

diff --git a/templates/add-apt-repository.sh.epp b/templates/add-apt-repository.sh.epp
new file mode 100644 (file)
index 0000000..e23c485
--- /dev/null
+++ b/templates/add-apt-repository.sh.epp
@@ -0,0 +1,8 @@
+<%- | Array $command, String $sources_list_d_path, String $sources_list_d_filename | -%>
+
+<%= $command.join(' ') %>
+
+if [ $? -gt 0 ]; then
+   rm <%= $sources_list_d_path %>/<%= $sources_list_d_filename %>
+   exit 1
+fi