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 all 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
27 changes: 12 additions & 15 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,9 @@
#
# SecureBoot:
# SignBackup: true
# DeleteUnsigned: false
# SignMethod: sbctl
# KeyDir: /etc/sbkeys
# KeyFileName: /etc/sbkeys/DB.key
# CrtFileName: /etc/sbkeys/DB.crt
#
# The configuration keys should be self-explanatory.

Expand Down Expand Up @@ -47,18 +47,18 @@
my $ESP = $Global->{BootMountPoint};

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 +72,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 $CrtFileName $ZBM/$_ 2>&1";
if ( $verify_output =~ /Signature verification OK/ ) {
say "File $_ is already signed.";
next;
}
system "sbsign --key $KeyFileName --cert $CrtFileName $ZBM/$_ --output $ZBM/$_";
} 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";