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.

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)] }]

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

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) %>
 </div>
 <%= 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" } %>
 </div>
 
-<%= 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 } %>
 

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,13 +396,33 @@ 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"}
         assert_equal attrs_out, @cr.parse_args(attrs_in)
       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"}})

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

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