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.

Sign up for GitHub

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

Jump to bottom

tests/nested, tests/lib/snaps: make core20-fde-dbx test more realistic, add required dependencies #14952

Open
wants to merge 80 commits into
base: fde-manager-features
Choose a base branch
from
Open

tests/nested, tests/lib/snaps: make core20-fde-dbx test more realistic, add required dependencies #14952

wants to merge 80 commits into from

Conversation

bboozzoo
Copy link
Contributor

@bboozzoo bboozzoo commented Jan 20, 2025 •
edited by jira bot
Loading

The PR aims to make tests/nested/manual/core20-fde-dbx test more realistic by performing an update of DBX such that the effects of resealing the keys can be observed and confirmed.

Unfortunately, this branch introduces some complexity. In order to be able to update DBX, the payload needs to be authenticated, and signed by enrolled keys. Since this cannot be easily performed during CI, the branch ships pre-built assets under tests/lib/nested-assets/dbx. Those include a set of keys used as PK, KEK, signing key, db key and a couple of exclusion keys, along with prebuilt payload for key enrollment. PK, KEK, DB, and initial dbx have all been enrolled to ovmf, and are stored in OVMF_VARS.test.fd.

The branch also introduces a testing snap test-snapd-efitools which ships the content of efitools package. I have requested a name in the store and it is currently awaiting approval.

Related: SNAPDENG-32510

valentindavid and others added 30 commits January 10, 2025 10:50
@valentindavid
.github: run CI for fde-manager-features PRs
28e3a6b
@valentindavid
boot/seal.go: split parameter gathering from writing backends in rese…
c2ba202 
…aling
@valentindavid
overlord: add barebone fdemgr
6fb0c14
@valentindavid
overlord/fdestate/fdemgr.go: fix a typo in doc
4e9cfe6
@valentindavid
many: move resealing backend from boot to fdestate's backend
44e5507 
We also make the FDE state manager install the the backend function to
be associated with the state.
@valentindavid
secboot: add bootstrap key abstraction
d66cff9 
Because we will need to enroll multiple keys, we need to make the
first key at volume creation a bootstrap key that we remove in the
end.  This commit does not implement it, but it does add the
abstraction where to allow us to do it.
@valentindavid
bootloader/bootloadertest: allow better mocking of kernel in boot chains
30789a3
@valentindavid
boot/boot_test.go: fix tests with multiple kernels
f413462
@valentindavid
boot/seal.go: document fields for ResealKeyForBootChainsParams
558df0d
@valentindavid
boot/bootchain.go: make some types and fields public
a6b1578
@valentindavid
overlord/fdestate/backend: add basic test for resealing
fa5325a
@valentindavid
tests: verify that breaking the model results in a locked disk
fcb4848 
When using FDE with hooks or tpm, modifying the run model in the boot
partition should result in disk that does not unlock. A recovery must
be used in that case.
@valentindavid
boot/seal_test.go: split some tests with overlord/fdestate/backend
9b8f335
@valentindavid
boot: mock ResealKeyForBootChains instead of ResealKeys
a89fffb
@valentindavid
secboot: remove dead mock for secboot.AddRecoveryKeyToLUKS2Container
f1b2f69 
Its use was removed in 3988a93 which was in PR canonical#11715
@valentindavid
go.mod: update secboot to 42c7ea9715b3
9124271 
Also update github.com/mvo5/goconfigparser to latest.
@valentindavid
boot: move sealing code to fdestate backend
8f55f3e
@valentindavid
boot/seal.go: remove some TODO that had been fixed
102b843
@valentindavid
boot: split sealing tests and move some to fdestate's backend
4f94210
@valentindavid
boot: move some functions to overlord/fdestate/backend
149fb12
@valentindavid
many: update secboot and use new key format
e8cf666
@valentindavid
github: enable macos tests on fde-manager-features
2d8ac35
@valentindavid
secboot: use uuid of luks2 instead of partition
98f6147 
`by-partuuid` does not make much sense because it uselessly assumes
that it is a partition. Conceptually we should not care about it.  It
also makes the resolution more complex as we need to fetch information
about the device which we do not really need at this point.

It is more common to resolve by filesystem UUID than part UUID. For
instance cryptsetup accepts path as
`UUID=deadbeef-dead-dead-dead-deaddeafbeef`. But it does not accept
this kind of syntax for partitions.
@bboozzoo @valentindavid
overlord/fdestate: stubs for EFI secure boot keys database updates
c4885b4 
Stubs

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
@bboozzoo @valentindavid
daemon: access checker for a connected interface on the slot side and…
7f7e04e 
… process being root

Add a new access checker which verifies that the request is coming from
a root user and if the process is a snap, a required interface is
connected, with that snap present on the slot side.

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
@bboozzoo @valentindavid
daemon: add /v2/system-secureboot endpoint for pushing updates to sec…
09e1053 
…ureboot key DBs

Add a new endpoint for integration with a local manager of EFI
secureboot key databases.

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
@valentindavid
secboot: factor code for encrypted disk activation
b295266 
This factors and simplifies the TPM and FDE Hook code together. This
does not yet factor the key file base one.
@valentindavid @bboozzoo
overlord/fdestate: keep FDE state up to date (canonical#14516)
a956714 
* overlord/fdestate: keep FDE state up to date

StartUp() initializes the empty profiles, and reseal updates them.

* secboot: reexeport secboot's kernel-key-not-found error

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* overlord/fdestate: use correct mount point for ubuntu-data

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* overlord/fdestate: skip key verification when key not in keyring

For interim compatibilty, the key used to unlock ubuntu-save may not be
present in the kernel keyring, so allow key digest verification step to
be skipped in such scenario.

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* secboot: use secboot marshallers instead of encoding/json for PCR profiles

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

---------

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
Co-authored-by: Maciej Borzecki <maciej.borzecki@canonical.com>
@valentindavid
many: remove dead code
1761024
@valentindavid
daemon/access_test.go: fix test
a15e239
valentindavid and others added 14 commits January 10, 2025 14:44
@valentindavid
secboot/bootstrap_container.go: fix naming in error messages
41870fe
@valentindavid
secboot/secboot_sb.go: add fixmes for untested functions
7447243
@valentindavid
secboot/bootstrap_container_sb.go: rename a field to a more correct name
8d2059b
@ZeyadYasser @valentindavid
many: move secboot.EncryptionType under gadget/device (canonical#14855)
0ddcb0e 
* fixup! many: formatting improvements

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

---------

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
@ZeyadYasser @valentindavid
many: update install api to support passphrase authentication (canoni…
042c0b7 
…cal#14845)

* many: add skeleton to expose encryption features for target install system

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

* many: update install api to support passphrase authentication

This just allows passing the authentication options during the
setup-storage-encryption install step but the underlying
passphrase support implementation is not there yet.

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

* fixup! secboot: add AuthModePIN

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

* fixup! client: add pin-auth feature enum

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

* fixup! daemon: use field initializers for readability

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

* fixup! o/devicestate: fix test matching old error message

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

* fixup! daemon: drop default initialization values

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

* fixup! many: move VolumesAuthOptions under gadget/device

This avoids import snapd/secboot from the client package

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

* fixup! many: mark setup task when volumes auth is set

This protects against an unexpected restart of snapd
where the cached auth options will not be there. In
this case, the task should detect this and fail.

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

* fixup! gadget/install: formatting improvements

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

---------

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
@ZeyadYasser @valentindavid
tests/lib/muinstaller: add passphrase authentication support (canonic…
2af9fae 
…al#14849)

* tests/lib/muinstaller: add passphrase authentication support

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

* fixup! tests/lib/muinstaller: switch to latest install-api branch

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

* fixup! tests/lib/muinstaller: switch snapd lib to fde-branch

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>

---------

Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
@bboozzoo @valentindavid
overlord/fdestate: EFI DBX support (canonical#14615)
5ec2e43 
Final pieces of EFI DBX support in fdestate, which implement the following logical 'actions' triggered through the snapd API:

'prepare' - triggered before fwupd or other agent performs the actual update, results in resealing of keys with the content of the DBX update
'cleanup - triggered after the update has completed, results in resealing of keys with the current content of DBX
'startup' - triggered whenever said agent starts up, such that we can detect whenever an unexpected reboot occurred or the agent crashed/restarted, which may trigger a reseal if an update was previously started.
The updates are tracked as 'external' operations in fdestate.
* tests/nested/manual/core20-fde-dbx: spread test for DBX related operations

Add a spread test for DBX related operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* overlord/fdestate: implement EFI DBX operations

Implement operations for notifying snapd of changes to the EFI DBX.

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! overlord/fdestate: implement EFI DBX operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! overlord/fdestate: implement EFI DBX operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! tests/nested/manual/core20-fde-dbx: spread test for DBX related operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! tests/nested/manual/core20-fde-dbx: spread test for DBX related operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! overlord/fdestate: implement EFI DBX operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! tests/nested/manual/core20-fde-dbx: spread test for DBX related operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! overlord/fdestate: implement EFI DBX operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! tests/nested/manual/core20-fde-dbx: spread test for DBX related operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! overlord/fdestate: implement EFI DBX operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! overlord/fdestate: implement EFI DBX operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! overlord/fdestate: implement EFI DBX operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! tests/nested/manual/core20-fde-dbx: spread test for DBX related operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

* fixup! overlord/fdestate: implement EFI DBX operations

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>

---------

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
@valentindavid @pedronis
secboot/keys: add wrapper secboot plainkey
f656320
@valentindavid @pedronis
many: propagate the primary key from when we set encrypted containers
fac84cc
@valentindavid @pedronis
boot: make useTokens public
252de94
@valentindavid @pedronis
overlord/install/install.go: use plainkey for save key
0faa6d9
@valentindavid @pedronis
tests/nested: check for plainkey used when tokens are in use
39dc92b
@valentindavid @pedronis
tests/nested/manual/hybrid-remodel: use fakestore
12c6f2a
@valentindavid @Meulengracht
secboot: fix building of primary digest
640541f
@bboozzoo bboozzoo added ⛔ Blocked Run nested The PR also runs tests inluded in nested suite labels Jan 20, 2025
valentindavid
valentindavid reviewed Jan 20, 2025
View reviewed changes
tests/lib/nested-assets/dbx/README.md
# run without snapshot mode
qemu-system-x86_64 -enable-kvm -smp 4 -m 2048 -cpu host -machine q35 -global ICH9-LPC.disable_s3=1 \
-drive file=./OVMF_CODE.secboot.fd,if=pflash,format=raw,unit=0,readonly=on \
-drive file=./OVMF_VARS.test.fd,if=pflash,format=raw \
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to always take the OVMF_VARS that comes from the OVMF_CODE.secboot.fd. The one without secboot, and enable secboot on it. Then that would answer the question: "how do we update OVMF_VARS for a new OVMF_CODE?"

valentindavid
valentindavid reviewed Jan 20, 2025
View reviewed changes
tests/lib/nested-assets/dbx/README.md Outdated
Comment on lines 46 to 54
# device configuration -> secure boot -> custom
# 1. enroll PK
# 2. enroll KEK
# 3. enroll DB
# 4. enroll DBX?

# reset
# pwoeroff
```
Copy link
Contributor

Choose a reason for hiding this comment

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

If we build efitools, we can create LockDown.efi. Then we can create an startup.nsh script that calls it then shutdown. We just need OVMF with a shell.

@bboozzoo
tests/lib/nested: allow using custom custom OVMF state file
0efd76b 
Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
@bboozzoo
tests/lib/nested-assets: OVMF state file for dbx tests
0cf4516 
Import pre-built OVMF state file and data set required for dbx spread
tests. The procedure for enrolling new keys cannot be easily
automated, so prebuilt files is all we have for now.

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
@bboozzoo
tests/nested/manual/core20-fde-dbx: use prebuilt OVMF variables block…
ad6c5d6 
… with enrolled keys

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
@bboozzoo
tests/lib/snaps/store/test-snapd-efitools: test snap for manipulating…
c5d36bf 
… efi variables

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
@bboozzoo
tests/nested/manual/core20-fde-dbx: perform actual DBX update
4d87c2c 
Update the test to be more realistic and perform an actual update of DBX
EFI variable.

Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
@bboozzoo bboozzoo force-pushed the bboozzoo/realistic-dbx-test branch from 96b4063 to 4d87c2c Compare January 21, 2025 14:40
@bboozzoo bboozzoo changed the title [WIP] tests/nested, tests/lib/snaps: make core20-fde-dbx test more realistic, add required dependencies tests/nested, tests/lib/snaps: make core20-fde-dbx test more realistic, add required dependencies Jan 21, 2025
@bboozzoo
fixup! tests/lib/nested-assets: OVMF state file for dbx tests
2b078a5 
Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
@bboozzoo
Copy link
Contributor Author

This can benefit from #14960

@pedronis pedronis force-pushed the fde-manager-features branch from 7005c64 to 25f9119 Compare January 29, 2025 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valentindavid valentindavid valentindavid left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Run nested The PR also runs tests inluded in nested suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bboozzoo @valentindavid @ZeyadYasser