From 0f128c539581fd282bb48dbfff9394bbe463d91c Mon Sep 17 00:00:00 2001 From: Leos Stejskal Date: Mon, 17 Jun 2024 15:26:55 +0200 Subject: [PATCH 1/3] Fixes #37566 - Libvirt - UEFI & SecureBoot support Co-authored-by: Ewoud Kohl van Wijngaarden --- .../foreman/model/libvirt.rb | 31 +++++++++++++++++-- .../form/libvirt/_base.html.erb | 24 ++++++++++++++ .../javascripts/compute_resource/libvirt.js | 7 +++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/app/models/compute_resources/foreman/model/libvirt.rb b/app/models/compute_resources/foreman/model/libvirt.rb index 23d367de42b..ef50d2284a1 100644 --- a/app/models/compute_resources/foreman/model/libvirt.rb +++ b/app/models/compute_resources/foreman/model/libvirt.rb @@ -11,7 +11,29 @@ def self.available? Fog::Compute.providers.include?(:libvirt) end - # Some getters/setters for the attrs Hash + def self.firmware_types + { + "efi" => N_("EFI"), + "bios" => N_("BIOS"), + }.freeze + end + + def os_firmware + attrs[:os_firmware].presence || "efi" + end + + def os_firmware=(firmware) + attrs[:os_firmware] = firmware + end + + def os_firmware_features + attrs[:os_firmware_features].presence || {} + end + + def os_firmware_features=(attrs) + attrs[:os_firmware_features].merge attrs + end + def display_type attrs[:display].presence || 'vnc' end @@ -289,7 +311,12 @@ def vm_instance_defaults :display => { :type => display_type, :listen => Setting[:libvirt_default_console_address], :password => random_password(console_password_length(display_type)), - :port => '-1' } + :port => '-1' }, + :os_firmware => 'efi', + :os_firmware_features => { + "secure-boot" => "no", + "enrolled-keys" => "no", + } ) end diff --git a/app/views/compute_resources_vms/form/libvirt/_base.html.erb b/app/views/compute_resources_vms/form/libvirt/_base.html.erb index ad88274d096..1b9656e4af5 100644 --- a/app/views/compute_resources_vms/form/libvirt/_base.html.erb +++ b/app/views/compute_resources_vms/form/libvirt/_base.html.erb @@ -21,3 +21,27 @@ <%= compute_specific_js(compute_resource, "nic_info") %> + +<%= select_f f, :os_firmware, + Foreman::Model::Libvirt.firmware_types, + :first, + :last, + {}, + { :label => _("Firmware"), + :label_size => "col-md-2", + :onchange => "tfm.computeResource.libvirt.firmwareSelected(this);", + } +%> +<% + feature_attrs = ActiveSupport::HashWithIndifferentAccess.new(f.object.os_firmware_features) + is_bios = f.object.os_firmware == 'bios' +%> + +
+<%= f.fields_for :os_firmware_features do |ff| + secure_field = checkbox_f(ff, 'secure-boot', { checked: feature_attrs['secure-boot'] == 'yes', label: _("Secure Boot"), disabled: is_bios }, 'yes', 'no') + keys_field = checkbox_f(ff, 'enrolled-keys', { checked: feature_attrs['enrolled-keys'] == 'yes', label: _("Enrolled keys"), disabled: is_bios }, 'yes', 'no') + secure_field + keys_field + end +%> +
diff --git a/webpack/assets/javascripts/compute_resource/libvirt.js b/webpack/assets/javascripts/compute_resource/libvirt.js index 842b1304f5a..3a269b83a74 100644 --- a/webpack/assets/javascripts/compute_resource/libvirt.js +++ b/webpack/assets/javascripts/compute_resource/libvirt.js @@ -138,3 +138,10 @@ export function allocationSwitcher(element, action) { $(element).button('toggle'); return false; } + +export function firmwareSelected(item) { + const selected = $(item).val(); + const inputs = $("#os_firmware_features input[type='checkbox']"); + + inputs.attr('disabled', selected === 'bios'); +} From 4c40ca941fb87d3163fcd45e28d688427c6969b1 Mon Sep 17 00:00:00 2001 From: nofaralfasi Date: Mon, 22 Jul 2024 10:45:13 +0300 Subject: [PATCH 2/3] Refactor Secure Boot support for Libvirt - Updated the Libvirt VM creation form. - Added a new firmware type for Secure Boot. - Enable `enrolled-keys` by default when Secure Boot is activated. - Removed unnecessary methods from the Libvirt model. - Moved firmware-related methods to the ComputeResource model for shared use between VMware and Libvirt. --- app/models/compute_resource.rb | 18 +++++++ .../foreman/model/libvirt.rb | 51 +++++++++---------- app/models/concerns/pxe_loader_support.rb | 2 + .../form/libvirt/_base.html.erb | 31 +++-------- bundler.d/libvirt.rb | 4 +- .../javascripts/compute_resource/libvirt.js | 7 --- 6 files changed, 53 insertions(+), 60 deletions(-) diff --git a/app/models/compute_resource.rb b/app/models/compute_resource.rb index a555156e9bd..0a9333a3cb1 100644 --- a/app/models/compute_resource.rb +++ b/app/models/compute_resource.rb @@ -399,6 +399,24 @@ def normalize_vm_attrs(vm_attrs) vm_attrs end + def firmware_types + { + "automatic" => N_("Automatic"), + "bios" => N_("BIOS"), + "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' # special case for secure boot + else + vm_obj.firmware + end + end + protected def memory_gb_to_bytes(memory_size) diff --git a/app/models/compute_resources/foreman/model/libvirt.rb b/app/models/compute_resources/foreman/model/libvirt.rb index ef50d2284a1..e843dff51c1 100644 --- a/app/models/compute_resources/foreman/model/libvirt.rb +++ b/app/models/compute_resources/foreman/model/libvirt.rb @@ -11,29 +11,6 @@ def self.available? Fog::Compute.providers.include?(:libvirt) end - def self.firmware_types - { - "efi" => N_("EFI"), - "bios" => N_("BIOS"), - }.freeze - end - - def os_firmware - attrs[:os_firmware].presence || "efi" - end - - def os_firmware=(firmware) - attrs[:os_firmware] = firmware - end - - def os_firmware_features - attrs[:os_firmware_features].presence || {} - end - - def os_firmware_features=(attrs) - attrs[:os_firmware_features].merge attrs - end - def display_type attrs[:display].presence || 'vnc' end @@ -170,6 +147,27 @@ def new_vm(attr = { }) opts[:boot_order] = %w[hd] opts[:boot_order].unshift 'network' unless attr[:image_id] + # This value comes from PxeLoaderSupport#firmware_type + firmware_type = opts.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 opts[:firmware] == 'automatic' + opts[:firmware] = (firmware_type == 'none' || firmware_type.empty?) ? 'bios' : firmware_type + end + + # Adjust firmware and secure_boot values for Libvirt compatibility + if opts[:firmware] == 'uefi_secure_boot' + opts[:firmware_features] = { 'secure-boot' => 'yes', 'enrolled-keys' => 'yes' } + opts[:loader] = { 'secure' => 'yes' } + end + # Libvirt expects the firmware type to be 'efi' instead of 'uefi' + if opts[:firmware]&.start_with?('uefi') + opts[:firmware] = 'efi' + else + opts[:firmware] = 'bios' + end + vm = client.servers.new opts vm.memory = opts[:memory] if opts[:memory] vm @@ -312,11 +310,8 @@ def vm_instance_defaults :listen => Setting[:libvirt_default_console_address], :password => random_password(console_password_length(display_type)), :port => '-1' }, - :os_firmware => 'efi', - :os_firmware_features => { - "secure-boot" => "no", - "enrolled-keys" => "no", - } + :firmware => 'automatic', + :firmware_features => { "secure-boot" => "no", } ) end 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/libvirt/_base.html.erb b/app/views/compute_resources_vms/form/libvirt/_base.html.erb index 1b9656e4af5..51744633264 100644 --- a/app/views/compute_resources_vms/form/libvirt/_base.html.erb +++ b/app/views/compute_resources_vms/form/libvirt/_base.html.erb @@ -8,6 +8,13 @@ <% checked = params[:host] && params[:host][:compute_attributes] && params[:host][:compute_attributes][:start] || '1' %> <%= checkbox_f f, :start, { :checked => (checked == '1'), :help_inline => _("Power ON this machine"), :label => _('Start'), :label_size => "col-md-2"} if new_vm && controller_name != "compute_attributes" %> +<% firmware_type = new_vm ? 'automatic' : compute_resource.firmware_type(f.object) %> +<%= field(f, :firmware, :disabled => !new_vm, :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, {:checked => (firmware_type == type), :disabled => !new_vm, :value => type, :text => _(name) } + end.join(' ').html_safe +end %> + <% arch ||= nil ; os ||= nil images = possible_images(compute_resource, arch, os) @@ -21,27 +28,3 @@ <%= compute_specific_js(compute_resource, "nic_info") %> - -<%= select_f f, :os_firmware, - Foreman::Model::Libvirt.firmware_types, - :first, - :last, - {}, - { :label => _("Firmware"), - :label_size => "col-md-2", - :onchange => "tfm.computeResource.libvirt.firmwareSelected(this);", - } -%> -<% - feature_attrs = ActiveSupport::HashWithIndifferentAccess.new(f.object.os_firmware_features) - is_bios = f.object.os_firmware == 'bios' -%> - -
-<%= f.fields_for :os_firmware_features do |ff| - secure_field = checkbox_f(ff, 'secure-boot', { checked: feature_attrs['secure-boot'] == 'yes', label: _("Secure Boot"), disabled: is_bios }, 'yes', 'no') - keys_field = checkbox_f(ff, 'enrolled-keys', { checked: feature_attrs['enrolled-keys'] == 'yes', label: _("Enrolled keys"), disabled: is_bios }, 'yes', 'no') - secure_field + keys_field - end -%> -
diff --git a/bundler.d/libvirt.rb b/bundler.d/libvirt.rb index 80b3f8c5a12..584f15e52e7 100644 --- a/bundler.d/libvirt.rb +++ b/bundler.d/libvirt.rb @@ -1,4 +1,6 @@ group :libvirt do - gem 'fog-libvirt', '>= 0.12.0' + # gem 'fog-libvirt', '>= 0.9.0' gem 'ruby-libvirt', '~> 0.5', :require => 'libvirt' + # TODO: Remove this line after merging the PR + gem 'fog-libvirt', github: 'nofaralfasi/fog-libvirt', branch: 'sb_libvirt' end diff --git a/webpack/assets/javascripts/compute_resource/libvirt.js b/webpack/assets/javascripts/compute_resource/libvirt.js index 3a269b83a74..842b1304f5a 100644 --- a/webpack/assets/javascripts/compute_resource/libvirt.js +++ b/webpack/assets/javascripts/compute_resource/libvirt.js @@ -138,10 +138,3 @@ export function allocationSwitcher(element, action) { $(element).button('toggle'); return false; } - -export function firmwareSelected(item) { - const selected = $(item).val(); - const inputs = $("#os_firmware_features input[type='checkbox']"); - - inputs.attr('disabled', selected === 'bios'); -} From 81a503a9c3e2ea5bc41361c88438d768be8c3204 Mon Sep 17 00:00:00 2001 From: nofaralfasi Date: Thu, 5 Sep 2024 20:08:23 +0300 Subject: [PATCH 3/3] Refactor Libvirt Firmware Handling for Better Testability - Moved firmware logic into dedicated methods. - Updated tests for improved coverage and workflow alignment. --- .../foreman/model/libvirt.rb | 45 ++++++++-------- test/models/compute_resources/libvirt_test.rb | 51 +++++++++++++++++++ 2 files changed, 75 insertions(+), 21 deletions(-) diff --git a/app/models/compute_resources/foreman/model/libvirt.rb b/app/models/compute_resources/foreman/model/libvirt.rb index e843dff51c1..098758255fc 100644 --- a/app/models/compute_resources/foreman/model/libvirt.rb +++ b/app/models/compute_resources/foreman/model/libvirt.rb @@ -147,26 +147,8 @@ def new_vm(attr = { }) opts[:boot_order] = %w[hd] opts[:boot_order].unshift 'network' unless attr[:image_id] - # This value comes from PxeLoaderSupport#firmware_type - firmware_type = opts.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 opts[:firmware] == 'automatic' - opts[:firmware] = (firmware_type == 'none' || firmware_type.empty?) ? 'bios' : firmware_type - end - - # Adjust firmware and secure_boot values for Libvirt compatibility - if opts[:firmware] == 'uefi_secure_boot' - opts[:firmware_features] = { 'secure-boot' => 'yes', 'enrolled-keys' => 'yes' } - opts[:loader] = { 'secure' => 'yes' } - end - # Libvirt expects the firmware type to be 'efi' instead of 'uefi' - if opts[:firmware]&.start_with?('uefi') - opts[:firmware] = 'efi' - else - opts[:firmware] = 'bios' - end + handle_automatic_firmware(opts) + configure_firmware_settings(opts) vm = client.servers.new opts vm.memory = opts[:memory] if opts[:memory] @@ -311,7 +293,7 @@ def vm_instance_defaults :password => random_password(console_password_length(display_type)), :port => '-1' }, :firmware => 'automatic', - :firmware_features => { "secure-boot" => "no", } + :firmware_features => { "secure-boot" => "no" } ) end @@ -348,5 +330,26 @@ def validate_volume_capacity(vol) raise Foreman::Exception.new(N_("Please specify volume size. You may optionally use suffix 'G' to specify volume size in gigabytes.")) end end + + def handle_automatic_firmware(opts) + # This value comes from PxeLoaderSupport#firmware_type + firmware_type = opts.delete(:firmware_type).to_s + + # Handle automatic firmware setting based on PXE Loader + # if no PXE Loader is set, we will set it to bios by default + if opts[:firmware] == 'automatic' + opts[:firmware] = firmware_type.presence || 'bios' + end + end + + def configure_firmware_settings(opts) + # Adjust firmware and secure boot settings for Libvirt compatibility + if opts[:firmware] == 'uefi_secure_boot' + opts[:firmware_features] = { 'secure-boot' => 'yes', 'enrolled-keys' => 'yes' } + opts[:loader] = { 'secure' => 'yes' } + end + + opts[:firmware] = opts[:firmware]&.start_with?('uefi') ? 'efi' : 'bios' + end end end diff --git a/test/models/compute_resources/libvirt_test.rb b/test/models/compute_resources/libvirt_test.rb index 9bf15229ad9..fc6fdad458a 100644 --- a/test/models/compute_resources/libvirt_test.rb +++ b/test/models/compute_resources/libvirt_test.rb @@ -124,6 +124,57 @@ class Foreman::Model::LibvirtTest < ActiveSupport::TestCase end end + describe '#handle_automatic_firmware' do + setup do + @cr = FactoryBot.build_stubbed(:libvirt_cr) + end + + test 'sets firmware based on automatic and firmware_type scenarios' do + scenarios = [ + [{ firmware: 'automatic' }, { firmware: 'bios' }], + [{ firmware: 'automatic', firmware_type: :bios }, { firmware: 'bios' }], + [{ firmware: 'automatic', firmware_type: :uefi }, { firmware: 'uefi' }], + [{ firmware: 'automatic', firmware_type: :uefi_secure_boot }, { firmware: 'uefi_secure_boot' }], + ] + + scenarios.each do |input, expected| + @cr.send(:handle_automatic_firmware, input) + assert_equal(expected, input) + end + end + + test 'does not change firmware if it is not automatic' do + attrs_in = { firmware_type: :uefi_secure_boot, firmware: 'uefi' } + @cr.send(:handle_automatic_firmware, attrs_in) + assert_equal({ firmware: 'uefi' }, attrs_in) + end + end + + describe '#configure_firmware_settings' do + setup do + @cr = FactoryBot.build_stubbed(:libvirt_cr) + end + + test 'sets firmware to BIOS when no firmware is provided' do + attrs_in = { firmware: '' } + @cr.send(:configure_firmware_settings, attrs_in) + assert_equal({ firmware: 'bios' }, attrs_in) + end + + test 'sets firmware to EFI when firmware_type is uefi' do + attrs_in = { firmware: 'uefi' } + @cr.send(:configure_firmware_settings, attrs_in) + assert_equal({ firmware: 'efi' }, attrs_in) + end + + test 'sets EFI firmware with SecureBoot enabled and secure loader when firmware type is uefi_secure_boot' do + attrs_in = { firmware: 'uefi_secure_boot' } + attrs_out = { firmware: 'efi', firmware_features: { 'secure-boot' => 'yes', 'enrolled-keys' => 'yes' }, loader: { 'secure' => 'yes' } } + @cr.send(:configure_firmware_settings, attrs_in) + assert_equal attrs_out, attrs_in + end + end + describe '#new_volume' do let(:cr) { FactoryBot.build_stubbed(:libvirt_cr) }