Skip to content
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

Implement optimization migrations #441

Merged
merged 10 commits into from
Apr 9, 2024
Merged

Implement optimization migrations #441

merged 10 commits into from
Apr 9, 2024

Conversation

@sosthene-nitrokey sosthene-nitrokey force-pushed the space-migrate branch 6 times, most recently from 3f1f3b8 to 760f2c3 Compare February 26, 2024 15:01
@sosthene-nitrokey sosthene-nitrokey force-pushed the space-migrate branch 2 times, most recently from 803653b to e16757b Compare March 7, 2024 14:44
@sosthene-nitrokey sosthene-nitrokey force-pushed the space-migrate branch 4 times, most recently from 1bbae18 to 420e168 Compare March 26, 2024 08:52
@nitrokey-ci
Copy link
Collaborator

nitrokey-ci commented Mar 26, 2024

No significant changes.

Insignifcant changes
metric value change
binary-size-nk3am 1194682 -2962 (-0.00%)
binary-size-nk3am-test 2018799 🔴 +17064 (+0.01%)
binary-size-nk3xn 429864 -1500 (-0.00%)
binary-size-nk3xn-test 546620 -884 (-0.00%)
binary-size-nkpk 731876 -5098 (-0.01%)

@sosthene-nitrokey
Copy link
Collaborator Author

I made the migrations optional for for the trussed-auth and trussed-se050-backend and disabled them in this PR. I also removed the FIDO migrations from this PR.

That way we can merge this without merging the migrations until the FIDO certification is done.

@sosthene-nitrokey sosthene-nitrokey marked this pull request as ready for review March 27, 2024 10:09
@sosthene-nitrokey sosthene-nitrokey force-pushed the space-migrate branch 2 times, most recently from f0921a7 to d39f02a Compare March 27, 2024 10:35
@robin-nitrokey
Copy link
Member

I will need some more time to properly review and think through the admin-app changes. IMHO we could merge a first version that updates trussed-auth, trussed-se050-backend, trussed and the affected dependencies but keeps the V0 version unconditionally as soon as the use_raw fields are replaced with a filesystem version . Then we could have a second PR that introduces the migrations and updates admin-app. This would make sure that we don’t have too many open PRs, can make a new release soon without pulling in the migrations and have enough time to review the migrations.

@sosthene-nitrokey
Copy link
Collaborator Author

Ok, i'll see if that's possible. Since admin-app is only a dependency of trussed-auth for the tests it might be fine.

@robin-nitrokey
Copy link
Member

Of course we can also keep it in one PR if that’s easier. I just wanted to mention that option because I think it should not be a big change and it would reduce the amount of unmerged changes.

@sosthene-nitrokey
Copy link
Collaborator Author

The trussed-auth PR bumps to v0.3.0 so it needs to update all the apps that depend on it anyways. So that would still be 2 huge PRs.

Copy link
Member

@robin-nitrokey robin-nitrokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could also use the FirmwareVersion from the migrated crates to select the executed migrations. Something along the lines of:

enum FilesystemLayout {
    V0,
    V1,
}

impl FilesystemLayout {
    fn trussed_auth(&self) -> trussed_auth::FilesystemLayout {
        match self {
            Self::V0 => trussed_auth::FilesystemLayout::V0,
            Self::V1 => trussed_auth::FilesystemLayout::V1,
        }
    }
}

// ...
    Migrator {
        migrate: |ifs, _efs| {
            trussed_auth::migrate::migrate_remove_dat(
                ifs,
                &[
                    path!("opcard"),
                    path!("webcrypt"),
                    path!("secrets"),
                    path!("piv"),
                ],
            )
        },
        enable: |old: FilesystemLayout, new: FilesystemLayout| {
            trussed_auth::migrate::enable_migration(old.trussed_auth(), new.trussed_auth())
        },
    },
// ...

This would reduce the number of magic constants in the runner and would keep the details about the migration inside the migrated crates.

Comment on lines 376 to 404
// App 2: secrets
#[cfg(feature = "secrets-app")]
apps.push(&mut self.oath).ok().unwrap();
if self.migrated_successfully {
// App 2: secrets
#[cfg(feature = "secrets-app")]
apps.push(&mut self.oath).ok().unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be clearer to skip creating the relevant apps if migration failed instead of skipping them in apdu_dispatch and ctaphid_dispatch, similar to what we do with opcard on a config error.

@@ -335,8 +350,18 @@ impl<R: Runner> Apps<R> {
version,
data.version_string,
data.status(),
migrations::MIGRATORS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to migrate on a config error? I would prefer to just trigger a migration error in that case and disable the affected apps.

components/apps/src/migrations.rs Outdated Show resolved Hide resolved
Comment on lines +6 to +15
pub(crate) const MIGRATION_VERSION_SPACE_EFFICIENCY: u32 = 1;

#[cfg(feature = "backend-auth")]
pub(crate) const TRUSSED_AUTH_FS_LAYOUT: trussed_auth::FilesystemLayout =
trussed_auth::FilesystemLayout::V1;
#[cfg(feature = "se050")]
pub(crate) const SE050_BACKEND_FS_LAYOUT: trussed_se050_backend::FilesystemLayout =
trussed_se050_backend::FilesystemLayout::V1;

pub(crate) const MIGRATORS: &[Migrator] = &[
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can just be pub instead of pub(crate) since the module is private

@sosthene-nitrokey sosthene-nitrokey mentioned this pull request Apr 4, 2024
4 tasks
@robin-nitrokey robin-nitrokey mentioned this pull request Apr 5, 2024
Copy link
Member

@robin-nitrokey robin-nitrokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@sosthene-nitrokey sosthene-nitrokey merged commit 88b46a2 into main Apr 9, 2024
9 checks passed
@sosthene-nitrokey sosthene-nitrokey deleted the space-migrate branch April 9, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants