Merge pull request #1052 from puppetlabs/maint-harden_ppa

Harden PPA defined type

diff --git a/examples/mark.pp b/examples/mark.pp
new file mode 100644 (file)
index 0000000..52bae6e
--- /dev/null
+++ b/examples/mark.pp
@@ -0,0 +1,3 @@
+apt::mark { 'vim':
+  setting => 'auto',
+}

diff --git a/lib/puppet/provider/apt_key/apt_key.rb b/lib/puppet/provider/apt_key/apt_key.rb
index 56f9a072e518b25c83f06c4a5c846dc7492dbbfe..115ed316de3288b0e54ea84e27c71fa1dda00320 100644 (file)
--- a/lib/puppet/provider/apt_key/apt_key.rb
+++ b/lib/puppet/provider/apt_key/apt_key.rb
@@ -1,7 +1,11 @@
 # frozen_string_literal: true
 
 require 'open-uri'
-require 'net/ftp'
+begin
+  require 'net/ftp'
+rescue LoadError
+  # Ruby 3.0 changed net-ftp to a default gem
+end
 require 'tempfile'
 
 Puppet::Type.type(:apt_key).provide(:apt_key) do
@@ -124,6 +128,9 @@ Puppet::Type.type(:apt_key).provide(:apt_key) do
       f.close
       f
     else
+      exceptions = [OpenURI::HTTPError]
+      exceptions << Net::FTPPermError if defined?(Net::FTPPermError)
+
       begin
         # Only send basic auth if URL contains userinfo
         # Some webservers (e.g. Amazon S3) return code 400 if empty basic auth is sent
@@ -138,7 +145,7 @@ Puppet::Type.type(:apt_key).provide(:apt_key) do
           parsed_value.userinfo = ''
           key = open(parsed_value, http_basic_authentication: user_pass).read
         end
-      rescue OpenURI::HTTPError, Net::FTPPermError => e
+      rescue *exceptions => e
         raise(_('%{_e} for %{_resource}') % { _e: e.message, _resource: resource[:source] })
       rescue SocketError
         raise(_('could not resolve %{_resource}') % { _resource: resource[:source] })

diff --git a/manifests/mark.pp b/manifests/mark.pp
index f4b7de0a37bf65f4ddbd9342c0d10b1ff8c288c2..08e396c02f1e28fe93e0bedca97b2f08eef395f6 100644 (file)
--- a/manifests/mark.pp
+++ b/manifests/mark.pp
@@ -8,16 +8,31 @@
 define apt::mark (
   Enum['auto','manual','hold','unhold'] $setting,
 ) {
-  case $setting {
-    'unhold': {
-      $unless_cmd = undef
-    }
-    default: {
-      $unless_cmd = "/usr/bin/apt-mark show${setting} ${title} | /bin/fgrep -qs ${title}"
-    }
+  if $title !~ /^[a-zA-Z0-9\-_]+$/ {
+    fail("Invalid package name: ${title}")
   }
-  exec { "/usr/bin/apt-mark ${setting} ${title}":
-    onlyif => "/usr/bin/dpkg -l ${title}",
-    unless => $unless_cmd,
+
+  if $setting == 'unhold' {
+    $unless_cmd = undef
+  } else {
+    $action = "show${setting}"
+
+    # It would be ideal if we could break out this command in to an array of args, similar
+    # to $onlyif_cmd and $command. However, in this case it wouldn't work as expected due
+    # to the inclusion of a pipe character.
+    # When passed to the exec function, the posix provider will strip everything to the right of the pipe,
+    # causing the command to return a full list of packages for the given action.
+    # The trade off is to use an interpolated string knowing that action is built from an enum value and
+    # title is pre-validated.
+    $unless_cmd = ["/usr/bin/apt-mark ${action} ${title} | grep ${title} -q"]
+  }
+
+  $onlyif_cmd = [['/usr/bin/dpkg', '-l', $title]]
+  $command = ['/usr/bin/apt-mark', $setting, $title]
+
+  exec { "apt-mark ${setting} ${title}":
+    command => $command,
+    onlyif  => $onlyif_cmd,
+    unless  => $unless_cmd,
   }
 }

diff --git a/spec/defines/mark_spec.rb b/spec/defines/mark_spec.rb
index b451bedd30673e20813b4519a91619a49e40c174..6c673c41c36ae4fbd48470daba9a2f8acedfadef 100644 (file)
--- a/spec/defines/mark_spec.rb
+++ b/spec/defines/mark_spec.rb
@@ -32,7 +32,7 @@ describe 'apt::mark', type: :define do
     end
 
     it {
-      is_expected.to contain_exec('/usr/bin/apt-mark manual my_source')
+      is_expected.to contain_exec('apt-mark manual my_source')
     }
   end
 
@@ -47,4 +47,50 @@ describe 'apt::mark', type: :define do
       is_expected.to raise_error(Puppet::PreformattedError, %r{expects a match for Enum\['auto', 'hold', 'manual', 'unhold'\], got 'foobar'})
     end
   end
+
+  [
+    'package',
+    'package1',
+    'package_name',
+    'package-name',
+  ].each do |value|
+    describe 'with a valid resource title' do
+      let :title do
+        value
+      end
+
+      let :params do
+        {
+          'setting' => 'manual',
+        }
+      end
+
+      it do
+        is_expected.to contain_exec("apt-mark manual #{title}")
+      end
+    end
+  end
+
+  [
+    '|| ls -la ||',
+    'packakge with space',
+    'package<>|',
+    '|| touch /tmp/foo.txt ||',
+  ].each do |value|
+    describe 'with an invalid resource title' do
+      let :title do
+        value
+      end
+
+      let :params do
+        {
+          'setting' => 'manual',
+        }
+      end
+
+      it do
+        is_expected.to raise_error(Puppet::PreformattedError, %r{Invalid package name: #{title}})
+      end
+    end
+  end
 end