boot/grub.go: add new bootchain

[valentindavid]

We need to resolve the boot chains another place based on the trusted assets we encountered to be installed. At this point it could be any chain. We will need to discover later what the correct chain is.

Also make TrustedAssets return an unsorted data structure to make sure we do not use the order like the comments claimed.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 86.63102% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 78.88%. Comparing base (6a7ecfe) to head (25b4ef9).
Report is 76 commits behind head on master.

Files	Patch %	Lines
boot/seal.go	80.68%	11 Missing and 6 partials ⚠️
boot/assets.go	82.60%	5 Missing and 3 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #13412     +/-   ##
=========================================
  Coverage   78.88%   78.88%             
=========================================
  Files        1037     1040      +3     
  Lines      132757   133944   +1187     
=========================================
+ Hits       104722   105668    +946     
- Misses      21503    21675    +172     
- Partials     6532     6601     +69     

Flag	Coverage Δ
unittests	78.88% <86.63%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[valentindavid]

Needed for #13205

[valentindavid]

I have found a bug. We do not seem to remove files that should be removed. So moving from boot entry back to non boot entry (that is removing the fbx64.efi) would not work.

I wonder if we should add "removed" content to the gadget.yaml.

[pedronis]

did a pass, I'm slightly wary of the changes to assets.go

[valentindavid]

I had to rebase. So I cleaned up the history.

[pedronis]

thank you, mainly some comments/questions about using Ids as Paths in BootFiles

[pedronis]

is this not supported? will it be?

[valentindavid]

Oh! I am missing a fixme here.

I need to fix the deletion of gadget content that is not available in the next gadget. In case of downgrade, the "next gadget" should not have "fbx64.efi", so we should remove it, because it changes the behavior of shim.

I will do this fix in a different PR. But I will for sure comment here on why this does not work. And why it needs to work.

[valentindavid]

I have added the explanation.

[pedronis]

this is a bit strange given that BootFile has a still a field called Path, I thought we would keep the path as they were and use the TrustedAssets map to get to id to use in the cache

[valentindavid]

Thank you for the tip. I did not realize I could do that. I have fixed it.

[pedronis]

the comment should go away or be changed now

[pedronis]

why the []string? assetNames are now unique again and have one asset assigned to them?

[valentindavid]

I was verifying that we had only one later. But now I have rewritten it so that we verify it right away.

[pedronis]

buildBootAssets could take the TrustedAssets map and then BootFile could still carry paths?

[valentindavid]

Done

[valentindavid]

@olivercalder I have added you as reviewer. This has some changes from your branch to add the new boot chain. But it does not do anything with the variables yet. I will rebase your branch on top of this. I thought you would like to have a look.

[pedronis]

thank you, some detail comments on the last round of changes

[pedronis]

the comment should go away or be changed now

[pedronis]

should we call this ...AllBootChains to clarify what is about a bit more?

[valentindavid]

This tests the internal function runModeBootChains. This is why I named it like that. Function recoveryBootChainsForSystems is already tested by TestRecoveryBootChainsForSystems. And we were missing tests for runModeBootChains. Not sure what to name it if not TestRunModeBootChains.

[pedronis]

I see, do have higher level tests then that test the case where we need to consider both old and new grub assets locations? I saw only this one doing that but maybe I missed them

[valentindavid]

Right, I think we are missing a test for ResealKeyToModeenv with both chain present at the same time. We do test both. But separately.

[valentindavid]

I have modified TestResealKeyToModeenvWithSystemFallback to also cover cases where we upgrade or downgrade. This calls ResealKeyToModeenv which is higher level.

But that is starting to be complicated. Specially because of ordering and factorization of boot chains. There is lots of ways to express allowed boot chains. But we only test one, which is very hard to guess. I spend a day to make it work.

[pedronis]

we probably want to keep this comment before the log below?

[valentindavid]

Not all trusted assets will be present now. So I am thinking we should remove the notice. But the logic is still fine. If the asset is not in the cache, then we should ignore it.

[pedronis]

this is strange, it should either be a log or a panic, no?

[valentindavid]

That was some debugging that I committed by mistake.

[pedronis]

thank you, looking good, just some remarks about comments and a question about the test combinations #13412 (comment)

[pedronis]

s/names in/names recorded in/ is perhaps slightly clearer?

[pedronis]

s/names in/names recorded in/

[pedronis]

this probably merit a comment why the new logic is ok with not finding some asset chains

[valentindavid]

This one can only be generated by a broken bootloader. So it should be an error.

[alfonsosanchezbeato]

I have a question about the spread test

[alfonsosanchezbeato]

Shouldn't this work by removing the content of the EFI boot vars, as a temporary workaround? Or by removing the files under EFI/ubuntu/.

[valentindavid]

Well, we do not touch variables in this PR. This will come in the Oliver's PR. It is better to remove EFI/ubuntu and EFI/boot/fbx64.efi. If the variables are wrong, it will fallback anyway. But this is what I want installation of gadget to do. A "content" entry that disappears should imply removing the file.

[alfonsosanchezbeato]

But maybe still worth running this part of the test by removing "manually" the files, even while keeping the comment so when removing from snapd is implemented we can remove the workaround?

[valentindavid]

I will try preventing reboot with systemd-inhibit and see if I can remove the assets before reboot.

[alfonsosanchezbeato]

Right, it is true that being faster than the reboot can be a problem, so please ignore this if it gets too hacky. Approving now, thanks for the changes.

[alfonsosanchezbeato]

Nice! Maybe the comment above need to be slightly adapted now explaining the workaround.

[alfonsosanchezbeato]

super-nitpick: 2024 now 🙂

[alfonsosanchezbeato]

typo: s/the list load chains/the list of load chains/

[pedronis]

thank you, small issue with the error message also there seem to be formatting issues as the static checks are failing

[pedronis]

s/Asset/internal error: asset/ error always start with lowercase

[olivercalder]

Looks really good, thank you!

[olivercalder]

I don't believe relativeTarget is used anymore in this function?

[valentindavid]

Correct. Not easy to spot. I have removed it.

[olivercalder]

I think a comment here would be very useful to explain if it is expected behavior that there are multiple assets of the same name, and if not, why we use byAssetName instead of trustedAssets in the latter for loop. It seems to me that constructing and using byAssetName guarantees there are no reused asset names and is thus safer, but a comment to this effect would be useful. Something like "same as trustedAssets, but with uniqueness guarantee"

[valentindavid]

I changed this multiple times. So it seems in the end it was just about filtering duplicates. I will simplify the code here.

[valentindavid]

I have simplified the code.

[olivercalder]

I can't seem to find where TrustedAssetsList is actually defined outside of mocking, but perhaps s/TrustedAssetsList/TrustedAssetsMap/g, since it's now a map instead of a list?

[valentindavid]

Changed it and all the tests to use a map.

[olivercalder]

Why no tag?

[valentindavid]

Yes, I should may comment on it. The idea about using "asset names" instead of the path is because current implementation uses base names. So for backward compliance, we need to use no tag for all the paths that were used before. I will comment it.

[valentindavid]

Ah now I understand. fb was not an old one. I can put boot: for this one.

[olivercalder]

2024 here as well :)

[alfonsosanchezbeato]

LGTM

[pedronis]

thank you, one wondering

[pedronis]

I wonder as we are very much in control of this from the bootloader side, whether this should be an actual error? cc @alfonsosanchezbeato @bboozzoo

[alfonsosanchezbeato]

Probably this should return as an internal error, I agree

[olivercalder]

Looks great, thanks for taking on all of this!

[valentindavid]

Everything should be fixed. I will let the CI run and then autosquash it.