tests: rebuild OVMF and use generated keys #14960
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
valentindavid
added
the
Run nested
valentindavid
force-pushed
the
valentindavid/build-ovmf
branch
from
ca6ff95 to
a01e5da
Compare
|
Mon Jan 27 17:33:42 UTC 2025 Failures:Preparing:
Executing:
Restoring:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14960 +/- ##
=========================================
Coverage ? 78.25%
=========================================
Files ? 1155
Lines ? 153556
Branches ? 0
=========================================
Hits ? 120170
Misses ? 25990
Partials ? 7396
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bboozzoo
reviewed
Jan 22, 2025
tests/lib/snaps/ovmf/snapcraft.yaml
Outdated
| - nasm | ||
| - acpica-tools | ||
| plugin: nil | ||
| source: https://github.com/tianocore/edk2.git |
There was a problem hiding this comment.
could we use ovmf and efi-shell from 24.04 instead?
valentindavid
force-pushed
the
valentindavid/build-ovmf
branch
from
a01e5da to
8cce59c
Compare
bboozzoo
reviewed
Jan 22, 2025
There was a problem hiding this comment.
The remaining open question is whether we should have a tests which uses ovmf packages from the repo to run the VM like the docs page at https://ubuntu.com/core/docs/testing-with-qemu state. Or perhaps that's a job to be run in some other repository (core-base?)
tests/lib/snaps/ovmf/snapcraft.yaml
Outdated
| @@ -0,0 +1,140 @@ | |||
| name: ovmf | |||
There was a problem hiding this comment.
we can name it test-snapd-ovmf, update description to mention that that it's ovmf binaries with some testing keys enrolled used for snapd CI and push it to the store
There was a problem hiding this comment.
I've opened a name request for test-snapd-ovmf. Can move the snap files to tests/lib/snaps/store/test-snapd-ovmf ?
tests/lib/snaps/ovmf/snapcraft.yaml
Outdated
Comment on lines
86
to
87
| install -Dm644 -t "${CRAFT_PART_INSTALL}/secboot" \ | ||
| LockDown.efi {PK,KEK,DB}.{key,crt} |
There was a problem hiding this comment.
this will generate new keys each time during the build which isn't obvious. Probably not a problem but users need to be aware of it. Or we can drop pre-generated keys into the snap.
There was a problem hiding this comment.
The point here is people not to test explicitly a key. They have to test if a key matches specifically DB or KEK. And not just a constant hash in some test.
tests/lib/snaps/ovmf/snapcraft.yaml
Outdated
| -drive "if=pflash,file=${OVMF}_VARS.fd,format=raw" \ | ||
| -drive "if=virtio,file=${CRAFT_STAGE}/lockdown.img,format=raw" >qemu.out 2>qemu.err </dev/null | ||
| install -Dm644 "${OVMF}_VARS.fd" "${CRAFT_PART_INSTALL}/fw/${OVMF}_VARS.secboot.fd" |
There was a problem hiding this comment.
Suggested change
| install -Dm644 "${OVMF}_VARS.fd" "${CRAFT_PART_INSTALL}/fw/${OVMF}_VARS.secboot.fd" | |
| install -Dm644 "${OVMF}_VARS.fd" "${CRAFT_PART_INSTALL}/fw/${OVMF}_VARS.secboot-testkeys.fd" |
tests/lib/nested.sh
Outdated
| return | ||
| fi | ||
| if ! [ -f "${NESTED_ASSETS_DIR}/ovmf.snap" ]; then | ||
| (cd "${TESTSLIB}/snaps/ovmf"; run_snapcraft --use-lxd --verbosity quiet --output="${NESTED_ASSETS_DIR}/ovmf.snap") |
There was a problem hiding this comment.
I think I'd rather download the snap from store rather than build it, or even fail explicitly and expect run-spread to do the download.
There was a problem hiding this comment.
I agree. I will keep it in the draft for now. When the tests seem to work, I will add some CI step that uploads it.
We have |
bboozzoo
reviewed
Jan 22, 2025
tests/lib/snaps/ovmf/snapcraft.yaml
Outdated
| @@ -0,0 +1,140 @@ | |||
| name: ovmf | |||
There was a problem hiding this comment.
I've opened a name request for test-snapd-ovmf. Can move the snap files to tests/lib/snaps/store/test-snapd-ovmf ?
tests/lib/snaps/ovmf/snapcraft.yaml
Outdated
Comment on lines
5
to
6
| summary: EDK2 | ||
| description: EDK2 |
There was a problem hiding this comment.
Suggested change
| summary: EDK2 | |
| description: EDK2 | |
| summary: Pre-built OVMF blobs with test keys for snapd CI | |
| description: | | |
| Pre-built OVMF blobs with enrolled test keys for use in snapd CI loop. | |
| The following known keys are enrolled: | |
| - snakeoil - ## ref? | |
| - kernel PPA | |
| <whichever we key we add> |
valentindavid
force-pushed
the
valentindavid/build-ovmf
branch
6 times, most recently
from
19ba700 to
81ccf1d
Compare
bboozzoo
reviewed
Jan 24, 2025
tests/lib/snaps/ovmf/snapcraft.yaml
Outdated
There was a problem hiding this comment.
can you rename the directory to tests/lib/snaps/store/test-snapd-ovmf?
valentindavid
force-pushed
the
valentindavid/build-ovmf
branch
8 times, most recently
from
cdc4f7a to
855b088
Compare
UEFI expects non volatile memory which `-bios` can not provide. So to have a behavior that every where what we expect, we should stop using `-bios`. Otherwise, we cannot test UEFI boot entries, sbat revocation, DBX update, etc.
valentindavid
force-pushed
the
valentindavid/build-ovmf
branch
4 times, most recently
from
9574bbf to
5bfbbd3
Compare
valentindavid
force-pushed
the
valentindavid/build-ovmf
branch
4 times, most recently
from
6701186 to
1092cf5
Compare
valentindavid
force-pushed
the
valentindavid/build-ovmf
branch
from
1092cf5 to
c67349b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on #14959
The generated OVMF_VARS also contains microsoft "other" db cert. And also the certs for the pc-kernel edge. As well as the now deprecated snakeoil cert.