Skip to content

Fixes #37823 - Add Secure Boot and Virtual TPM support for VMware #10324

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.

Sign up for GitHub

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

Merged
stejskalleos merged 1 commit into from
Dec 9, 2024
Merged

Fixes #37823 - Add Secure Boot and Virtual TPM support for VMware #10324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 29 additions & 16 deletions app/models/compute_resources/foreman/model/vmware.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,14 +201,6 @@ def storage_controller_types
}
end

def firmware_types
{
"automatic" => N_("Automatic"),
"bios" => N_("BIOS"),
"efi" => N_("EFI"),
}
end

def disk_mode_types
{
"persistent" => _("Persistent"),
Expand Down Expand Up @@ -487,8 +479,9 @@ def parse_args(args)

args.except!(:hardware_version) if args[:hardware_version] == 'Default'

firmware_type = args.delete(:firmware_type)
args[:firmware] = firmware_mapping(firmware_type) if args[:firmware] == 'automatic'
firmware_type = args.delete(:firmware_type).to_s
args.merge!(process_firmware_attributes(args[:firmware], firmware_type))
args[:virtual_tpm] = validate_tpm_compatibility(args[:virtual_tpm], args[:firmware])

args.reject! { |k, v| v.nil? }
args
Expand Down Expand Up @@ -521,7 +514,7 @@ def create_vm(args = { })
clone_vm(args)
else
vm = new_vm(args)
vm.firmware = 'bios' if vm.firmware == 'automatic'
raise ArgumentError, errors.full_messages.join(';') if errors.any?
vm.save
end
rescue Fog::Vsphere::Compute::NotFound => e
Expand Down Expand Up @@ -827,11 +820,6 @@ def vm_instance_defaults
)
end

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

def set_vm_volumes_attributes(vm, vm_attrs)
volumes = vm.volumes || []
vm_attrs[:volumes_attributes] = Hash[volumes.each_with_index.map { |volume, idx| [idx.to_s, volume.attributes.merge(:size_gb => volume.size_gb)] }]
Expand Down Expand Up @@ -862,5 +850,30 @@ def valid_cloudinit_for_customspec?(cloudinit)
def cachekey_with_cluster(key, cluster_id = nil)
cluster_id.nil? ? key.to_sym : "#{key}-#{cluster_id}".to_sym
end

# Generates Secure Boot settings for VMware, based on the provided firmware type.
#
# @param firmware [String] The firmware type.
# @return [Hash] A hash with secure boot settings if applicable.
def generate_secure_boot_settings(firmware)
firmware == 'uefi_secure_boot' ? { "secure_boot" => true } : {}
end

# Validates TPM compatibility based on the firmware type and virtual TPM setting.
# Adds an error if TPM is enabled with BIOS firmware, which is incompatible.
# This error is later raised as an `ArgumentError` in the `#create_vm` method.
#
# @param virtual_tpm [String] indicates if the virtual TPM is enabled ('1') or disabled ('0').
# @param firmware [String] the firmware type.
# @return [Boolean] the cast value of virtual_tpm after validation.
def validate_tpm_compatibility(virtual_tpm, firmware)
virtual_tpm = ActiveModel::Type::Boolean.new.cast(virtual_tpm)

if virtual_tpm && firmware == 'bios'
errors.add :base, _('TPM is not compatible with BIOS firmware. Please change Firmware or disable TPM.')
end

virtual_tpm
end
end
end
12 changes: 10 additions & 2 deletions app/views/compute_resources_vms/form/vmware/_base.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
<%= counter_f(f, :corespersocket, label: _('Cores per socket'), recommended_max_value: compute_resource.max_cpu_count, value: f.object.corespersocket || 1) %>
</div>
<%= text_f f, :memory_mb, :class => "col-md-2", :label => _("Memory (MB)") %>
<%= field(f, :firmware, :label => _('Firmware'), :label_size => "col-md-2") do

<% firmware_type = new_vm ? 'automatic' : compute_resource.firmware_type(f.object.firmware, f.object.secure_boot) %>
<%= field(f, :firmware, :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, {:disabled => !new_vm, :value => type, :text => _(name)}
radio_button_f f, :firmware, { :checked => (firmware_type == type), :disabled => !new_vm, :value => type, :text => _(name) }
end.join(' ').html_safe
end %>
<%= selectable_f f, :cluster, compute_resource.clusters, { :include_blank => _('Please select a cluster') },
Expand Down Expand Up @@ -49,6 +51,12 @@ end %>
{ :disabled => images.empty?, :label => _('Image'), :label_size => "col-md-2" } %>
</div>

<%= checkbox_f f, :virtual_tpm, { :help_inline => _("Add Virtual TPM module to the VM."),
:label => _('Virtual TPM'),
:label_help => _("Only compatible with UEFI firmware."),
:label_size => "col-md-2",
:disabled => !new_vm } %>

<%= compute_specific_js(compute_resource, "nic_info") %>

<%= react_component('StorageContainer', { data: {
Expand Down
67 changes: 51 additions & 16 deletions test/models/compute_resources/vmware_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase

mock_vm = mock('vm')
mock_vm.expects(:save).returns(mock_vm)
mock_vm.expects(:firmware).returns('biod')

cr = FactoryBot.build_stubbed(:vmware_cr)
cr.expects(:parse_networks).with(attrs_in).returns(attrs_parsed)
Expand Down Expand Up @@ -145,17 +144,6 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase
@cr.expects(:new_vm).returns(mock_vm)
@cr.create_vm(args)
end

test 'converts automatic firmware to bios default' do
args = {"provision_method" => "build"}
mock_vm = mock('vm')
mock_vm.expects(:save).returns(mock_vm)
mock_vm.stubs(:firmware).returns('automatic')
mock_vm.expects(:firmware=).with('bios')
@cr.stubs(:parse_networks).returns(args)
@cr.expects(:new_vm).returns(mock_vm)
@cr.create_vm(args)
end
end

test "#create_vm calls clone_vm when image provisioning" do
Expand Down Expand Up @@ -266,8 +254,11 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase
@cr = FactoryBot.build_stubbed(:vmware_cr)
end

test "converts empty hash" do
assert_empty(@cr.parse_args(HashWithIndifferentAccess.new))
test "defaults to BIOS firmware when no firmware is provided" do
args = HashWithIndifferentAccess.new
expected_firmware = { firmware: "bios" }

assert_equal expected_firmware, @cr.parse_args(args)
end

test "converts form attrs to fog attrs" do
Expand Down Expand Up @@ -297,7 +288,7 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase
}
)
# All keys must be symbolized
attrs_out = {:cpus => "1", :interfaces => [{:type => "VirtualVmxnet3", :network => "network-17", :_delete => ""}], :volumes => [{:size_gb => "1", :_delete => ""}]}
attrs_out = { :cpus => "1", :interfaces => [{ :type => "VirtualVmxnet3", :network => "network-17", :_delete => "" }], :volumes => [{ :size_gb => "1", :_delete => "" }], :firmware => "bios" }
assert_equal attrs_out, @cr.parse_args(attrs_in)
end

Expand Down Expand Up @@ -328,7 +319,7 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase
},
}
)
attrs_out = {:cpus => "1", :interfaces => [{:type => "VirtualVmxnet3", :network => "network-17", :_delete => ""}], :volumes => [{:size_gb => "1", :_delete => ""}]}
attrs_out = {:cpus => "1", :interfaces => [{:type => "VirtualVmxnet3", :network => "network-17", :_delete => ""}], :volumes => [{:size_gb => "1", :_delete => ""}], :firmware => "bios" }
assert_equal attrs_out, @cr.parse_args(attrs_in)
end

Expand Down Expand Up @@ -375,6 +366,7 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase
:_delete => "",
},
],
:firmware => "bios",
}
assert_equal attrs_out, @cr.parse_args(attrs_in)
end
Expand Down Expand Up @@ -1072,4 +1064,47 @@ def mock_network(id, name, virtualswitch = nil)
check_vm_attribute_names(cr)
end
end

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

test "returns secure boot settings when firmware is 'uefi_secure_boot'" do
assert_equal({ "secure_boot" => true }, @cr.send(:generate_secure_boot_settings, 'uefi_secure_boot'))
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

describe '#validate_tpm_compatibility' do
before do
@cr = FactoryBot.build_stubbed(:vmware_cr)
end

test 'returns true and no errors when firmware is EFI and virtual_tpm is enabled' do
assert_equal true, @cr.send(:validate_tpm_compatibility, '1', 'efi')
assert_empty @cr.errors.full_messages
end

test 'returns false and no errors when firmware is EFI and virtual_tpm is disabled' do
assert_equal false, @cr.send(:validate_tpm_compatibility, '0', 'efi')
assert_empty @cr.errors.full_messages
Copy link
Contributor

@stejskalleos stejskalleos Dec 3, 2024

Choose a reason for hiding this comment

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

Suggested change
assert_empty @cr.errors.full_messages
assert_empty @cr.errors

Nit: Isn't this enough to check?

end

test 'returns false and no errors when firmware is BIOS and virtual_tpm is disabled' do
assert_equal false, @cr.send(:validate_tpm_compatibility, '0', 'bios')
assert_empty @cr.errors.full_messages
end

test 'returns true and adds an error when firmware is BIOS and virtual_tpm is enabled' do
assert_equal true, @cr.send(:validate_tpm_compatibility, '1', 'bios')
assert_includes @cr.errors.full_messages, 'TPM is not compatible with BIOS firmware. Please change Firmware or disable TPM.'
end
end
end
Loading