Add support for confidential VMs #3265

mresvanis · 2023-03-10T11:07:45Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This change adds support for Confidential VMs and Trusted launch for VMs.

Azure Confidential VMs are defined by their SecurityProfile.SecurityType ConfidentialVM, which should be defined along with the OSDisk.ManagedDisk.SecurityProfile.SecurityEncryptionType field. Trusted launch for VMs is defined by the SecurityProfile.SecurityType TrustedLaunch, which should be defined along with the SecurityProfile.UefiSettings section, i.e. the SecureBootEnabled and VTpmEnabled fields.

Related image-builder PR.

Which issue(s) this PR fixes:
Fixes #3264

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Add SecurityProfile SecurityType to API
Add OSDisk VMDiskSecurityProfile to API
Add SecurityProfile UefiSettings with SecureBootEnabled and VTpmEnabled to API

mresvanis · 2023-03-15T13:50:13Z

/test pull-cluster-api-provider-azure-e2e

nawazkh · 2023-03-15T21:00:10Z

I started looking into this PR, will take sometime in going over the Azure Docs and the new fields being added.
Thanks for all the work!

CecileRobertMichon · 2023-04-04T17:33:30Z

@nawazkh are you still reviewing this?

nawazkh · 2023-04-04T22:47:10Z

Extremely sorry, It slipped my radar after I was half way done reviewing this PR. I will get started on it again!
Sorry @mresvanis !

willie-yao

Thanks for all your hard work on this! I just had one comment and a general question: I think it'd be great to have an E2E test for this feature. This may be out of scope for this PR but this seems like a great feature that can be tested E2E.

api/v1beta1/azuremachine_webhook_test.go

mresvanis · 2023-04-18T14:00:05Z

Thanks for all your hard work on this! I just had one comment and a general question: I think it'd be great to have an E2E test for this feature. This may be out of scope for this PR but this seems like a great feature that can be tested E2E.

@willie-yao Thank you for taking the time to review this change :)

I agree that having an E2E test for this feature would be quite useful. In order to keep this PR at its current size, would it be an option to open an issue for the E2E test and then tackle it in a future PR? Or would you prefer to have the E2E test as part of this PR?

(I would be happy to propose a change for the E2E test either way, I would just need a bit more time)

willie-yao · 2023-04-18T21:09:49Z

In order to keep this PR at its current size, would it be an option to open an issue for the E2E test and then tackle it in a future PR?

Ideally, we would want to have an E2E test be a part of this PR in order to ensure the feature is fully working E2E. Since the next release is planned for May 2, it would be totally okay to take some time. The feature won't be available to users until then anyways.

Would this be enough time for you to implement an E2E test? I apologize for the rush on this.. it's on us for not reviewing this PR sooner 😔

mresvanis · 2023-04-19T08:30:50Z

Would this be enough time for you to implement an E2E test? I apologize for the rush on this.. it's on us for not reviewing this PR sooner

Absolutely no problem, I'll get started as soon as I can and ping you here when I have something up for review. Thanks again for your time.

nawazkh

This is great effort @mresvanis , thanks for taking the time in putting this together!
Also, I apologize for sharing my review so late.

I would generally share all the review at once, but I am breaking it in chunks here to get the ball rolling. I plan on completing reviewing this PR in couple more hours.

api/v1beta1/types.go

nawazkh

Part 2/2 of the review.
I have skipped reviewing azuremachine_validation(.go and _test.go), azuremachine_validation_test.go since we are expecting for some refactor.

nawazkh · 2023-04-19T19:50:15Z

azure/services/virtualmachines/spec.go

+	if storageProfile.OsDisk.ManagedDisk != nil &&
+		storageProfile.OsDisk.ManagedDisk.SecurityProfile != nil &&
+		storageProfile.OsDisk.ManagedDisk.SecurityProfile.SecurityEncryptionType != "" {
+		if s.SecurityProfile.EncryptionAtHost != nil && *s.SecurityProfile.EncryptionAtHost {
+			return nil, azure.WithTerminalError(errors.New("encryption at host is not supported when SecurityEncrytionType is set"))
+		}
+
+		securityProfile.UefiSettings = &compute.UefiSettings{}
+
+		if s.SecurityProfile.SecureBoot != "" && s.SecurityProfile.SecureBoot == infrav1.SecureBootPolicyEnabled {
+			securityProfile.UefiSettings.SecureBootEnabled = pointer.Bool(true)
+		} else {
+			securityProfile.UefiSettings.SecureBootEnabled = pointer.Bool(false)
+		}
+
+		if storageProfile.OsDisk.ManagedDisk.SecurityProfile.SecurityEncryptionType == compute.SecurityEncryptionTypesDiskWithVMGuestState && !*securityProfile.UefiSettings.SecureBootEnabled {
+			return nil, azure.WithTerminalError(errors.Errorf("secure boot should be enabled when SecurityEncrytionType is set to %s", compute.SecurityEncryptionTypesDiskWithVMGuestState))
+		}
+
+		if s.SecurityProfile.VirtualizedTrustedPlatformModule != "" && s.SecurityProfile.VirtualizedTrustedPlatformModule == infrav1.VirtualizedTrustedPlatformModulePolicyDisabled {
+			return nil, azure.WithTerminalError(errors.New("vTPM should be enabled when SecurityEncrytionType is set"))
+		}
+
+		securityProfile.UefiSettings.VTpmEnabled = pointer.Bool(true)
+
+		securityProfile.SecurityType = compute.SecurityTypesConfidentialVM
+
+		return securityProfile, nil
+	}


Would it be better if we enforce these validations at webhook level so that there are lesser chances for a user to run into Terminal Error ?

I also think, if we were to refactor the securityProfile as per earlier comments, then we would run into lesser (almost none) validations and have more of assignments at this stage of v1beta1 -> azure conversion.

Would it be better if we enforce these validations at webhook level so that there are lesser chances for a user to run into Terminal Error ?

I absolutely agree, these validations are quite useful at the webhook level and we have them here. The latter is initially called through here. Does this makes sense?

Makes sense, thanks!

@CecileRobertMichon do you know why we perform validations in generateSecurityProfile() func?
I am trying to understand the reasoning behind generateSecurityProfile() returning azure.WithTerminalError().
Can we not perform all the validations at webhook level ? Is it because we need s of type *VMSpec populated for these validations, and s can only be populated later in the reconciliation and not at the webhook level?

nawazkh · 2023-04-19T19:54:43Z

azure/services/virtualmachines/spec.go

+	if s.SecurityProfile.SecureBoot != "" {
+		if s.SKU.HasCapability(resourceskus.TrustedLaunchDisabled) && s.SecurityProfile.SecureBoot == infrav1.SecureBootPolicyEnabled {
+			return nil, azure.WithTerminalError(errors.Errorf("secure boot is not supported for VM type %s", s.Size))
+		}
+
+		if s.SecurityProfile.SecureBoot == infrav1.SecureBootPolicyEnabled {
+			securityProfile.UefiSettings.SecureBootEnabled = pointer.Bool(true)
+			securityProfile.SecurityType = compute.SecurityTypesTrustedLaunch
+		}
+	}


However, I am not sure if s.SKU.HasCapability(...) can be run at webhook level. If we cannot perform s.SKU.HasCapability(...) validations at webhook, then we might have to leave some of these dependent validations here.

What do you think @CecileRobertMichon @willie-yao ?

azure/services/resourceskus/sku.go

codecov-commenter · 2023-04-20T13:22:54Z

Codecov Report

Patch coverage: 85.83% and project coverage change: -0.24 ⚠️

Comparison is base (f20a204) 53.33% compared to head (353a109) 53.10%.

❗ Current head 353a109 differs from pull request most recent head 720566f. Consider uploading reports for the commit 720566f to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3265      +/-   ##
==========================================
- Coverage   53.33%   53.10%   -0.24%     
==========================================
  Files         185      182       -3     
  Lines       18433    18367      -66     
==========================================
- Hits         9832     9754      -78     
  Misses       8063     8063              
- Partials      538      550      +12

Impacted Files	Coverage Δ
api/v1beta1/types.go	`60.71% <ø> (ø)`
azure/services/resourceskus/sku.go	`0.00% <ø> (ø)`
api/v1beta1/azuremachine_validation.go	`83.54% <82.97%> (-0.17%)`	⬇️
azure/services/virtualmachines/spec.go	`88.35% <87.67%> (-0.64%)`	⬇️

... and 20 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

api/v1beta1/azuremachine_validation.go

alexeldeib · 2023-04-20T15:36:22Z

api/v1beta1/azuremachine_validation.go

+	if profile != nil && securityEncryptionType != "" {
+		if profile.EncryptionAtHost != nil && *profile.EncryptionAtHost && securityEncryptionType == SecurityEncryptionTypeDiskWithVMGuestState {
+			allErrs = append(allErrs, field.Invalid(fieldPath.Child("EncryptionAtHost"), profile.EncryptionAtHost,
+				"EncryptionAtHost cannot be set to 'true' when securityEncryptionType is set to DiskWithVMGuestState"))


cc @anujmaheshwari1 I wonder if we do this correctly for AKS...

alexeldeib · 2023-04-20T15:43:00Z

api/v1beta1/azuremachine_validation.go

+					VirtualizedTrustedPlatformModulePolicyEnabled)))
+		}
+
+		if securityEncryptionType == SecurityEncryptionTypeDiskWithVMGuestState {


are you sure DiskWithVMGuestState is what you want? I've only used CVM with VMGuestStateOnly

either way, VMGuestStateOnly should be a valid setting for CVM, I think, and works with secure boot.

alexeldeib · 2023-04-20T15:45:22Z

azure/services/virtualmachines/spec.go

+	securityProfile.UefiSettings = &compute.UefiSettings{}
+
+	if s.SecurityProfile.SecureBoot != "" {
+		if s.SKU.HasCapability(resourceskus.TrustedLaunchDisabled) && s.SecurityProfile.SecureBoot == infrav1.SecureBootPolicyEnabled {


need a gen1/gen2 check here as well. I believe you only get TrustedLaunchDisabled from SKU API on gen2 sizes which do not support it, it's annoyingly inverted from most capabilities.

as-is I believe you allow a gen1 VM size with TL + secure boot which is invalid.

could sanity check sku api to confirm --

az vm list-skus -r virtualMachines -s Something_Thats_only_gen1 (Standard_NC6?)

azure/services/virtualmachines/spec.go

alexeldeib · 2023-04-20T15:48:53Z

apologies for drive-by review but hope I added some useful comments -- happen to have worked a little on CVM/TL from AKS perspective so was curious :)

alexeldeib · 2023-04-20T16:26:22Z

some other random high level thoughts

sku api in webhooks: your intuition is correct I don't think we do that today due to requirements to fetch creds, call azure, etc. would be nice
other early validation: you can technically regex a lot of stuff without the sku api. I think CVM may work for that case, but you'd have to update the regex in the future. I got tired of manually updating sizes for AKS which is why I wrote the sku api clients to begin with. your mileage may vary :)

nawazkh · 2023-05-12T19:19:37Z

This PR looks good to me!

CecileRobertMichon · 2023-05-12T20:21:13Z

I still don't see any documentation in the PR, @mresvanis are you still working on adding it?

mresvanis · 2023-05-15T05:54:59Z

I still don't see any documentation in the PR, @mresvanis are you still working on adding it?

@CecileRobertMichon yes, I was waiting for the image-builder PR to get merged first, but I will add the documentation today so that you have time to review it.

mresvanis · 2023-05-15T13:34:02Z

@CecileRobertMichon I added documentation for both Confidential VMs and Trusted Launch for VMs, by adding 2 new topics. Please feel free to review. Thank you.

nawazkh · 2023-05-17T02:42:03Z

I looked into the newly added docs and they look good to me! 🚀
I like the thought of waiting until we have a defined way of generating CVM images and then merging this doc with the steps.

/lgtm

k8s-ci-robot · 2023-05-17T02:42:09Z

LGTM label has been added.

Git tree hash: ab33867f4da255c6876eb53b839ff1231377fc59

mresvanis · 2023-06-02T08:25:15Z

@mboersma thank you for your input in the respective image-builder PR! Since that has been merged, your thoughts here would also be much appreciated.

mboersma

Thank you so much for your work and your patience. This is great!

I only found some typos and had some minor doc comments, feel free to ignore those you disagree with.

api/v1beta1/types.go

azure/services/virtualmachines/spec.go

azure/services/virtualmachines/spec_test.go

azure/services/virtualmachines/spec.go

api/v1beta1/types.go

docs/book/src/topics/confidential-vms.md

docs/book/src/topics/trusted-launch-for-vms.md

This change adds support for Confidential VMs and Trusted launch for VMs. Azure Confidential VMs are defined by their SecurityProfile.SecurityType ConfidentialVM, which should be defined along with the OSDisk.ManagedDisk.SecurityProfile.SecurityEncryptionType field. Trusted launch for VMs is defined by the SecurityProfile.SecurityType TrustedLaunch, which should be defined along with the SecurityProfile.UefiSettings section, i.e. the SecureBootEnabled and VTpmEnabled fields. Signed-off-by: Michail Resvanis <mresvani@redhat.com>

mboersma

/lgtm

k8s-ci-robot · 2023-06-12T15:03:55Z

LGTM label has been added.

Git tree hash: 4410040d6da82f0cc54a283e907b1a716c33f59d

CecileRobertMichon

/approve

k8s-ci-robot · 2023-06-12T15:17:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Mar 10, 2023

k8s-ci-robot requested review from marosset and mboersma March 10, 2023 11:07

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 10, 2023

mresvanis force-pushed the add-confidential-vm-support branch from f1afdb5 to f0e5485 Compare March 10, 2023 11:08

mresvanis mentioned this pull request Mar 10, 2023

MGMT-13627: Add ConfidentialVM options to AzureMachineProviderSpec openshift/api#1403

Merged

mresvanis mentioned this pull request Mar 21, 2023

MGMT-13713: Add confidential VM support openshift/machine-api-provider-azure#54

Merged

jackfrancis added this to the v1.9 milestone Apr 6, 2023

willie-yao reviewed Apr 14, 2023

View reviewed changes

api/v1beta1/azuremachine_webhook_test.go Show resolved Hide resolved

nawazkh requested changes Apr 19, 2023

View reviewed changes

api/v1beta1/types.go Outdated Show resolved Hide resolved

api/v1beta1/types.go Show resolved Hide resolved

api/v1beta1/types.go Outdated Show resolved Hide resolved

api/v1beta1/types.go Outdated Show resolved Hide resolved

nawazkh requested changes Apr 19, 2023

View reviewed changes

alexeldeib reviewed Apr 20, 2023

View reviewed changes

api/v1beta1/azuremachine_validation.go Outdated Show resolved Hide resolved

alexeldeib reviewed Apr 20, 2023

View reviewed changes

api/v1beta1/azuremachine_validation.go Outdated Show resolved Hide resolved

alexeldeib reviewed Apr 20, 2023

View reviewed changes

api/v1beta1/azuremachine_validation.go Outdated Show resolved Hide resolved

alexeldeib reviewed Apr 20, 2023

View reviewed changes

azure/services/virtualmachines/spec.go Outdated Show resolved Hide resolved

mresvanis force-pushed the add-confidential-vm-support branch from 2fc3e0c to 6e04db1 Compare May 9, 2023 10:06

mresvanis force-pushed the add-confidential-vm-support branch 2 times, most recently from 8574389 to 353a109 Compare May 15, 2023 13:31

k8s-ci-robot assigned nawazkh May 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2023

CecileRobertMichon mentioned this pull request May 18, 2023

SGX-enabled VMs #488

Closed

mboersma reviewed Jun 9, 2023

View reviewed changes

mresvanis force-pushed the add-confidential-vm-support branch from 353a109 to 3582327 Compare June 12, 2023 11:47

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2023

k8s-ci-robot requested a review from nawazkh June 12, 2023 11:47

mresvanis force-pushed the add-confidential-vm-support branch from 3582327 to cbaaaca Compare June 12, 2023 11:54

mresvanis force-pushed the add-confidential-vm-support branch from cbaaaca to 720566f Compare June 12, 2023 12:15

mboersma reviewed Jun 12, 2023

View reviewed changes

k8s-ci-robot assigned mboersma Jun 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2023

CecileRobertMichon reviewed Jun 12, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2023

k8s-ci-robot merged commit 4cfde35 into kubernetes-sigs:main Jun 12, 2023

mresvanis deleted the add-confidential-vm-support branch June 12, 2023 15:35

mresvanis mentioned this pull request Jul 11, 2023

MGMT-13628: add support for confidential VMs on Azure openshift/installer#7312

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for confidential VMs #3265

Add support for confidential VMs #3265

mresvanis commented Mar 10, 2023 •

edited

Loading

mresvanis commented Mar 15, 2023

nawazkh commented Mar 15, 2023

CecileRobertMichon commented Apr 4, 2023

nawazkh commented Apr 4, 2023

willie-yao left a comment

mresvanis commented Apr 18, 2023 •

edited

Loading

willie-yao commented Apr 18, 2023

mresvanis commented Apr 19, 2023

nawazkh left a comment

nawazkh left a comment

nawazkh Apr 19, 2023

nawazkh Apr 19, 2023

mresvanis Apr 20, 2023

nawazkh Apr 26, 2023

nawazkh Apr 19, 2023

codecov-commenter commented Apr 20, 2023 •

edited by codecov bot

Loading

alexeldeib Apr 20, 2023

alexeldeib Apr 20, 2023

alexeldeib Apr 20, 2023 •

edited

Loading

alexeldeib Apr 20, 2023

alexeldeib commented Apr 20, 2023

alexeldeib commented Apr 20, 2023

nawazkh commented May 12, 2023

CecileRobertMichon commented May 12, 2023

mresvanis commented May 15, 2023

mresvanis commented May 15, 2023

nawazkh commented May 17, 2023

k8s-ci-robot commented May 17, 2023

mresvanis commented Jun 2, 2023

mboersma left a comment

mboersma left a comment

k8s-ci-robot commented Jun 12, 2023

CecileRobertMichon left a comment

k8s-ci-robot commented Jun 12, 2023

Add support for confidential VMs #3265

Add support for confidential VMs #3265

Conversation

mresvanis commented Mar 10, 2023 • edited Loading

mresvanis commented Mar 15, 2023

nawazkh commented Mar 15, 2023

CecileRobertMichon commented Apr 4, 2023

nawazkh commented Apr 4, 2023

willie-yao left a comment

Choose a reason for hiding this comment

mresvanis commented Apr 18, 2023 • edited Loading

willie-yao commented Apr 18, 2023

mresvanis commented Apr 19, 2023

nawazkh left a comment

Choose a reason for hiding this comment

nawazkh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 20, 2023 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib commented Apr 20, 2023

alexeldeib commented Apr 20, 2023

nawazkh commented May 12, 2023

CecileRobertMichon commented May 12, 2023

mresvanis commented May 15, 2023

mresvanis commented May 15, 2023

nawazkh commented May 17, 2023

k8s-ci-robot commented May 17, 2023

mresvanis commented Jun 2, 2023

mboersma left a comment

Choose a reason for hiding this comment

mboersma left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 12, 2023

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 12, 2023

mresvanis commented Mar 10, 2023 •

edited

Loading

mresvanis commented Apr 18, 2023 •

edited

Loading

codecov-commenter commented Apr 20, 2023 •

edited by codecov bot

Loading

alexeldeib Apr 20, 2023 •

edited

Loading