EFI & Secure Boot

[stejskalleos]

No description provided.

[ekohl]

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

[stejskalleos]

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]

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

[ekohl]

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]

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]

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

[stejskalleos]

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

[jloeser]

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

[davispuh]

Looking forward to this getting merged.

[ekohl]

I think this isn't a good test because there's a very high chance the feature is just slightly different. In particular, when I run this test manually I see it includes:

    <firmware>
      <feature name="secure-boot" enabled="no"/>
      <feature name="enrolled-keys" enabled="no"/>
    </firmware>

Note the space before /> is missing.

Did you intend:

Suggested change

	secure_boot = !xml.include?('<feature name="secure-boot" enabled="no" />')
	enrolled_keys = !xml.include?('<feature name="enrolled-keys" enabled="no" />')
	secure_boot = xml.include?('<feature name="secure-boot" enabled="no"/>')
	enrolled_keys = xml.include?('<feature name="enrolled-keys" enabled="no"/>')

[ekohl]

Should this be:

Suggested change

	if os_firmware == "efi"
	if os_firmware == "efi" && os_firmware_features.any?

[ekohl]

Also, please make sure we deal with nil

Suggested change

	if os_firmware == "efi"
	if os_firmware == "efi" && os_firmware_features&.any?

[nofaralfasi]

I don't have permission to push changes to this PR, so I created a new one: #155.