many: add reprovision API - #17034
Conversation
github-actions
Bot
added
Needs Documentation -auto-
|
Fri Jul 24 04:38:05 UTC 2026 Failures:Preparing:
Executing:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list of the below tests you wish to run (unskip plus test list must be valid yaml)
|
f8dc8f0 to
02ddab7
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #17034 +/- ##
===========================================
+ Coverage 78.92% 89.16% +10.23%
===========================================
Files 1398 1396 -2
Lines 196163 382473 +186310
Branches 2462 2462
===========================================
+ Hits 154830 341040 +186210
- Misses 32045 32105 +60
- Partials 9288 9328 +40
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a8debde to
7c3be44
Compare
4a6a91e to
4df1ded
Compare
4df1ded to
dc377e6
Compare
dcb3525 to
dab10a3
Compare
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
thanks, gave this a deeper look, some comments and some questions.
Also a LLM review futther noted this:
Rollback/cleanup state isn't asserted. The error tests (e.g. TestDoReprovisionMakeRunnableError) only assert the returned error; none verify that cleanup() renamed snapd-reprovision-* keys back to their originals and removed bootstrap keys. Since cleanup is the core safety mechanism, its resulting key state should be asserted on at least one error path.
|
|
||
| # TODO:FDEM: Grow test to check that we can reprovision and reseal | ||
| # then reboot without recovery keys | ||
| remote.exec "sudo snap debug api /v2/systems?running=true 2>/dev/null" | gojq '.result["storage-encryption"].support' | MATCH "available" |
There was a problem hiding this comment.
ah, this is incomplete because we haven't added the reprovision state yet?
| return err | ||
| } | ||
| if oldKeyMatches { | ||
| // We must have been restarted in the middle. The backed up keys are the correct ones. |
There was a problem hiding this comment.
can you mention setp 6 in here
There was a problem hiding this comment.
what should happen if we are past step 6, shouldn't we just do the clean ups?
There was a problem hiding this comment.
"cleanup" reverts back to the old keys. So no, once we have passed step 6, we are committed. We will try to remove the old keys. But if we fail before, they will be there still.
If we retry, we will reprovision again, because we are done with reprovision, all the keys new keys are in place and the old keys will just be ignored.
We could maybe add the primary key digest to the task maybe so that we can detect that we are done, and we jump to directly to the other cleanup (the clean up of old keys). We just have to verify the plainkey recovered recovered the new primary key, then we know we have switched.
There was a problem hiding this comment.
can you leave a comment about what happens now in that case and also a TODO
There was a problem hiding this comment.
I will also rename "cleanup" to "revertReprovisionAttempt" so it is a bit more clear.
| volumesAuth, | ||
| checkResult) | ||
|
|
||
| err = bootMakeRunnableReprovision( |
There was a problem hiding this comment.
does this do the right things with the auth value?
a. Create and persist a new storage primary key and endorsement key, if required.
b. Rotate the authorization value for the lockout hierarchy as described in Rotating the TPM lockout hierarchy authorization value.
b. Using the new lockout hierarchy authorization value, configure the dictionary attack parameters as configured by the provision operation.
c. Create a new NV counter index for PCR policy revocation, if required.
There was a problem hiding this comment.
It will call tpm provision function in partial mode. Which should do the 3 first points (at least the rotation and configuration of DA, I can double check the first point).
The nv counter is created when sealing objects if I remember correctly.
There was a problem hiding this comment.
EnsureProvisioned is the method that sets up the lockout hierarchy, DA, as well as the SRK and EK. So the 3 first points are done together in that function.
The last one we do it in overlord/fdestate/backend/seal.go. We just look up for a free index in a range, and use that for the calls to create new keys.
| return false, err | ||
| } | ||
|
|
||
| containerReader, err := container.OpenRead(ctx) |
There was a problem hiding this comment.
should we call Close on this later on?
| chg := st.NewChange(reprovisionKind, fmt.Sprintf("Reprovision")) | ||
| reprovisionTask := st.NewTask("fde-reprovision", fmt.Sprintf("Reprovision")) |
There was a problem hiding this comment.
Reprovision encrypted disks perhaps?
There was a problem hiding this comment.
should we stop starting a reprovision though if there is already one running?
There was a problem hiding this comment.
I am not totally sure what you meant. Should we refrain from starting a new reprovision? Or should we stop the previous ones?
There was a problem hiding this comment.
sorry, I meant make sure that there is at most one reprovision running, so return a conflict error if there is one still going and not yet finished
There was a problem hiding this comment.
I added a call to checkFDEChangeConflict which will prevent from a conflicting reprovision task.
|
|
||
| st := t.State() | ||
| st.Lock() | ||
| defer st.Unlock() |
There was a problem hiding this comment.
related to conflict and the fact that now this will not run concurrently to other seal/resealing tasks should we unlock during some of it?
There was a problem hiding this comment.
We should probably unlock while sealing yes. However, reprovision happens in very special circumstance. Resealing happens very often. So maybe we can wait and see if people complain that snapd is not responsive while they are repairing their installation. I do not expect anybody to care. Maybe I am wrong.
There was a problem hiding this comment.
I am also in favor of defensive state locking until someone complains.
|
|
||
| chg, err := devicestateReprovision(st) | ||
| if err != nil { | ||
| return BadRequest("unexpected error: %s", err) |
There was a problem hiding this comment.
we probably need to do proper error mapping as we might want to return conflicts error from Reprovision
ZeyadYasser
left a comment
ZeyadYasser
left a comment
There was a problem hiding this comment.
did a pass, I will try to do another pass tomorrow morning on the handler code
| sbSetProtectorKeys(protectorKey) | ||
| defer sbSetProtectorKeys() |
There was a problem hiding this comment.
as this is global state in secboot, this needs to be guarded with an external lock. is this usually called with the state lock held? maybe updating the doc comment to reflect this to callers is enough.
There was a problem hiding this comment.
I think we should change that in secboot. I do not think it makes sense to have it as a global state.
ZeyadYasser
left a comment
ZeyadYasser
left a comment
There was a problem hiding this comment.
did a pass over the re-provision handler
| return fmt.Errorf("multiple containers found with role system-data") | ||
| } | ||
| dataDisk = disk | ||
| case "system-save": |
There was a problem hiding this comment.
can we add a TODO comment that if we later support other containers (e.g. user containers) this would need to be updated
There was a problem hiding this comment.
Are we going to use the same keys for user containers (specifically, the same recovery key)? This can probably be reprovisioned separately.
There was a problem hiding this comment.
Also there is a big difference between a disk where we take system configuration from and a disk we do not. Because for the system ones we need lock them with PCRs then to specific context within the initrd. Whereas non system disks do not have the same restrictions. Which means we probably do not need the same pcr profiles.
What do you have in mind with user containers?
There was a problem hiding this comment.
I don't think there are any designs for user containers, but I remember it was something that might be considered in the future.
| continue | ||
| } | ||
| // This rename replaces the rename.from if it exists | ||
| if err := secbootRenameContainerKey(disk, rename.to, rename.from); err != nil { |
There was a problem hiding this comment.
wouldn't this fail if rename.to and rename.from both exist?
There was a problem hiding this comment.
We only log a warning.
There was a problem hiding this comment.
Let me double check. I think it should be just overwriting
There was a problem hiding this comment.
You are right. I will have to fix that.
There was a problem hiding this comment.
Should be fixed now.
| if rename.from == "default" && disk == saveDisk.DevPath() { | ||
| continue | ||
| } | ||
| // This rename replaces the rename.from if it exists |
There was a problem hiding this comment.
why are we not removing leftover keyslots before renaming?
There was a problem hiding this comment.
Should be fixed now.
| // Step 1. rename existing keyslots that we will overwrite | ||
| for _, rename := range renames { | ||
| for _, disk := range []string{dataDisk.DevPath(), saveDisk.DevPath()} { | ||
| if err := secbootRenameContainerKey(disk, rename.from, rename.to); err != nil { |
There was a problem hiding this comment.
instead of ignoring failures due to potentially missing keyslots due to a re-run, you could obtain the current subset of keyslots given a set using m.GetKeyslots(keyslotRefs), this way we could simple hard error here.
something like:
snapd/overlord/fdestate/handlers.go
Lines 230 to 236 in e556fea
There was a problem hiding this comment.
That is a "time of check to time of use" pattern. I prefer to try and fail.
There was a problem hiding this comment.
Sorry... try and warn.
There was a problem hiding this comment.
Alright, I think I see the potential issues here.
There was a problem hiding this comment.
I now check for existing keys, and skip rename if it did not exist.
| for _, rename := range renames { | ||
| for _, disk := range []string{dataDisk.DevPath(), saveDisk.DevPath()} { | ||
| if err := secbootRenameContainerKey(disk, rename.from, rename.to); err != nil { | ||
| logger.Noticef("WARNING: could not rename %s to %s on %s", rename.from, rename.to, disk) |
There was a problem hiding this comment.
I wrote a comment #17034 (comment), we already have a mechanism for detecting the target keyslots to be robust against re-runs.
| if err := secbootSaveCheckResult(setupData.checkContext, device.PreinstallCheckResultUnder(dirs.SnapSaveDir)); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
This alters reality a bit, "preinstall" file means something else now after reprovisioning. I don't have strong opinions here but might be worth writing a comment highlighting this fact.
There was a problem hiding this comment.
Though "preinstall" is not correct for that file name. It is more of "run check context". We cannot really rename that file. I will comment it.
There was a problem hiding this comment.
I added a comment in gadget/device/encrypt.go
In general, we should do a refactoring all the "preinstall" and rename most of them. But that is going to be a big refactoring-only PR.
|
|
||
| st := t.State() | ||
| st.Lock() | ||
| defer st.Unlock() |
There was a problem hiding this comment.
I am also in favor of defensive state locking until someone complains.
| return fmt.Errorf("multiple containers found with role system-data") | ||
| } | ||
| dataDisk = disk | ||
| case "system-save": |
There was a problem hiding this comment.
I don't think there are any designs for user containers, but I remember it was something that might be considered in the future.
| continue | ||
| } | ||
| if err := secbootDeleteContainerKey(disk, rename.to); err != nil { | ||
| logger.Debugf("could remove %s on %s", rename.to, disk) |
There was a problem hiding this comment.
| logger.Debugf("could remove %s on %s", rename.to, disk) | |
| logger.Debugf("could not remove %s on %s", rename.to, disk) |
There was a problem hiding this comment.
nitpick, Usually errors are prefixed with cannot ....
| } | ||
| for _, rename := range renames { | ||
| if hasPlatformKeyslot[rename.to] && hasPlatformKeyslot[rename.from] { | ||
| nv, err := secbootGetPCRHandleFromToken(disk, rename.from) |
There was a problem hiding this comment.
step 1 says erasing of NV index should be done for the new keys, I am confused why rename.from is passed.
1. Erase the nv indices from new keys that were not usedThere was a problem hiding this comment.
During revert cleanup "from" and "to" are inverted. Maybe I should call them "newName" and "oldName".
| if err := secbootDeleteContainerKey(disk, rename.to); err != nil { | ||
| logger.Debugf("could remove %s on %s", rename.to, disk) | ||
| } | ||
| if err := secbootRenameContainerKey(disk, rename.to, rename.from); err != nil { |
There was a problem hiding this comment.
This will now always fail since rename.to is deleted above, right?
There was a problem hiding this comment.
Oops, yes, we should remove "rename.from" (the new key).
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
some minor things, I still need to re-review the main task handler
| return nil, err | ||
| } | ||
|
|
||
| chg := st.NewChange(reprovisionKind, fmt.Sprintf("Reprovision")) |
There was a problem hiding this comment.
nitpick: the summary should be the same as the task I think
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
did another pass, minor things plus test questions, do we need test that run doReprovision twice one after another for various scenarios in the first of the two?
|
|
||
| st := t.State() | ||
| st.Lock() | ||
| defer st.Unlock() |
| } | ||
| } | ||
| } | ||
| // This one always need to be the last one in case we crash |
There was a problem hiding this comment.
// the save disk default key needs to be handled last outside the loop in case we crash
perhaps
| // For example due to previous run not cleaned up. | ||
| // We know it is not a key that is still in use because we | ||
| // checked snapd-reprovision-default previously. | ||
| logger.Debugf("cannot delete %s on %s: %v", rename.old, disk, err) |
There was a problem hiding this comment.
can't we ue the known keys information to produce slightly more precise logs?
| } | ||
|
|
||
| if key == "snapd-reprovision-default" && disk == saveDisk.DevPath() { | ||
| // always the last one to remove |
| continue | ||
| } | ||
| if err := secbootDeleteContainerKey(disk, rename.new); err != nil { | ||
| logger.Debugf("cannot remove %s on %s", rename.old, disk) |
There was a problem hiding this comment.
this should say rename.new?
| // remediate the issues. | ||
| func Reprovision(st *state.State) (*state.Change, error) { | ||
| if err := fdestate.CheckFDEChangeConflict(st); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
also a test getting here, and also checking the reverse case of something else against reprovision being present, unless that's covered in fdestate
There was a problem hiding this comment.
It is also tested in overlord/fdestate/conflict_test.go.
| defer st.Lock() | ||
| return devicestate.DoReprovision(s.mgr, t) | ||
| }() | ||
| c.Assert(err, ErrorMatches, "protector key failed") |
There was a problem hiding this comment.
shouldn't we check dataKeys and saveKeys here and in similar error tests?
There was a problem hiding this comment.
I have refactored to make the check for rollback a function and added it everywhere where it fails.
| defer devicestate.MockBootMakeRunnableReprovision(func(model *asserts.Model, protector secboot.KeyProtectorFactory, encryption *boot.EncryptionSetup) error { | ||
| bootMakeRunnableReprovisionCalls++ | ||
|
|
||
| // TODO: check primary key |
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
looking good up to the known TODOs, one question about reverts
|
|
||
| // Step 6. write the protector key | ||
| if err := keysSaveProtectorKey(protectorKey, saveKeyPath); err != nil { | ||
| return fmt.Errorf("cannot save the system-save key: %v", err) |
There was a problem hiding this comment.
@valentindavid if we fail here we will still revert but we will need to reboot/restart to recreate the state that we lost above, should we save the state when nil, to put it back if we hit this case?
ZeyadYasser
left a comment
ZeyadYasser
left a comment
There was a problem hiding this comment.
Overall looks really good, Thank you!
My only concerns are about cleanup of recovery-keys and edge-case of double re-runs.
| if oldKeyMatches { | ||
| // We must have been restarted in the middle, before step 6. | ||
| // The backed up keys are the correct ones, we need to cancel that previous | ||
| // attempt and them back to expected name. |
There was a problem hiding this comment.
| // attempt and them back to expected name. | |
| // attempt and then go back to expected name. |
| {"default-recovery", "snapd-reprovision-default-recovery"}, | ||
| } | ||
|
|
||
| // TODO: we show allow unlocking during sealing since hashing |
There was a problem hiding this comment.
| // TODO: we show allow unlocking during sealing since hashing | |
| // TODO: we should allow unlocking during sealing since hashing |
b6782d5 to
ea75e9c
Compare
github-actions
Bot
added
the
Auto rerun spread
9a17d26 to
484b15e
Compare
484b15e to
c29b9b0
Compare
|
Tests failing here are also failing in other prs. No new errors introduced. |
4 participants
No description provided.