-
Notifications
You must be signed in to change notification settings - Fork 42
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
EFI & Secure Boot #155
base: master
Are you sure you want to change the base?
EFI & Secure Boot #155
Conversation
I encountered an issue with the changes in this PR. For VMs not created by Foreman (either manually or by another tool), the firmware type is not always set, making it unclear what the firmware type is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at my os_loader
implementation around https://github.com/fog/fog-libvirt/pull/134/files#r1686901783. I'm not sure why @stejskalleos dropped the os_loader
, but from reading https://libvirt.org/kbase/secureboot.html and seeing libvirt 8.0.0 is in EL8 then I suspect you can't use EL8 libvirt & secureboot this way.
Technically we an probe for the version (look for libversion
in the code). It would be nice to throw a clean error if we detect too old versions like EL7 hypervisors.
secure_boot = !xml.include?('<feature name="secure-boot" enabled="no" />') | ||
enrolled_keys = !xml.include?('<feature name="enrolled-keys" enabled="no" />') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect these features to show up.
secure_boot = !xml.include?('<feature name="secure-boot" enabled="no" />') | |
enrolled_keys = !xml.include?('<feature name="enrolled-keys" enabled="no" />') | |
secure_boot = xml.include?('<feature name="secure-boot" enabled="no" />') | |
enrolled_keys = xml.include?('<feature name="enrolled-keys" enabled="no" />') |
If they're not expected, I'd use a much broader matcher just to make sure it's not some XML formatting:
secure_boot = !xml.include?('<feature name="secure-boot" enabled="no" />') | |
enrolled_keys = !xml.include?('<feature name="enrolled-keys" enabled="no" />') | |
secure_boot = !xml.include?('secure-boot') | |
enrolled_keys = !xml.include?('enrolled-keys') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my bad. I forgot to fix this.
# Foreman expects the firmware to be 'uefi_sb' if SB is enabled | ||
def firmware(xml) | ||
firmware_type = xml_elements(xml, "domain/os", "firmware").first || 'bios' | ||
return 'uefi_sb' if firmware_type == 'efi' && secure_boot_enabled?(xml) | ||
|
||
firmware_type | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should add Foreman specific things here. Now the method may be useful, but then I'd clearly describe its behavior using yardoc tags. Foreman is the consumer of the API, fog-libvirt is authoritative in how it behaves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think firmware
should exactly output what was also input as an attribute. So I'd just expect xml_elements(xml, "domain/os", "firmware")
. Then you also read firmware_features
back and set that attribute. That keeps it consistent that when you create a VM with certain attributes, you can read them back exactly as they were.
In other words, there's a to_xml
and this can be seen as a from_xml
.
You are right; their documentation can be confusing. I was reading from https://libvirt.org/formatdomain.html#bios-bootloader: But I guess that's not true for older versions. |
I agree it's often confusing. I'd recommend setting up an EL8 hypervisor (Foreman can connect to libvirt using SSH) and try it out. Often the best way |
.select { |feature| feature[:enabled] == 'yes' } | ||
.map { |feature| feature[:name] } | ||
|
||
required_features = ['secure-boot', 'enrolled-keys'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK you don't need enrolled-keys
for secure-boot. That's just the default set of keys, but users can define their own keys and sign their own kernels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to their documentation, enrolled-keys are needed for SB. Is it necessary to also provide an option for the user to add their own keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://libvirt.org/kbase/secureboot.html#additional-information says that without enrolled-keys
it allows running unsigned code, but I wonder if that's really true. This last bit clarifies with how I think it's supposed to work:
The main difference between using a build of EDKII that has Secure Boot support but without keys enrolled and one that doesn't have Secure Boot support at all is that, with the former, you could enroll your own keys and securely run an operating system that you've built and signed yourself. If you are only planning to run existing, off-the-shelf operating system images, then the two configurations are functionally equivalent.
So the idea is that enrolled-keys
contains the default keys, but if you don't have or want those you can enroll your own keys.
enabled_features = xml_elements(xml, "domain/os/firmware/feature") | ||
.select { |feature| feature[:enabled] == 'yes' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you filter it directly with xpath?
enabled_features = xml_elements(xml, "domain/os/firmware/feature") | |
.select { |feature| feature[:enabled] == 'yes' } | |
enabled_features = xml_elements(xml, "domain/os/firmware/feature[@enabled='yes']") |
firmware_type | ||
end | ||
|
||
def secure_boot_enabled?(xml) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thought I had: you can implement secure_boot?
on the Server
object that inspects the attributes.
I have another idea on how to implement this. Moving it to WIP until it's ready. |
Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
5a05cd7
to
1f88090
Compare
1f88090
to
534c382
Compare
- Renamed firmware-related attributes to align with VMware conventions. - Added the `loader` attribute to determine if SB is enabled.
534c382
to
ba8b95c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍏 LGTM
Tested together with theforeman/foreman#10321, BIOS, UEFI & Secure Boot works fine.
attribute :firmware | ||
attribute :firmware_features | ||
attribute :secure_boot | ||
attribute :loader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to myself:
The loader
element points to a specific boot binary (BIOS/UEFI), while firmware
refers to the type or particular features used to configure or load the guest VM.
EFI and Secure Boot support for Libvirt.