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

flash-gui.sh: Extend NPF archive format to ZIP, improve workflow #1526

Merged
merged 8 commits into from
Nov 17, 2023

Conversation

JonathonHall-Purism
Copy link
Collaborator

The NPF file extension can be configured on a per-board basis, default is ZIP. Fixed some issues in the implementation (don't require /tmp/verified_rom/ in sha256sum.txt, show integrity error instead of dying if ZIP itself is corrupt as that is more likely, etc.), details in commit messages.

Update package is built in default target now, removed create_npf.sh.

@daringer This should still be compatible with any npf archives already published (/tmp/verified_rom/ prefix is removed if present). Do you want export CONFIG_BRAND_UPDATE_PKG_EXT=npf added to your boards?

@tlaurion I refactored prompts a little so .tgz only appears for talos-2 (and .rom does not appear on talos-2). I can't really do any further refactoring to better integrate talos-2 with the zip method. If you want to improve anything there I'm happy to review/discuss, we could also merge this and improve later as this is better than the current master for all boards.

So help me, I added a confirmation prompt 😱 zip/npf somehow ended up without one, went straight to flash after the file selector. It's just too easy IMO to accidentally select the wrong file, and flashing is sensitive/irreversible. I brought back the prompt message from before the NPF introduction.

The prompt after rebooting now shows the file that was selected, not the file that was flashed (which is some temp file when selecting an update package).

The .rom prompt is unchanged, I expect to carry a patch downstream in PureBoot for a while softening the message there until we are confident that .zip support has rolled out widely enough to switch to it.

Allow configuring the ZIP-format update file extension with
CONFIG_BRAND_UPDATE_PKG_EXT in board config.  Default is 'zip'.

Create update package in the default Makefile target.  Delete
create_npf.sh.

Do not require /tmp/verified_rom in the update file package's
sha256sum.txt (but allow it for backward compatibility).

Show the integrity error if unzip fails instead of dying (which returns
to main menu with no explanation, error is left on recovery console).
This is the most likely way corruption would be detected as ZIP has
CRCs.  The sha256sum is still present for more robust detection.

Don't require the ROM to be the first file in sha256sum.txt since it
raises complexity of adding more files to the update archive in the
future.  Instead require that the package contains exactly one file
matching '*.rom'.

Restore confirmation prompt for the update-package flow, at some point
this was lost.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
talos-2 (only) uses .tgz instead of .rom for updates.  Currently, both
are treated as alternatives to a ZIP-format update archive with
SHA-256 integrity check, extend that to the prompts to reduce clutter.

Reflow the "You will need ... your BIOS image" prompt to fit on
fbwhiptail.

The .tgz format could be better integrated with the ZIP updates, but
this needs more work specific to talos-2.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
The only purpose of legacy flash boards is to be flashed over vendor
firmware using an exploit, to then flash non-maximized Heads firmware.

They are never upgraded to another legacy flash build, and they move
the coreboot ROM from the build directory, so don't build an update
package for those boards.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
@daringer
Copy link
Collaborator

@daringer This should still be compatible with any npf archives already published (/tmp/verified_rom/ prefix is removed if present). Do you want export CONFIG_BRAND_UPDATE_PKG_EXT=npf added to your boards?

no, don't think this is useful for upstream - this will only confuse people, we'll do this in our releases thanks a lot for adding it and all the improvements

@JonathonHall-Purism
Copy link
Collaborator Author

@tlaurion Per our discussion, legacy-flash boards no longer try to create an update package, using CONFIG_LEGACY_FLASH=y. As I mentioned in Makefile that config is intended for the various behaviors specific to legacy-flash boards only and will facilitate cleaning those behaviors out if we remove the legacy-flash boards.

Will watch for CI to finish in case there are further issues 👍

@tlaurion
Copy link
Collaborator

tlaurion commented Nov 14, 2023

Building and testing here locally for flash/reflash : perfect for standard boards (x230 tested with rom then zip).

Makefile Show resolved Hide resolved
Only add the update package to all if it is actually being built, fixes
default target with CONFIG_LEGACY_FLASH=y.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
@tlaurion
Copy link
Collaborator

tlaurion commented Nov 14, 2023

@daringer This should still be compatible with any npf archives already published (/tmp/verified_rom/ prefix is removed if present). Do you want export CONFIG_BRAND_UPDATE_PKG_EXT=npf added to your boards?

no, don't think this is useful for upstream - this will only confuse people, we'll do this in our releases thanks a lot for adding it and all the improvements

@daringer you sure? I would have thought that nitrokey would want npf files proposed to heads/nitrokey downloaders instead of zip files for their boards, built that way through Heads or released from Nitrokey. Otherwise as this PR, zip files will be proposed to them from Heads, and npf from Nitrokey releases later on? This would be confusing and creating a wall between the two projects, breaking interoperability and users willing to switch from one rom to another between Nitrokey releases.

As this PR, upstream will propose the following, including for nitrokey boards:
20231114_133956_8955408646211444385

or

signal-2023-11-14-135949

AND produce zip files for download; not npf files

@tlaurion
Copy link
Collaborator

@JonathonHall-Purism
Unfortunately, the flow is broken for Talos-2 here since tgz is considered a "plain" rom from flash-gui.sh, where logic for validation happens under flash.sh

signal-2023-11-14-151718
signal-2023-11-14-151745

@JonathonHall-Purism
Copy link
Collaborator Author

@JonathonHall-Purism Unfortunately, the flow is broken for Talos-2 here since tgz is considered a "plain" rom from flash-gui.sh, where logic for validation happens under flash.sh

We discussed this, this issue is in master (from #1485). In retrospect, when we discovered the issues in these changes we probably should have backed it out so it could be fixed properly rather than rushing in more changes on top (#1514, which when through several iterations, and now this PR).

I don't think a revert is necessarily needed at this point if this PR is mergeable. talos-2 has a regression originating from #1485 that still isn't fixed, but I'm not sure the impact is great enough to justify a revert going that far back.

@JonathonHall-Purism
Copy link
Collaborator Author

The last talos-2 CI failure just failed to download a toolchain source archive (I touched Makefile so it invalidated the whole cache). I'll retry failed when the pipeline completes

talos-2 builds its own tgz update package that is not currently
integrated with the zip method.  While the zip method right now would
theoretically if the tgz was inside it, this would have to be hooked
up for talos-2 specifically.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
@JonathonHall-Purism
Copy link
Collaborator Author

Had to push something to trigger a new CircleCI pipeline, so re-signed the last commit and force pushed.

I guess CircleCI changed they way there Github integration worked, I had to create a new account, which means I couldn't access the old pipeline to retry it. Had to set it up again and trigger a new pipeline.

@daringer
Copy link
Collaborator

daringer commented Nov 15, 2023

The question is maybe to be asked from another direction: Should HEADS maybe accept both npf & zip as provided file, this would reduce migration issues.

Generally, I see no issue in migrating to .zip also mainly to be consistent with upstream - accepting both might just reduce pain here for the users.

Changed my mind ;) let's just support .zip upstream, would even vote against the config value - fully supporting a consistent way...

tlaurion added a commit to tlaurion/heads that referenced this pull request Nov 15, 2023
…sh prompt when talos-2 ato validate hashes against hashes provided under tgz through flash.sh validation (still offer zip and tgz, which tgz might change to zip later but only tgz offered through builds)

Attempt to address linuxboot#1526 (comment)

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Nov 15, 2023
…h-gui.sh Bypass flash-gui.sh prompt and use flash.sh to validate sha256sum.txt (still offer zip and tgz, which tgz might change to zip later but only tgz offered through builds atm)

Attempt to address linuxboot#1526 (comment)

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
@tlaurion
Copy link
Collaborator

tlaurion commented Nov 15, 2023

Ok so with b8428f3 talos flashed through bmc and then attemtping upgrades from usb tgz:

signal-2023-11-15-105153
signal-2023-11-15-105232
signal-2023-11-15-105343

NOTE: @krystian-hebel the change in how the board builds tgz changed. hashes.txt is now sha256sum.txt, which breaks backward compatibility for tgz creation and verification from that commit on.

Considering anybody using Heads currently(?) on Talos-2 is probably flashing through BMC per official firmware upgrade instructions (which do not state how to upgrade internally anyway), I think this change to unify rom hashing filename with other boards is a good step to do now so that further hashing files (internal initrd scripts, built tools/libraries/kernel modules etc or higher verification of cbfs content in goal of forward sealing) arrives in a good time, which is prior of official instructions promoting internal upgrades.

So once someone uses b8428f3 or later and attempts to flash internally, this will be the flow they will get.

tlaurion added a commit to tlaurion/heads that referenced this pull request Nov 15, 2023
…h-gui.sh Bypass flash-gui.sh prompt and use flash.sh to validate sha256sum.txt (still offer zip and tgz, which tgz might change to zip later but only tgz offered through builds atm)

Attempt to address linuxboot#1526 (comment)

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion added a commit to tlaurion/heads that referenced this pull request Nov 15, 2023
Add this to linuxboot#1526 since coreboot related buildstack files missing should not prevent future builds once a single successful build is supposed to create a cache containing those files having difficulties being downloaded
Signed-off-by: Thierry Laurion <insurgo@riseup.net>
tlaurion and others added 2 commits November 16, 2023 08:44
…sh prompt when talos-2 ato validate hashes against hashes provided under tgz through flash.sh validation (still offer zip and tgz, which tgz might change to zip later but only tgz offered through builds)

Attempt to address linuxboot#1526 (comment)

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
Nitrokey is going to switch from npf to zip per discussion.  Remove
this config.

Signed-off-by: Jonathon Hall <jonathon.hall@puri.sm>
@JonathonHall-Purism
Copy link
Collaborator Author

Thanks all 👍

@tlaurion Picked the fixes for talos-2. Per chat we'll hold off on the caching change for now.

@daringer Thanks for the consideration and keeping us informed, I've removed the brand config for the package extension, we'll use zip everywhere.

Tested this quickly on Librem 15v3 to confirm no regressions flashing .zip or .rom. If I can get CI to cooperate, this should be good to go.

I think CI will probably cooperate now - ftpmirror.gnu.org is back up, and I reluctantly gave CircleCI access to private repos too since there seems to be no other option. (I don't actually have any at the moment, but annoyed to give access that isn't needed...) I tried switching to the new "GitHub App" method but that had problems cloning dependencies that I did not want to troubleshoot, hopefully the OAuth method sticks around for a while. We'll see 🤞

Copy link
Collaborator

@tlaurion tlaurion left a comment

Choose a reason for hiding this comment

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

Tested extensively. We might want to readd confirmation for talos-2 later on, but at least this is coherent as is and put things in better state then master.

Let's merge and thank you for this!

@JonathonHall-Purism JonathonHall-Purism merged commit f5377b3 into linuxboot:master Nov 17, 2023
51 checks passed
tlaurion added a commit to tlaurion/heads that referenced this pull request Nov 17, 2023
…sh prompt when talos-2 ato validate hashes against hashes provided under tgz through flash.sh validation (still offer zip and tgz, which tgz might change to zip later but only tgz offered through builds)

Attempt to address linuxboot#1526 (comment)

Signed-off-by: Thierry Laurion <insurgo@riseup.net>
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.

None yet

3 participants