VMware - Secure Boot & Virtual TPM

[stejskalleos]

Add support for Secure Boot and TPM
Inspired by https://github.com/ananace/foreman_vmware_advanced
Required FOG PR: fog/fog-vsphere#305

[chris1984]

One comment, looks like tests are failing since we don't have fog-vsphere built with your change.

[ekohl]

With the firmware we have a field auto. Wouldn't it make more sense to add a value there for UEFI + SecureBoot? The default is auto, which derives it from the PXE loader. We should enhance that to automatically detect if secureboot was desired.

[nofaralfasi]

With the firmware we have a field auto. Wouldn't it make more sense to add a value there for UEFI + SecureBoot? The default is auto, which derives it from the PXE loader. We should enhance that to automatically detect if secureboot was desired.

I didn't know that's how the Automatic option works. I'll have to adjust my recent changes accordingly.

[ekohl]

It works by deriving the firmware_type from the pxe_loader:

foreman/app/models/concerns/pxe_loader_support.rb

Lines 49 to 59 in 7ce3336

	def firmware_type(pxe_loader)
	case pxe_loader
	when 'None'
	:none
	when /UEFI/
	:uefi
	else
	:bios
	end
	end
	end

This is then used in the firmware_mapping:

foreman/app/models/compute_resources/foreman/model/vmware.rb

Lines 818 to 821 in 7ce3336

	def firmware_mapping(firmware_type)
	return 'efi' if firmware_type == :uefi
	'bios'
	end

And in parsing the arguments this is used to determine the firmware:

foreman/app/models/compute_resources/foreman/model/vmware.rb

Line 495 in 7ce3336

args[:firmware] = firmware_mapping(firmware_type) if args[:firmware] == 'automatic'

In my libvirt EFI patch I copied this pattern:
https://github.com/theforeman/foreman/pull/9965/files#diff-525a301145c328b1c4d2e6dd7668a9ce4b9f64edb7b2876a4cdebeb5ee2689f9R162-R177

You can see I already added a TODO there to include secure boot.

This is completely lost in #10209.

[nofaralfasi]

Requires: fog/fog-vsphere#305

[nofaralfasi]

UI changes in my commit:

• Added a new firmware type UEFI Secure Boot for Secure Boot.
• Removed the Secure Boot checkbox (replaced it with the new firmware type).
• Added a label helper indicating that TPM is only compatible with EFI firmware.
• Hid the TPM option from the UI when it is not relevant.

[ekohl]

Test failure looks related. I think you need to delete (and then handle) virtual_tpm in parse_args.

[nofaralfasi]

Test failure looks related. I think you need to delete (and then handle) virtual_tpm in parse_args.

I thought it's because we're missing the fog/fog-vsphere#305 PR.

[ekohl]

No, it's because it tries to assign it to the host model now instead of transforming it to a vsphere api parameter

[chris1984]

@nofaralfasi did you want to address @ekohl concerns before I test, that way we can test it just once and get this in faster once the code looks good.

[ekohl]

concerns before I test, that way we can test it just once and get this in faster once the code looks good.

I assure you, you'll see the exact same failures as the automated tests so there's no point in testing manually.

[nofaralfasi]

No, it's because it tries to assign it to the host model now instead of transforming it to a vsphere api parameter

Where do you see it in the tests result? I still think it's because of the missing PR from fog-vsphere.

[ekohl]

Oh yes, I think I misread that. Apologies for that. Perhaps you can modify the PR to actually point to the PR so we can see if the tests would pass otherwise.

[nofaralfasi]

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

[ekohl]

Here it will also be confusing if we have both EFI and UEFI. A user may assume that it's incompatible with UEFI Secure Boot while in practice it's not.

You may solve it like this:

Suggested change

	:label_help => _("Only compatible with EFI firmware."),
	:label_help => _("Only compatible with (U)EFI firmware."),

But I'd argue to standardize everywhere on UEFI. Wikipedia also redirects from EFI to UEFI.

[nofaralfasi]

If you think that’s clearer, I’ll proceed with the change.

[ekohl]

I'd still prefer to align everything on 1 term so it's clear it's the same under the hood, just secure boot or not.

[ekohl]

I'd also expect test cases that cover BIOS firmware and UEFI secure boot

[nofaralfasi]

I need to test the create_vm method. Testing only parse_args is not sufficient.

[nofaralfasi]

In progress: improve testing for virtual_tpm.

[sbernhard]

For me, this is pretty hard to understand why the firmware type is changed from uefi to efi. Maybe a comment why this needs to be done would help for the furture.
Additionally, I think the code can be simplified:

args[:firmware] = 'efi' if args[:firmware]&.start_with?('uefi')
args[:secure_boot] = args[:firmware] == 'uefi_secure_boot'

[ekohl]

I think that comment should describe the block above (automatic firmware) as well. The handling as a whole is not intuitive so a comment can make sense.

[ekohl]

I agree the current code isn't great at relaying information.

What happens now is the orchestration calls setCompute:

foreman/app/models/concerns/orchestration/compute.rb

Lines 98 to 118 in 2f947f7

	def setCompute
	logger.info "Adding Compute instance for #{name}"
	if compute_attributes.nil?
	failure _("Failed to find compute attributes, please check if VM %s was deleted") % name
	return false
	end
	# TODO: extract the merging into separate class in combination
	# with ComputeAttributesMerge and InterfacesMerge http://projects.theforeman.org/issues/14536
	final_compute_attrs = compute_attributes.merge(compute_resource.host_compute_attrs(self))
	self.vm = compute_resource.create_vm(final_compute_attrs)
	rescue => e
	# Workaround bug when the exception itself contains unresolved string placeholder
	# Example: Foreman could not find a required vSphere resource. Check if Foreman has the required permissions and the resource exists. Reason: %s
	# See: https://projects.theforeman.org/issues/32273
	begin
	failure _("Failed to create a compute %{compute_resource} instance %{name}: %{message}\n ") % { :compute_resource => compute_resource, :name => name, :message => e.message }, e
	rescue => e2
	logger.warn "Got #{e2.class} when accessing #{e.class} message attribute, falling back to message_untranslated containing #{e.message_untranslated}"
	failure _("Failed to create a compute %{compute_resource} instance %{name}: %{message}\n ") % { :compute_resource => compute_resource, :name => name, :message => e.message_untranslated }
	end
	end

This calls the create_vm which calls parse_args. This means you can't really use the errors framework to get specific field level errors and only can raise an exception. Worse, we're actually performing expensive API calls (in parse_network) when we could fail early.

Still, parse_args can raise an exception and that should bubble up. You can then also add tests without having to test the whole create_vm method.

My thought process behind it is that you should fail as early as possible. Longer term I'd suggest that we should refactor setCompute to call something like normalize_args that effectively does that parse_args does now and returns early. Before any connections to the real compute resource have been made.

[nofaralfasi]

This calls the create_vm which calls parse_args. This means you can't really use the errors framework to get specific field level errors and only can raise an exception. Worse, we're actually performing expensive API calls (in parse_network) when we could fail early.

Before that, we are calling the new_vm method also from here compute_object. So if we raise the error from parse_args, the exception will result in breaking the whole page.

Update:
I can catch the exception in the compute_object method, which will break the form and display an error message to the user. I was trying to avoid this because I consider it an edge case and didn't want an error that breaks the whole form. However, this approach ensures that the exception is presented in a user-friendly manner, and it's been handled before sending the values to VMware.

My thought process behind it is that you should fail as early as possible...

I agree, but as I mentioned before, I propose adding validations for all the values before we send them to VMware.

[ekohl]

Before that, we are calling the new_vm method also from here compute_object. So if we raise the error from parse_args, the exception will result in breaking the whole page.

Ah, this is something I missed altogether. Then I think your approach is good enough now and we need do something more to untangle the mess.

These are the reasons the host creation/edit form is so complex and why so many attempts to improve it end up failing.

[ekohl]

Given what you told me, I think the previous version with the raising in create_vm was indeed better. I'd prefer to revert back to that, or try what I suggest here in this review. I don't know if this works, but my untested theory is that the error should show up in the form field with TPM itself.

[ekohl]

Given what you pointed out, perhaps we can go back to errors.add(:virtual_tpm, _('TPM is not compatible with BIOS firmware. Please change Firmware or disable TPM.'))

Then later (I'll add a comment there too) we can check for errors

[ekohl]

Then you can raise the error here (probably with some message):

Suggested change

	vm = new_vm(args)
	vm = new_vm(args)
	raise ArgumentError if errors.any?

[nofaralfasi]

This PR needs to be updated to align with the changes in foreman#10321.

[nofaralfasi]

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