fog / fog-libvirt

libvirt provider for fog
MIT License
16 stars 42 forks source link

EFI & Secure Boot #141

Open stejskalleos opened 3 weeks ago

stejskalleos commented 2 weeks ago

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

Also, what's different than https://github.com/fog/fog-libvirt/pull/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 commented 2 weeks ago

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

ekohl commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

stejskalleos commented 2 weeks ago

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

jloeser commented 2 weeks ago

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 https://github.com/theforeman/foreman/pull/10209#discussion_r1648583006).