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

Add SecureBoot support for arbitrary operating systems to "Grub2 UEFI" PXE loaders #2145

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jloeser
Copy link

@jloeser jloeser commented Apr 13, 2023

This adds a section about new SecureBoot support. It only works in combination with the following patchset:

theforeman/foreman#9864
theforeman/smart-proxy#877

RFC: https://community.theforeman.org/t/add-secureboot-support-for-arbitrary-distributions/32601/1

@jloeser jloeser force-pushed the secureboot-support-poc branch 2 times, most recently from 7be85e5 to 419b2a7 Compare August 25, 2023 12:37
@maximiliankolb
Copy link
Contributor

Converted to draft PR for now because the linked PRs in foreman and smart_proxy have not been merged yet.

@pr-processor pr-processor bot added the Waiting on contributor Requires an action from the author label Dec 7, 2023
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Jan 25, 2024
@maximiliankolb maximiliankolb marked this pull request as ready for review February 7, 2024 10:06
Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

Some open questions for Goars or Jan.

@apinnick Do you ACK the capitalization of "Secure Boot"? And should it be "Secure Boot-enabled hosts" or "hosts with Secure Boot enabled"?

Rest LGTM from a writers perspective.

@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Needs re-review labels Feb 7, 2024
@apinnick
Copy link
Contributor

apinnick commented Feb 7, 2024

@apinnick Do you ACK the capitalization of "Secure Boot"? And should it be "Secure Boot-enabled hosts" or "hosts with Secure Boot enabled"?

@maximiliankolb I prefer "UEFI Secure Boot" because it is a UEFI security feature but I have no objection to "Secure Boot".

@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Feb 8, 2024
@maximiliankolb maximiliankolb changed the title New PXE loader "Grub2 UEFI SecureBoot (target OS)" Add SecureBoot support for arbitrary operating systems to "Grub2 UEFI" PXE loaders Mar 11, 2024
@goarsna
Copy link
Contributor

goarsna commented Mar 12, 2024

Hi there,
after discussions with @lzap and others on the community thread, we changed the implementation to a new approach, that extends the existing Grub2 UEFI SecureBoot and Grub2 UEFI HTTPS SecureBoot PXE loaders to support Secure Boot for arbitrary operating systems. With that, I also had to rework the documentation, so another round of review is needed...

For reference as the PRs mentioned in the first post in this PR are not valid anymore:
The PRs this documentation PR belongs to are

Thanks for all your reviews and input!

@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author Needs re-review and removed Waiting on contributor Requires an action from the author labels Mar 12, 2024
Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

Applied all feedback minus moving the attributes. Kindly re-review @Lennonka

Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

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

LGTM

@maximiliankolb
Copy link
Contributor

Thanks for the ACK. Code in "foreman" has not been merged yet. I'll press the button as soon as the other PRs are accepted too.

@Lennonka Lennonka added the Waiting for code Requires merge of related code in another repository before it can be merged label Mar 25, 2024
Comment on lines +22 to +30
ifeval::["{client-os}" == "Debian"]
* Ensure that `ar` and `xz` are installed on your {SmartProxy}.
endif::[]
ifeval::["{client-os}" == "Ubuntu"]
* Ensure that `ar`, `xz`, and `zstd` are installed on your {SmartProxy}.
endif::[]
ifeval::["{client-pkg-ext}" == "rpm"]
* Ensure that `cpio` is installed on your {SmartProxy}.
endif::[]
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we make these packaging dependencies if they're required?

Copy link
Contributor

Choose a reason for hiding this comment

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

At the time of packaging/installing Smart Proxy, you don't know yet which Client OS you want to provision/manage with Foreman.

* Ensure that `cpio` is installed on your {SmartProxy}.
endif::[]

.Procedure
Copy link
Member

Choose a reason for hiding this comment

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

If this is all common, why don't we ship a helper script that takes several inputs and saves it to the correct places? That makes both the procedure and testing easier. We can also ensure all commands we use are properly ensured via packaging dependencies.

There's precedent for this: https://github.com/theforeman/smart-proxy/blob/develop/sbin/foreman-prepare-realm is installed (here for RPM). Then the documentation refers to this:

. Create a realm proxy user, `realm-{smart-proxy-context}`, and the relevant roles in {FreeIPA}:
+
[options="nowrap", subs="+quotes,verbatim,attributes"]
----
# foreman-prepare-realm admin realm-{smart-proxy-context}

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great idea; I will try and investigate. It's not trivial to get the correct RPM/DEB package and extract the right binary. Two examples from orcharhino docs: Setup Secure Boot for SLES & Setup Secure Boot for Debian. From a user perspective, it would be very handy.

For now, I think docs are the way to go. I will ping you once I have a draft PR in smart-proxy.

@pr-processor pr-processor bot added the Waiting on contributor Requires an action from the author label Jul 11, 2024
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Jul 16, 2024
Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

Rebased to "master".

Comment on lines +22 to +30
ifeval::["{client-os}" == "Debian"]
* Ensure that `ar` and `xz` are installed on your {SmartProxy}.
endif::[]
ifeval::["{client-os}" == "Ubuntu"]
* Ensure that `ar`, `xz`, and `zstd` are installed on your {SmartProxy}.
endif::[]
ifeval::["{client-pkg-ext}" == "rpm"]
* Ensure that `cpio` is installed on your {SmartProxy}.
endif::[]
Copy link
Contributor

Choose a reason for hiding this comment

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

At the time of packaging/installing Smart Proxy, you don't know yet which Client OS you want to provision/manage with Foreman.

* Ensure that `cpio` is installed on your {SmartProxy}.
endif::[]

.Procedure
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great idea; I will try and investigate. It's not trivial to get the correct RPM/DEB package and extract the right binary. Two examples from orcharhino docs: Setup Secure Boot for SLES & Setup Secure Boot for Debian. From a user perspective, it would be very handy.

For now, I think docs are the way to go. I will ping you once I have a draft PR in smart-proxy.

@goarsna
Copy link
Contributor

goarsna commented Jul 16, 2024

FYI: @nofaralfasi and I had a discussion last week that resulted in some changes we want to implement on the Smart Proxy side which are summarized in the Smart Proxy PR. These changes also will affect this PR. Further updates will follow soonish / in the upcoming weeks (as soon as I have time to work on this again).

Use SecureBoot options to enable a client to download the `shim.efi` bootstrap bootloader that then loads the signed `grubx64.efi`.
Other PXE loaders like PXELinux UEFI, Grub2 ELF or iPXE Chain, require additional configuration. These workflows are not documented at the moment.
For BIOS systems, select the *PXELinux BIOS* option to enable a provisioned node to download the `pxelinux.0` file over TFTP.
For UEFI systems, select the *Grub2 UEFI* option to enable a TFTP client to download `grubx64.efi` file, or select the *Grub2 UEFI HTTP* option to enable an UEFI HTTP client to download `grubx64.efi` with the HTTP Boot feature.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is outside the scope of this PR. Just FYI.

GRUB is an acronym. The UI labels should be "GRUB2 ..."

@ekohl ekohl marked this pull request as draft July 24, 2024 13:45
@ekohl
Copy link
Member

ekohl commented Jul 24, 2024

Per the last comment, I'm moving this back to draft.

@goarsna
Copy link
Contributor

goarsna commented Sep 9, 2024

Rebased

@maximiliankolb maximiliankolb marked this pull request as ready for review September 10, 2024 13:20
@goarsna
Copy link
Contributor

goarsna commented Sep 10, 2024

I updated the PR to reflect the discussed changes. Thanks @maximiliankolb for marking it as ready.

@@ -0,0 +1,105 @@
[id="configuring-{smart-proxy-context}-to-provision-{client-os-context}-on-Secure-Boot-enabled-hosts"]
= Configuring {SmartProxy} to provision {client-os} on Secure Boot enabled hosts
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
= Configuring {SmartProxy} to provision {client-os} on Secure Boot enabled hosts
= Configuring {SmartProxy} to provision {client-os} on Secure Booted hosts

Would this be acceptable? It would make the title shorter.

Copy link
Contributor

@goarsna goarsna Sep 18, 2024

Choose a reason for hiding this comment

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

IMO we should leave this as is. Hosts with enabled "Secure Boot" are "securely booted", that's right. But "Secure Boot" is the proper name for the mechanism and the protocol used to achieve this and the term is also used in the UEFI specification (see page 1411).

But i have to admit the title is pretty long 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

That's okay. I don't have any other ideas how to make it shorter 😆

Copy link
Author

Choose a reason for hiding this comment

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

"[...] on Secure Booted hosts" doesn't make any sense here. Please keep "[...] Secure Boot enabled hosts" as suggested by @goarsna.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Waiting for code Requires merge of related code in another repository before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants