-
Notifications
You must be signed in to change notification settings - Fork 598
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
secboot,fdestate: use boot mode for FDE hooks #15049
base: master
Are you sure you want to change the base?
secboot,fdestate: use boot mode for FDE hooks #15049
Conversation
Mon Feb 17 16:49:57 UTC 2025 Failures:Preparing:
Executing:
Restoring:
|
084f35b
to
49b1aa8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15049 +/- ##
=========================================
Coverage ? 78.08%
=========================================
Files ? 1179
Lines ? 157485
Branches ? 0
=========================================
Hits ? 122970
Misses ? 26880
Partials ? 7635
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
e804c4e
to
12e6592
Compare
b08f0b7
to
33ba6bf
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.
this looks reasonable to me
I removed the 2.68 milestone, we do not strictly need it, just #15057 |
7a1855b
to
331f02c
Compare
Set the authorized boot modes for FDE hook keys. For now the run+recover key allows "run" and "recover", while the recover key allows "recover" and "factory-reset".
There should be 3 different keys for FDE hooks. The run+recover key should be allowed for boot modes "run" and "recover". While recover key on data disk should be allowed on "recover". And finally recovery on save disk should be allowed in "recover" and "factory-reset". Here we split the profiles for "recover" for disks "data" and "save", so that we can set different authorized boot modes.
331f02c
to
6cff916
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.
did a first pass. Do we need an explicit test about the transition for what we are storing currently in the state and what we will store after?
@@ -87,6 +87,8 @@ type SealKeyRequest struct { | |||
// The file to store the key data. If empty, the key data will | |||
// be saved to the token. | |||
KeyFile string | |||
// The boot modes allow (i.e. snapd_recovery_mode kernel parameter) |
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.
typo: "allowed" or "to allow"
@@ -1793,7 +1793,7 @@ func (s *resealTestSuite) TestHooksResealHappy(c *C) { | |||
} | |||
|
|||
resealCalls := 0 | |||
restore := fdeBackend.MockSecbootResealKeysWithFDESetupHook(func(keys []secboot.KeyDataLocation, primaryKeyFile string, models []secboot.ModelForSealing) error { | |||
restore := fdeBackend.MockSecbootResealKeysWithFDESetupHook(func(keys []secboot.KeyDataLocation, primaryKeyFile string, models []secboot.ModelForSealing, bootModes []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.
do we need to check what gets passed in?
func doReseal(manager FDEStateManager, method device.SealingMethod, rootdir string) error { | ||
runParams, err := manager.Get("run+recover", "all") | ||
runParamsData, err := manager.Get("run+recover", "system-data") |
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 will fallback with to containerRole all anyway on the transition?
@@ -186,6 +186,11 @@ type ResealKeysParams struct { | |||
TPMPolicyAuthKeyFile string | |||
} | |||
|
|||
type ResealKeysParamsForHooks struct { |
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.
does this need a doc comment? can you explain why it is not used here but only defined?
@valentindavid also do we need somehow to clean up the entries for |
2 commits:
Set the authorized boot modes for FDE hook keys. For now the run+recover key allows "run" and "recover", while the recover key allows "recover" and "factory-reset".
There should be 3 different keys for FDE hooks. The run+recover key should be allowed for boot modes "run" and "recover". While recover key on data disk should be allowed on "recover". And finally recovery on save disk should be allowed in "recover" and "factory-reset". Here we split the profiles for "recover" for disks "data" and "save", so that we can set different authorized boot modes.