Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #37566 - Add UEFI Secure Boot Firmware to Libvirt #10321

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions app/models/compute_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,68 @@ def normalize_vm_attrs(vm_attrs)
vm_attrs
end

# Returns a hash of firmware type identifiers and their corresponding labels for use in the VM creation form.
#
# @return [Hash<String, String>] a hash mapping firmware type identifiers to labels.
def firmware_types
{
"automatic" => N_("Automatic"),
"bios" => N_("BIOS"),
"uefi" => N_("UEFI"),
"uefi_secure_boot" => N_("UEFI Secure Boot"),
}.freeze
end

# Converts the firmware type from a VM object to the Foreman-compatible format.
#
# @param firmware [String] The firmware type from the VM object.
# @param secure_boot [Boolean] Indicates if secure boot is enabled for the VM.
# @return [String] The converted firmware type.
def firmware_type(firmware, secure_boot)
if firmware == 'efi'
secure_boot ? 'uefi_secure_boot' : 'uefi' # Adjust for secure boot
else
firmware
end
end

# Converts a firmware type from Foreman format to a CR-compatible format.
# If no specific type is provided, defaults to 'bios'.
#
# @param firmware_type [String] The firmware type in Foreman format.
# @return [String] The converted firmware type.
def normalize_firmware_type(firmware_type)
case firmware_type
when 'uefi', 'uefi_secure_boot'
'efi'
else
'bios'
end
end

# Resolves the firmware setting when it is 'automatic' based on the provided firmware_type, or defaults to 'bios'.
#
# @param firmware [String] The current firmware setting.
# @param firmware_type [String] The type of firmware to be used if firmware is 'automatic'.
# @return [String] the resolved firmware.
def resolve_automatic_firmware(firmware, firmware_type)
return firmware unless firmware == 'automatic'
firmware_type.presence || 'bios'
end

# Processes firmware attributes to configure firmware and secure boot settings.
#
# @param firmware [String] The firmware setting to be processed.
# @param firmware_type [String] The firmware type based on the provided PXE Loader.
# @return [Hash] A hash containing the processed firmware attributes.
def process_firmware_attributes(firmware, firmware_type)
firmware = resolve_automatic_firmware(firmware, firmware_type)

attrs = generate_secure_boot_settings(firmware)
attrs[:firmware] = normalize_firmware_type(firmware)
attrs
end

protected

def memory_gb_to_bytes(memory_size)
Expand Down
22 changes: 21 additions & 1 deletion app/models/compute_resources/foreman/model/libvirt.rb
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,9 @@ def new_vm(attr = { })
opts[:boot_order] = %w[hd]
opts[:boot_order].unshift 'network' unless attr[:image_id]

firmware_type = opts.delete(:firmware_type).to_s
opts.merge!(process_firmware_attributes(opts[:firmware], firmware_type))

vm = client.servers.new opts
vm.memory = opts[:memory] if opts[:memory]
vm
Expand Down Expand Up @@ -289,7 +292,9 @@ 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' },
:firmware => 'automatic',
:firmware_features => { "secure-boot" => "no" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for having strings as keys in the inner hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the reason for using strings as keys here is that Libvirt expects these values in this format and converts them to XML accordingly. You can see this conversion in the Libvirt code here. For more details, refer to the related PR.

)
end

Expand Down Expand Up @@ -326,5 +331,20 @@ 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

# Generates Secure Boot settings for Libvirt based on the provided firmware type.
# The `secure_boot` setting is used to properly configure and display the Firmware in the `compute_attributes` form.
#
# @param firmware [String] The firmware type.
# @return [Hash] A hash with secure boot settings if applicable.
def generate_secure_boot_settings(firmware)
return {} unless firmware == 'uefi_secure_boot'

{
firmware_features: { 'secure-boot' => 'yes', 'enrolled-keys' => 'yes' },
loader: { 'secure' => 'yes' },
secure_boot: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For other keys and values, you use 'key' => 'yes'. Wouldn't it make sense to do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The secure_boot: true setting here is a special case, required only for displaying the correct firmware in the compute_attributes form. We don't process this setting in fog-libvirt the same way we do when creating a new VM. Therefore, if we change its format, there’s little reason to use it, as we could instead rely on the loader attribute. This approach simplifies the usage of the ComputeResource#firmware_type method, aligning it with how we handle VMs.

}
end
end
end
2 changes: 2 additions & 0 deletions app/models/concerns/pxe_loader_support.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ def firmware_type(pxe_loader)
case pxe_loader
when 'None'
:none
when /SecureBoot/
:uefi_secure_boot
when /UEFI/
:uefi
else
Expand Down
7 changes: 7 additions & 0 deletions app/views/compute_resources_vms/form/libvirt/_base.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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 = compute_resource.firmware_type(f.object.firmware, f.object.secure_boot) %>
<%= field(f, :firmware, :disabled => !new_vm, :label => _('Firmware'), :label_help => _("Choose 'Automatic' to set the firmware based on the host's 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)
Expand Down
4 changes: 3 additions & 1 deletion bundler.d/libvirt.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
group :libvirt do
gem 'fog-libvirt', '>= 0.12.0'
# gem 'fog-libvirt', '>= 0.12.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
69 changes: 69 additions & 0 deletions test/models/compute_resource_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -391,4 +391,73 @@ def setup
assert_equal volume_attributes, volumes
end
end

describe '#firmware_type' do
before do
@cr = compute_resources(:mycompute)
end

test "returns firmware unchanged when firmware is not 'efi'" do
assert_equal 'bios', @cr.firmware_type('bios', true)
assert_equal 'bios', @cr.firmware_type('bios', false)
assert_empty(@cr.firmware_type('', true))
assert_nil(@cr.firmware_type(nil, false))
end

test "returns 'uefi' when firmware is 'efi' and secure boot is not enabled" do
assert_equal 'uefi', @cr.firmware_type('efi', false)
assert_equal 'uefi', @cr.firmware_type('efi', nil)
end

test "returns 'uefi_secure_boot' when firmware is 'efi' and secure boot is enabled" do
assert_equal 'uefi_secure_boot', @cr.firmware_type('efi', true)
end
end

describe '#normalize_firmware_type' do
before do
@cr = compute_resources(:mycompute)
end

test "returns 'efi' when firmware is 'uefi'" do
assert_equal 'efi', @cr.normalize_firmware_type('uefi')
end

test "returns 'bios' for non-uefi firmware types" do
assert_equal 'bios', @cr.normalize_firmware_type('bios')
assert_equal 'bios', @cr.normalize_firmware_type('none')
assert_equal 'bios', @cr.normalize_firmware_type('')
assert_equal 'bios', @cr.normalize_firmware_type(nil)
end

test "returns 'efi' when firmware is 'uefi_secure_boot'" do
assert_equal 'efi', @cr.normalize_firmware_type('uefi_secure_boot')
end
end

describe '#resolve_automatic_firmware' do
before do
@cr = compute_resources(:mycompute)
end

test "returns firmware_type when firmware is 'automatic' and firmware_type is present" do
assert_equal 'uefi', @cr.send(:resolve_automatic_firmware, 'automatic', 'uefi')
assert_equal 'bios', @cr.send(:resolve_automatic_firmware, 'automatic', 'bios')
assert_equal 'none', @cr.send(:resolve_automatic_firmware, 'automatic', 'none')
assert_equal 'uefi_secure_boot', @cr.send(:resolve_automatic_firmware, 'automatic', 'uefi_secure_boot')
end

test "returns firmware unchanged when not 'automatic'" do
assert_equal 'uefi', @cr.send(:resolve_automatic_firmware, 'uefi', 'bios')
assert_equal 'bios', @cr.send(:resolve_automatic_firmware, 'bios', 'uefi')
assert_equal 'uefi', @cr.send(:resolve_automatic_firmware, 'uefi', false)
assert_equal 'bios', @cr.send(:resolve_automatic_firmware, 'bios', '')
assert_equal 'uefi_secure_boot', @cr.send(:resolve_automatic_firmware, 'uefi_secure_boot', '')
end

test "returns 'bios' when firmware is 'automatic' and firmware_type is not present" do
assert_equal 'bios', @cr.send(:resolve_automatic_firmware, 'automatic', '')
assert_equal 'bios', @cr.send(:resolve_automatic_firmware, 'automatic', nil)
end
end
end
22 changes: 22 additions & 0 deletions test/models/compute_resources/libvirt_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -270,4 +270,26 @@
check_vm_attribute_names(cr)
end
end

describe '#generate_secure_boot_settings' do
before do
@cr = FactoryBot.build_stubbed(:libvirt_cr)
end

test "returns secure boot settings when firmware is 'uefi_secure_boot'" do
expected_sb_settings = {
firmware_features: { 'secure-boot' => 'yes', 'enrolled-keys' => 'yes' },
loader: { 'secure' => 'yes' },
}

assert_equal expected_sb_settings, @cr.send(:generate_secure_boot_settings, 'uefi_secure_boot')

Check failure on line 285 in test/models/compute_resources/libvirt_test.rb

View workflow job for this annotation

GitHub Actions / test:units - Ruby 2.7 and Node 18 on PostgreSQL 13

Failure: test_0001_returns secure boot settings when firmware is 'uefi_secure_boot' --- expected +++ actual @@ -1 +1 @@ -{:firmware_features=>{"secure-boot"=>"yes", "enrolled-keys"=>"yes"}, :loader=>{"secure"=>"yes"}} +{:firmware_features=>{"secure-boot"=>"yes", "enrolled-keys"=>"yes"}, :loader=>{"secure"=>"yes"}, :secure_boot=>true}

Check failure on line 285 in test/models/compute_resources/libvirt_test.rb

View workflow job for this annotation

GitHub Actions / test:units - Ruby 2.7 and Node 14 on PostgreSQL 13

Failure: test_0001_returns secure boot settings when firmware is 'uefi_secure_boot' --- expected +++ actual @@ -1 +1 @@ -{:firmware_features=>{"secure-boot"=>"yes", "enrolled-keys"=>"yes"}, :loader=>{"secure"=>"yes"}} +{:firmware_features=>{"secure-boot"=>"yes", "enrolled-keys"=>"yes"}, :loader=>{"secure"=>"yes"}, :secure_boot=>true}

Check failure on line 285 in test/models/compute_resources/libvirt_test.rb

View workflow job for this annotation

GitHub Actions / test:units - Ruby 3.0 and Node 14 on PostgreSQL 13

Failure: test_0001_returns secure boot settings when firmware is 'uefi_secure_boot' --- expected +++ actual @@ -1 +1 @@ -{:firmware_features=>{"secure-boot"=>"yes", "enrolled-keys"=>"yes"}, :loader=>{"secure"=>"yes"}} +{:firmware_features=>{"secure-boot"=>"yes", "enrolled-keys"=>"yes"}, :loader=>{"secure"=>"yes"}, :secure_boot=>true}

Check failure on line 285 in test/models/compute_resources/libvirt_test.rb

View workflow job for this annotation

GitHub Actions / test:units - Ruby 3.0 and Node 18 on PostgreSQL 13

Failure: test_0001_returns secure boot settings when firmware is 'uefi_secure_boot' --- expected +++ actual @@ -1 +1 @@ -{:firmware_features=>{"secure-boot"=>"yes", "enrolled-keys"=>"yes"}, :loader=>{"secure"=>"yes"}} +{:firmware_features=>{"secure-boot"=>"yes", "enrolled-keys"=>"yes"}, :loader=>{"secure"=>"yes"}, :secure_boot=>true}
end

test "returns an empty hash for firmware types other than 'uefi_secure_boot'" do
assert_empty @cr.send(:generate_secure_boot_settings, 'uefi')
assert_empty @cr.send(:generate_secure_boot_settings, 'bios')
assert_empty @cr.send(:generate_secure_boot_settings, '')
assert_empty @cr.send(:generate_secure_boot_settings, nil)
end
end
end
4 changes: 4 additions & 0 deletions test/models/concerns/pxe_loader_support_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
5 changes: 5 additions & 0 deletions test/models/host_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading