It should be enough to enable or disable the enrolled-keys feature
to control whether Secure Boot is enforced, but there's a slight
complication: many distro packages for edk2 include, in addition
to general purpose firmware images, builds that are targeting the
Confidential Computing use case.

For those, the firmware descriptor will not advertise the
enrolled-keys feature, which will technically make them suitable
for satisfying a configuration such as

  <os firmware='efi'>
    <firmware>
      <feature state='off' name='enrolled-keys'/>
    </firmware>
  </os>

In practice, users will expect the general purpose build to be
used in this case. Explicitly asking for the secure-boot feature
to be enabled achieves that result at the cost of some slight
additional verbosity.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 docs/kbase/secureboot.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/docs/kbase/secureboot.rst b/docs/kbase/secureboot.rst
index 8f151c1f2a..5fa59ad5e2 100644
--- a/docs/kbase/secureboot.rst
+++ b/docs/kbase/secureboot.rst
@@ -14,6 +14,7 @@ ask for Secure Boot to be enabled with
 
   <os firmware='efi'>
     <firmware>
+      <feature enabled='yes' name='secure-boot'/>
       <feature enabled='yes' name='enrolled-keys'/>
     </firmware>
   </os>
@@ -24,6 +25,7 @@ and for it to be disabled with
 
   <os firmware='efi'>
     <firmware>
+      <feature enabled='yes' name='secure-boot'/>
       <feature enabled='no' name='enrolled-keys'/>
     </firmware>
   </os>
@@ -44,6 +46,7 @@ snippet:
   <os firmware='efi'>
     <loader secure='yes'/>
     <firmware>
+      <feature enabled='yes' name='secure-boot'/>
       <feature enabled='yes' name='enrolled-keys'/>
     </firmware>
   </os>
-- 
2.37.1

On Wed, Aug 03, 2022 at 06:15:24PM +0200, Andrea Bolognani wrote:
> It should be enough to enable or disable the enrolled-keys feature
> to control whether Secure Boot is enforced, but there's a slight
> complication: many distro packages for edk2 include, in addition
> to general purpose firmware images, builds that are targeting the
> Confidential Computing use case.
> 
> For those, the firmware descriptor will not advertise the
> enrolled-keys feature, which will technically make them suitable
> for satisfying a configuration such as
> 
>   <os firmware='efi'>
>     <firmware>
>       <feature state='off' name='enrolled-keys'/>
>     </firmware>
>   </os>
> 
> In practice, users will expect the general purpose build to be
> used in this case. Explicitly asking for the secure-boot feature
> to be enabled achieves that result at the cost of some slight
> additional verbosity.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  docs/kbase/secureboot.rst | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/docs/kbase/secureboot.rst b/docs/kbase/secureboot.rst
> index 8f151c1f2a..5fa59ad5e2 100644
> --- a/docs/kbase/secureboot.rst
> +++ b/docs/kbase/secureboot.rst
> @@ -14,6 +14,7 @@ ask for Secure Boot to be enabled with
>  
>    <os firmware='efi'>
>      <firmware>
> +      <feature enabled='yes' name='secure-boot'/>
>        <feature enabled='yes' name='enrolled-keys'/>
>      </firmware>
>    </os>
> @@ -24,6 +25,7 @@ and for it to be disabled with
>  
>    <os firmware='efi'>
>      <firmware>
> +      <feature enabled='yes' name='secure-boot'/>
>        <feature enabled='no' name='enrolled-keys'/>
>      </firmware>
>    </os>

If we want secureboot disabled, this looks wrong. It just enables
secureboot, but without any keys.  We need enabled=no to ask for
a firmware without SecureBoot at all.

> @@ -44,6 +46,7 @@ snippet:
>    <os firmware='efi'>
>      <loader secure='yes'/>
>      <firmware>
> +      <feature enabled='yes' name='secure-boot'/>
>        <feature enabled='yes' name='enrolled-keys'/>
>      </firmware>
>    </os>



> -- 
> 2.37.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Wed, Aug 03, 2022 at 05:29:15PM +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 03, 2022 at 06:15:24PM +0200, Andrea Bolognani wrote:
> >    <os firmware='efi'>
> >      <firmware>
> > +      <feature enabled='yes' name='secure-boot'/>
> >        <feature enabled='no' name='enrolled-keys'/>
> >      </firmware>
> >    </os>
>
> If we want secureboot disabled, this looks wrong. It just enables
> secureboot, but without any keys.  We need enabled=no to ask for
> a firmware without SecureBoot at all.

Mh. From a practical standpoint, the scenarios

  * firmware has secure boot support but there are no enrolled keys
  * firmware doesn't have secure boot support

are pretty much equivalent: either way, unsigned code will be allowed
to run.

And, although that doesn't seem the case anywhere at the moment, I
can imagine that at some point a distro will decide to only ship a
single firmware build. That would be comparable to how you can't
really find a motherboard that doesn't come with secure boot support
at this point, and your only choice is whether you want the feature
to be active.

So I don't disagree with you, but I'm also not completely convinced
that we should *not* document a way to disable secure boot while
still picking a firmware build that contains the feature.

Would squashing the diff below be a good enough compromise in your
opinion?


diff --git a/docs/kbase/secureboot.rst b/docs/kbase/secureboot.rst
index 5fa59ad5e2..68732701df 100644
--- a/docs/kbase/secureboot.rst
+++ b/docs/kbase/secureboot.rst
@@ -19,7 +19,17 @@ ask for Secure Boot to be enabled with
     </firmware>
   </os>

-and for it to be disabled with
+and for it to be disabled with either
+
+::
+
+  <os firmware='efi'>
+    <firmware>
+      <feature enabled='no' name='secure-boot'/>
+    </firmware>
+  </os>
+
+or

 ::

@@ -30,8 +40,8 @@ and for it to be disabled with
     </firmware>
   </os>

-These configuration will cause unsigned guest operating systems to
-be rejected and allowed respectively.
+The first configuration will cause unsigned guest operating systems
+to be rejected, while the remaining two will allow running them.


 Older libvirt versions
-- 
Andrea Bolognani / Red Hat / Virtualization

On Thu, Aug 04, 2022 at 03:32:32AM -0500, Andrea Bolognani wrote:
> On Wed, Aug 03, 2022 at 05:29:15PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Aug 03, 2022 at 06:15:24PM +0200, Andrea Bolognani wrote:
> > >    <os firmware='efi'>
> > >      <firmware>
> > > +      <feature enabled='yes' name='secure-boot'/>
> > >        <feature enabled='no' name='enrolled-keys'/>
> > >      </firmware>
> > >    </os>
> >
> > If we want secureboot disabled, this looks wrong. It just enables
> > secureboot, but without any keys.  We need enabled=no to ask for
> > a firmware without SecureBoot at all.
> 
> Mh. From a practical standpoint, the scenarios
> 
>   * firmware has secure boot support but there are no enrolled keys
>   * firmware doesn't have secure boot support
> 
> are pretty much equivalent: either way, unsigned code will be allowed
> to run.

Yes & no - one allows you to enroll custom keys, the other doesn't
allow it. For most people that distinction doesn't matter but it is
a significant difference.

I don't mind documenting both, but we should explain why we are
illustrating two different mechanisms, as when the question is
"how to I disable secureboot" an answer saying "secure_boot enabled=yes"
simply looks wrong.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Thu, Aug 04, 2022 at 10:29:12AM +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 04, 2022 at 03:32:32AM -0500, Andrea Bolognani wrote:
> > On Wed, Aug 03, 2022 at 05:29:15PM +0100, Daniel P. Berrangé wrote:
> > > On Wed, Aug 03, 2022 at 06:15:24PM +0200, Andrea Bolognani wrote:
> > > >    <os firmware='efi'>
> > > >      <firmware>
> > > > +      <feature enabled='yes' name='secure-boot'/>
> > > >        <feature enabled='no' name='enrolled-keys'/>
> > > >      </firmware>
> > > >    </os>
> > >
> > > If we want secureboot disabled, this looks wrong. It just enables
> > > secureboot, but without any keys.  We need enabled=no to ask for
> > > a firmware without SecureBoot at all.
> >
> > Mh. From a practical standpoint, the scenarios
> >
> >   * firmware has secure boot support but there are no enrolled keys
> >   * firmware doesn't have secure boot support
> >
> > are pretty much equivalent: either way, unsigned code will be allowed
> > to run.
>
> Yes & no - one allows you to enroll custom keys, the other doesn't
> allow it. For most people that distinction doesn't matter but it is
> a significant difference.
>
> I don't mind documenting both, but we should explain why we are
> illustrating two different mechanisms, as when the question is
> "how to I disable secureboot" an answer saying "secure_boot enabled=yes"
> simply looks wrong.

Right :)

There's an underlying issue with naming, because what most people
would think of when talking about the state of secure boot is,
counterintuitively, not actually controlled by the "secure-boot"
firmware feature. That's the reason why I wrote this kbase in the
first place: if the XML mechanism matched people's intuition, it
wouldn't be necessary.

I have tried to explain why there are two firmware features
controlling what is, in most people's mind, a binary knob in the
"additional information" section. I'll attempt to expand upon that
without making it too complicated in v2.

-- 
Andrea Bolognani / Red Hat / Virtualization