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

Introduce API endpoints for SEV attestation #7197

Merged
merged 12 commits into from Jun 30, 2023

Conversation

vasiliy-ul
Copy link
Contributor

@vasiliy-ul vasiliy-ul commented Feb 8, 2022

What this PR does / why we need it:

The PR introduces new API endpoints for SEV attestation. It is based on the initial design proposal. It allows to perform the necessary pre-launch attestation steps:

  • retrieving platform certificates for verification sev/fetchcertchain: the API call is routed to the node where the VM is scheduled and it is processed by virt-handler. The certificates are fetched from libvirt by node-labeller in the init-container context (thus ATM the support of certificates rotation is limited); UPD: the call is routed to the virt-launcher pod where the corresponding libvirt API is invoked;
  • setting session parameters sev/setupsession: the call is routed to the virt-launcher pod where the corresponding libvirt API is invoked;
  • querying launch measurement sev/querylaunchmeasurement: the call is routed to the virt-launcher pod where the corresponding libvirt API is invoked. The measurement is returned to the caller;
  • injecting launch secret sev/injectsecret: the call is routed to the virt-launcher pod where the corresponding libvirt API is invoked.

The attestation process can be run either manually by calling the corresponding endpoints (e.g. using virtctl) or in an automated way by leveraging an attestation server and a controller that will monitor the VMI state and perform the required actions. The E2E functional test in tests/launchsecurity/sev.go demonstrates the general flow.

Attestation can be requested via VMI spec yaml:

spec:
  startStrategy: "Paused"
  domain:
    firmware:
      bootloader:
        efi:
          secureBoot: false
    launchSecurity:
      sev:
        attestation: {}

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Experimantal support of SEV attestation via the new API endpoints

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 8, 2022
@vasiliy-ul vasiliy-ul marked this pull request as draft February 8, 2022 13:33
@vasiliy-ul vasiliy-ul mentioned this pull request Feb 8, 2022
15 tasks
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2022
@kubevirt-bot kubevirt-bot added size/XXL and removed size/XL needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 10, 2022
@vasiliy-ul vasiliy-ul force-pushed the sev-attestation branch 2 times, most recently from e16ee9f to 7bb8eb2 Compare March 9, 2022 13:44
@vasiliy-ul vasiliy-ul force-pushed the sev-attestation branch 2 times, most recently from 775b7e0 to bb14be1 Compare March 18, 2022 09:08
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2022
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2022
@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 5, 2022
@vasiliy-ul vasiliy-ul changed the title WIP Introduce API endpoints for SEV attestation Introduce API endpoints for SEV attestation Apr 5, 2022
@vasiliy-ul vasiliy-ul marked this pull request as ready for review April 5, 2022 16:44
@kubevirt-bot 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 5, 2022
Introduce sev/querylaunchmeasurement API endpoint for VMI. Apart from
the measurement itself it returns the data needed to calculate the
expected value as specified in AMD SEV specification:

  HMAC(0x04 || API_MAJOR || API_MINOR || BUILD ||
       GCTX.POLICY || GCTX.LD || MNONCE; GCTX.TIK)

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
Introduce sev/setupsession API endpoint for VMI. It can be used to
provide the session launch blob and the Diffie-Hellman key for a guest
that has been scheduled to run on a particular node.

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
Introduce sev/injectlaunchsecret API endpoint for VMI to inject an
encrypted secret into a paused guest.

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
This reverts fetching of the certs from the node labeller. If fetched
directly in virt-launcher context that will automatically handle
certificates rotation since the API call will return the most recent
state.

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
Ensure the request fails if VMI is not running.

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
Do not hardcode the expected parameters since they are bound to a
specific hardware instance. Instead try to detect those in runtime using
virsh cli.

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
Use sevctl tool to prepare the parameters in runtime.

Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2023
@vasiliy-ul
Copy link
Contributor Author

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

@vasiliy-ul
Copy link
Contributor Author

/test pull-kubevirt-e2e-k8s-1.26-sig-storage

@vasiliy-ul
Copy link
Contributor Author

Hi @alicefr, @iholder101. Previously, you reviewed the code here. Could you please re-lgtm/approve the PR (as you see fit)? I now rebased it and fixed all the conflicts (mostly with the generated code). There are no functional changes. The only thing is that I've moved the addition of sevctl tool to the test image into a separate PR (which is already merged #9892). Thanks!

@alicefr, @iholder101, a gentle ping :) Would appreciate it if you could re-lgtm or approve the PR. Thanks!


manager, _ := NewLibvirtDomainManager(mockConn, testVirtShareDir, testEphemeralDiskDir, nil, ovmfDir, ephemeralDiskCreatorMock, metadataCache)
sevMeasurementInfo, err := manager.GetLaunchMeasurement(vmi)
if runtime.GOARCH == "amd64" {
Copy link
Member

@alicefr alicefr Jun 27, 2023

Choose a reason for hiding this comment

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

Shouldn't we skip all the SEV suite if this isn't amd64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about skipping, since it may hide potential issues. If the code is expected to error-out on non-amd64, IMHO, better to have a test in place to ensure it. In this particular case, manager.GetLaunchMeasurement fails due to missing ovmf code binary with sev support.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, fine to me!

@@ -197,6 +187,31 @@ var _ = Describe("[sig-compute]AMD Secure Encrypted Virtualization (SEV)", decor
}
}

parseVirshInfo := func(info string, expectedKeys []string) map[string]string {
Copy link
Member

@alicefr alicefr Jun 27, 2023

Choose a reason for hiding this comment

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

This might be a good function to move to a shared file for utils. It can be done in a following PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, this is a good candidate for common utils. Though, on the other hand, I think it makes sense to move it when there are several users of that function. Otherwise, the utils tend to grow.

return entries
}

toUint := func(s string) uint {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above we could move it to a common shared file

@alicefr
Copy link
Member

alicefr commented Jun 27, 2023

/approve
/hold
Let's give some time to @iholder101 if he wants to have another look, but to me, it looks good
@vasiliy-ul great work and thanks for the perseverance!

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2023
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alicefr

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 27, 2023
@vasiliy-ul
Copy link
Contributor Author

Thanks a lot @alicefr for the review and valuable input!

@iholder101
Copy link
Contributor

Sorry for the delay @vasiliy-ul!

Thanks very much for this PR! Appreciate your contributions a lot, I know it wasn't easy :)

/lgtm
/unhold

@kubevirt-bot kubevirt-bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 29, 2023
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Jun 30, 2023

@vasiliy-ul: 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-e2e-k8s-1.23-sig-compute 460fdec link true /test pull-kubevirt-e2e-k8s-1.23-sig-compute
pull-kubevirt-fossa 3528f6a link false /test pull-kubevirt-fossa

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.

@vasiliy-ul
Copy link
Contributor Author

Thank you for the review, @iholder101!

/test pull-kubevirt-unit-test

@kubevirt-bot kubevirt-bot merged commit 34daa9c into kubevirt:main Jun 30, 2023
37 checks passed
@vasiliy-ul vasiliy-ul deleted the sev-attestation branch June 30, 2023 08:33
@dbasunag
Copy link

dbasunag commented Jul 19, 2023

Looks like this PR broke https://github.com/openshift/openshift-restclient-python/blob/master/openshift/dynamic/discovery.py#L133, which expects VMInstancesSEVFetchCertChain/VMInstancesSEVQueryLaunchMeasurement/VMInstancesSEVSetupSession/VMInstancesSEVInjectLaunchSecret to be of the format 'a/b' not a/b/c

cc: @myakove

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet