Allow multiple RoT versions in the same repository #6586

labbott · 2024-09-17T13:23:37Z

No description provided.

Currently we require all versions of the RoT and bootloader for a particular type (sled, psc, switch) to have identical versions. This is a stricter requirement than what we actually care about: All images signed with the same key should be the same version. Relax this requirement for both the RoT and its bootloader

labbott · 2024-09-19T13:26:02Z

This is a re-application of #5580 which was reverted because it was only a partial fix and #5851 which never actually merged . The final pieces on top of that are some refactoring in update_tracker.rs to actually choose the right archive and some extra steps for bootloaders

jgallagher · 2024-09-19T14:55:47Z

sp-sim/src/gimlet.rs

@@ -1351,12 +1351,16 @@ impl SpHandler for Handler {
            (SpComponent::ROT, b"NAME", _, _) => ROT_NAME,
            (SpComponent::ROT, b"VERS", 0, _) => ROT_VERS0,
            (SpComponent::ROT, b"VERS", 1, _) => ROT_VERS1,
+            // gimlet staging/devel hash
+            (SpComponent::ROT, b"SIGN", _, _) => &"11594bb5548a757e918e6fe056e2ad9e084297c9555417a025d8788eacf55daf".as_bytes(),


Will we need to keep these hashes in sync with upstream changes over time? ("need" here may not be the right word since this is the simulator and not relevant to production, but maybe it could affect a4x2 or other test envs in some way?)

I'm not sure what kind of upstream changes you are thinking of. This hash should never change as it's the same one baked into the CMPA at manufacturing time. We could change it to simulate another set of signing keys.

I wasn't thinking of anything specific - mostly naivety! Just to confirm my understanding - the only way this changes is if we rotate our root manufacturing key?

jgallagher · 2024-09-19T14:58:13Z

wicket/src/events.rs

@@ -32,7 +32,7 @@ pub enum Event {
    /// TUF repo artifacts unpacked by wicketd, and event reports
    ArtifactsAndEventReports {
        system_version: Option<SemverVersion>,
-        artifacts: Vec<ArtifactId>,
+        artifacts: Vec<(ArtifactId, Option<Vec<u8>>)>,


What is the Option<Vec<u8>>? Maybe worth converting this from a tuple to a struct so we can give it a name?

The option is the sign vec. You are right that this should be clarified.

jgallagher · 2024-09-19T14:59:15Z

wicket/src/state/inventory.rs

+    // of the bootloader is going to be identical to the RoT.
+    pub fn rot_sign(&self) -> Option<Vec<u8>> {
+        match self.rot_active_slot() {
+            None => return None,


Tiny nit - I think we could remove the outer match via

match self.rot_active_slot()? { RotSlot::A => ...

jgallagher · 2024-09-19T15:02:00Z

wicket/src/state/inventory.rs

@@ -138,6 +138,13 @@ fn version_or_unknown(caboose: Option<&SpComponentCaboose>) -> String {
    caboose.map(|c| c.version.as_str()).unwrap_or("UNKNOWN").to_string()
 }

+fn caboose_sign(caboose: Option<&SpComponentCaboose>) -> Option<Vec<u8>> {
+    match caboose {
+        None => None,


Style nit - I think I'd make this function take a non-optional &SpComponentCaboose, since we can't do anything useful with None. That would also make it clear from the signature that even from a non-optional caboose, we may or may not have a sign value.

jgallagher · 2024-09-19T15:05:07Z

wicket/src/state/update.rs

+                    .entry(known)
+                    .and_modify(|x| x.push((id.version.clone(), s.clone())))
+                    .or_insert(vec![(id.version.clone(), s.clone())]);


Nit - I think this would be equivalent:

Suggested change

.entry(known)

.and_modify(|x| x.push((id.version.clone(), s.clone())))

.or_insert(vec![(id.version.clone(), s.clone())]);

.entry(known)

.or_default()

.push((id.version.clone(), s.clone()));

and not require repeating the tuple.

jgallagher · 2024-09-19T15:06:13Z

wicket/src/state/update.rs

@@ -21,12 +21,14 @@ use std::collections::BTreeMap;
 use std::fmt::Display;
 use wicketd_client::types::{ArtifactId, SemverVersion};

+type ArtifactVersions = Vec<(SemverVersion, Option<Vec<u8>>)>;


Should this be a map instead of a vec? Is it okay to have multiple identical versions with potentially-different Option<Vec<u8>> values?

I was trying to map the existing behavior of the map of artifact kind to one version to a map of artifact kind to a vec of all possible versions. I think this is the same issue with using a tuple instead of a named struct. I'll see if that makes it more obvious what's going on here.

jgallagher · 2024-09-19T15:07:08Z

wicket/src/ui/panes/overview.rs

@@ -1130,7 +1130,9 @@ fn append_caboose(
        git_commit,
        // Currently `name` is always the same as `board`, so we'll skip it.


Is this comment still accurate? I know it's true for production images, but could this bit of code see non-prod builds with different names?

I think you are correct and it could be helpful to confirm this via wicket.

jgallagher · 2024-09-19T15:12:46Z

wicket/src/ui/panes/update.rs

+            };
+            let cnt = v.len();
+            for (vers, sign) in v {
+                match (sign, component.rot_sign()) {


I don't understand the for/match here; it looks like it could return different values depending on the ordering of v, maybe? E.g., if v has two entries, one with a sign and one without, and component.rot_sign() is none, will we either return vers (line 2455) if the "without a sign" is first or UNKNOWN (MISSING SIGN) (line 2461) if the "with a sign" is first?

The assumption is that either all entries will have a SIGN or none of them. This should be checked elsewhere or handled/documented better. This is also related to the type you pointed out in the Vec which is very much a theme here and something I need to rework.

jgallagher · 2024-09-19T15:17:20Z

update-common/src/artifacts/update_plan.rs

+    let sign = match caboose.sign() {
+        Ok(sign) => sign,
+        Err(error) => {
+            return Err(RepositoryError::ReadHubrisCabooseBoard { id, error });


Should this be

Suggested change

return Err(RepositoryError::ReadHubrisCabooseBoard { id, error });

return Err(RepositoryError::ReadHubrisCabooseSign { id, error });

(or maybe something like ReadHubrisCabooseValue { id, error, key: "sign" })?

jgallagher · 2024-09-19T15:20:19Z

wicketd/src/update_tracker.rs

-                available_artifacts,
-                available_artifacts_version,
+                //available_artifacts,
+                //available_artifacts_version,


Couple leftover dead fields here

labbott marked this pull request as draft September 17, 2024 13:23

labbott force-pushed the multi_rot_drifting branch from 3749ff7 to 1497b2f Compare September 18, 2024 20:42

jgallagher reviewed Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple RoT versions in the same repository #6586

Allow multiple RoT versions in the same repository #6586

labbott commented Sep 17, 2024

labbott commented Sep 19, 2024

jgallagher Sep 19, 2024

labbott Sep 19, 2024

jgallagher Sep 19, 2024

jgallagher Sep 19, 2024

labbott Sep 19, 2024

jgallagher Sep 19, 2024

jgallagher Sep 19, 2024

jgallagher Sep 19, 2024

jgallagher Sep 19, 2024

labbott Sep 19, 2024

jgallagher Sep 19, 2024

labbott Sep 19, 2024

jgallagher Sep 19, 2024

labbott Sep 19, 2024

jgallagher Sep 19, 2024

jgallagher Sep 19, 2024

		@@ -1130,7 +1130,9 @@ fn append_caboose(
		git_commit,
		// Currently `name` is always the same as `board`, so we'll skip it.

	return Err(RepositoryError::ReadHubrisCabooseBoard { id, error });
	return Err(RepositoryError::ReadHubrisCabooseSign { id, error });

Allow multiple RoT versions in the same repository #6586

Are you sure you want to change the base?

Allow multiple RoT versions in the same repository #6586

Conversation

labbott commented Sep 17, 2024

labbott commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment