-
Notifications
You must be signed in to change notification settings - Fork 45
[reconfigurator] RoT planner support #8421
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
base: main
Are you sure you want to change the base?
Conversation
dev-tools/reconfigurator-cli/tests/output/target-release-stdout
Outdated
Show resolved
Hide resolved
@davepacheco just as an FYI this is what I have for the RoT planner support so far. It's not finished yet, but the main bulk of it is done. I just need to refine some bits and add more tests. A big chunk of the lines of code are generated test files btw. |
I'm not sure how soon we're planning to demo this as part of the update bring-up. As you all know, I'll be out next week. So, if you'd like to use this as part of the demo soon, please feel free to modify/merge/close/superseed this PR as you all see fit, if this is something you'd like to do to speed up things 😄 |
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.
#8478 may change the way this test works if implemented before I merge this PR
Heya! 👋 Just a tiny ping that this is ready to review :) |
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.
Nice! This is looking good but there are a few details to nail down.
dev-tools/reconfigurator-cli/tests/output/target-release-stdout
Outdated
Show resolved
Hide resolved
let found_active_version = | ||
ArtifactVersion::new(active_caboose.caboose.version.clone()) | ||
.map_err(|e| { | ||
MgsUpdateStatusError::FailedArtifactVersionParse(e) | ||
})?; |
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 isn't very important but it looks like for the SP case we just operated on strings without assuming we could parse them. Do we need to parse the RoT versions?
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.
Just to populate ExpectedActiveRotSlot
. I thought it'd be cleaner to use the same type as
expected_active_slot
since it is a struct containing two fields rather than just a version or a slot.
Don't have a super strong opinion about this though. Happy to change if you think what I did doesn't make sense
// - "name" matching the board name (found above from caboose) | ||
// - "kind" matching one of the known RoT kinds |
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.
For the RoT, don't we need to check that it's signed with the same key (SIGN
in caboose matches something in the metadata)?
I think the corresponding Wicket code path uses a common verify
function to do this (sorry, I don't have the reference handy at the moment). I wonder if we need to be using the same thing. (I wouldn't say we necessarily do. We already duplicate some of the Wicket behavior because the shape of the code is different. But we should at least make sure it covers the same logic, and if it's non-trivial, think about commonizing it.)
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.
Ah, aha! I took a look at the code and I think you mean I need to verify the CMPA/CFPA pages?
omicron/wicketd/src/update_tracker.rs
Lines 2027 to 2036 in b7b24d5
/// Via `client`, ask the target RoT for its CMPA/CFPA pages, then loop | |
/// through our `available_artifacts` to find one that verifies. | |
/// | |
/// For backwards compatibility with RoTs that do not know how to return | |
/// their CMPA/CFPA pages, if we fail to fetch them _and_ | |
/// `available_artifacts` has exactly one item, we will return that one | |
/// item. | |
/// | |
/// This is also applicable to the RoT bootloader which follows the | |
/// same vaildation method |
I couldn't really find anything about checking SIGN. Unless I'm looking in the wrong place?
To do this I'll need to make API calls directly to MGS, this isn't done in the planner yet. I'm assuming it's OK to do so.
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.
To do this I'll need to make API calls directly to MGS, this isn't done in the planner yet. I'm assuming it's OK to do so.
I think we should strongly prefer not doing so; the planner currently operates as essentially a pure function of its inputs, and it'd be very nice to not lose that. I think in this case, the latest inventory may already have what you need: inventory collections include CMPA/CFPA pages. If we need more than that from MGS, can we add it to inventory instead of making the planner ask directly?
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.
I think the verification against CMPA/CFPA is achieving the same thing as the signature check I was thinking of. Agreed with John here -- we should already have this in inventory and if we don't then we should add it.
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.
Got it! I almost went in an unnecessary rabbit hole there 😅
I took a look at both options:
- Verifying the
CMPA/CFPA
the same way wicket does would be kind of complicated because, we're only using the Tuf repo metadata here, so I can't really create aRawHubrisArchive
to verify against without massively changing the way we handle artifacts. - I would love to use the option of making sure
SIGN
in caboose matches something in the metadata. But I don't really know what to match it against lol. There's only the artifact's hash in the metadata, but that won't be the same. Am I missing something really obvious here?
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.
I would love to use the option of making sure SIGN in caboose matches something in the metadata. But I don't really know what to match it against lol. There's only the artifact's hash in the metadata, but that won't be the same. Am I missing something really obvious here?
I doubt there is a straightforward way to verify the signature in Omicron. It's not a comparison. You'd need to hash the image and then verify it given some RSA public key. Personally, I wouldn't worry about it right now.
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.
We chatted about this in the update watercooler. We don't want to pass the entire artifacts to the planner and came up with a couple of possible paths forward:
- The planner could give a list of all the possible artifacts to the executor, and the executor verifies the artifacts the same way wicket does today.
- We somehow extract this information from an artifact before, and include it as part of the TUF metadata.
I'll catch up with @plotnick next week to take a swing at option 2 which is much nicer.
artifact_version: artifact.id.version.clone(), | ||
}) | ||
} | ||
|
||
#[cfg(test)] | ||
mod test { |
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.
I think we could augment a lot of the unit tests here and add new ones to cover the new RoT logic.
This commit implements support for RoT updates in the planner and support for reconfigurator-cli updates as well.
TODO: