From d94961a6fef83b150c09fed6be8dc0956cc6e755 Mon Sep 17 00:00:00 2001 From: nofaralfasi Date: Thu, 18 Jul 2024 12:21:07 +0300 Subject: [PATCH] Refactor Secure Boot & TPM Support for VMware - Added a new firmware type for Secure Boot. - Handles TPM conflict with BIOS. - Removed unnecessary methods from the VMware model. --- .../compute_resources/foreman/model/vmware.rb | 57 ++++++++++--------- app/models/concerns/pxe_loader_support.rb | 2 + .../form/vmware/_base.html.erb | 12 ++-- test/models/compute_resources/vmware_test.rb | 22 ++++++- .../concerns/pxe_loader_support_test.rb | 4 ++ test/models/host_test.rb | 5 ++ 6 files changed, 67 insertions(+), 35 deletions(-) diff --git a/app/models/compute_resources/foreman/model/vmware.rb b/app/models/compute_resources/foreman/model/vmware.rb index 653613daa45..87ffb1fc3e2 100644 --- a/app/models/compute_resources/foreman/model/vmware.rb +++ b/app/models/compute_resources/foreman/model/vmware.rb @@ -206,8 +206,18 @@ def firmware_types { "automatic" => N_("Automatic"), "bios" => N_("BIOS"), - "efi" => N_("EFI"), - } + "uefi" => N_("UEFI"), + "uefi_secure_boot" => N_("UEFI Secure Boot"), + }.freeze + end + + # Returns the firmware type based on the VM object + def firmware_type(vm_obj) + if vm_obj.firmware == 'efi' + vm_obj.secure_boot ? "uefi_secure_boot" : "uefi" + else + vm_obj.firmware + end end def disk_mode_types @@ -488,8 +498,24 @@ def parse_args(args) args.except!(:hardware_version) if args[:hardware_version] == 'Default' - firmware_type = args.delete(:firmware_type) - args[:firmware] = firmware_mapping(firmware_type) if args[:firmware] == 'automatic' + # This value comes from PxeLoaderSupport#firmware_type + firmware_type = args.delete(:firmware_type).to_s + + # automatic firmware type is determined by the PXE Loader + # if no PXE Loader is set, we will set it to bios by default + if args[:firmware] == 'automatic' + args[:firmware] = (firmware_type == 'none' || firmware_type.empty?) ? 'bios' : firmware_type + end + + # Adjust firmware and secure_boot values for VMware compatibility + args[:secure_boot] = true if args[:firmware] == 'uefi_secure_boot' + # VMware expects the firmware type to be 'efi' + args[:firmware] = 'efi' if args[:firmware]&.start_with?('uefi') + + args[:virtual_tpm] = ActiveRecord::Type::Boolean.new.cast(args[:virtual_tpm]) + if args[:firmware] == 'bios' && args[:virtual_tpm] + errors.add :base, _('TPM is not compatible with BIOS firmware. Please change Firmware or disable TPM.') + end args.reject! { |k, v| v.nil? } args @@ -522,7 +548,7 @@ def create_vm(args = { }) clone_vm(args) else vm = new_vm(args) - vm.firmware = 'bios' if vm.firmware == 'automatic' + raise ArgumentError, errors.full_messages.join(';') if errors.any? vm.save end rescue Fog::Vsphere::Compute::NotFound => e @@ -772,22 +798,6 @@ def normalize_vm_attrs(vm_attrs) normalized end - def secure_boot - attrs[:secure_boot] ||= false - end - - def secure_boot=(enabled) - attrs[:secure_boot] = ActiveRecord::Type::Boolean.new.cast(enabled) - end - - def virtual_tpm - attrs[:virtual_tpm] ||= false - end - - def virtual_tpm=(enabled) - attrs[:virtual_tpm] = ActiveRecord::Type::Boolean.new.cast(enabled) - end - private def dc @@ -844,11 +854,6 @@ def vm_instance_defaults ) end - def firmware_mapping(firmware_type) - return 'efi' if firmware_type == :uefi - 'bios' - end - def set_vm_volumes_attributes(vm, vm_attrs) volumes = vm.volumes || [] vm_attrs[:volumes_attributes] = Hash[volumes.each_with_index.map { |volume, idx| [idx.to_s, volume.attributes.merge(:size_gb => volume.size_gb)] }] diff --git a/app/models/concerns/pxe_loader_support.rb b/app/models/concerns/pxe_loader_support.rb index c98632d7691..5fbeed8fbef 100644 --- a/app/models/concerns/pxe_loader_support.rb +++ b/app/models/concerns/pxe_loader_support.rb @@ -50,6 +50,8 @@ def firmware_type(pxe_loader) case pxe_loader when 'None' :none + when /SecureBoot/ + :uefi_secure_boot when /UEFI/ :uefi else diff --git a/app/views/compute_resources_vms/form/vmware/_base.html.erb b/app/views/compute_resources_vms/form/vmware/_base.html.erb index 07dd71bab89..5e794b30cb0 100644 --- a/app/views/compute_resources_vms/form/vmware/_base.html.erb +++ b/app/views/compute_resources_vms/form/vmware/_base.html.erb @@ -6,9 +6,11 @@ <%= counter_f(f, :corespersocket, label: _('Cores per socket'), recommended_max_value: compute_resource.max_cpu_count, value: f.object.corespersocket || 1) %> <%= text_f f, :memory_mb, :class => "col-md-2", :label => _("Memory (MB)") %> -<%= field(f, :firmware, :label => _('Firmware'), :label_size => "col-md-2") do + +<% firmware_type = compute_resource.firmware_type(f.object) %> +<%= field(f, :firmware, :label => _('Firmware'), :label_help => _("Choose 'Automatic' to set the firmware based on the PXE Loader. If no PXE Loader is selected, it defaults to BIOS."), :label_size => "col-md-2") do compute_resource.firmware_types.collect do |type, name| - radio_button_f f, :firmware, {:disabled => !new_vm, :value => type, :text => _(name)} + radio_button_f f, :firmware, {:checked => (firmware_type == type), :disabled => !new_vm, :value => type, :text => _(name)} end.join(' ').html_safe end %> <%= selectable_f f, :cluster, compute_resource.clusters, { :include_blank => _('Please select a cluster') }, @@ -49,13 +51,9 @@ end %> { :disabled => images.empty?, :label => _('Image'), :label_size => "col-md-2" } %> -<%= checkbox_f f, :secure_boot, { :help_inline => _("Enable Secure Bott for provisioning."), - :label => _('Secure Boot'), - :label_size => "col-md-2", - :disabled => !new_vm } %> - <%= checkbox_f f, :virtual_tpm, { :help_inline => _("Add Virtual TPM module to the VM."), :label => _('Virtual TPM'), + :label_help => _("Only compatible with UEFI firmware."), :label_size => "col-md-2", :disabled => !new_vm } %> diff --git a/test/models/compute_resources/vmware_test.rb b/test/models/compute_resources/vmware_test.rb index f55c253eadd..2c119466279 100644 --- a/test/models/compute_resources/vmware_test.rb +++ b/test/models/compute_resources/vmware_test.rb @@ -34,7 +34,6 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase mock_vm = mock('vm') mock_vm.expects(:save).returns(mock_vm) - mock_vm.expects(:firmware).returns('biod') cr = FactoryBot.build_stubbed(:vmware_cr) cr.expects(:parse_networks).with(attrs_in).returns(attrs_parsed) @@ -151,7 +150,6 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase mock_vm = mock('vm') mock_vm.expects(:save).returns(mock_vm) mock_vm.stubs(:firmware).returns('automatic') - mock_vm.expects(:firmware=).with('bios') @cr.stubs(:parse_networks).returns(args) @cr.expects(:new_vm).returns(mock_vm) @cr.create_vm(args) @@ -398,6 +396,12 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase assert_equal attrs_out, @cr.parse_args(attrs_in) end + test 'chooses EFI firmware with SecureBoot enabled when firmware type is uefi_secure_boot and firmware is automatic' do + attrs_in = HashWithIndifferentAccess.new(:firmware_type => :uefi_secure_boot, 'firmware' => 'automatic') + attrs_out = {:firmware => "efi", :secure_boot => true} + assert_equal attrs_out, @cr.parse_args(attrs_in) + end + test 'chooses BIOS firmware when no pxe loader is set and firmware is automatic' do attrs_in = HashWithIndifferentAccess.new('firmware' => 'automatic') attrs_out = {:firmware => "bios"} @@ -405,6 +409,20 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase end end + context 'virtual_tpm' do + test 'sets virtual_tpm to true when firmware is UEFI and virtual_tpm is enabled' do + attrs_in = HashWithIndifferentAccess.new(:firmware => 'uefi', :virtual_tpm => '1') + attrs_out = { :firmware => 'efi', :virtual_tpm => true } + assert_equal attrs_out, @cr.parse_args(attrs_in) + end + + test 'adds an error when firmware is BIOS and virtual_tpm is enabled' do + args = HashWithIndifferentAccess.new(:firmware => 'bios', :virtual_tpm => '1') + @cr.parse_args(args) + assert_includes @cr.errors.full_messages, 'TPM is not compatible with BIOS firmware. Please change Firmware or disable TPM.' + end + end + test "doesn't modify input hash" do # else compute profiles won't save properly attrs_in = HashWithIndifferentAccess.new("interfaces_attributes" => {"0" => {"network" => "network-17"}}) diff --git a/test/models/concerns/pxe_loader_support_test.rb b/test/models/concerns/pxe_loader_support_test.rb index 9a431957234..6636504b0d5 100644 --- a/test/models/concerns/pxe_loader_support_test.rb +++ b/test/models/concerns/pxe_loader_support_test.rb @@ -195,5 +195,9 @@ def setup test 'defaults to bios firmware' do assert_equal :bios, DummyPxeLoader.firmware_type('Anything') end + + test 'detects uefi_secure_boot firmware' do + assert_equal :uefi_secure_boot, DummyPxeLoader.firmware_type('Grub2 UEFI SecureBoot') + end end end diff --git a/test/models/host_test.rb b/test/models/host_test.rb index 8865243d3ef..02faa254e3b 100644 --- a/test/models/host_test.rb +++ b/test/models/host_test.rb @@ -3313,6 +3313,11 @@ def to_managed! host = FactoryBot.build_stubbed(:host, :managed, :pxe_loader => "Grub2 UEFI") assert_equal :uefi, host.firmware_type end + + test 'should be :uefi_secure_boot for host with uefi_secure_boot loader' do + host = FactoryBot.build_stubbed(:host, :managed, :pxe_loader => "Grub2 UEFI SecureBoot") + assert_equal :uefi_secure_boot, host.firmware_type + end end describe '#templates_used' do