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

systemd: Add Boot type to system information #19371

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

leomoty
Copy link
Contributor

@leomoty leomoty commented Sep 21, 2023

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.

Copy link
Member

@allisonkarlitskaya allisonkarlitskaya left a 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;
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

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

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.

Copy link
Contributor Author

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",
Copy link
Member

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?

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 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

@leomoty
Copy link
Contributor Author

leomoty commented Sep 25, 2023

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.

@allisonkarlitskaya
Copy link
Member

@garrett So here's where the line shows up:

image

And not here, which is where I thought it would:

image

In addition to "BIOS or Legacy" the current code also has the possibility to display:

  • "EFI (Secure Boot enabled)"
  • "EFI (Secure Boot disabled)"

@garrett
Copy link
Member

garrett commented Nov 14, 2023

Looks good overall!

Questions:

  1. Should we move the left column on the details page? It would make things more balanced, and it kind of feels "tacked on" in the screenshots.
  2. How important is the information? (Would it be needed on the summary card, or is it just an implementation detail from the point of view of an administrator?)
  3. "BIOS or Legacy" should probably say "Legacy BIOS", right?
  4. Should we use strings like "EFI, secure boot on" and "EFI, secure boot off" instead?

@allisonkarlitskaya
Copy link
Member

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...

@garrett
Copy link
Member

garrett commented Nov 14, 2023

OK, let's do this:

My suggestions above, with @allisonkarlitskaya's stacked on top.

So that's:

  1. Move CPU to the left column on the details page, as I suggested above.
  2. Also add EFI/BIOS information to summary card, as @allisonkarlitskaya suggests.
  3. Keep "BIOS or legacy" (note: fix the L to be lowercase).
  4. Change EFI strings to "EFI, secure boot on" / "EFI, secure boot off".

How's that?

@MrGrymReaper
Copy link

MrGrymReaper commented Dec 17, 2023

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?

@MrGrymReaper
Copy link

MrGrymReaper commented Dec 29, 2023

@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".

@jelly
Copy link
Member

jelly commented Mar 28, 2024

Write so we can detect on x86:

  • EFI /sys/firmware/efi
  • BIOS /sys/firmware/efi is absent
  • EFI Secureboot available /sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c

It seems this UUID is not constant according to the arch wiki.

[jelle@t14s][~]%od --address-radix=n --format=u1 /sys/firmware/efi/efivars/SecureBoot-*
   6   0   0   0   0

On ARM?

@MrGrymReaper
Copy link

Write so we can detect on x86:

  • EFI /sys/firmware/efi
  • BIOS /sys/firmware/efi is absent
  • EFI Secureboot available /sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c

It seems this UUID is not constant according to the arch wiki.

[jelle@t14s][~]%od --address-radix=n --format=u1 /sys/firmware/efi/efivars/SecureBoot-*
   6   0   0   0   0

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.

@jelly
Copy link
Member

jelly commented Mar 28, 2024

Write so we can detect on x86:

  • EFI /sys/firmware/efi
  • BIOS /sys/firmware/efi is absent
  • EFI Secureboot available /sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c

It seems this UUID is not constant according to the arch wiki.

[jelle@t14s][~]%od --address-radix=n --format=u1 /sys/firmware/efi/efivars/SecureBoot-*
   6   0   0   0   0

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.

@jelly
Copy link
Member

jelly commented Mar 28, 2024

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.

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.

Display boot type information (EFI, BIOS, Secure Boot, etc.)
6 participants