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

many: accidently the whole fde #15010

Merged

Conversation

valentindavid
Copy link
Contributor

No description provided.

@valentindavid valentindavid added the Run nested The PR also runs tests inluded in nested suite label Jan 31, 2025
@github-actions github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jan 31, 2025
Copy link

github-actions bot commented Jan 31, 2025

Fri Feb 7 09:05:58 UTC 2025

Spread tests skipped

@valentindavid valentindavid force-pushed the valentindavid/fde-branch-merge-test branch from f3c559a to a72e507 Compare January 31, 2025 15:41
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 77.09110% with 430 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@6079ed1). Learn more about missing BASE report.
Report is 174 commits behind head on master.

Files with missing lines Patch % Lines
overlord/fdestate/backend/reseal.go 70.91% 67 Missing and 31 partials ⚠️
overlord/fdestate/dbx.go 82.25% 55 Missing and 22 partials ⚠️
boot/seal.go 77.03% 36 Missing and 12 partials ⚠️
boot/boottest/fde.go 0.00% 45 Missing ⚠️
overlord/fdestate/backend/seal.go 79.56% 27 Missing and 11 partials ⚠️
overlord/devicestate/handlers_install.go 67.82% 23 Missing and 14 partials ⚠️
overlord/fdestate/fdemgr.go 75.00% 22 Missing and 6 partials ⚠️
daemon/api_system_secureboot.go 73.68% 14 Missing and 6 partials ⚠️
boot/makebootable.go 76.19% 10 Missing ⚠️
gadget/install/install.go 82.50% 6 Missing and 1 partial ⚠️
... and 8 more
Additional details and impacted files
@@            Coverage Diff            @@
##             master   #15010   +/-   ##
=========================================
  Coverage          ?   78.15%           
=========================================
  Files             ?     1168           
  Lines             ?   156529           
  Branches          ?        0           
=========================================
  Hits              ?   122329           
  Misses            ?    26607           
  Partials          ?     7593           
Flag Coverage Δ
unittests 78.15% <77.09%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@valentindavid valentindavid force-pushed the valentindavid/fde-branch-merge-test branch 2 times, most recently from 3fb3b97 to 8ab94b7 Compare February 4, 2025 11:37
@valentindavid valentindavid changed the title many: change FDE many: accidently the whole fde Feb 4, 2025
@valentindavid valentindavid force-pushed the valentindavid/fde-branch-merge-test branch 4 times, most recently from 4c41e58 to ad7b8c4 Compare February 6, 2025 12:05
We also make the FDE state manager install the the backend function to
be associated with the state.
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.
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.
Also update github.com/mvo5/goconfigparser to latest.
valentindavid and others added 19 commits February 6, 2025 15:11
There was a bug where the protector key file for the plainkey
of the ubuntu-save partition was incorrectly used as the unlock
key, the fix is to first check the unlock key from the keyring
if it is not there, then probably the key file is for the unlock
key and no plainkey is used.

Signed-off-by: Zeyad Gouda <[email protected]>

* fixup! secboot: use default-fallback to be consistent with reality

Signed-off-by: Zeyad Gouda <[email protected]>

---------

Signed-off-by: Zeyad Gouda <[email protected]>
Some old kernels do not store keys in the keyring at
all. Unfortunately, we do need an FDE state so that we can reseal, as
resealing needs to save/retrieve the policies in/from the state.
Mock the architecture so that boot assets can be properly identified
even when running unit tests on ARM64.

Signed-off-by: Maciej Borzecki <[email protected]>
…cal#14945)

* tests/nested/core/core20-fault-inject-on-refresh: disable FDE

Refresh from store get some kernel that have an old initrd that do not
support FDE installation from new snapd.
…vice

There is a bug in old UC20 kernels where udev does not retrieve data
from cold plugged disks initialized in initrd.

It was fixed in:

* canonical/core-initrd#58
* canonical/core-initrd#203
…t detected

Because we do not count reboots correctly, if we did not see the
second reboot, we should not fail.
We used 4 handles from 0x01880001-0x01880004 for PCR policies. We
always used a pair and would switch to the other pair after factory
reset.

No instead we look for free handle in range 0x01880005-0x0188000F. We
also use only one handle per primary key.

When we factory reset, we should remove the handles correctly since
the old primary key for the save partition should have been the same
as the data partition that was removed, thus the handles should be the
same.

We also now correctly populate the FDE state with the correct handles.
In the case we update snapd with and old kernel, then reseal, the FDE
state might be not set because we did not find the primary key from
the initrd that set it in the legacy path. So if we do not find the
key in the keyring, we should try and see if we find it in the old
path.
…anonical#15021)

Snapd needs `KeyringMode=shared`, but `snap-failure` runs it without.
We either need to add access to the keyring or disable initialization
of FDE state when re-seeding.
If a plainkey token is used then the key sealed with FDE hook should
have the same primary key. This was added for TPM, but I forgot to
add it to FDE hook.
This a temporary measure since pc-kernel in store does not have a
fresh enough snap-bootstrap.
The encrypted case is more complex and is covered in
tests/nested/manual/remodel-uc-to-next-version-fakestore
Until now, we were just delete old keys on factory-reset instead of
renaming if cryptsetup was too old. But this is a bit too dangerous.
Instead we copy-delete them.

Second, we need to convert old keyslots to have a name so we can
remove them after.
@valentindavid valentindavid force-pushed the valentindavid/fde-branch-merge-test branch from ad7b8c4 to e325e60 Compare February 6, 2025 14:13
@valentindavid valentindavid marked this pull request as ready for review February 6, 2025 20:11
@pedronis pedronis added Skip spread Indicate that spread job should not run and removed Run nested The PR also runs tests inluded in nested suite labels Feb 7, 2025
@pedronis pedronis requested review from pedronis and bboozzoo February 7, 2025 08:59
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

LGTM

@pedronis pedronis merged commit dcffe49 into canonical:master Feb 7, 2025
66 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Documentation -auto- Label automatically added which indicates the change needs documentation Skip spread Indicate that spread job should not run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants