contrib: add key/cert options and signature check for sbsign #695

Adito5393 · 2024-11-13T21:39:45Z

Closes #693

zdykstra · 2024-11-13T21:43:32Z

contrib/zbm-sign.pl

  } else {
    die "Sign method $SignMethod not valid.";
  }
-
-  if ( $DeleteUnsigned && $SignMethod eq "sbctl" ) {


Why was this whole block removed? It seems like it will fundamentally change the behavior of this script.

Really? It does only 1 of the 2 things (when DeleteUnsigned is enabled):

Print a message if sbctl is used

Clean up after the inability of sbsign to sign in place... with the new code, that is no longer the case

Maybe I should remove the option DeleteUnsigned since it is no longer needed.

What am I missing? I see this block of code as no longer needed.

Are you sure that sbsign won't corrupt its input if you try to overwrite it with output? Is that the case for several prior versions of sbsign?

If it's safe, the better approach would be to replace DeleteUnsigned with something a bit more meaningful like Overwrite and handle that above by just changing what you're passing as the output name to sbsign.

Then I propose UseOriginalFileName:

true -> *.efi = stays the same.

false (default) -> *.signed.efi

Or you can think of a better boolean var name?
And I will write the logics to always output to a different file. (solving above discussion)

Do you still want to offer the option, that is available for the sbsigntools method, to keep the original unsigned EFI files?
Since sbctl signs in place, why would we want to keep the unsigned EFI files?

I proposed, and still prefer, Overwrite, which is both shorter and more descriptive.

zdykstra · 2024-11-13T21:44:41Z

The commit message will also need to be formulated to be consistent with previous commits to this project.

Adito5393 · 2024-11-14T08:04:47Z

I did my best with the PR title. Feel free to edit it to match the project standards.

I have tested it on Debian Bookworm with sbsigntool version 0.9.4-3.1.

contrib/zbm-sign.pl

ahesford · 2024-11-14T11:09:58Z

contrib/zbm-sign.pl

@@ -56,9 +60,9 @@
  or die "Cannot open ZBM dir: $ZBM";

 if ($SignBackups) {
-  @EFIBins = grep { !/signed\.efi$/i and /\.efi/i } readdir $ZBM_dir;
+  @EFIBins = sort grep { !/signed\.efi$/i and /\.efi/i } readdir $ZBM_dir;


Glad you asked. The sbverify command modifies the efi files last modified timestamp resulting in refind showing as the default ZBM the last of the this array (last of the array is first in rEFInd list). This sort solution allows the user to controll the rEFInd order via naming.

Do you see an issue with using sort?

Yes, this is not the place to hack around rEFInd sorting specifics. There is no need to sort unless this script depends on the order of the array, which it does not.

ahesford · 2024-11-14T11:10:19Z

contrib/zbm-sign.pl

 } else {
-  @EFIBins = grep { !/signed\.efi$/i and !/backup/i and /\.efi/i } readdir $ZBM_dir;
+  @EFIBins = sort grep { !/signed\.efi$/i and !/backup/i and /\.efi/i } readdir $ZBM_dir;


As above, why sort?

ahesford · 2024-11-14T11:11:09Z

contrib/zbm-sign.pl

@@ -72,17 +76,14 @@
  if ( $SignMethod eq "sbctl" ) {
    system "sbctl sign $ZBM/$_";
  } elsif ( $SignMethod eq "sbsign" ) {
-    $Unsigned = substr( $_, 0, -4 );
-    system "sbsign --key $KeyDir/DB.key --cert $KeyDir/DB.crt $ZBM/$_ --output $ZBM/$Unsigned-signed.efi";
+    my $verify_output = "sbverify --cert $KeyDir/$CrtFileName $ZBM/$_ 2>&1";


You aren't actually running this command, so the test on the output you are expecting will always fail.

Furthermore, if possible, it would be better to distinguish whether the input is already signed by the exit code, rather than grepping output for a magic string.

Off-topic: I'm glad I'm invested time into this PR.

On-topic: This exact line of code behaves as expected. I can see it only signs the files that need signing. How do you explain it?
Should I replace the double qoutes with qx or backticks? What do you prefer?

I will look into the exit codes and see if that is an option.

The issue is not with the sbsign call.

my $verify_output = "sbverify --cert $KeyDir/$CrtFileName $ZBM/$_ 2>&1";

just sets a string, it doesn't actually run sbverify.

ahesford · 2024-11-14T11:15:09Z

contrib/zbm-sign.pl

+      say "File $_ is already signed.";
+      next;
+    }
+    system "sbsign --key $KeyDir/$KeyFileName --cert $KeyDir/$CrtFileName $ZBM/$_ --output $ZBM/$_";


Replacing the unsigned input with the signed output is destructive and probably shouldn't be the default behavior. It seems better to leave this as it was, creating a separate signed file.

This is exactly the default behavior of sbctl. The Arch wiki shows an example using the same approach:

sbsign --key db.key --cert db.crt --output /boot/vmlinuz-linux /boot/vmlinuz-linux

Have you had bad experiences with sbsign leaving you with a corrupt file?

Never assume that a process will complete successfully, especially when you are changing existing behavior that is less destructive. As I noted earlier, replacing the DeleteUnsigned option with an Overwrite and using that to control whether --output is passed at all is a better approach than always clobbering inputs.

ahesford · 2024-11-14T11:19:45Z

contrib/zbm-sign.pl

  } else {
    die "Sign method $SignMethod not valid.";
  }
-
-  if ( $DeleteUnsigned && $SignMethod eq "sbctl" ) {


Are you sure that sbsign won't corrupt its input if you try to overwrite it with output? Is that the case for several prior versions of sbsign?

If it's safe, the better approach would be to replace DeleteUnsigned with something a bit more meaningful like Overwrite and handle that above by just changing what you're passing as the output name to sbsign.

ahesford · 2024-11-14T18:18:39Z

contrib/zbm-sign.pl

+my $KeyFileName    = $SecureBoot->{KeyFileName};
+my $CrtFileName    = $SecureBoot->{CrtFileName};


The Name part seems superfluous, and CertFile would be a bit more readable than CrtFile.

Add checks before sbsign & config the cert and key

3b72c0d

zdykstra reviewed Nov 13, 2024

View reviewed changes

Remove unused DeleteUnsigned config

fc1a7ae

Adito5393 changed the title ~~Add checks before sbsign & config the cert and key~~ contrib: improve zbm-sign usage of sbsign usecase Nov 14, 2024

Adito5393 changed the title ~~contrib: improve zbm-sign usage of sbsign usecase~~ contrib: add key/cert options and signature check for sbsign Nov 14, 2024

ahesford requested changes Nov 14, 2024

View reviewed changes

Replace KeyDir from config with full path names

3a362b5

ahesford reviewed Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib: add key/cert options and signature check for sbsign #695

contrib: add key/cert options and signature check for sbsign #695

Adito5393 commented Nov 13, 2024

zdykstra Nov 13, 2024

Adito5393 Nov 14, 2024 •

edited

Loading

ahesford Nov 14, 2024

Adito5393 Nov 14, 2024 •

edited

Loading

ahesford Nov 14, 2024

zdykstra commented Nov 13, 2024

Adito5393 commented Nov 14, 2024 •

edited

Loading

ahesford Nov 14, 2024

Adito5393 Nov 14, 2024

ahesford Nov 14, 2024

ahesford Nov 14, 2024

ahesford Nov 14, 2024

Adito5393 Nov 14, 2024

ahesford Nov 14, 2024

ahesford Nov 14, 2024

Adito5393 Nov 14, 2024

ahesford Nov 14, 2024

ahesford Nov 14, 2024

ahesford Nov 14, 2024

		my $KeyFileName = $SecureBoot->{KeyFileName};
		my $CrtFileName = $SecureBoot->{CrtFileName};

contrib: add key/cert options and signature check for sbsign #695

Are you sure you want to change the base?

contrib: add key/cert options and signature check for sbsign #695

Conversation

Adito5393 commented Nov 13, 2024

Choose a reason for hiding this comment

Adito5393 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adito5393 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdykstra commented Nov 13, 2024

Adito5393 commented Nov 14, 2024 • edited Loading

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

Adito5393 Nov 14, 2024 •

edited

Loading

Adito5393 Nov 14, 2024 •

edited

Loading

Adito5393 commented Nov 14, 2024 •

edited

Loading