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

EFI & Secure Boot #141

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

Conversation

stejskalleos
Copy link

No description provided.

Copy link
Contributor

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Please don't change formatting in the same commit. Also, what's different than #134? Can't that be used/merged?

@stejskalleos
Copy link
Author

Yeah that's my editor settings, need to disable it.

Also, what's different than #134? Can't that be used/merged?

I can't edit current PRs/branches of others, so I thought I would create a new one since we agreed that I would continue with the work.

@stejskalleos
Copy link
Author

Tests are failing on master as well, don't know the details yet.

@ekohl
Copy link
Contributor

ekohl commented Jun 18, 2024

I can't edit current PRs/branches of others, so I thought I would create a new one since we agreed that I would continue with the work.

I said that I thought the fog-libvirt patch was OK, but should only be merged once we have the Foreman code in a mergeable state so we know it's a good API. Taking a commit from someone else without attributing the original author is a poor practice that leans towards plagiarism. Even if it's open source, crediting the original author is important. And if there is prior work, explaining why your version is different.

Tests are failing on master as well, don't know the details yet.

#140

@stejskalleos
Copy link
Author

crediting the original author is important.

Yeah I didn't want to steal your work, I can credit you for sure, just wanted to make it fast as possible.

@stejskalleos
Copy link
Author

@ekohl I assigned you in the https://github.com/theforeman/foreman/pull/10209/files wym to take care of the review ?

@stejskalleos
Copy link
Author

Rebased, added @ekohl as author & tested with Fedora 39 UEFI + SecureBoot. Ready for review
Foreman PR: theforeman/foreman#10209

@jloeser
Copy link

jloeser commented Jun 21, 2024

We should implement it according to https://libvirt.org/kbase/secureboot.html:

Enable SB:

  <firmware>
    <feature enabled='yes' name='secure-boot'/>
    <feature enabled='yes' name='enrolled-keys'/>
  </firmware>
</os>

Disable SB:

<os firmware='efi'>
  <firmware>
    <feature enabled='no' name='secure-boot'/>
  </firmware>
</os>

Providing <loader secure="yes|no"/> is not sufficient (see theforeman/foreman#10209 (comment)).

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
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.

None yet

3 participants