Harden PPA defined type

chelnak · chelnak · commit c26ad2a54f31 · 2022-08-18T07:28:14.000Z
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
@@ -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
@@ -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
@@ -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
@@ -91,7 +91,7 @@
         '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
@@ -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 @@
     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 @@
     $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 @@
       $_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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -91,7 +91,7 @@`
`91`	`91`	`'key' => '630239CC130E1A7FD81A27B140976EAF437D05B5',`
`92`	`92`	`'repos' => 'main universe multiverse restricted',`
`93`	`93`	`}`
`94`		`- $ppa_options = '-y'`
	`94`	`+ $ppa_options = ['-y']`
`95`	`95`	`$ppa_package = 'software-properties-common'`
`96`	`96`	`$auth_conf_owner = '_apt'`
`97`	`97`	`}`