-
Notifications
You must be signed in to change notification settings - Fork 595
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
base: fde-manager-features
Are you sure you want to change the base?
overlord/devicestate,secboot: fix factory reset on older versions #14471
Conversation
e820f9a
to
21ed272
Compare
21ed272
to
4b3991d
Compare
fcc5e2e
to
24018ee
Compare
4b3991d
to
f463a03
Compare
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
3fe46ac
to
29dd378
Compare
This might still need #14718 to work. We will see. |
c0efc73
to
d43620a
Compare
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. |
I have opened canonical/secboot#349 to have a way to write factory reset in a way that works. |
d43620a
to
bfbb339
Compare
This also depends on canonical/secboot#355 |
Draft, because it needs PRs to be merged on secboot. |
5802840
to
5ec2e43
Compare
bfbb339
to
f503fb1
Compare
f503fb1
to
288d176
Compare
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.
288d176
to
7c2e31f
Compare
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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?
if err := secbootDeleteKeys(diskPath, toDelete); err != nil { | ||
return fmt.Errorf("cannot delete previous keys: %w", err) | ||
} | ||
if err := secbootRemoveOldDiskKeys(diskPath); err != nil { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
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.