Merge pull request kubevirt#3529 from jean-edouard/defaulttosb

Enable Secure Boot by default when EFI is enabled
attlee-wang · Jun 21, 2020 · b8543ba · b8543ba
2 parents d036d9d + aa144fe
commit b8543ba
Show file tree

Hide file tree

Showing 12 changed files with 65 additions and 18 deletions.
diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json
@@ -6219,7 +6219,7 @@
     "type": "object",
     "properties": {
      "secureBoot": {
-      "description": "If set, SecureBoot will be enabled and the OVMF roms will be swapped for SecureBoot-enabled ones. Requires SMM to be enabled. Defaults to false",
+      "description": "If set, SecureBoot will be enabled and the OVMF roms will be swapped for SecureBoot-enabled ones. Requires SMM to be enabled. Defaults to true",
       "type": "boolean"
      }
     }

diff --git a/examples/vmi-alpine-efi.yaml b/examples/vmi-alpine-efi.yaml
@@ -14,7 +14,8 @@ spec:
         name: containerdisk
     firmware:
       bootloader:
-        efi: {}
+        efi:
+          secureBoot: false
     machine:
       type: ""
     resources:

diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
@@ -1144,7 +1144,7 @@ func validateDomainSpec(field *k8sfield.Path, spec *v1.DomainSpec) []metav1.Stat
 	causes = append(causes, validateFirmware(field.Child("firmware"), spec.Firmware)...)
 
 	if spec.Firmware != nil && spec.Firmware.Bootloader != nil && spec.Firmware.Bootloader.EFI != nil &&
-		spec.Firmware.Bootloader.EFI.SecureBoot != nil && *spec.Firmware.Bootloader.EFI.SecureBoot &&
+		(spec.Firmware.Bootloader.EFI.SecureBoot == nil || *spec.Firmware.Bootloader.EFI.SecureBoot) &&
 		(spec.Features == nil || spec.Features.SMM == nil || !*spec.Features.SMM.Enabled) {
 		causes = append(causes, metav1.StatusCause{
 			Type:    metav1.CauseTypeFieldValueInvalid,

diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go
@@ -2286,10 +2286,16 @@ var _ = Describe("Validating VMICreate Admitter", func() {
 			Expect(len(causes)).To(Equal(0))
 		})
 
-		It("should accept EFI", func() {
+		It("should accept EFI with SMM", func() {
 			vmi := v1.NewMinimalVMI("testvmi")
 			vmi.Spec.Subdomain = "testsubdomain"
 
+			_true := true
+			vmi.Spec.Domain.Features = &v1.Features{
+				SMM: &v1.FeatureState{
+					Enabled: &_true,
+				},
+			}
 			vmi.Spec.Domain.Firmware = &v1.Firmware{
 				Bootloader: &v1.Bootloader{
 					EFI: &v1.EFI{},
@@ -2300,10 +2306,47 @@ var _ = Describe("Validating VMICreate Admitter", func() {
 			Expect(len(causes)).To(Equal(0))
 		})
 
+		It("should not accept EFI without SMM", func() {
+			vmi := v1.NewMinimalVMI("testvmi")
+			vmi.Spec.Subdomain = "testsubdomain"
+
+			vmi.Spec.Domain.Firmware = &v1.Firmware{
+				Bootloader: &v1.Bootloader{
+					EFI: &v1.EFI{},
+				},
+			}
+
+			causes := ValidateVirtualMachineInstanceSpec(k8sfield.NewPath("fake"), &vmi.Spec, config)
+			Expect(len(causes)).To(Equal(1))
+		})
+
+		It("should accept EFI without secureBoot and without SMM", func() {
+			vmi := v1.NewMinimalVMI("testvmi")
+			vmi.Spec.Subdomain = "testsubdomain"
+
+			_false := false
+			vmi.Spec.Domain.Firmware = &v1.Firmware{
+				Bootloader: &v1.Bootloader{
+					EFI: &v1.EFI{
+						SecureBoot: &_false,
+					},
+				},
+			}
+
+			causes := ValidateVirtualMachineInstanceSpec(k8sfield.NewPath("fake"), &vmi.Spec, config)
+			Expect(len(causes)).To(Equal(0))
+		})
+
 		It("should not accept BIOS and EFI together", func() {
 			vmi := v1.NewMinimalVMI("testvmi")
 			vmi.Spec.Subdomain = "testsubdomain"
 
+			_true := true
+			vmi.Spec.Domain.Features = &v1.Features{
+				SMM: &v1.FeatureState{
+					Enabled: &_true,
+				},
+			}
 			vmi.Spec.Domain.Firmware = &v1.Firmware{
 				Bootloader: &v1.Bootloader{
 					EFI:  &v1.EFI{},

diff --git a/pkg/virt-launcher/virtwrap/api/converter.go b/pkg/virt-launcher/virtwrap/api/converter.go
@@ -705,7 +705,7 @@ func Convert_v1_VirtualMachine_To_api_Domain(vmi *v1.VirtualMachineInstance, dom
 		}
 
 		if vmi.Spec.Domain.Firmware.Bootloader != nil && vmi.Spec.Domain.Firmware.Bootloader.EFI != nil {
-			if vmi.Spec.Domain.Firmware.Bootloader.EFI.SecureBoot != nil && *vmi.Spec.Domain.Firmware.Bootloader.EFI.SecureBoot {
+			if vmi.Spec.Domain.Firmware.Bootloader.EFI.SecureBoot == nil || *vmi.Spec.Domain.Firmware.Bootloader.EFI.SecureBoot {
 				domain.Spec.OS.BootLoader = &Loader{
 					Path:     filepath.Join(c.OVMFPath, EFICodeSecureBoot),
 					ReadOnly: "yes",

diff --git a/pkg/virt-launcher/virtwrap/api/converter_test.go b/pkg/virt-launcher/virtwrap/api/converter_test.go
@@ -2171,7 +2171,9 @@ var _ = Describe("Converter", func() {
 
 				vmi.Spec.Domain.Firmware = &v1.Firmware{
 					Bootloader: &v1.Bootloader{
-						EFI: &v1.EFI{},
+						EFI: &v1.EFI{
+							SecureBoot: False(),
+						},
 					},
 				}
 				domainSpec := vmiToDomainXMLToDomainSpec(vmi, c)
@@ -2186,9 +2188,7 @@ var _ = Describe("Converter", func() {
 			It("should configure the EFI bootloader if EFI secure option", func() {
 				vmi.Spec.Domain.Firmware = &v1.Firmware{
 					Bootloader: &v1.Bootloader{
-						EFI: &v1.EFI{
-							SecureBoot: True(),
-						},
+						EFI: &v1.EFI{},
 					},
 				}
 				domainSpec := vmiToDomainXMLToDomainSpec(vmi, c)

diff --git a/staging/src/kubevirt.io/client-go/api/v1/openapi_generated.go b/staging/src/kubevirt.io/client-go/api/v1/openapi_generated.go
diff --git a/staging/src/kubevirt.io/client-go/api/v1/schema.go b/staging/src/kubevirt.io/client-go/api/v1/schema.go
@@ -222,7 +222,7 @@ type EFI struct {
 	// If set, SecureBoot will be enabled and the OVMF roms will be swapped for
 	// SecureBoot-enabled ones.
 	// Requires SMM to be enabled.
-	// Defaults to false
+	// Defaults to true
 	// +optional
 	SecureBoot *bool `json:"secureBoot,omitempty"`
 }

diff --git a/staging/src/kubevirt.io/client-go/api/v1/schema_swagger_generated.go b/staging/src/kubevirt.io/client-go/api/v1/schema_swagger_generated.go
diff --git a/staging/src/kubevirt.io/client-go/apis/snapshot/v1alpha1/openapi_generated.go b/staging/src/kubevirt.io/client-go/apis/snapshot/v1alpha1/openapi_generated.go
diff --git a/tests/utils.go b/tests/utils.go
@@ -1965,7 +1965,9 @@ func NewRandomVMIWithEFIBootloader() *v1.VirtualMachineInstance {
 	vmi.Spec.Domain.Resources.Requests[k8sv1.ResourceMemory] = resource.MustParse("1Gi")
 	vmi.Spec.Domain.Firmware = &v1.Firmware{
 		Bootloader: &v1.Bootloader{
-			EFI: &v1.EFI{},
+			EFI: &v1.EFI{
+				SecureBoot: NewBool(false),
+			},
 		},
 	}
 
@@ -1985,9 +1987,7 @@ func NewRandomVMIWithSecureBoot() *v1.VirtualMachineInstance {
 	}
 	vmi.Spec.Domain.Firmware = &v1.Firmware{
 		Bootloader: &v1.Bootloader{
-			EFI: &v1.EFI{
-				SecureBoot: NewBool(true),
-			},
+			EFI: &v1.EFI{}, // SecureBoot should default to true
 		},
 	}
 

diff --git a/tools/vms-generator/utils/utils.go b/tools/vms-generator/utils/utils.go
@@ -401,10 +401,13 @@ func GetVMISecureBoot() *v1.VirtualMachineInstance {
 func GetVMIAlpineEFI() *v1.VirtualMachineInstance {
 	vmi := getBaseVMI(VmiAlpineEFI)
 
+	_false := false
 	addContainerDisk(&vmi.Spec, fmt.Sprintf("%s/%s:%s", DockerPrefix, imageAlpine, DockerTag), busVirtio)
 	vmi.Spec.Domain.Firmware = &v1.Firmware{
 		Bootloader: &v1.Bootloader{
-			EFI: &v1.EFI{},
+			EFI: &v1.EFI{
+				SecureBoot: &_false,
+			},
 		},
 	}