Merge pull request #991 from aschnell/master

fix creation of secure key for new partitions (bsc#1154267)

package/yast2-storage-ng.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Mon Oct 28 14:22:24 CET 2019 - aschnell@suse.com
+
+- fix creation of secure key for new partitions (bsc#1154267)
+- 4.2.52
+
 -------------------------------------------------------------------
 Wed Oct 23 11:51:58 UTC 2019 - Imobach Gonzalez Sosa <igonzalezsosa@suse.com>
 

package/yast2-storage-ng.spec

@@ -16,7 +16,7 @@
 #
 
 Name:           yast2-storage-ng
-Version:        4.2.51
+Version:        4.2.52
 Release:        0
 Summary:        YaST2 - Storage Configuration
 License:        GPL-2.0-only OR GPL-3.0-only

src/lib/y2storage/encryption_processes/pervasive.rb

@@ -51,10 +51,7 @@ def create_device(blk_device, dm_name)
       # @see Base#pre_commit
       #
       # If there is no secure key to be used for this device, a new key is
-      # generated. In addition to that, the "zkey cryptsetup" command is
-      # executed to know the sequence of commands that must be used to set the
-      # pervasive encryption and the LUKS format options are adjusted
-      # accordingly.
+      # generated.
       #
       # @param device [Encryption] encryption that will be created in the system
       def pre_commit(device)
@@ -63,19 +60,25 @@ def pre_commit(device)
         # API to allow sharing a new key among several volumes.
         @secure_key ||= generate_secure_key(device)
 
-        @zkey_cryptsetup_output = execute_zkey_cryptsetup(device)
-        return if @zkey_cryptsetup_output.empty?
+        master_key_file = @secure_key.filename
+        sector_size = sector_size_for(device.blk_device)
 
-        device.format_options = luksformat_options_string + " --pbkdf pbkdf2"
+        device.format_options = "--master-key-file #{master_key_file.shellescape} --key-size 1024 "\
+                                "--cipher paes-xts-plain64"
+        device.format_options += " --sector-size #{sector_size}" if sector_size
+        device.format_options += " --pbkdf pbkdf2"
       end
 
       # @see Base#post_commit
       #
-      # Executes the extra commands reported by the former call to
-      # "zkey cryptsetup".
+      # Adds the device to the secure key if needed and executes the
+      # extra commands reported by a call to "zkey cryptsetup".
       #
       # @param device [Encryption] encryption that has just been created in the system
       def post_commit(device)
+        @secure_key.add_device_and_write(device) unless @secure_key.for_device?(device)
+
+        zkey_cryptsetup_output = execute_zkey_cryptsetup(device)
         commands = zkey_cryptsetup_output[1..-1]
         return if commands.nil?
 
@@ -127,16 +130,16 @@ def cheetah_recorder
         Recorder.new(Yast::Y2Logger.instance)
       end
 
-      # Generates a new secure key for the given encryption device and registers
-      # it into the keys database of the system
+      # Generates a new secure key for the given encryption device and
+      # registers it into the keys database of the system. The secure
+      # does not include the volume since that may not exist yet.
       #
       # @param device [Encryption]
       # @return [SecureKey]
       def generate_secure_key(device)
         key_name = "YaST_#{device.dm_table_name}"
         key = SecureKey.generate(
           key_name,
-          volumes:     [device],
           sector_size: sector_size_for(device.blk_device)
         )
         log.info "Generated secure key #{key.name}"
@@ -153,17 +156,6 @@ def execute_zkey_cryptsetup(device)
         command = [ZKEY, "cryptsetup", "--volumes", name]
         Yast::Execute.locally(*command, stdout: :capture)&.lines&.map(&:strip) || []
       end
-
-      # Options to be passed to the "cryptsetup luksFormat" during the commit
-      # phase
-      #
-      # @see Luks#format_options
-      #
-      # @return [String]
-      def luksformat_options_string
-        luksformat_command = zkey_cryptsetup_output.first
-        luksformat_command.split("luksFormat ").last.gsub(/ \/dev[^\s]*/, "")
-      end
     end
   end
 end

src/lib/y2storage/encryption_processes/secure_key.rb

@@ -99,8 +99,23 @@ def plain_name(device)
       #   saving the volume entry in the keys database.
       #
       # @param device [Encryption]
+      # @return [SecureKeyVolume] the newly added SecureKeyVolume
       def add_device(device)
         @volume_entries << SecureKeyVolume.new_from_encryption(device)
+        @volume_entries.last
+      end
+
+      # Adds the given device to the list of volumes registered for this key
+      #
+      # @param device [Encryption]
+      # @return [SecureKeyVolume] the newly added SecureKeyVolume
+      def add_device_and_write(device)
+        secure_key_volume = add_device(device)
+
+        Yast::Execute.locally(ZKEY, "change", "--name", name, "--volumes",
+          "+#{secure_key_volume}")
+
+        secure_key_volume
       end
 
       # Registers the key in the keys database by invoking "zkey generate"
@@ -144,6 +159,13 @@ def add_zkey_volumes(string)
         @volume_entries += volumes.map { |str| SecureKeyVolume.new_from_str(str) }
       end
 
+      # Full filename of the secure key file.
+      #
+      # @return [String]
+      def filename
+        File.join(repo_dir, name + ".skey")
+      end
+
       # Copies the files of this key from the current keys repository to the
       # repository of a target system
       #

test/y2storage/encryption_processes/pervasive_test.rb

@@ -90,7 +90,8 @@
 
     let(:generated_key) do
       instance_double(Y2Storage::EncryptionProcesses::SecureKey,
-        plain_name: "/dev/dasdc1", dm_name: "cr_1", name: "secure_xtskey1")
+        plain_name: "/dev/dasdc1", dm_name: "cr_1", name: "secure_xtskey1",
+        filename: "/etc/zkey/repository/secure_xtskey1.skey")
     end
 
     before do
@@ -101,37 +102,34 @@
 
     it "generates a new secure key for the device" do
       expect(Y2Storage::EncryptionProcesses::SecureKey).to receive(:generate)
-        .with("YaST_cr_sda", volumes: [encryption], sector_size: 4096).and_return(generated_key)
+        .with("YaST_cr_sda", sector_size: 4096).and_return(generated_key)
       subject.pre_commit(encryption)
     end
 
     context "when the block size of the underlying device is greater than 4k" do
       let(:block_size) { Y2Storage::DiskSize.new(8192) }
 
       it "sets the sector-size for the encryption key to 4096" do
-        expect(Y2Storage::EncryptionProcesses::SecureKey).to receive(:generate)
-          .with("YaST_cr_sda", volumes: [encryption], sector_size: 4096).and_return(generated_key)
         subject.pre_commit(encryption)
+        expect(encryption.format_options).to include("--sector-size 4096")
       end
     end
 
     context "when the block size of the underlying device is 4k" do
       let(:block_size) { Y2Storage::DiskSize.new(4096) }
 
       it "sets the sector-size for the encryption key to 4096" do
-        expect(Y2Storage::EncryptionProcesses::SecureKey).to receive(:generate)
-          .with("YaST_cr_sda", volumes: [encryption], sector_size: 4096).and_return(generated_key)
         subject.pre_commit(encryption)
+        expect(encryption.format_options).to include("--sector-size 4096")
       end
     end
 
     context "when the block size of the underlying device less than 4k" do
       let(:block_size) { Y2Storage::DiskSize.new(2048) }
 
-      it "sets the sector-size for the encryption key to 4096" do
-        expect(Y2Storage::EncryptionProcesses::SecureKey).to receive(:generate)
-          .with("YaST_cr_sda", volumes: [encryption], sector_size: nil).and_return(generated_key)
+      it "does not set the sector-size for the encryption key" do
         subject.pre_commit(encryption)
+        expect(encryption.format_options).to_not include("--sector-size")
       end
     end
 
@@ -146,7 +144,11 @@
 
     it "sets LUKS format options" do
       subject.pre_commit(encryption)
-      expect(encryption.format_options).to include("--foo bar --dummy --pbkdf pbkdf2")
+      expect(encryption.format_options).to include("--master-key-file "\
+        "/etc/zkey/repository/secure_xtskey1.skey")
+      expect(encryption.format_options).to include("--key-size 1024")
+      expect(encryption.format_options).to include("--cipher paes-xts-plain64")
+      expect(encryption.format_options).to include("--pbkdf pbkdf2")
     end
   end
 
@@ -155,23 +157,42 @@
 
     let(:secure_key) do
       instance_double(Y2Storage::EncryptionProcesses::SecureKey,
-        plain_name: "/dev/dasdc1", dm_name: "cr_1", name: "secure_xtskey1")
+        plain_name: "/dev/dasdc1", dm_name: "cr_1", name: "secure_xtskey1",
+        filename: "/etc/zkey/repository/secure_xtskey1.skey")
+    end
+
+    let(:secure_key_volume) do
+      instance_double(Y2Storage::EncryptionProcesses::SecureKeyVolume,
+        plain_name: "/dev/dasdc1", dm_name: "cr_1", to_s: "/dev/dasdc1:cr_1")
     end
 
     before do
       subject.pre_commit(encryption)
     end
 
     it "executes commands reported by zkey cryptsetup skipping the first one" do
+      allow(secure_key).to receive(:for_device?).and_return(false)
+      expect(secure_key).to receive(:add_device_and_write).and_return(secure_key_volume)
+      expect(Yast::Execute).to receive(:locally).with(/zkey-cryptsetup/, any_args)
+      expect(Yast::Execute).to receive(:locally).with("third-command")
+      subject.post_commit(encryption)
+    end
+
+    it "executes commands reported by zkey cryptsetup skipping the first one" do
+      allow(secure_key).to receive(:for_device?).and_return(true)
+      expect(secure_key).to_not receive(:add_device)
       expect(Yast::Execute).to receive(:locally).with(/zkey-cryptsetup/, any_args)
       expect(Yast::Execute).to receive(:locally).with("third-command")
       subject.post_commit(encryption)
     end
 
     it "adds the --key-file option to the setvp command" do
-      allow(Yast::Execute).to receive(:locally)
-      expect(Yast::Execute).to receive(:locally)
-        .with(/zkey-cryptsetup/, "setvp", "--volumes", "/dev/dasdc1", "--key-file", "-", any_args)
+      allow(secure_key).to receive(:for_device?).and_return(true)
+      expect(Yast::Execute).to receive(:locally).with(/zkey/, "cryptsetup", "--volumes",
+        "/dev/dasdc1", any_args)
+      expect(Yast::Execute).to receive(:locally).with(/zkey-cryptsetup/, "setvp", "--volumes",
+        "/dev/dasdc1", "--key-file", "-", any_args)
+      expect(Yast::Execute).to receive(:locally).with("third-command")
       subject.post_commit(encryption)
     end
   end

test/y2storage/encryption_processes/secure_key_test.rb

@@ -63,7 +63,7 @@ def zkey_output(file)
   end
 
   describe "#generate" do
-    it "runs zkey to create the a LUKS2 key with the given name and sector size" do
+    it "runs zkey to create the LUKS2 key with the given name and sector size" do
       expect(Yast::Execute).to receive(:locally).with(
         "/usr/bin/zkey", "generate", "--name", key.name, "--xts", "--keybits", "256",
         "--volume-type", "LUKS2", "--sector-size", key.sector_size.to_s
@@ -83,4 +83,27 @@ def zkey_output(file)
       end
     end
   end
+
+  describe "#filename" do
+    it "returns the correct filename" do
+      expect(key.filename).to eq("/etc/zkey/repository/cr.skey")
+    end
+  end
+
+  describe "#add_device_and_write" do
+    let(:blk_device) do
+      instance_double(Y2Storage::BlkDevice, udev_full_ids: ["/dev/dasdc1"])
+    end
+
+    let(:device) do
+      instance_double(Y2Storage::Encryption, blk_device: blk_device, dm_table_name: "cr_1")
+    end
+
+    it "calls zkey to add the volume" do
+      expect(Yast::Execute).to receive(:locally).with(/zkey/, "change", "--name", "cr",
+        "--volumes", "+/dev/dasdc1:cr_1", any_args)
+
+      key.add_device_and_write(device)
+    end
+  end
 end

test/y2storage/pervasive_encryption_test.rb

@@ -100,11 +100,11 @@ def zkey_output(file)
       context "if the 'zkey cryptsetup' command fails" do
         let(:zkey_cryptsetup) { nil }
 
-        it "does not modify the options for 'cryptsetup luksFormat'" do
+        it "anyway sets options for 'cryptsetup luksFormat'" do
           enc = blk_device.encrypt(method: pervasive)
           manager.commit
 
-          expect(enc.format_options).to be_empty
+          expect(enc.format_options).to include("--master-key-file")
         end
       end
 
@@ -115,11 +115,11 @@ def zkey_output(file)
             "third command"
         end
 
-        it "generates arguments for 'cryptsetup luksFormat'" do
+        it "ignores arguments from 'zkey' for 'cryptsetup luksFormat'" do
           enc = blk_device.encrypt(method: pervasive)
           manager.commit
 
-          expect(enc.format_options).to eq "--one two --three --pbkdf pbkdf2"
+          expect(enc.format_options).to_not include("--one two")
         end
 
         it "executes the post-commit commands providing the password when needed" do
@@ -166,7 +166,7 @@ def zkey_output(file)
       it "tries to generate a new secure key with the appropriate name and arguments" do
         expect(Yast::Execute).to receive(:locally).with(
           /zkey/, "generate", "--name", "YaST_cr_dasdc1_1", "--xts", "--keybits", "256",
-          "--volume-type", "LUKS2", "--sector-size", "4096", "--volumes", "/dev/dasdc1:cr_dasdc1"
+          "--volume-type", "LUKS2", "--sector-size", "4096"
         )
 
         blk_device.encrypt(method: pervasive)