Merge pull request #6498 from chef/apt_repo_cleanup

Apt repo cleanup and testing expansion

2 files changed, 231 insertions, 62 deletions

diff --git a/lib/chef/provider/apt_repository.rb b/lib/chef/provider/apt_repository.rb
index c16f1e5767..1909ed8034 100644
--- a/lib/chef/provider/apt_repository.rb
+++ b/lib/chef/provider/apt_repository.rb
@@ -29,14 +29,15 @@ class Chef
 
       provides :apt_repository, platform_family: "debian"
 
-      LIST_APT_KEYS = "apt-key list".freeze
       LIST_APT_KEY_FINGERPRINTS = "apt-key adv --list-public-keys --with-fingerprint --with-colons".freeze
 
       def load_current_resource
       end
 
       action :add do
-        unless new_resource.key.nil?
+        if new_resource.key.nil?
+          Chef::Log.debug "No 'key' property specified skipping key import"
+        else
           new_resource.key.each do |k|
             if is_key_id?(k) && !has_cookbook_file?(k)
               install_key_from_keyserver(k)
@@ -56,16 +57,10 @@ class Chef
           action :nothing
         end
 
-        components = if is_ppa_url?(new_resource.uri) && new_resource.components.empty?
-                       "main"
-                     else
-                       new_resource.components
-                     end
-
         repo = build_repo(
           new_resource.uri,
           new_resource.distribution,
-          components,
+          repo_components,
           new_resource.trusted,
           new_resource.arch,
           new_resource.deb_src
@@ -96,19 +91,27 @@ class Chef
               ignore_failure true
               action :nothing
             end
-
           end
+        else
+          Chef::Log.debug("/etc/apt/sources.list.d/#{new_resource.name}.list does not exist. Nothing to do")
         end
       end
 
+      # is the provided ID a key ID from a keyserver. Looks at length and HEX only values
+      # @param [String] id the key value passed by the user that *may* be an ID
       def is_key_id?(id)
         id = id[2..-1] if id.start_with?("0x")
         id =~ /^\h+$/ && [8, 16, 40].include?(id.length)
       end
 
+      # run the specified command and extract the fingerprints from the output
+      # accepts a command so it can be used to extract both the current key's fingerprints
+      # and the fingerprint of the new key
+      # @param [String] cmd the command to run
+      #
+      # @return [Array] an array of fingerprints
       def extract_fingerprints_from_cmd(cmd)
         so = shell_out(cmd)
-        so.run_command
         so.stdout.split(/\n/).map do |t|
           if z = t.match(/^fpr:+([0-9A-F]+):/)
             z[1].split.join
@@ -116,11 +119,23 @@ class Chef
         end.compact
       end
 
-      def key_is_valid?(cmd, key)
+      # see if the keyfile is invalid such as a text file that is not actually a gpg key
+      # @param [String] keyfile the path to the keyfile
+      #
+      # @return [Boolean] is the key file invalid
+      def keyfile_is_invalid?(keyfile)
+        so = shell_out("gpg #{keyfile}")
+        so.error?
+      end
+
+      # validate the key against the apt keystore to see if that version is expired
+      # @param [String] key
+      #
+      # @return [Boolean] is the key valid or not
+      def key_is_valid?(key)
         valid = true
 
-        so = shell_out(cmd)
-        so.run_command
+        so = shell_out("apt-key list")
         so.stdout.split(/\n/).map do |t|
           if t =~ %r{^\/#{key}.*\[expired: .*\]$}
             Chef::Log.debug "Found expired key: #{t}"
@@ -133,14 +148,27 @@ class Chef
         valid
       end
 
+      # return the specified cookbook name or the cookbook containing the
+      # resource.
+      #
+      # @return [String] name of the cookbook
       def cookbook_name
         new_resource.cookbook || new_resource.cookbook_name
       end
 
+      # determine if a cookbook file is available in the run
+      # @param [String] path the path to the cookbook file
+      #
+      # @return [Boolean] cookbook file exists or doesn't
       def has_cookbook_file?(fn)
         run_context.has_cookbook_file_in_cookbook?(cookbook_name, fn)
       end
 
+      # determine if there are any new keys by comparing the fingerprints of installed
+      # keys to those of the passed file
+      # @param [String] file the keyfile of the new repository
+      #
+      # @return [Boolean] true: no new keys in the file. false: there are new keys
       def no_new_keys?(file)
         # Now we are using the option --with-colons that works across old os versions
         # as well as the latest (16.10). This for both `apt-key` and `gpg` commands
@@ -149,37 +177,57 @@ class Chef
         (installed_keys & proposed_keys).sort == proposed_keys.sort
       end
 
+      # Given the provided key URI determine what kind of chef resource we need
+      # to fetch the key
+      # @param [String] uri the uri of the gpg key (local path or http URL)
+      #
+      # @raise [Chef::Exceptions::FileNotFound] Key isn't remote or found in the current run
+      #
+      # @return [Symbol] :remote_file or :cookbook_file
+      def key_type(uri)
+        if uri.start_with?("http")
+          :remote_file
+        elsif has_cookbook_file?(uri)
+          :cookbook_file
+        else
+          raise Chef::Exceptions::FileNotFound, "Cannot locate key file: #{uri}"
+        end
+      end
+
+      # Fetch the key using either cookbook_file or remote_file, validate it,
+      # and install it with apt-key add
+      # @param [String] key the key to install
+      #
+      # @raise [RuntimeError] Invalid key which can't verify the apt repository
+      #
+      # @return [void]
       def install_key_from_uri(key)
         key_name = key.gsub(/[^0-9A-Za-z\-]/, "_")
         cached_keyfile = ::File.join(Chef::Config[:file_cache_path], key_name)
-        type = if key.start_with?("http")
-                 :remote_file
-               elsif has_cookbook_file?(key)
-                 :cookbook_file
-               else
-                 raise Chef::Exceptions::FileNotFound, "Cannot locate key file"
-               end
 
-        declare_resource(type, cached_keyfile) do
+        declare_resource(key_type(key), cached_keyfile) do
           source key
           mode "0644"
           sensitive new_resource.sensitive
           action :create
         end
 
-        raise "The key #{cached_keyfile} is invalid and cannot be used to verify an apt repository." unless key_is_valid?("gpg #{cached_keyfile}", "")
+        raise "The key #{cached_keyfile} is invalid and cannot be used to verify an apt repository." if keyfile_is_invalid?(cached_keyfile)
 
         declare_resource(:execute, "apt-key add #{cached_keyfile}") do
           sensitive new_resource.sensitive
           action :run
-          not_if do
-            no_new_keys?(cached_keyfile)
-          end
+          not_if { no_new_keys?(cached_keyfile) }
           notifies :run, "execute[apt-cache gencaches]", :immediately
         end
       end
 
-      def install_key_from_keyserver(key, keyserver = new_resource.keyserver)
+      # build the apt-key command to install the keyserver
+      # @param [String] key the key to install
+      # @param [String] keyserver the key server to use
+      #
+      # @return [String] the full apt-key command to run
+      def keyserver_install_cmd(key, keyserver)
         cmd = "apt-key adv --recv"
         cmd << " --keyserver-options http-proxy=#{new_resource.key_proxy}" if new_resource.key_proxy
         cmd << " --keyserver "
@@ -190,22 +238,37 @@ class Chef
                end
 
         cmd << " #{key}"
+        cmd
+      end
 
+      # @param [String] key
+      # @param [String] keyserver
+      #
+      # @raise [RuntimeError] Invalid key which can't verify the apt repository
+      #
+      # @return [void]
+      def install_key_from_keyserver(key, keyserver = new_resource.keyserver)
         declare_resource(:execute, "install-key #{key}") do
-          command cmd
+          command keyserver_install_cmd(key, keyserver)
           sensitive new_resource.sensitive
           not_if do
             present = extract_fingerprints_from_cmd(LIST_APT_KEY_FINGERPRINTS).any? do |fp|
               fp.end_with? key.upcase
             end
-            present && key_is_valid?(LIST_APT_KEYS, key.upcase)
+            present && key_is_valid?(key.upcase)
           end
           notifies :run, "execute[apt-cache gencaches]", :immediately
         end
 
-        raise "The key #{key} is invalid and cannot be used to verify an apt repository." unless key_is_valid?(LIST_APT_KEYS, key.upcase)
+        raise "The key #{key} is invalid and cannot be used to verify an apt repository." unless key_is_valid?(key.upcase)
       end
 
+      # @param [String] owner
+      # @param [String] repo
+      #
+      # @raise [RuntimeError] Could not access the Launchpad PPA API
+      #
+      # @return [void]
       def install_ppa_key(owner, repo)
         url = "https://launchpad.net/api/1.0/~#{owner}/+archive/#{repo}"
         key_id = Chef::HTTP::Simple.new(url).get("signing_key_fingerprint").delete('"')
@@ -214,12 +277,33 @@ class Chef
         raise "Could not access Launchpad ppa API: #{e.message}"
       end
 
+      # determine if the repository URL is a PPA
+      # @param [String] url the url of the repository
+      #
+      # @return [Boolean] is the repo URL a PPA
       def is_ppa_url?(url)
         url.start_with?("ppa:")
       end
 
+      # determine the repository's components:
+      #  - "components" property if defined
+      #  - "main" if "components" not defined and the repo is a PPA URL
+      #  - otherwise nothing
+      #
+      # @return [String] the repository component
+      def repo_components
+        if is_ppa_url?(new_resource.uri) && new_resource.components.empty?
+          "main"
+        else
+          new_resource.components
+        end
+      end
+
+      # given a PPA return a PPA URL in http://ppa.launchpad.net format
+      # @param [String] ppa the ppa URL
+      #
+      # @return [String] full PPA URL
       def make_ppa_url(ppa)
-        return unless is_ppa_url?(ppa)
         owner, repo = ppa[4..-1].split("/")
         repo ||= "ppa"
 
@@ -227,6 +311,14 @@ class Chef
         "http://ppa.launchpad.net/#{owner}/#{repo}/ubuntu"
       end
 
+      # build complete repo text that will be written to the config
+      # @param [String] uri
+      # @param [Array] components
+      # @param [Boolean] trusted
+      # @param [String] arch
+      # @param [Boolean] add_src
+      #
+      # @return [String] complete repo config text
       def build_repo(uri, distribution, components, trusted, arch, add_src = false)
         uri = make_ppa_url(uri) if is_ppa_url?(uri)
 
diff --git a/spec/unit/provider/apt_repository_spec.rb b/spec/unit/provider/apt_repository_spec.rb
index cabf6551c9..ca85831a06 100644
--- a/spec/unit/provider/apt_repository_spec.rb
+++ b/spec/unit/provider/apt_repository_spec.rb
@@ -47,7 +47,6 @@ EOF
 describe Chef::Provider::AptRepository do
   let(:new_resource) { Chef::Resource::AptRepository.new("multiverse") }
 
-  let(:shellout_env) { { env: { "LANG" => "en_US", "LANGUAGE" => "en_US" } } }
   let(:provider) do
     node = Chef::Node.new
     events = Chef::EventDispatch::Dispatcher.new
@@ -60,15 +59,22 @@ describe Chef::Provider::AptRepository do
   end
 
   let(:apt_key_finger) do
-    r = double("Mixlib::ShellOut", stdout: APT_KEY_FINGER, exitstatus: 0, live_stream: true)
-    allow(r).to receive(:run_command)
-    r
+    double("shell_out", stdout: APT_KEY_FINGER, exitstatus: 0, error?: false)
   end
 
   let(:gpg_finger) do
-    r = double("Mixlib::ShellOut", stdout: GPG_FINGER, exitstatus: 0, live_stream: true)
-    allow(r).to receive(:run_command)
-    r
+    double("shell_out", stdout: GPG_FINGER, exitstatus: 0, error?: false)
+  end
+
+  let(:gpg_shell_out_success) do
+    double("shell_out", :stdout => "pub  2048R/7BD9BF62 2011-08-19 nginx signing key <signing-key@nginx.com>",
+                        :exitstatus => 0, :error? => false)
+  end
+
+  let(:gpg_shell_out_failure) do
+    double("shell_out", :stderr => "gpg: no valid OpenPGP data found.\n
+                                    gpg: processing message failed: eof",
+                        :exitstatus => 1, :error? => true)
   end
 
   let(:apt_fingerprints) do
@@ -83,35 +89,39 @@ C5986B4F1257FFA86632CBA746181433FBB75451
   end
 
   describe "#is_key_id?" do
-    it "should detect a key" do
+    it "detects a key" do
       expect(provider.is_key_id?("A4FF2279")).to be_truthy
     end
-    it "should detect a key with a hex signifier" do
+    it "detects a key with a hex signifier" do
       expect(provider.is_key_id?("0xA4FF2279")).to be_truthy
     end
-    it "should reject a key with the wrong length" do
+    it "rejects a key with the wrong length" do
       expect(provider.is_key_id?("4FF2279")).to be_falsey
     end
-    it "should reject a key with non-hex characters" do
+    it "rejects a key with non-hex characters" do
       expect(provider.is_key_id?("A4KF2279")).to be_falsey
     end
   end
 
   describe "#extract_fingerprints_from_cmd" do
-    before do
-      expect(Mixlib::ShellOut).to receive(:new).and_return(apt_key_finger)
-    end
-
-    it "should run the desired command" do
-      expect(apt_key_finger).to receive(:run_command)
+    it "runs the desired command" do
+      expect(provider).to receive(:shell_out).and_return(apt_key_finger)
       provider.extract_fingerprints_from_cmd(apt_key_finger_cmd)
     end
 
-    it "should return a list of key fingerprints" do
+    it "returns a list of key fingerprints" do
+      expect(provider).to receive(:shell_out).and_return(apt_key_finger)
       expect(provider.extract_fingerprints_from_cmd(apt_key_finger_cmd)).to eql(apt_fingerprints)
     end
   end
 
+  describe "#cookbook_name" do
+    it "returns 'test' when the cookbook property is set" do
+      new_resource.cookbook("test")
+      expect(provider.cookbook_name).to eq("test")
+    end
+  end
+
   describe "#no_new_keys?" do
     before do
       allow(provider).to receive(:extract_fingerprints_from_cmd).with(apt_key_finger_cmd).and_return(apt_fingerprints)
@@ -119,14 +129,14 @@ C5986B4F1257FFA86632CBA746181433FBB75451
 
     let(:file) { "/tmp/remote-gpg-keyfile" }
 
-    it "should match a set of keys" do
+    it "matches a set of keys" do
       allow(provider).to receive(:extract_fingerprints_from_cmd)
         .with("gpg --with-fingerprint --with-colons #{file}")
         .and_return(Array(apt_fingerprints.first))
       expect(provider.no_new_keys?(file)).to be_truthy
     end
 
-    it "should notice missing keys" do
+    it "notices missing keys" do
       allow(provider).to receive(:extract_fingerprints_from_cmd)
         .with("gpg --with-fingerprint --with-colons #{file}")
         .and_return(%w{ F36A89E33CC1BD0F71079007327574EE02A818DD })
@@ -134,11 +144,71 @@ C5986B4F1257FFA86632CBA746181433FBB75451
     end
   end
 
+  describe "#key_type" do
+    it "returns :remote_file with an http URL" do
+      expect(provider.key_type("https://www.chef.io/key")).to eq(:remote_file)
+    end
+
+    it "returns :cookbook_file with a chef managed file" do
+      expect(provider).to receive(:has_cookbook_file?).and_return(true)
+      expect(provider.key_type("/foo/bar.key")).to eq(:cookbook_file)
+    end
+
+    it "throws exception if an unknown file specified" do
+      expect(provider).to receive(:has_cookbook_file?).and_return(false)
+      expect { provider.key_type("/foo/bar.key") }.to raise_error(Chef::Exceptions::FileNotFound)
+    end
+  end
+
+  describe "#keyserver_install_cmd" do
+    it "returns keyserver install command" do
+      expect(provider.keyserver_install_cmd("ABC", "gpg.mit.edu")).to eq("apt-key adv --recv --keyserver hkp://gpg.mit.edu:80 ABC")
+    end
+
+    it "uses proxy if key_proxy property is set" do
+      new_resource.key_proxy("proxy.mycorp.dmz:3128")
+      expect(provider.keyserver_install_cmd("ABC", "gpg.mit.edu")).to eq("apt-key adv --recv --keyserver-options http-proxy=proxy.mycorp.dmz:3128 --keyserver hkp://gpg.mit.edu:80 ABC")
+    end
+
+    it "properly handles keyservers passed with hkp:// URIs" do
+      expect(provider.keyserver_install_cmd("ABC", "hkp://gpg.mit.edu")).to eq("apt-key adv --recv --keyserver hkp://gpg.mit.edu ABC")
+    end
+  end
+
+  describe "#is_ppa_url" do
+    it "returns true if the URL starts with ppa:" do
+      expect(provider.is_ppa_url?("ppa://example.com")).to be_truthy
+    end
+
+    it "returns false if the URL does not start with ppa:" do
+      expect(provider.is_ppa_url?("example.com")).to be_falsey
+    end
+  end
+
+  describe "#repo_components" do
+    it "returns 'main' if a PPA and components property not set" do
+      expect(provider).to receive(:is_ppa_url?).and_return(true)
+      expect(provider.repo_components).to eq("main")
+    end
+
+    it "returns components property if a PPA and components is set" do
+      new_resource.components(["foo"])
+      expect(provider).to receive(:is_ppa_url?).and_return(true)
+      expect(provider.repo_components).to eq(["foo"])
+    end
+
+    it "returns components property if not a PPA" do
+      new_resource.components(["foo"])
+      expect(provider).to receive(:is_ppa_url?).and_return(false)
+      expect(provider.repo_components).to eq(["foo"])
+    end
+  end
+
   describe "#install_ppa_key" do
     let(:url) { "https://launchpad.net/api/1.0/~chef/+archive/main" }
     let(:key) { "C5986B4F1257FFA86632CBA746181433FBB75451" }
 
-    it "should get a key" do
+    it "gets a key" do
       simples = double("HTTP")
       allow(simples).to receive(:get).and_return("\"#{key}\"")
       expect(Chef::HTTP::Simple).to receive(:new).with(url).and_return(simples)
@@ -148,42 +218,49 @@ C5986B4F1257FFA86632CBA746181433FBB75451
   end
 
   describe "#make_ppa_url" do
-    it "should ignore non-ppa repositories" do
-      expect(provider.make_ppa_url("some_string")).to be_nil
-    end
-
-    it "should create a URL" do
+    it "creates a URL" do
       expect(provider).to receive(:install_ppa_key).with("chef", "main").and_return(true)
       expect(provider.make_ppa_url("ppa:chef/main")).to eql("http://ppa.launchpad.net/chef/main/ubuntu")
     end
   end
 
   describe "#build_repo" do
-    it "should create a repository string" do
+    it "creates a repository string" do
       target = %Q{deb      "http://test/uri" unstable main\n}
       expect(provider.build_repo("http://test/uri", "unstable", "main", false, nil)).to eql(target)
     end
 
-    it "should create a repository string with no distribution" do
+    it "creates a repository string with no distribution" do
       target = %Q{deb      "http://test/uri" main\n}
       expect(provider.build_repo("http://test/uri", nil, "main", false, nil)).to eql(target)
     end
 
-    it "should create a repository string with source" do
+    it "creates a repository string with source" do
       target = %Q{deb      "http://test/uri" unstable main\ndeb-src  "http://test/uri" unstable main\n}
       expect(provider.build_repo("http://test/uri", "unstable", "main", false, nil, true)).to eql(target)
     end
 
-    it "should create a repository string with options" do
+    it "creates a repository string with options" do
       target = %Q{deb      [trusted=yes] "http://test/uri" unstable main\n}
       expect(provider.build_repo("http://test/uri", "unstable", "main", true, nil)).to eql(target)
     end
 
-    it "should handle a ppa repo" do
+    it "handles a ppa repo" do
       target = %Q{deb      "http://ppa.launchpad.net/chef/main/ubuntu" unstable main\n}
       expect(provider).to receive(:make_ppa_url).with("ppa:chef/main").and_return("http://ppa.launchpad.net/chef/main/ubuntu")
       expect(provider.build_repo("ppa:chef/main", "unstable", "main", false, nil)).to eql(target)
     end
   end
 
+  describe "#keyfile_is_invalid?" do
+    it "returns true if the file is invalid" do
+      expect(provider).to receive(:shell_out).and_return(gpg_shell_out_failure)
+      expect(provider.keyfile_is_invalid?("/foo/bar.key")).to be_truthy
+    end
+
+    it "returns false if the file is valid" do
+      expect(provider).to receive(:shell_out).and_return(gpg_shell_out_success)
+      expect(provider.keyfile_is_invalid?("/foo/bar.key")).to be_falsey
+    end
+  end
 end