Extend virtctl to Support Interactive SEV Attestation #7595

Fuzzy-Math · 2022-04-19T17:58:45Z

What this PR does / why we need it:

This PR extends virtctl to support interactive attestation of SEV enabled workloads.

The following subcommands have been added:

virtctl sev fetch-cert-chain
virtctl sev setup-session
virtctl sev query-measurement
virtctl sev inject-secret

Special notes for your reviewer:

This PR relies on Introduce API endpoints for SEV attestation #7197 for intial SEV enablement
Implementation based off initial design proposal
Until Introduce API endpoints for SEV attestation #7197 is merged, another branch is being maintained to allow building and testing of these changes

Release note:

Extending virtctl to support interactive attestation of AMD SEV enabled workloads

kubevirt-bot · 2022-04-19T17:58:55Z

Hi @Fuzzy-Math. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Fuzzy-Math · 2022-04-19T17:59:55Z

/hold

vasiliy-ul

Looks good. Just added a few minor comments.

vasiliy-ul · 2022-04-20T05:34:02Z

pkg/virtctl/sev/attestation.go

+			if pathProvided {
+				if _, err := file.Write(data); err != nil {
+					log.Log.Errorf("[SEV] Failed to write certificate chain to %s", outpath)
+					return fmt.Errorf("Error %s/%s: %v", namespace, vmiName, err)
+				}
+				log.Log.Infof("[SEV]Writing platform certificate chain to %s", outpath)
+			} else {
+				fmt.Printf("%s\n", string(data))
+			}


I think the code can be simplified by using https://pkg.go.dev/io/ioutil#WriteFile . That way you will not need the file, err = os.OpenFile(...) part above. Same applies for the COMMAND_QUERY_MEASUREMENT case.

Using WriteFile() would overwrite the file if it already existed. As designed, I don't allow overwriting existing files and it's a failure condition if the file exists.

pkg/virtctl/sev/attestation_test.go

pkg/virtctl/sev/attestation.go

fabiand · 2022-05-06T13:27:34Z

@alicefr thoughts?

vladikr · 2022-05-06T13:30:03Z

@jean-edouard @iholder-redhat

iholder101 · 2022-05-08T15:41:13Z

@jean-edouard @iholder-redhat

Thanks for the FYI!
/assign @iholder-redhat

alicefr · 2022-05-09T07:03:27Z

pkg/virtctl/sev/attestation.go

+		}
+		defer file.Close()
+
+		if sevPlatformInfo, err := virtClient.VirtualMachineInstance(namespace).SEVFetchCertChain(vmiName); err != nil {


I'd rewrite this as:

sevPlatformInfo, err := virtClient.VirtualMachineInstance(namespace).SEVFetchCertChain(vmiName) if err != nil { return fmt.Errorf("Error %s/%s: %v", namespace, vmiName, err) }

It can avoid one level indentation

alicefr · 2022-05-09T07:07:13Z

pkg/virtctl/sev/attestation.go

+
+	case COMMAND_FETCH_CERT_CHAIN:
+		var file *os.File
+		pathProvided := outpath != ""


Can you evaluate this statement before the switch as you are repeating it 3 times there?

alicefr · 2022-05-09T07:09:44Z

pkg/virtctl/sev/attestation.go

+				return fmt.Errorf("Error %s/%s: %v", namespace, vmiName, err)
+			}
+		}
+		defer file.Close()


Should be this placed inside the if pathProvided as otherwise you don't open the file?Just wondering if you don't get a nil pointer?

Generally for the write and open of the file you could create a function since you are repeating it. It could also help improve the readability of the code

kubevirt-bot · 2023-07-07T07:44:24Z

@dhiller: Reopened this PR.

In response to this:

/reopen
/remove-lifecycle rotten

/test pull-kubevirt-e2e-k8s-1.26-sev

@Fuzzy-Math @alicefr @iholder101 @xpivarc IIUC we now have a manual test lane for AMD SEV testing in place.

Up to you folks whether you want to continue pushing this one.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kubevirt-bot · 2023-07-07T07:44:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from iholder101. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

vasiliy-ul · 2023-07-07T08:20:17Z

/reopen /remove-lifecycle rotten

/test pull-kubevirt-e2e-k8s-1.26-sev

@Fuzzy-Math @alicefr @iholder101 @xpivarc IIUC we now have a manual test lane for AMD SEV testing in place.

Up to you folks whether you want to continue pushing this one.

#7197 is now merged. I think this PR is also good to go in (after rebase and another review round).

alicefr · 2023-07-07T08:28:26Z

pkg/virtctl/sev/attestation.go

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Copyright 2022


This probably can be updated

alicefr · 2023-07-07T08:36:46Z

pkg/virtctl/sev/attestation.go

+	if outpath != "" {
+		file, err := os.OpenFile(outpath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, output_perm)
+		if err != nil {
+			return fmt.Errorf(errFmt, namespace, vmiName, outpath, err)
+		}
+		defer file.Close()
+
+		_, err = file.Write(data)
+		if err != nil {
+			return fmt.Errorf(errFmt, namespace, vmiName, outpath, err)
+		}
+		log.Log.Infof(logFmt, outpath)
+	} else {
+		fmt.Printf("%s\n", string(data))
+	}


Suggested change

if outpath != "" {

file, err := os.OpenFile(outpath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, output_perm)

if err != nil {

return fmt.Errorf(errFmt, namespace, vmiName, outpath, err)

}

defer file.Close()

_, err = file.Write(data)

if err != nil {

return fmt.Errorf(errFmt, namespace, vmiName, outpath, err)

}

log.Log.Infof(logFmt, outpath)

} else {

fmt.Printf("%s\n", string(data))

}

if output == "" {

fmt.Printf("%s\n", string(data))

return nil

}

file, err := os.OpenFile(outpath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, output_perm)

if err != nil {

return fmt.Errorf(errFmt, namespace, vmiName, outpath, err)

}

defer file.Close()

_, err = file.Write(data)

if err != nil {

return fmt.Errorf(errFmt, namespace, vmiName, outpath, err)

}

log.Log.Infof(logFmt, outpath)

alicefr · 2023-07-07T08:45:19Z

pkg/virtctl/sev/attestation_test.go

+
+	Describe("The 'setup-session' command", func() {
+		Context("Creation", func() {
+			It("should succeed", func() {


Do we need this test?

alicefr · 2023-07-07T08:46:14Z

pkg/virtctl/sev/attestation_test.go

+				Expect(cmd).ToNot(BeNil())
+			})
+		})
+		Context("With no argument", func() {


What about grouping this and the next 3 tests into a table?

alicefr · 2023-07-07T08:49:00Z

pkg/virtctl/sev/attestation_test.go

+			})
+		})
+		Context("With no argument", func() {
+			It("should fail", func() {


Again, as above, try to group tests that has very similar code into a table

alicefr · 2023-07-07T08:50:46Z

@vasiliy-ul @Fuzzy-Math do we plan to add also functional tests for this?

alicefr · 2023-07-07T08:50:54Z

/ok-to-test

vasiliy-ul · 2023-07-07T08:53:08Z

@vasiliy-ul @Fuzzy-Math do we plan to add also functional tests for this?

I think yes, now it makes sense since the API is in place.

dhiller · 2023-07-07T09:41:08Z

@Fuzzy-Math I think a run of make generate and committing the changes is missing here: https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/7595/pull-kubevirt-generate/1677238768936947712#1:build-log.txt%3A1396

kubevirt-bot · 2023-10-05T10:29:24Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

kubevirt-bot · 2023-11-04T11:24:21Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle rotten

kubevirt-bot · 2023-12-04T11:33:23Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

kubevirt-bot · 2023-12-04T11:33:27Z

@kubevirt-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dhiller · 2023-12-11T09:11:44Z

There's CI support on a periodic base for this here: https://testgrid.k8s.io/kubevirt-periodics#periodic-kubevirt-e2e-k8s-1.26-sev

/reopen
/remove-lifecycle rotten

kubevirt-bot · 2023-12-11T09:11:47Z

@dhiller: Reopened this PR.

In response to this:

There's CI support on a periodic base for this here: https://testgrid.k8s.io/kubevirt-periodics#periodic-kubevirt-e2e-k8s-1.26-sev

/reopen
/remove-lifecycle rotten

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dhiller · 2023-12-11T09:12:32Z

/test pull-kubevirt-e2e-k8s-1.26-sev

Let's see how the presubmit goes

dhiller · 2023-12-11T09:17:16Z

Hey @vasiliy-ul @Fuzzy-Math, are you folks up to picking this up again?

vasiliy-ul · 2023-12-11T09:44:59Z

Hey @vasiliy-ul @Fuzzy-Math, are you folks up to picking this up again?

Hi @dhiller. Thank you for the ping. The PR probably needs a rebase and also some updates to address the comments. Yes, definitely, we will look at it.

dhiller · 2023-12-11T10:06:38Z

@vasiliy-ul I've converted it back to draft, just to reduce spent CI resources. Reconvert any time when it's ready.

kubevirt-bot · 2024-05-04T02:37:44Z

@Fuzzy-Math: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-unit-test-arm64	`34e5ce0`	link	false	`/test pull-kubevirt-unit-test-arm64`
pull-kubevirt-goveralls	`34e5ce0`	link	false	`/test pull-kubevirt-goveralls`
pull-kubevirt-unit-test	`34e5ce0`	link	true	`/test pull-kubevirt-unit-test`
pull-kubevirt-build-arm64	`34e5ce0`	link	true	`/test pull-kubevirt-build-arm64`
pull-kubevirt-build	`34e5ce0`	link	true	`/test pull-kubevirt-build`
pull-kubevirt-generate	`34e5ce0`	link	true	`/test pull-kubevirt-generate`
pull-kubevirt-e2e-k8s-1.29-sig-compute-migrations	`34e5ce0`	link	true	`/test pull-kubevirt-e2e-k8s-1.29-sig-compute-migrations`
pull-kubevirt-e2e-k8s-1.30-sig-compute-serial	`34e5ce0`	link	true	`/test pull-kubevirt-e2e-k8s-1.30-sig-compute-serial`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

kubevirt-bot requested review from dhiller and xpivarc April 19, 2022 17:58

kubevirt-bot added the size/XL label Apr 19, 2022

kubevirt-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 19, 2022

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 19, 2022

Fuzzy-Math changed the title ~~[WIP]Extend virtctl to Support Interactive SEV Attestation~~ Extend virtctl to Support Interactive SEV Attestation Apr 19, 2022

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 19, 2022

Fuzzy-Math force-pushed the sev-attestation branch from f0132f7 to ca79d71 Compare April 20, 2022 01:48

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Apr 20, 2022

Fuzzy-Math force-pushed the sev-attestation branch from ca79d71 to 0091577 Compare April 20, 2022 01:51

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Apr 20, 2022

vasiliy-ul mentioned this pull request Apr 20, 2022

AMD SEV technology enablement #6991

Open

15 tasks

vasiliy-ul reviewed Apr 20, 2022

View reviewed changes

vasiliy-ul reviewed Apr 21, 2022

View reviewed changes

pkg/virtctl/sev/attestation.go Outdated Show resolved Hide resolved

pkg/virtctl/sev/attestation.go Outdated Show resolved Hide resolved

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2022

vasiliy-ul mentioned this pull request Apr 25, 2022

Introduce API endpoints for SEV attestation #7197

Merged

Fuzzy-Math force-pushed the sev-attestation branch from d13029c to 692eb49 Compare April 25, 2022 16:11

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2022

kubevirt-bot assigned iholder101 May 8, 2022

alicefr reviewed May 9, 2022

View reviewed changes

kubevirt-bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jul 7, 2023

alicefr reviewed Jul 7, 2023

View reviewed changes

kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 7, 2023

kubevirt-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 5, 2023

kubevirt-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 4, 2023

kubevirt-bot closed this Dec 4, 2023

kubevirt-bot reopened this Dec 11, 2023

kubevirt-bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Dec 11, 2023

dhiller marked this pull request as draft December 11, 2023 10:05

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2023

Extend virtctl to Support Interactive SEV Attestation #7595

Are you sure you want to change the base?

Extend virtctl to Support Interactive SEV Attestation #7595

Conversation

Fuzzy-Math commented Apr 19, 2022 • edited

kubevirt-bot commented Apr 19, 2022

Fuzzy-Math commented Apr 19, 2022

vasiliy-ul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiand commented May 6, 2022

vladikr commented May 6, 2022

iholder101 commented May 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicefr May 9, 2022 • edited

Choose a reason for hiding this comment

kubevirt-bot commented Jul 7, 2023

kubevirt-bot commented Jul 7, 2023

vasiliy-ul commented Jul 7, 2023 • edited

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

alicefr Jul 7, 2023 • edited

Choose a reason for hiding this comment

alicefr commented Jul 7, 2023

alicefr commented Jul 7, 2023

vasiliy-ul commented Jul 7, 2023

dhiller commented Jul 7, 2023

kubevirt-bot commented Oct 5, 2023

kubevirt-bot commented Nov 4, 2023

kubevirt-bot commented Dec 4, 2023

kubevirt-bot commented Dec 4, 2023

dhiller commented Dec 11, 2023

kubevirt-bot commented Dec 11, 2023

dhiller commented Dec 11, 2023

dhiller commented Dec 11, 2023

vasiliy-ul commented Dec 11, 2023

dhiller commented Dec 11, 2023

kubevirt-bot commented May 4, 2024

Fuzzy-Math commented Apr 19, 2022 •

edited

alicefr May 9, 2022 •

edited

vasiliy-ul commented Jul 7, 2023 •

edited

alicefr Jul 7, 2023 •

edited