-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
systemd: Add Boot type to system information #19371
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
A couple of comments about the implementation. Please also note that this proposed feature is currently lacking design sign-off.
@@ -120,6 +120,16 @@ function findMemoryDevices(udevdb, info) { | |||
info.memory = memoryArray; | |||
} | |||
|
|||
async function getBootType() { | |||
const secure_boot_file = cockpit.manifests.system.config.secure_boot_file; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the indirection? Storing this in the manifest seems very strange to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't able to create the folder structure for /sys/firmware/efi/efivars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean you could just hardcode it directly in the JS... This is a well-known value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please do -- this isn't configuration.
const result = await cockpit.file(secure_boot_file, { binary: true }).read(); | ||
return `EFI (Secure Boot ${result[4] === 1 ? "enabled" : "disabled"})`; | ||
} catch { | ||
return "BIOS or Legacy"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should check if /proc/sys/kernel/arch
contains x86
before reporting BIOS
. If we're not on x86 it might make sense to exclude this field entirely, unless we want to do some more research on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is okay, I can wait, and I understand about missing the design bit
@@ -710,6 +713,26 @@ machine : 8561 | |||
b.wait_text('#memory-listing tr:nth-of-type(2) td[data-label=Rank]', "Single rank") | |||
b.wait_in_text('#memory-listing tr:nth-of-type(2) td[data-label=Speed]', "2400 MT/s") | |||
|
|||
# Pretend UEFI and Secure Boot is enabled | |||
m.execute("echo -en '\\x06\\x00\\x00\\x00\\x01' > /tmp/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c") | |||
self.write_file("/etc/cockpit/systemd.override.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Now I understand. You're doing this to give the tests a hook to mock in a new value.
How about a bind mount instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried, couldn't create the missing folder structure, if you have any proposal I am up for it. I am sure creating a VM and booting it with EFI enabled is overkill
This is now stale due to #19378, do you want me to rebase it as is? or wait until we have a design? I can implement the other bit afterwards also. |
@garrett So here's where the line shows up: And not here, which is where I thought it would: In addition to "BIOS or Legacy" the current code also has the possibility to display:
|
Looks good overall! Questions:
|
I think it's "BIOS or Legacy" because if we're in this situation, the only thing we know is that we're not on EFI. It might be that we're on some other arch or so, and then "Legacy BIOS" would be wrong. I assumed this would be in the main summary screen because that card looks kinda empty and secure boot is kinda a matter of health, depending on your perspective... |
OK, let's do this: My suggestions above, with @allisonkarlitskaya's stacked on top. So that's:
How's that? |
This helps but however EFI isn't just with Secure Boot enabled or disabled on X86_64 (x64), as some systems aren't even equipped with the possibility of having Secure Boot but with UEFI (EFI) based BIOS firmware. Also armhf64 has the potential for Secure Boot (even though a recent update broke it), Microsoft and Linux distributions are looking into fixing it! Thus a check for it being enabled, disabled etc on the 64 bit Arm architecture would be a very good idea! Can these be taken into account for being in the final merge please? |
@garrett @leomoty The above can be taken into account by changing the EFI strings to go "EFI, secure boot on", "EFI, secure boot off" / "EFI, secure boot unavailable". As well as introducing those strings for armhf64 in an appropriate fashion as follows "armhf64, secure boot on", "armhf64, secure boot off" / "armhf64, secure boot unavailable". |
Write so we can detect on x86:
It seems this UUID is not constant according to the arch wiki.
On ARM? |
On x86 under /sys/firmware/efi/efivars/ there wouldn't be the Secure Boot entry and/or certain security related values wouldn't be present. I don't know Arm either however that doesn't mean that it's not possible for such a situation to possible. However speaking to Arm and/or Ampere would help with these checks. |
I quickly read the U-boot code as it provides EFI for ARM and it also supports SecureBoot. But that's just one flavour of ARM. So I'm initially going to look at x86_64, and ARM + UEFI on Fedora. |
Quickly hacked up #20235 needs tests and ironing out some small details. (Also testing on ARM64 to see if I am right). P.S. added Co-Authored-By leomoty so your contribution does not go lost. |
I know for sure it might need to fix some wording, so trying to get the feedback early.
Fixes #19368
Can you please take a look, @allisonkarlitskaya?
Oh also, running these tests locally is messy, I get so many pixel diffs, so I really only focused on running the actual test that has the info.