From f12806d62b3c91863f26afff2766672f3ff456ed 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 | 58 +++++++++++-------- app/models/concerns/pxe_loader_support.rb | 2 + .../form/vmware/_base.html.erb | 10 ++-- bundler.d/vmware.rb | 4 +- test/models/compute_resources/vmware_test.rb | 16 ++++- .../concerns/pxe_loader_support_test.rb | 4 ++ test/models/host_test.rb | 5 ++ 7 files changed, 66 insertions(+), 33 deletions(-) diff --git a/app/models/compute_resources/foreman/model/vmware.rb b/app/models/compute_resources/foreman/model/vmware.rb index a22c2b8306d..85b334f4545 100644 --- a/app/models/compute_resources/foreman/model/vmware.rb +++ b/app/models/compute_resources/foreman/model/vmware.rb @@ -207,7 +207,16 @@ def firmware_types "automatic" => N_("Automatic"), "bios" => N_("BIOS"), "efi" => N_("EFI"), - } + "uefi_secure_boot" => N_("UEFI Secure Boot"), + }.freeze + end + + def firmware_type(vm_obj) + if vm_obj.firmware == 'efi' + vm_obj.secure_boot ? "uefi_secure_boot" : "efi" + else + vm_obj.firmware + end end def disk_mode_types @@ -488,8 +497,22 @@ 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' + firmware_type = args.delete(:firmware_type).to_s + + if args[:firmware] == 'automatic' + if firmware_type == 'none' || firmware_type.empty? + args[:firmware] = 'bios' + else + args[:firmware] = firmware_type + end + end + + if args[:firmware]&.start_with?('uefi') + args[:secure_boot] = true if args[:firmware] == 'uefi_secure_boot' + args[:firmware] = 'efi' + end + + args[:virtual_tpm] = ActiveRecord::Type::Boolean.new.cast(args.delete(:virtual_tpm)) args.reject! { |k, v| v.nil? } args @@ -511,6 +534,12 @@ def parse_networks(args) args end + def check_tpm_compatibility(vm) + if vm.firmware == 'bios' && vm.virtual_tpm + raise ArgumentError, _('TPM is not compatible with BIOS firmware. Please change Firmware or disable TPM.') + end + end + def create_vm(args = { }) vm = nil test_connection @@ -522,7 +551,7 @@ def create_vm(args = { }) clone_vm(args) else vm = new_vm(args) - vm.firmware = 'bios' if vm.firmware == 'automatic' + check_tpm_compatibility(vm) vm.save end rescue Fog::Vsphere::Compute::NotFound => e @@ -772,22 +801,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 @@ -843,11 +856,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..1dfe8fa6775 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)") %> + +<% firmware_type = compute_resource.firmware_type(f.object) %> <%= field(f, :firmware, :label => _('Firmware'), :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 EFI firmware."), :label_size => "col-md-2", :disabled => !new_vm } %> diff --git a/bundler.d/vmware.rb b/bundler.d/vmware.rb index 0a8a0a84ef9..bc91ccf395e 100644 --- a/bundler.d/vmware.rb +++ b/bundler.d/vmware.rb @@ -1,3 +1,5 @@ group :vmware do - gem 'fog-vsphere', '>= 3.6.4', '< 4.0' + # gem 'fog-vsphere', '>= 3.6.4', '< 4.0' + # TODO: Remove this once the fog-vsphere gem is released, and upgrade to the latest version + gem 'fog-vsphere', :git => "https://github.com/fog/fog-vsphere.git", :branch => "master" end diff --git a/test/models/compute_resources/vmware_test.rb b/test/models/compute_resources/vmware_test.rb index f55c253eadd..b29c94d9f9b 100644 --- a/test/models/compute_resources/vmware_test.rb +++ b/test/models/compute_resources/vmware_test.rb @@ -140,6 +140,7 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase mock_vm = mock('vm') mock_vm.expects(:save).returns(mock_vm) mock_vm.stubs(:firmware).returns('bios') + mock_vm.stubs(:virtual_tpm).returns(false) @cr.stubs(:parse_networks).returns(args) @cr.expects(:clone_vm).times(0) @cr.expects(:new_vm).returns(mock_vm) @@ -151,7 +152,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 +398,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 +411,14 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase end end + context 'virtual_tpm' do + test 'sets virtual_tpm to true when firmware is EFI and virtual_tpm is enabled' do + attrs_in = HashWithIndifferentAccess.new(:firmware => 'efi', :virtual_tpm => '1') + attrs_out = { :firmware => 'efi', :virtual_tpm => true } + assert_equal attrs_out, @cr.parse_args(attrs_in) + 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 13b10911567..a305671589d 100644 --- a/test/models/host_test.rb +++ b/test/models/host_test.rb @@ -3281,6 +3281,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