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

overlord/devicestate,secboot: fix factory reset on older versions #14471

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

Conversation

valentindavid
Copy link
Contributor

@valentindavid valentindavid commented Sep 5, 2024

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.

@github-actions github-actions bot added the Run Nested -auto- Label automatically added in case nested tests need to be executed label Sep 5, 2024
@valentindavid valentindavid added the FDE Manager Pull requests that target FDE manager branch label Sep 10, 2024
@valentindavid valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from e820f9a to 21ed272 Compare September 11, 2024 10:11
@valentindavid valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from 21ed272 to 4b3991d Compare September 20, 2024 08:41
@valentindavid valentindavid added the Run nested The PR also runs tests inluded in nested suite label Sep 20, 2024
@valentindavid valentindavid reopened this Sep 20, 2024
@pedronis pedronis force-pushed the fde-manager-features branch from fcc5e2e to 24018ee Compare October 18, 2024 11:27
@valentindavid valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from 4b3991d to f463a03 Compare October 18, 2024 11:53
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 26.53061% with 36 lines in your changes missing coverage. Please review.

Please upload report for BASE (fde-manager-features@af9b482). Learn more about missing BASE report.

Files with missing lines Patch % Lines
secboot/secboot_sb.go 14.28% 24 Missing ⚠️
overlord/devicestate/handlers_install.go 38.88% 7 Missing and 4 partials ⚠️
secboot/secboot_tpm.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                   Coverage Diff                   @@
##             fde-manager-features   #14471   +/-   ##
=======================================================
  Coverage                        ?   78.18%           
=======================================================
  Files                           ?     1160           
  Lines                           ?   154973           
  Branches                        ?        0           
=======================================================
  Hits                            ?   121158           
  Misses                          ?    26304           
  Partials                        ?     7511           
Flag Coverage Δ
unittests 78.18% <26.53%> (?)

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/remodel-then-factory-reset branch 2 times, most recently from 3fe46ac to 29dd378 Compare November 19, 2024 11:38
@valentindavid valentindavid marked this pull request as ready for review November 19, 2024 11:38
@valentindavid
Copy link
Contributor Author

This might still need #14718 to work. We will see.

@valentindavid valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch 4 times, most recently from c0efc73 to d43620a Compare November 20, 2024 09:30
@valentindavid
Copy link
Contributor Author

We are missing a piece, I have forgotten about that. We need to be able to convert LUKS2 disk without tokens to ones with tokens, just for the names, so we can rename and delete them.

@valentindavid
Copy link
Contributor Author

I have opened canonical/secboot#349 to have a way to write factory reset in a way that works.

@valentindavid valentindavid force-pushed the valentindavid/remodel-then-factory-reset branch from d43620a to bfbb339 Compare December 10, 2024 10:46
@valentindavid
Copy link
Contributor Author

This also depends on canonical/secboot#355

@valentindavid
Copy link
Contributor Author

Draft, because it needs PRs to be merged on secboot.

@valentindavid valentindavid changed the title tests: verify that factory reset after remodeling to a new snapd works overlord/devicestate,secboot: fix factory reset on older versions Jan 24, 2025
@valentindavid valentindavid marked this pull request as ready for review January 24, 2025 14:56
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/remodel-then-factory-reset branch from 288d176 to 7c2e31f Compare January 24, 2025 15:00
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.

some questions/comments

@@ -348,7 +348,7 @@ func AddBootstrapKeyOnExistingDisk(node string, newKey keys.EncryptionKey) error
// Rename key slots on LUKS2 container. If the key slot does not
// exist, it is ignored. If cryptsetup does not support renaming, then
// the key slots are instead removed.
func RenameOrDeleteKeys(node string, renames map[string]string) error {
func RenameKeys(nonAtomic *NonAtomicOperationAllowedFlag, node string, renames map[string]string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

are there calls to RenameKeys that will not pass nonAtomic?

Comment on lines +1391 to +1394
if err := secbootDeleteKeys(diskPath, toDelete); err != nil {
return fmt.Errorf("cannot delete previous keys: %w", err)
}
if err := secbootRemoveOldDiskKeys(diskPath); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it a bit strange that we both call DeleteKeys and then call a helper that does a bit of that as well, at a minimum these need comments about what each does

// names to those keyslots. This is needed to convert the save disk
// during a factory reset.
func ConvertOldDisk(devicePath string) error {
if err := sb.NameLegacyLUKS2ContainerKey(devicePath, 0, "old-default-key"); err != nil && !errors.Is(err, sb.KeyslotAlreadyHasANameErr) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this does nothing if the slot has already a name right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FDE Manager Pull requests that target FDE manager branch Run Nested -auto- Label automatically added in case nested tests need to be executed Run nested The PR also runs tests inluded in nested suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants