tool: drop buggy condition for when to sign

[Myaats]

Fixes #140

My theory is that from the last tool update sbattach made removing the image signature from the signed image during the resign test match the initial input. This likely caused the condition at https://github.com/nix-community/lanzaboote/blob/master/rust/tool/src/install.rs#L440 to return false for what I assume is the first time.

I am a bit confused for why this condition exists as it should in theory only return false if the EFI file matches the unsigned input unless I have missed something. The only way to implement that check I can think off is to verify it has been correctly signed, remove the signature table and then compare with the input, at which point it becomes easier to just always sign.

[RaitoBezarius]

Fixes #140

My theory is that from the last tool update sbattach made removing the image signature from the signed image during the resign test match the initial input. This likely caused the condition at https://github.com/nix-community/lanzaboote/blob/master/rust/tool/src/install.rs#L440 to return false for what I assume is the first time.

I am a bit confused for why this condition exists as it should in theory only return false if the EFI file matches the unsigned input unless I have missed something. The only way to implement that check I can think off is to verify it has been correctly signed, remove the signature table and then compare with the input, at which point it becomes easier to just always sign.

Hmmm, I'm not exactly sure about this change, @nikstur is the original author who may have the full context in his head.

Here's how I read it.

(1) If a newer systemd boot is available or systemd-boot is not signed
(2) If systemd-boot does not exist at the target location or the hash differs (e.g. it's unsigned for example)
(3) Forcibly sign and install systemd-boot

In all the cases, the test failing should probably not fixed by getting rid of condition (2) IMHO, but by fixing the non-idempotency of sbattach for example.

[Myaats]

Here's how I read it.

(1) If a newer systemd boot is available or systemd-boot is not signed (2) If systemd-boot does not exist at the target location or the hash differs (e.g. it's unsigned for example) (3) Forcibly sign and install systemd-boot

The condition compared the hash of the unsigned input with the signed systemd-boot image/generation image in the "ESP". This means only an identical hash will end up as false, and has likely never happened until removing the signature made the previously signed image identical with the unsigned input. This made lanzaboote not resign it in the test and failing the verify assert.

In all the cases, the test failing should probably not fixed by getting rid of condition (2) IMHO, but by fixing the non-idempotency of sbattach for example.

This was likely caused by sbattach fully stripping the signature table returning the signed image to be identical with the previous unsigned image used as input skipping signing. I'd argue it is the condition that does not make sense as removing it will not reduce the amount of times lanzaboote signs files as already signed images will always mismatch the unsigned image. However removing it will remove this single edge case of comparing the unsigned image with an identical unsigned image in the ESP partition.

I do not see any better way to solve this (input welcome!) as a correct condition would have to check either of these:

• (1) The image itself (unsigned) is not identical
• (2) The image has not been signed by the correct key

Checking these two would not necessarily be faster than just always resigning.

[RaitoBezarius]

I see your point. Ugh, this is not ideal.

Thanksfully, with the help of @baloo we got into goblin support for computing Authentihashes and reading the certificate table.

So theoretically, it should be possible to implement (1) & (2).

Give me a bit of time rereading properly again the whole condition logic, I would appreciate if @nikstur could double check this PR as it's not totally my code.

Otherwise, I feel like we can just merge it and move forward ideally with a proper implementation later.

[nikstur]

Please don't merge this before I have the time to look over it. I should find some time next week.

[Myaats]

I can change the patch to just comment out the condition with a TODO comment and explanation instead of dropping the other function. This may it easier to review and merge as a stop-gap. I have read through the signing code and I am certain the condition never did what it was meant to in the first place and what was actually checked was not uncovered until the last sbsigntool update broke its assumptions.

My question is should the entire check just be dropped as so far it has always signed all existing images. This may be negative for systems with low write endurance e.g. eMMC-based storage, but it also always guaranteed you have signed EFI images from the latest version of the tools involved (including lanzaboote). Hence ensuring determinism in a way.

If a proper check is added this guarantee may be hard to continue as the signing tool, lanzaboote version and settings should all be considered inputs to the ideally deterministic signing process and would all need to be checked to not break peoples assumptions when configuring lanzaboote from their nix configurations in my opinion.

[blitz]

@nikstur What is a good stop-gap solution here? This is breaking people's update process right now.

[blitz]

@nikstur What is a good stop-gap solution here? This is breaking people's update process right now.

Mmh. This PR fails the integration tests when I update dependencies. This may or may not be a better situation than failing to build in the first place. ;) I'll have to look into this in more detail. I should have a solution here in the coming days. Sorry for the long delay.

[Myaats]

Mmh. This PR fails the integration tests when I update dependencies. This may or may not be a better situation than failing to build in the first place. ;) I'll have to look into this in more detail. I should have a solution here in the coming days. Sorry for the long delay.

Able to post the error? I just updated all the inputs for the flake locally and ran the tool tests again, and everything still passes here. It definitely fixed the issue of lanzaboote not signing images if the unsigned image matches the image in the ESP.

[blitz]

The tests do succeed if I only update the nixpkgs input. I guess the other error I saw is unrelated.

[blitz]

@nikstur Can you check whether this change was sensible? I merged it, because @Myaats argument made sense to me, the code passed the tests and because it blocked updates.

[nikstur]

I think this is sensible. @Myaats thank you for the analysis and the fix! I image this was frustrating to track down and has taken quite some time.