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 - Libvirt - UEFI & SecureBoot support #10209

Closed
wants to merge 3 commits into from

Conversation

stejskalleos
Copy link
Contributor

@stejskalleos stejskalleos commented Jun 17, 2024

Required fog-libvirt PR: fog/fog-libvirt#155

@stejskalleos
Copy link
Contributor Author

Rebased, added @ekohl as author & tested with Fedora 39 UEFI + SecureBoot. Ready for review

@stejskalleos
Copy link
Contributor Author

@ekohl @jloeser Rebased and updated (Together with FOG PR)
Now using

  <firmware>
    <feature enabled='yes' name='secure-boot'/>
    <feature enabled='yes' name='enrolled-keys'/>
  </firmware>

Tested with shimx64.efi & Fedora 39.

@davispuh
Copy link

Nice work, I'm also interested in this.

Comment on lines 28 to 50
<%= 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'
%>

<div id="os_firmware_features">
<%= 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
%>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should hide these options when editing a host and display them only when creating a new host. Since there is no way to edit these fields and we don’t know the actual values (we’re displaying the default ones), I don’t see a reason to include them in the edit form.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

In #9965 I copied the VMware pattern to automatically detect the firmware based on the PXE loader. That should determine if secure boot it used by default.

@@ -291,7 +313,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',
Copy link
Member

Choose a reason for hiding this comment

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

This uses os_firmware and VMware uses firmware. That's inconsistent.

@nofaralfasi
Copy link
Contributor

Requires: fog/fog-libvirt#155.

@nofaralfasi nofaralfasi changed the title Fixes #37566 - Libvirt - UEFI & SecureBoot support [WIP] Fixes #37566 - Libvirt - UEFI & SecureBoot support Jul 25, 2024
@nofaralfasi nofaralfasi marked this pull request as draft July 25, 2024 10:07
@nofaralfasi
Copy link
Contributor

I have another idea on how to implement this. Moving it to WIP until it's ready.

@nofaralfasi
Copy link
Contributor

nofaralfasi commented Aug 12, 2024

While determining the firmware type in Libvirt is straightforward when creating a VM, it can be more complex for existing VMs. The firmware can be specified in various places, such as os.firmware, os.loader.type, or it might not be specified at all. Additionally, there are factors to consider, such as the machine returning different values depending on whether it is running or not. For example, the loader attribute may only be available when the machine is running. Similarly, the Secure Boot feature can be enabled using os.firmware.features with secure-boot set to yes, but there are other methods to enable Secure Boot as well (refer to Libvirt Secure Boot and Libvirt BIOS/Bootloader Documentation for more details).

Given these complexities, we have two options: either refrain from displaying the firmware type for existing machines, or display it with the understanding that the information may not always be accurate.

stejskalleos and others added 2 commits September 9, 2024 13:01
- 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.
Comment on lines +334 to +343
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
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is also used in https://github.com/theforeman/foreman/pull/10225/files#diff-be4ac4f95deb530eb0fbcc834a4940573ba45b8d5923433305ac3aa1e0703328R501-R508.
I suggest moving this method into app/models/compute_resource.rb for shared use between VMware and Libvirt.

@nofaralfasi nofaralfasi changed the title [WIP] Fixes #37566 - Libvirt - UEFI & SecureBoot support Fixes #37566 - Libvirt - UEFI & SecureBoot support Sep 9, 2024
@nofaralfasi nofaralfasi marked this pull request as ready for review September 9, 2024 10:27
@nofaralfasi nofaralfasi requested a review from a team as a code owner September 9, 2024 10:27
- Moved firmware logic into dedicated methods.
- Updated tests for improved coverage and workflow alignment.
@nofaralfasi
Copy link
Contributor

Closing this PR in favor of a new one: #10321. The new PR addresses the same issue with updated changes. Please continue the discussion and review there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants