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.

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

91-sbctl.install: don't sign without signing keys #188

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

ajakk
Copy link
Contributor

@ajakk ajakk commented Jan 10, 2023

It's expected that signing doesn't work without having previously generated keys, so don't try to sign when keys don't exist.

Closes: #187
Signed-off-by: John Helmert III [email protected]


# exit without error if keys don't exist
# https://github.com/Foxboron/sbctl/issues/187
if ! test -d /usr/share/secureboot; then
Copy link
Owner

Choose a reason for hiding this comment

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

sbctl --json status | grep installed | awk '{print $2}' | tr -d ','

or something like this would probably be a better idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like that doesn't always output something when the keys exist?

# sbctl create-keys
Created Owner UUID e8be777f-2cc7-4da5-bcfa-36fd80241bca
Creating secure boot keys...✓
Secure boot keys created!
c1ef6e49abfa / # sbctl --json status | grep installed | awk '{print $2}' | tr -d ','
c1ef6e49abfa / # sbctl --json status

Copy link
Owner

Choose a reason for hiding this comment

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

Uhhhhhh, that is a bug :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can reproduce like:

$ docker run --volume ~/git/sbctl:/sbctl -it archlinux bash
[root@f5c510c0e4a7 sbctl]# pacman --noconfirm --noprogressbar -Syu
[snip]
[root@f5c510c0e4a7 sbctl]# pacman --noconfirm --noprogressbar -S make go asciidoc gcc git
[snip]
[root@f5c510c0e4a7 sbctl]# go build -buildvcs=false -o sbctl ./cmd/sbctl
[snip]
[root@f5c510c0e4a7 sbctl]# ./sbctl create-keys
Created Owner UUID e0812b04-a6dd-4344-99a1-4691f4c20e07
Creating secure boot keys...✓
Secure boot keys created!
[root@f5c510c0e4a7 sbctl]# ./sbctl --json status

.. where ~/git/sbctl is this repository, updated to dcdc703, latest at the time of writing

Copy link
Owner

Choose a reason for hiding this comment

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

Right, the docker container has access to efivars? Alternatively, do you get anything dropping --json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes:

 ./sbctl status
system is not booted with UEFI

Which makes sense, because the container is indeed not booted with UEFI. So in this way, sbctl status seems unsuitable as a condition for signing, because signing does work after only doing sbctl create-keys in this environment.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather find a better solution using status --json than having another file where we hardcode the /usr/share/secureboot path.

I'll mill over the code a little bit and try have it always output some status output with --json. There isn't really any good reason for it to short-circuit itself when it see there is no UEFI available. It could very well check key location and stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, thanks!

@ajakk ajakk force-pushed the issue187 branch 2 times, most recently from 1d444d2 to d224236 Compare January 21, 2024 23:34
@ajakk
Copy link
Contributor Author

ajakk commented Feb 8, 2024

Friendly ping! :)

@Foxboron
Copy link
Owner

Foxboron commented Feb 8, 2024

Sorry, been busy with the lower layers in go-uefi so I can test more easily. That involves solving this as I have virtual FS support to cover all the test cases :)

It's expected that signing doesn't work without having previously
generated keys, so don't try to sign when keys don't exist.

Closes: Foxboron#187
Signed-off-by: John Helmert III <[email protected]>
@ajakk ajakk temporarily deployed to Build, sign, release binaries February 12, 2024 12:06 — with GitHub Actions Inactive
@ajakk ajakk temporarily deployed to Build, sign, release binaries February 12, 2024 12:06 — with GitHub Actions Inactive
@ajakk ajakk temporarily deployed to Build, sign, release binaries February 12, 2024 12:06 — with GitHub Actions Inactive
@Foxboron
Copy link
Owner

Thanks!

@Foxboron Foxboron merged commit 1c0882b into Foxboron:master Feb 22, 2024
5 of 6 checks passed
@Foxboron
Copy link
Owner

@ajakk now it's properly fixed... :)

c4d803a

@behrmann
Copy link
Contributor

If you intend to add more things after the install in the JSON this will break because of the trailing comma and it should probably be something like

sbctl setup --print-state --json | awk '/installed/ { gsub(/,$/,"",$2); print $2 }'

Should I open a PR or is the state done and will not be extended?

@Foxboron
Copy link
Owner

Should I open a PR or is the state done and will not be extended?

Isn't this trivially solved by just globbing it?

[ "$(sbctl setup --print-state --json | grep installed | awk '{print $2}')" = *"true"* ];

or something

@behrmann
Copy link
Contributor

In dash I get

$ [ 'true,' = *"true"* ]; echo $?
1

@Foxboron
Copy link
Owner

More awk it is then :) Please submit a PR when you have the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kernel install hook should do nothing if keys do not exist
3 participants