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

Enabling VBS (vbsEnabled) and I/O MMU (vvtdEnabled) #1287

Merged
merged 3 commits into from
May 6, 2021

Conversation

bjoernf73
Copy link
Contributor

@bjoernf73 bjoernf73 commented Dec 18, 2020

Description

Virtualization Based Security (VBS) is an option for Windows VMs that enables a security feature that is part of any security baseline for Windows (DoD/DISA, CISecurity, Microsoft). VBS is in turn contingent on another unsupported (by the provider) option, I/O MMU.

In this PR, VBS and I/O MMU are implemented as

resource "vsphere_virtual_machine" "cloned_virtual_machine" {
  ...
  vvtd_enabled = true
  vbs_enabled = true
  ...
}

They target the api-flags vvtdEnabled (I/O MMU) and vbsEnabled (VBS) respectively.

The VMware vSphere API reference's documentation of the Data Object VirtualMachineFlagInfo(vim.vm.FlagInfo) states about vbsEnabled that:

"Flag to specify if Virtualization-based security is enabled for this virtual machine. If set to true when creating a new VM, the following VM properties might be modified automatically: - If vim.vm.FlagInfo.vvtdEnabled is not set to false, it is set to true. Else error is returned. - If vim.vm.ConfigSpec.nestedHVEnabled is not set to false, it is set to true. Else error is returned. - If vim.vm.BootOptions.efiSecureBootEnabled is not set to false, it is set to true. Else error is returned. - If vim.vm.firmware is not set to bios, it is set to efi. Else error is returned"

Notable is the formulation the following VM properties might be modified automatically with an emphasis on might.

At first I implemented vbs_enabled alone, believing that if the api-option vbsEnabled is switched to true, that will alter the dependency options I/O MMU (vvtd_enabled), Secure Boot(efi_secure_boot_enabled), and Hardware Virtualization passthru (nested_hv_enabled) - setting those options automatically to true. That is not the case, however. Setting the vbsEnabled alone seemed to work with the provider, and probably the flag is set - however, it has no effect, since I/O MMU (vvtd_enabled) is still unset, or probably false. This may be a deficiancy with the api?

Anyway, if the provider should be responsible for this logic, i.e. setting the contingent properties to the correct values, then more code will have to be added for that.

Also, I understand that vSphere 6.5 (and ESXi 6.5) is used for testing the provider? That will of course not work with these options, other than verifying that the provider correctly throws an error stating that vbs_enabled and vvtd_enabled is only supported on vSphere 6.7 and higher.

Feel free to give advice on how to make a test for this!

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
    Working on it. Trying to understand the how tests can be written.
  • Have you run the acceptance tests on this branch?
    Working on it. Trying to understand the how tests can be written

Output from acceptance testing:


...

Release Note

Release note for CHANGELOG:

Virtualization Based Security and I/O MMU enabled by vbs_enabled and vvtd_enbled in resource "vsphere_virtual_machine"

References

#1288
#677

@ghost ghost added size/s Relative Sizing: Small documentation Type: Documentation labels Dec 18, 2020
@hashicorp-cla
Copy link

hashicorp-cla commented Dec 18, 2020

CLA assistant check
All committers have signed the CLA.

@bjoernf73 bjoernf73 changed the title Enabling VBS (vbs_enabled -> vbsEnabled) and I/O MMU (vvtd_enabled ->… Enabling VBS (vbsEnabled) and I/O MMU (vvtdEnabled) Dec 18, 2020
@bjoernf73
Copy link
Contributor Author

Been looking through the tests (resource_vsphere_virtual_machine_tests.go) - it seems there are no tests on windows guests whatsoever? I've tried adding the code below - which I believe will create a windows resource (guest_id = "windows9Server64Guest" ), but it seems I now need to add the function that invokes the testAccResourceVSphereVirtualMachineConfigVbsEnabledAndVvtdEnabled function, and tests for the flags.

Anybody know if there is an existing function that can be called to test for flags set? In 'types.go', there are multiple types related to 'SnapshotDisabled', which is a boolean flag just like VbsEnabled and VvtdEnabled, but I'm struggeling to make sense of how it, or types in general, can be used in a test.

func testAccResourceVSphereVirtualMachineConfigVbsEnabledAndVvtdEnabled() string {
	return fmt.Sprintf(`


%s  // Mix and match config

resource "vsphere_virtual_machine" "vm" {
  name             = "testacc-test"
  resource_pool_id = "${vsphere_resource_pool.pool1.id}"
  datastore_id     = vsphere_nas_datastore.ds1.id

  num_cpus = 2
  memory   = 2048
  guest_id = "windows9Server64Guest"

  vbs_enabled = true
  firmware = "efi"
  vvtd_enabled = true
  nested_hv_enabled = true
  efi_secure_boot_enabled = true

  wait_for_guest_net_timeout = -1

  network_interface {
    network_id = "${data.vsphere_network.network1.id}"
  }

  disk {
    label = "disk0"
    size  = 20
  }
}
`,

		testAccResourceVSphereVirtualMachineConfigBase(),
	)
}

@bjoernf73
Copy link
Contributor Author

Is there any chance of this PR beeing accepted? The provider is quite useless for Windows if VBS cannot be set.

@koikonom
Copy link
Contributor

Hi @bjoernf73!

Thank you for your PR and sorry it took so long to respond.

You are definitely in the right path with regards to the testing functions. Here are the missing steps:

First you need to write a test function that checks the property matches the expected value:

func testAccResourceVSphereVirtualMachineCheckVVTD(expected bool) resource.TestCheckFunc {
	return func(s *terraform.State) error {
		props, err := testGetVirtualMachineProperties(s, "vm")
		if err != nil {
			return err
		}
		vvtdEnabled := *props.Config.Flags.VvtdEnabled
		if vvtdEnabled != expected {
			return fmt.Errorf("vvtd flag was %t, expected: %t", vvtdEnabled, expected)
		}
		return nil
	}
}

func testAccResourceVSphereVirtualMachineCheckVBS(expected bool) resource.TestCheckFunc {
	return func(s *terraform.State) error {
		props, err := testGetVirtualMachineProperties(s, "vm")
		if err != nil {
			return err
		}
	        vbsEnabled := *props.Config.Flags.VbsEnabled
		if vbsEnabled != expected {
			return fmt.Errorf("vbs flag was %t, expected: %t", vbsEnabled, expected)
		}
		return nil
	}
}

and then you'd have to write the actual test case:

func TestAccResourceVSphereVirtualMachine_vvtdAndVbs(t *testing.T) {
	resource.Test(t, resource.TestCase{
		PreCheck: func() {
			testAccPreCheck(t)
			testAccResourceVSphereVirtualMachinePreCheck(t)
		},
		Providers: testAccProviders,
		Steps: []resource.TestStep{
			{
				Config: testAccResourceVSphereVirtualMachineConfigVbsEnabledAndVvtdEnabled(),
				Check: resource.ComposeTestCheckFunc(
					testAccResourceVSphereVirtualMachineCheckVVTD(true),
					testAccResourceVSphereVirtualMachineCheckVBS(true),
				),
			},
		},
	})
}

I hope this helps.

@ghost ghost added size/m Relative Sizing: Medium and removed size/s Relative Sizing: Small labels May 6, 2021
@koikonom
Copy link
Contributor

koikonom commented May 6, 2021

Got round to adding the tests. Thank you very much for this PR, it will be added to the next release.

@koikonom koikonom merged commit a38959d into hashicorp:master May 6, 2021
@github-actions
Copy link

github-actions bot commented Jun 6, 2021

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Type: Documentation size/m Relative Sizing: Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants