Some clean-up and small fixes to self-update feature

src/lib/installation/driver_update.rb

@@ -1,7 +1,7 @@
 # encoding: utf-8
 
 # ------------------------------------------------------------------------------
-# Copyright (c) 2015 SUSE LLC
+# Copyright (c) 2016 SUSE LLC
 #
 # This program is free software; you can redistribute it and/or modify it under
 # the terms of version 2 of the GNU General Public License as published by the
@@ -21,43 +21,62 @@
 module Installation
   # Represents a driver update disk (DUD)
   #
-  # The DUD will be fetched from a given URL. At this time, HTTP, HTTPS, FTP
-  # and file:/ are supported.
+  # The DUD will be fetched from a given URL.
   class DriverUpdate
     include Yast::I18n # missing in yast2-update
     include Yast::Transfer::FileFromUrl # get_file_from_url
 
     class NotFound < StandardError; end
 
+    # Command to extract the content of the DUD
     EXTRACT_CMD = "gzip -dc %<source>s | cpio --quiet --sparse -dimu --no-absolute-filenames"
+    # Command to apply the DUD disk to inst-sys
     APPLY_CMD = "/etc/adddir %<source>s/inst-sys /" # openSUSE/installation-images
+    # Command to extract content of a signed (with PGP) DUD
     EXTRACT_SIG_CMD = "gpg --homedir %<homedir>s --batch --no-default-keyring --keyring %<keyring>s " \
       "--ignore-valid-from --ignore-time-conflict --output '%<unpacked>s' '%<source>s'"
+    # Command to verify a detached PGP signature
     VERIFY_SIG_CMD = "gpg --homedir %<homedir>s --batch --no-default-keyring --keyring %<keyring>s " \
       "--ignore-valid-from --ignore-time-conflict --verify '%<path>s'"
+    # Temporary name for driver updates
     TEMP_FILENAME = "remote.dud"
+    # Extension for unpacked driver updates after extracting the PGP signature
     UNPACKED_EXT = ".unpacked"
+    # Extension for detached PGP signatures
     SIG_EXT = ".asc"
 
     attr_reader :uri, :local_path, :keyring, :gpg_homedir
 
     # Constructor
     #
-    # @param uri [URI] Driver Update URI
-    def initialize(uri, keyring: keyring, gpg_homedir: nil)
+    # @param uri         [URI]      Driver Update URI
+    # @param keyring     [Pathname] Path to keyring to check signatures against
+    # @param gpg_homedir [Pathname] Path to GPG home dir
+    def initialize(uri, keyring: keyring, gpg_homedir: Pathname.new("/root/.gnupg"))
       Yast.import "Linuxrc"
       @uri = uri
       @local_path = nil
       @keyring = keyring
-      @gpg_homedir = gpg_homedir || "/root/.gnupg"
+      @gpg_homedir = gpg_homedir
       @signed = nil
     end
 
+    # Determines whether a driver update is signed or not
+    #
+    # Signature is checked while fetching and extracting the driver update. The reason
+    # is that we need to check the original files and we don't want to keep them
+    # after the update is extracted (to save some memory during installation).
+    #
+    # The driver update will be considered signed if the signature is OK and the
+    # public key is known. It will be false otherwise. For more details, check
+    # #check_gpg_output.
+    #
+    # @return [Boolean] True if it's correctly signed; false otherwise.
     def signed?
       @signed
     end
 
-    # Fetch the DUD and store it in the given directory
+    # Fetch the DUD and extract it in the given directory
     #
     # @param target [Pathname] Directory to extract the DUD to.
     def fetch(target)
@@ -66,27 +85,27 @@ def fetch(target)
         Dir.chdir(dir) do
           temp_file = Pathname.pwd.join(TEMP_FILENAME)
           download_file_to(temp_file)
-          clear_signature(temp_file)
+          clear_attached_signature(temp_file)
           check_detached_signature(temp_file) unless signed?
           extract(temp_file, local_path)
         end
       end
     end
 
-    def check_detached_signature(temp_file)
-      asc_file = temp_file.sub_ext("#{temp_file.extname}#{SIG_EXT}")
-      get_remote_file(uri.merge("#{uri}#{SIG_EXT}"), asc_file)
-      cmd = format(VERIFY_SIG_CMD, path: asc_file, keyring: keyring, homedir: gpg_homedir)
-      out = Yast::SCR.Execute(Yast::Path.new(".target.bash_output"), cmd)
-      ::FileUtils.rm(asc_file) if asc_file.exist?
-      @signed = check_gpg_output(out)
-    end
-
+    # Determine if gpg command was successful
+    #
+    # The signature was successfully signed if command error code was 0 and
+    # no warning was shown.
+    #
+    # @return [Boolean] True if signature check was successful; false otherwise.
     def check_gpg_output(out)
       out["exit"].zero? && !out["stderr"].include?("WARNING")
     end
 
-    # Apply the DUD to the running system
+    # Apply the DUD to inst-sys
+    #
+    # @see #adddir
+    # @see #run_update_pre
     def apply
       raise "Driver updated not fetched yet!" if local_path.nil?
       adddir
@@ -108,13 +127,39 @@ def extract(source, target)
       ::FileUtils.mv(update_dir, target)
     end
 
-    def clear_signature(path)
+    # Clear and check an attached signature
+    #
+    # If the file at 'path' is signed, it will extract its content checking its
+    # signature. As a side effect, the extracted file will be placed in 'path'.
+    #
+    # @return [Boolean] True if the package was successfuly signed; false otherwise.
+    def clear_attached_signature(path)
       unpacked_path = path.sub_ext(UNPACKED_EXT)
       cmd = format(EXTRACT_SIG_CMD, source: path, unpacked: unpacked_path,
                    keyring: keyring, homedir: gpg_homedir)
       out = Yast::SCR.Execute(Yast::Path.new(".target.bash_output"), cmd)
-      @signed = check_gpg_output(out)
       ::FileUtils.mv(unpacked_path, path) if unpacked_path.exist?
+      @signed = check_gpg_output(out)
+    end
+
+    # Check a detached signature for a DUD
+    #
+    # The signature will be taken from the same URL than the DUD but adding
+    # the suffix '.asc'.
+    #
+    # @return [Boolean] True if the signature is OK; false otherwise.
+    #
+    # @see #check_gpg_output
+    def check_detached_signature(temp_file)
+      # Download the detached signature
+      asc_file = temp_file.sub_ext("#{temp_file.extname}#{SIG_EXT}")
+      get_file(uri.merge("#{uri}#{SIG_EXT}"), asc_file)
+
+      # Verify the signature
+      cmd = format(VERIFY_SIG_CMD, path: asc_file, keyring: keyring, homedir: gpg_homedir)
+      out = Yast::SCR.Execute(Yast::Path.new(".target.bash_output"), cmd)
+      ::FileUtils.rm(asc_file) if asc_file.exist?
+      @signed = check_gpg_output(out) # Set signature
     end
 
     # Set up the target directory
@@ -133,7 +178,7 @@ def setup_target(dir)
     #
     # @return [True] true if download was successful
     def download_file_to(path)
-      get_remote_file(uri, path)
+      get_file(uri, path)
       raise NotFound unless path.exist?
       true
     end
@@ -167,7 +212,10 @@ def run_update_pre
       out["exit"].zero?
     end
 
-    def get_remote_file(location, path)
+    # Wrapper to get a file using Yast::Transfer::FileFromUrl#get_file_from_url
+    #
+    # @return [Boolean] true if the file was retrieved; false otherwise.
+    def get_file(location, path)
       get_file_from_url(scheme: location.scheme, host: location.host, urlpath: location.path,
                         localfile: path.to_s, urltok: {}, destdir: "")
     end

test/driver_update_test.rb

@@ -49,10 +49,6 @@
   end
 
   describe "#signed?" do
-    before do
-      #subject.fetch(target)
-    end
-
     context "if the signature is attached" do
       context "and signature is valid and trusted" do
         let(:url) { URI("file://#{FIXTURES_DIR}/fake.signed.dud") }
@@ -67,9 +63,9 @@
         let(:url) { URI("file://#{FIXTURES_DIR}/fake.signed+untrusted.dud") }
 
         it "returns false" do
-          allow(subject).to receive(:get_remote_file).with(any_args).and_call_original
-          allow(subject).to receive(:get_remote_file)
-            .with(URI("file://#{FIXTURES_DIR}/fake.signed+untrusted.dud.asc"), any_args).once.and_return(false)
+          allow(subject).to receive(:get_file).with(any_args).and_call_original
+          allow(subject).to receive(:get_file)
+            .with(URI("file://#{FIXTURES_DIR}/fake.signed+untrusted.dud.asc"), any_args).and_return(false)
           subject.fetch(target)
           expect(subject).to_not be_signed
         end
@@ -79,8 +75,8 @@
         let(:url) { URI("file://#{FIXTURES_DIR}/fake.signed+unknown.dud") }
 
         it "returns false" do
-          allow(subject).to receive(:get_remote_file).with(any_args).and_call_original
-          allow(subject).to receive(:get_remote_file)
+          allow(subject).to receive(:get_file).with(any_args).and_call_original
+          allow(subject).to receive(:get_file)
             .with(URI("file://#{FIXTURES_DIR}/fake.signed+unknown.dud.asc"), any_args).once.and_return(false)
           subject.fetch(target)
           expect(subject).to_not be_signed
@@ -116,13 +112,16 @@
       end
 
       context "and .asc file does not exist" do
+        let(:url) { URI("file://#{FIXTURES_DIR}/fake.dud") }
+
         before do
-          allow(subject).to receive(:get_remote_file)
+          allow(subject).to receive(:get_file).with(any_args).and_call_original
+          allow(subject).to receive(:get_file)
             .with(URI("file://#{FIXTURES_DIR}/fake.dud.asc"), any_args).once.and_return(false)
-          allow(subject).to receive(:get_remote_file).with(any_args).and_call_original
         end
 
         it "returns false" do
+          subject.fetch(target)
           expect(subject).to_not be_signed
         end
       end