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.

Sign up for GitHub

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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b72c0d
Add checks before sbsign & config the cert and key
Adito5393 Nov 13, 2024
fc1a7ae
Remove unused DeleteUnsigned config
Adito5393 Nov 14, 2024
3a362b5
Replace KeyDir from config with full path names
Adito5393 Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions contrib/zbm-sign.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@
#
# SecureBoot:
# SignBackup: true
# DeleteUnsigned: false
# SignMethod: sbctl
# KeyDir: /etc/sbkeys
# KeyFileName: DB.key
# CrtFileName: DB.crt
Adito5393 marked this conversation as resolved.
Show resolved Hide resolved
#
# The configuration keys should be self-explanatory.

Expand Down Expand Up @@ -48,17 +49,18 @@

my $SecureBoot = $config->{SecureBoot} or die "No config found, please edit /etc/zfsbootmenu/config.yaml";
my $KeyDir = $SecureBoot->{KeyDir};
my $DeleteUnsigned = $SecureBoot->{DeleteUnsigned};
my $KeyFileName = $SecureBoot->{KeyFileName};
my $CrtFileName = $SecureBoot->{CrtFileName};
Comment on lines +50 to +51
Copy link
Member

Choose a reason for hiding this comment

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

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

my $SignBackups = $SecureBoot->{SignBackup};
$SignMethod = $SecureBoot->{SignMethod};

opendir my $ZBM_dir, $ZBM
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;
Copy link
Member

Choose a reason for hiding this comment

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

Why sort?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

} 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;
Copy link
Member

Choose a reason for hiding this comment

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

As above, why sort?

}

say "Found: @EFIBins";
Expand All @@ -72,17 +74,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";
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

if ( $verify_output =~ /Signature verification OK/ ) {
say "File $_ is already signed.";
next;
}
system "sbsign --key $KeyDir/$KeyFileName --cert $KeyDir/$CrtFileName $ZBM/$_ --output $ZBM/$_";
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

} else {
die "Sign method $SignMethod not valid.";
}

if ( $DeleteUnsigned && $SignMethod eq "sbctl" ) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@Adito5393 Adito5393 Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

  1. Print a message if sbctl is used
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@Adito5393 Adito5393 Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

say "sbctl signs in place, not deleting $_";
} elsif ( $DeleteUnsigned && $SignMethod ne "sbctl" ) {
say "Deleting unsigned $_";
system "rm $ZBM/$_";
}
}
print "---------- FINISHED ----------\n";