diff --git a/package/yast2-storage-ng.changes b/package/yast2-storage-ng.changes index 7ecb118ad7..7efe6ae66a 100644 --- a/package/yast2-storage-ng.changes +++ b/package/yast2-storage-ng.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Thu Sep 5 15:03:50 UTC 2019 - Ancor Gonzalez Sosa + +- Partitioner: better handling of existing encryptions, including + the possibility of reusing them (related to jsc#SLE-7376). + ------------------------------------------------------------------- Wed Sep 04 14:46:12 CEST 2019 - aschnell@suse.com diff --git a/src/lib/y2partitioner/actions/controllers/encryption.rb b/src/lib/y2partitioner/actions/controllers/encryption.rb index 9c5e0033b0..5aac91ee96 100644 --- a/src/lib/y2partitioner/actions/controllers/encryption.rb +++ b/src/lib/y2partitioner/actions/controllers/encryption.rb @@ -27,51 +27,102 @@ module Controllers # device that has been edited by the given Filesystem controller. class Encryption < Base include Yast::Logger + include Yast::I18n + + # Action to perform when {#finish} is called + # + # * :keep preserves the encryption layer from the system devicegraph + # * :encrypt adds an encryption device or modifies the previously added one + # * :remove ensures the block device will not be encrypted + # + # @return [Symbol] :keep, :encrypt, :remove + attr_accessor :action # @return [String] Password for the encryption device - attr_accessor :encrypt_password + attr_accessor :password # Contructor # # @param fs_controller [Filesystem] see {#fs_controller} def initialize(fs_controller) super() + textdomain "storage" + @fs_controller = fs_controller + @password = blk_device.encrypted? ? encryption.password : "" + @action = actions.first end - # Whether a new encryption device will be created for the block device + # Whether the dialog to select and configure the action makes sense # # @return [Boolean] - def to_be_encrypted? - return false unless can_change_encrypt? + def show_dialog? + can_change_encrypt? && fs_controller.encrypt + end - fs_controller.encrypt && !blk_device.encrypted? + # Actions that make sense for the block device + # + # @see #action + # + # If there is more than one possible action, the user should be able to + # use the UI to select which one to perform + # + # @return [Array] + def actions + return @actions if @actions + + @actions = + if fs_controller.encrypt + if can_keep? + [:keep, :encrypt] + else + [:encrypt] + end + else + [:remove] + end end # Applies last changes to the block device at the end of the wizard, which # mainly means # # * removing unused LvmPv descendant (bsc#1129663) - # * encrypting the device or removing the encryption layer. + # * encrypting the device, modifying the encryption layer or removing it def finish return unless can_change_encrypt? remove_unused_lvm_pv - if to_be_encrypted? - blk_device.encrypt(password: encrypt_password) - elsif blk_device.encrypted? && !fs_controller.encrypt - blk_device.remove_encryption + return if action == :keep + + if action == :encrypt + finish_encrypt + else + finish_remove end ensure fs_controller.update_checkpoint end - # Name of the plain device + # Encryption device currently associated to the block device, if any # + # @return [Y2Storage::Encryption, nil] + def encryption + blk_device.encryption + end + + # Title to display in the dialog during the process # @return [String] - def blk_device_name - blk_device.name + def wizard_title + title = + if actions.include?(:keep) + _("Encryption for %s") + elsif new_encryption? + _("Modify encryption of %s") + else + _("Encrypt %s") + end + format(title, blk_device.name) end private @@ -90,6 +141,11 @@ def blk_device fs_controller.blk_device end + # Whether it's possible to remove or replace the encryption device + # currently associated to the block device + # + # @return [Boolean] false if the device is formatted in the system and + # the user wants to preserve that filesystem def can_change_encrypt? blk_device.filesystem.nil? || new?(blk_device.filesystem) end @@ -97,11 +153,48 @@ def can_change_encrypt? # Removes from the block device or its encryption layer a LvmPv not associated to an LvmVg # (bsc#1129663) def remove_unused_lvm_pv - device = blk_device.encryption || blk_device + device = encryption || blk_device lvm_pv = device.lvm_pv device.remove_descendants if lvm_pv&.descendants&.none? end + + # @see #finish + def finish_remove + return unless blk_device.encrypted? + + blk_device.remove_encryption + end + + # @see #finish + def finish_encrypt + if new_encryption? + encryption.password = password + else + blk_device.remove_encryption if blk_device.encrypted? + blk_device.encrypt(password: password) + end + end + + # Whether the block device is associated to an encryption device that + # does not exists in the system yet + # + # In other words, if the device is going to be (re)encrypted + # + # @return [Boolean] + def new_encryption? + blk_device.encrypted? && new?(encryption) + end + + # Whether it makes sense to offer the :keep action + # + # @return [Boolean] + def can_keep? + return false unless blk_device.encrypted? + return false if new?(encryption) + + encryption.active? + end end end end diff --git a/src/lib/y2partitioner/actions/controllers/filesystem.rb b/src/lib/y2partitioner/actions/controllers/filesystem.rb index 520fcd393a..7fd57dc08c 100644 --- a/src/lib/y2partitioner/actions/controllers/filesystem.rb +++ b/src/lib/y2partitioner/actions/controllers/filesystem.rb @@ -441,17 +441,27 @@ def update_checkpoint private def delete_filesystem - blk_device.remove_descendants + filesystem_parent.remove_descendants # Shadowing control of btrfs subvolumes might be needed if the deleted # filesystem had mount point Y2Storage::Filesystems::Btrfs.refresh_subvolumes_shadowing(working_graph) end def create_filesystem(type, label: nil) - blk_device.create_blk_filesystem(type) + filesystem_parent.create_blk_filesystem(type) filesystem.label = label unless label.nil? end + # Device containing the filesystem + # + # Unlike {#blk_device}, that always returns the plain device, this + # method will return the encryption device for encrypted filesystems + # + # @return [Y2Storage::BlkDevice] + def filesystem_parent + blk_device.encrypted? ? blk_device.encryption : blk_device + end + def restore_filesystem mount_path = filesystem.mount_path mount_by = filesystem.mount_by diff --git a/src/lib/y2partitioner/actions/filesystem_steps.rb b/src/lib/y2partitioner/actions/filesystem_steps.rb index 6164240b0c..8f5d92c9a6 100644 --- a/src/lib/y2partitioner/actions/filesystem_steps.rb +++ b/src/lib/y2partitioner/actions/filesystem_steps.rb @@ -71,7 +71,7 @@ def format_options def encrypt_password @encrypt_controller = Controllers::Encryption.new(fs_controller) - return :next unless encrypt_controller.to_be_encrypted? + return :next unless encrypt_controller.show_dialog? Dialogs::Encryption.run(encrypt_controller) end diff --git a/src/lib/y2partitioner/dialogs/encryption.rb b/src/lib/y2partitioner/dialogs/encryption.rb index 5cad190fef..a13d9e1c21 100644 --- a/src/lib/y2partitioner/dialogs/encryption.rb +++ b/src/lib/y2partitioner/dialogs/encryption.rb @@ -20,10 +20,12 @@ require "yast" require "y2partitioner/dialogs/base" require "y2partitioner/widgets/encrypt_password" +require "y2partitioner/widgets/controller_radio_buttons" module Y2Partitioner module Dialogs - # Ask for a password to assign to an encrypted device. + # Ask for the concrete action to perform when the goal is to obtain an + # encrypted device, including details like the password. # Part of {Actions::AddPartition} and {Actions::EditBlkDevice}. # Formerly MiniWorkflowStepPassword class Encryption < Base @@ -34,14 +36,87 @@ def initialize(controller) @controller = controller end + # @macro seeDialog def title - _("Encryption password for %s") % @controller.blk_device_name + @controller.wizard_title end + # @macro seeDialog def contents - HVSquash( - Widgets::EncryptPassword.new(@controller) - ) + main_widget = + if @controller.actions.include?(:keep) + ActionWidget.new(@controller) + else + Widgets::EncryptPassword.new(@controller) + end + + HVSquash(main_widget) + end + + # Internal widget used when both :keep and :encrypt options are possible + class ActionWidget < Widgets::ControllerRadioButtons + # @param controller [Actions::Controllers::Encryption] + # a controller collecting data for managing the encryption of a device + def initialize(controller) + textdomain "storage" + @controller = controller + end + + # @macro seeAbstractWidget + def label + _("Choose an Action") + end + + # @macro seeItemsSelection + def items + keep_label = + format(_("Preserve existing encryption (%s)"), encryption_type.to_human_string) + + [ + [:keep, keep_label], + [:encrypt, _("Encrypt the device (replaces current encryption)")] + ] + end + + # @see Widgets::ControllerRadioButtons + def widgets + @widgets ||= [ + CWM::Empty.new("empty"), + Widgets::EncryptPassword.new(@controller) + ] + end + + # @macro seeAbstractWidget + def init + self.value = @controller.action + # trigger disabling the other subwidgets + handle("ID" => value) + end + + # @macro seeAbstractWidget + def store + current_widget.store if current_widget.respond_to?(:store) + @controller.action = value + end + + # @macro seeAbstractWidget + def help + # helptext + _( + "

Choose the encryption layer.

\n" \ + "

The device is already encrypted in the system, it's possible to re-encrypt it" \ + "with new settings or to use the existing encryption layer.

" + ) + end + + private + + # Type of the encryption currently used by the device + # + # @return [Y2Storage::EncryptionType] + def encryption_type + @controller.encryption.type + end end end end diff --git a/src/lib/y2partitioner/widgets/encrypt_password.rb b/src/lib/y2partitioner/widgets/encrypt_password.rb index 3deffcc65f..67389b63da 100644 --- a/src/lib/y2partitioner/widgets/encrypt_password.rb +++ b/src/lib/y2partitioner/widgets/encrypt_password.rb @@ -31,7 +31,7 @@ def help # @macro seeAbstractWidget def validate - msg = checker.error_msg(pw1, pw2) + msg = checker.error_msg(pw1, pw2) if enabled? return true unless msg Yast::Report.Error(msg) @@ -41,7 +41,7 @@ def validate # @macro seeAbstractWidget def store - @controller.encrypt_password = pw1 + @controller.password = pw1 end # @macro seeAbstractWidget @@ -51,30 +51,19 @@ def cleanup # @macro seeCustomWidget def contents - Frame( - _("Encryption Password"), - MarginBox( - 1.45, - 0.5, - VBox( - Password( - Id(:pw1), - Opt(:hstretch), - # Label: get password for user root - # Please use newline if label is longer than 40 characters - _("&Enter a Password for your File System:"), - "" - ), - Password( - Id(:pw2), - Opt(:hstretch), - # Label: get same password again for verification - # Please use newline if label is longer than 40 characters - _("Reenter the Password for &Verification:"), - "" - ), - VSpacing(0.5) - ) + VBox( + Id(widget_id), + Password( + Id(:pw1), + Opt(:hstretch), + _("&Enter an Encryption Password:"), + @controller.password + ), + Password( + Id(:pw2), + Opt(:hstretch), + _("Reenter the Password for &Verification:"), + @controller.password ) ) end diff --git a/test/y2partitioner/actions/controllers/encryption_test.rb b/test/y2partitioner/actions/controllers/encryption_test.rb index 76d7903821..24fee02566 100755 --- a/test/y2partitioner/actions/controllers/encryption_test.rb +++ b/test/y2partitioner/actions/controllers/encryption_test.rb @@ -24,14 +24,17 @@ require "y2partitioner/actions/controllers/encryption" describe Y2Partitioner::Actions::Controllers::Encryption do - before { devicegraph_stub(scenario) } + before do + devicegraph_stub(scenario) + fs_controller.encrypt = encrypt + end + + let(:fs_controller) { Y2Partitioner::Actions::Controllers::Filesystem.new(device, "The title") } let(:scenario) { "mixed_disks_btrfs" } subject(:controller) { described_class.new(fs_controller) } - let(:fs_controller) { Y2Partitioner::Actions::Controllers::Filesystem.new(device, "The title") } - let(:device) { Y2Storage::BlkDevice.find_by_name(devicegraph, dev_name) } let(:devicegraph) { Y2Partitioner::DeviceGraphs.instance.current } @@ -42,30 +45,32 @@ let(:subvolumes) { Y2Storage::SubvolSpecification.fallback_list } - describe "#to_be_encrypted?" do + let(:encrypt) { false } + + describe "#show_dialog?" do context "when the currently editing device has a filesystem that existed previously" do it "returns false" do - expect(subject.to_be_encrypted?).to eq(false) + expect(subject.show_dialog?).to eq(false) end end context "when the currently editing device does not have a filesystem that existed previously" do before do - allow(fs_controller).to receive(:encrypt).and_return(encrypt) allow(device).to receive(:encrypted?).and_return(encrypted) + allow(device).to receive(:encryption).and_return(encryption) allow(device).to receive(:filesystem).and_return(filesystem) allow(fs_controller).to receive(:blk_device).and_return(device) end - let(:encrypt) { false } let(:encrypted) { false } + let(:encryption) { nil } let(:filesystem) { nil } context "and the device has not been marked to encrypt" do let(:encrypt) { false } it "returns false" do - expect(subject.to_be_encrypted?).to eq(false) + expect(subject.show_dialog?).to eq(false) end end @@ -74,9 +79,26 @@ context "and the device is currently encrypted" do let(:encrypted) { true } + let(:encryption) { double("Encryption", password: "123456", active?: true) } - it "returns false" do - expect(subject.to_be_encrypted?).to eq(false) + before do + allow(encryption).to receive(:exists_in_devicegraph?).and_return in_system + end + + context "with a preexisting encryption" do + let(:in_system) { true } + + it "returns true" do + expect(subject.show_dialog?).to eq(true) + end + end + + context "with a new encryption" do + let(:in_system) { false } + + it "returns true" do + expect(subject.show_dialog?).to eq(true) + end end end @@ -84,7 +106,7 @@ let(:encrypted) { false } it "returns true" do - expect(subject.to_be_encrypted?).to eq(true) + expect(subject.show_dialog?).to eq(true) end end end @@ -109,18 +131,69 @@ context "when it is possible to change the encrypt" do let(:can_change_encrypt) { true } + let(:scenario) { "logical_encrypted" } - before do - allow(fs_controller).to receive(:encrypt).and_return(encrypt) - allow(subject).to receive(:encrypt_password).and_return(password) - end + before { controller.password = password } let(:encrypt) { false } let(:password) { "12345678" } - context "and the device was already encrypted" do + context "and the device was already encrypted at startup" do + let(:dev_name) { "/dev/sda6" } + + context "and it is marked to be encrypted" do + let(:encrypt) { true } + + before { controller.action = action } + + context "and the existing encryption must be kept" do + let(:action) { :keep } + + it "preserves the existing encryption" do + sid = fs_controller.blk_device.encryption.sid + subject.finish + expect(fs_controller.blk_device.encryption.sid).to eq sid + end + + it "does not change the encryption password" do + orig_password = fs_controller.blk_device.encryption.password + + subject.finish + + expect(fs_controller.blk_device.encryption.password).to eq orig_password + expect(fs_controller.blk_device.encryption.password).to_not eq password + end + end + + context "and the device must be re-encrypted" do + let(:action) { :encrypt } + + it "encrypts the device, replacing the previous encryption" do + sid = fs_controller.blk_device.encryption.sid + expect(fs_controller.blk_device.encryption.password).to_not eq password + + subject.finish + expect(fs_controller.blk_device.encryption.sid).to_not eq sid + expect(fs_controller.blk_device.encryption.password).to eq password + end + end + end + + context "and it is not marked to be encrypted" do + let(:encrypt) { false } + + it "removes the encryption" do + expect(fs_controller.blk_device.encryption).to_not be_nil + subject.finish + expect(fs_controller.blk_device.encryption).to be_nil + end + end + end + + context "and the device was encrypted with the Partitioner" do + let(:dev_name) { "/dev/sda8" } + before do - device.remove_descendants encryption = device.create_encryption("foo") encryption.create_filesystem(Y2Storage::Filesystems::Type::EXT4) end @@ -128,11 +201,14 @@ context "and it is marked to be encrypted" do let(:encrypt) { true } - it "does nothing" do - devicegraph = Y2Partitioner::DeviceGraphs.instance.current.dup + it "modifies the encryption password" do + sid = fs_controller.blk_device.encryption.sid + expect(fs_controller.blk_device.encryption.password).to_not eq password + subject.finish - expect(devicegraph).to eq(Y2Partitioner::DeviceGraphs.instance.current) + expect(fs_controller.blk_device.encryption.sid).to eq sid + expect(fs_controller.blk_device.encryption.password).to eq password end end @@ -148,6 +224,8 @@ end context "and the device is not encrypted" do + let(:dev_name) { "/dev/sda5" } + context "and it is marked to be encrypted" do let(:encrypt) { true } @@ -175,12 +253,10 @@ let(:scenario) { "unused_lvm_pvs.xml" } let(:can_change_encrypt) { true } - let(:encrypt) { false } let(:password) { "12345678" } before do - allow(fs_controller).to receive(:encrypt).and_return(encrypt) - allow(subject).to receive(:encrypt_password).and_return(password) + allow(subject).to receive(:password).and_return(password) end context "which was not encrypted" do @@ -245,4 +321,38 @@ end end end + + describe "#wizard_title" do + let(:scenario) { "logical_encrypted" } + let(:encrypt) { true } + + context "when the current encryption layer was already there at startup" do + let(:dev_name) { "/dev/sda6" } + + it "returns the expected text" do + expect(controller.wizard_title).to include "Encryption for " + end + end + + context "when the current encryption layer was created by the Partitioner" do + let(:dev_name) { "/dev/sda8" } + + before do + encryption = device.create_encryption("foo") + encryption.create_filesystem(Y2Storage::Filesystems::Type::EXT4) + end + + it "returns the expected text" do + expect(controller.wizard_title).to include "Modify encryption " + end + end + + context "when the device is currently not encrypted" do + let(:dev_name) { "/dev/sda5" } + + it "returns the expected text" do + expect(controller.wizard_title).to include "Encrypt " + end + end + end end diff --git a/test/y2partitioner/dialogs/encryption_test.rb b/test/y2partitioner/dialogs/encryption_test.rb index 503cb9f8ba..bae29f26b4 100755 --- a/test/y2partitioner/dialogs/encryption_test.rb +++ b/test/y2partitioner/dialogs/encryption_test.rb @@ -25,9 +25,76 @@ require "y2partitioner/dialogs/encryption" describe Y2Partitioner::Dialogs::Encryption do - let(:controller) { double("FilesystemController", blk_device_name: "/dev/sda1") } + let(:controller) { double("FilesystemController", wizard_title: "Title", actions: actions) } - subject { described_class.new(controller) } + subject(:dialog) { described_class.new(controller) } - include_examples "CWM::Dialog" + context "when :encrypt is the only possible action" do + let(:actions) { [:encrypt] } + + include_examples "CWM::Dialog" + + describe "#contents" do + it "delegates everything to an EncryptPassword widget " do + content = dialog.contents.params + expect(content.size).to eq 1 + expect(content.first).to be_a Y2Partitioner::Widgets::EncryptPassword + end + end + end + + context "when :keep and :encrypt are both possible actions" do + let(:actions) { [:keep, :encrypt] } + + include_examples "CWM::Dialog" + + describe "#contents" do + it "delegates everything to an Encryption::Action widget " do + content = dialog.contents.params + expect(content.size).to eq 1 + expect(content.first).to be_a Y2Partitioner::Dialogs::Encryption::ActionWidget + end + end + end +end + +describe Y2Partitioner::Dialogs::Encryption::ActionWidget do + let(:controller) do + double("FilesystemController", actions: [:keep, :encrypt], encryption: encryption) + end + + let(:encryption) { double("Encryption", type: Y2Storage::EncryptionType::LUKS) } + + subject(:widget) { described_class.new(controller) } + + include_examples "CWM::CustomWidget" + + describe "#help" do + it "returns a string" do + expect(widget.help).to be_a(String) + end + end + + describe "#store" do + before do + allow(widget).to receive(:value).and_return :the_value + allow(widget).to receive(:current_widget) + end + + it "sets #action in the controller" do + expect(controller).to receive(:action=).with(:the_value) + widget.store + end + end + + describe "#init" do + before do + allow(controller).to receive(:action).and_return :the_value + end + + it "sets #value to the action given by the controller" do + expect(widget).to receive(:value=).with(:the_value) + widget.init + end + end end diff --git a/test/y2partitioner/keep_original_filesystems_test.rb b/test/y2partitioner/keep_original_filesystems_test.rb index a5455dc2a5..d599f08571 100755 --- a/test/y2partitioner/keep_original_filesystems_test.rb +++ b/test/y2partitioner/keep_original_filesystems_test.rb @@ -109,6 +109,8 @@ def format_partition fs_controller.new_filesystem(new_fs_type) fs_controller.encrypt = false + Y2Partitioner::Actions::Controllers::Encryption.new(fs_controller).finish + expect(partition.filesystem.type).to eq new_fs_type expect(partition.encrypted?).to eq false end diff --git a/test/y2partitioner/widgets/encrypt_password_test.rb b/test/y2partitioner/widgets/encrypt_password_test.rb index efd0e2c4ec..da6fd93f98 100755 --- a/test/y2partitioner/widgets/encrypt_password_test.rb +++ b/test/y2partitioner/widgets/encrypt_password_test.rb @@ -29,15 +29,19 @@ include_examples "CWM::AbstractWidget" - let(:controller) { double("FilesystemController", blk_device_name: "/dev/sda1") } + let(:controller) { double("FilesystemController", password: "secret123") } let(:pw1) { "password1" } let(:pw2) { "password2" } let(:password_checker) { Y2Storage::EncryptPasswordChecker.new } + let(:enabled) { true } before do allow(Y2Storage::EncryptPasswordChecker).to receive(:new).and_return(password_checker) allow(Yast::UI).to receive(:QueryWidget).with(Id(:pw1), :Value).and_return(pw1) allow(Yast::UI).to receive(:QueryWidget).with(Id(:pw2), :Value).and_return(pw2) + + # Validation always checks whether the widget is really active + allow(Yast::UI).to receive(:QueryWidget).with(Id(widget.widget_id), :Enabled).and_return enabled end describe "#contents" do @@ -48,6 +52,12 @@ pw2 = contents.nested_find { |i| i.is_a?(Yast::Term) && i.params == [:pw2] } expect(pw2).to_not be_nil end + + it "prefills the password fields with the initial password" do + contents = widget.contents + field = contents.nested_find { |i| i.value == :Password } + expect(field.params).to include "secret123" + end end describe "#validate" do @@ -73,13 +83,25 @@ .and_return("some error") end - it "returns false" do - expect(widget.validate).to eq(false) + context "when the widget is active" do + let(:enabled) { true } + + it "returns false" do + expect(widget.validate).to eq(false) + end + + it "displays an error message" do + expect(Yast::Report).to receive(:Error).with("some error") + widget.validate + end end - it "displays an error message" do - expect(Yast::Report).to receive(:Error).with("some error") - widget.validate + context "when the widget is disabled" do + let(:enabled) { false } + + it "returns true" do + expect(widget.validate).to eq(true) + end end end end @@ -92,7 +114,7 @@ describe "#store" do it "assigns password to the controller" do - expect(controller).to receive(:encrypt_password=).with(pw1) + expect(controller).to receive(:password=).with(pw1) widget.store end end