[PATCH 00/14] Improve mechanism for configuring allowed commands

[PATCH 00/14] Improve mechanism for configuring allowed commands

[Daniel P. Berrangé]

The QGA supports dynamically filtering what commands are enabled via a
combination of allow lists and deny lists. This is very flexible, but
at the same time very fragile.

Consider that a user wants to block all commands that allow unrestricted
file access/command execution, so they set the deny list when starting
QGA. Now their OS vendor issues a software update which includes a new
version of QGA. This new QGA version is liable to contain new commands,
some of which might undermine the intent of the user's configured deny
list.

IOW, the generic deny list functionality is inherently dangerous as a
mechanism for limiting risk exposure.

Using an allow list is much safer, but means on every update the user
should check the list of new commands to decide which are safe or not,
putting a burden on every user.

In the context of RHEL, there has been a long term deny list that blocks
use of guest-file and guest-exec commands, since they give unrestricted
access to the guest.

With the advent of confidential computing, a far greater number of QGA
commands are very unsafe to permit, and it is unreasonable to expect
each user and/or downstream vendor to repeat the work to figure out
what commands are OK.

This is a similar problem seen in the "seccomp" world where new syscalls
appear frequently and users can't be expected to understand all of them.
Systemd pioneered the approach of defining "profiles"  which group
together sets of syscalls, which we subsequently copied in QEMU.

This series applies this same conceptual idea to QGA command filtering,
making use of the QAPI "features" facility to associate commands into
one or more groups.

This grouping is then exposed via some new higher level command line
arguments.

* --no-unrestricted / -u

  A flag to block all the guest-file and guest-exec commands

  This replicates the policy RHEL currently defines via a deny list.

* --no-user-auth / -e

  A flag to block all the commands for manipulating user account
  authentication credentials.

* --confidential / -i

  A flag to block all commands, except for those which have been
  explicitly marked as not violating guest owner data privacy

This feature mechanism is further utilized internally to track the
commands which are safe to use while FS are frozen.

A key benefit of using the QAPI "features" facility is that these
groupings are visible in the documentation of the QGA commands.

By using these high level command lines arguments, deployments will
be safe wrt software upgrades, as long as QEMU maintainers apply
appropriate tags to any new commands.

The allow/deny list command line flags can still be used to further
refine the command lines, but ideally that would be rare.

A missing piece in this series is getting the --confidential flag to
be automatically passed to QGA when running in a confidential VM. This
is something that will likely be done via systemd unit files. My thought
is that the existing 'qemu-guest-agent.service' would get a parameter

   ConditionSecurity=!cvm

while a new qemu-guest-agent-confidential.service' would have the same
content but with ConditionSecurity=cvm instead, and would pass the
--confidential flag.

This series depends on the one I sent earlier:

  https://lists.nongnu.org/archive/html/qemu-devel/2024-06/msg00743.html

Daniel P. Berrangé (14):
  qapi: use "QAPI_FEATURE" as namespace for special features
  qapi: add helper for checking if a command feature is set
  qapi: cope with special feature names containing a '-'
  qapi: add a 'command-features' pragma
  qapi: stop hardcoding list of special features
  qapi: define enum for custom special features on commands
  qga: use special feature to mark those that can run when FS are frozen
  qga: add command line to limit commands for confidential guests
  qga: define commands which can be run in confidential mode
  qga: add command line to block unrestricted command/file access
  qga: mark guest-file-* commands with 'unrestricted' flag
  qga: mark guest-exec-* commands with 'unrestricted' flag
  qga: add command line to block user authentication commands
  qga: mark guest-ssh-* / guest-*-password commands with 'unrestricted'
    flag

 include/qapi/qmp/dispatch.h   |   1 +
 include/qapi/util.h           |   6 +-
 qapi/qapi-util.c              |   4 +-
 qapi/qmp-registry.c           |   5 +
 qapi/qobject-output-visitor.c |   4 +-
 qga/main.c                    |  66 ++++++---
 qga/qapi-schema.json          | 248 +++++++++++++++++++++++++++++++---
 scripts/qapi/commands.py      |  20 +++
 scripts/qapi/gen.py           |   2 +-
 scripts/qapi/parser.py        |   2 +
 scripts/qapi/schema.py        |  33 ++++-
 scripts/qapi/source.py        |   2 +
 12 files changed, 341 insertions(+), 52 deletions(-)

-- 
2.45.1

[PATCH 01/14] qapi: use "QAPI_FEATURE" as namespace for special features

[Daniel P. Berrangé]

This more clearly distinguishes the feature constants from other
QAPI constants.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qapi/util.h           | 4 ++--
 qapi/qapi-util.c              | 4 ++--
 qapi/qobject-output-visitor.c | 4 ++--
 scripts/qapi/gen.py           | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/qapi/util.h b/include/qapi/util.h
index 20dfea8a54..7698e789a9 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -12,8 +12,8 @@
 #define QAPI_UTIL_H
 
 typedef enum {
-    QAPI_DEPRECATED,
-    QAPI_UNSTABLE,
+    QAPI_FEATURE_DEPRECATED,
+    QAPI_FEATURE_UNSTABLE,
 } QapiSpecialFeature;
 
 typedef struct QEnumLookup {
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 65a7d18437..6bcab11117 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -43,13 +43,13 @@ bool compat_policy_input_ok(unsigned special_features,
                             const char *kind, const char *name,
                             Error **errp)
 {
-    if ((special_features & 1u << QAPI_DEPRECATED)
+    if ((special_features & 1u << QAPI_FEATURE_DEPRECATED)
         && !compat_policy_input_ok1("Deprecated",
                                     policy->deprecated_input,
                                     error_class, kind, name, errp)) {
         return false;
     }
-    if ((special_features & (1u << QAPI_UNSTABLE))
+    if ((special_features & (1u << QAPI_FEATURE_UNSTABLE))
         && !compat_policy_input_ok1("Unstable",
                                     policy->unstable_input,
                                     error_class, kind, name, errp)) {
diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c
index 74770edd73..ca8be3fe06 100644
--- a/qapi/qobject-output-visitor.c
+++ b/qapi/qobject-output-visitor.c
@@ -214,9 +214,9 @@ static bool qobject_output_policy_skip(Visitor *v, const char *name,
 {
     CompatPolicy *pol = &v->compat_policy;
 
-    return ((special_features & 1u << QAPI_DEPRECATED)
+    return ((special_features & 1u << QAPI_FEATURE_DEPRECATED)
             && pol->deprecated_output == COMPAT_POLICY_OUTPUT_HIDE)
-        || ((special_features & 1u << QAPI_UNSTABLE)
+        || ((special_features & 1u << QAPI_FEATURE_UNSTABLE)
             && pol->unstable_output == COMPAT_POLICY_OUTPUT_HIDE);
 }
 
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 6a8abe0041..9c590a1c2e 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -41,7 +41,7 @@
 
 
 def gen_special_features(features: Sequence[QAPISchemaFeature]) -> str:
-    special_features = [f"1u << QAPI_{feat.name.upper()}"
+    special_features = [f"1u << QAPI_FEATURE_{feat.name.upper()}"
                         for feat in features if feat.is_special()]
     return ' | '.join(special_features) or '0'
 
-- 
2.45.1

[PATCH 02/14] qapi: add helper for checking if a command feature is set

[Daniel P. Berrangé]

The 'qmp_command_has_feature' method returns true if the
requested feature has been set.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qapi/qmp/dispatch.h | 1 +
 qapi/qmp-registry.c         | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index f2e956813a..0dfcb549b6 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -51,6 +51,7 @@ void qmp_disable_command(QmpCommandList *cmds, const char *name,
 void qmp_enable_command(QmpCommandList *cmds, const char *name);
 
 bool qmp_command_is_enabled(const QmpCommand *cmd);
+bool qmp_command_has_feature(const QmpCommand *cmd, unsigned feature);
 bool qmp_command_available(const QmpCommand *cmd, Error **errp);
 const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index 485bc5e6fc..392f0e5c5a 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -74,6 +74,11 @@ bool qmp_command_is_enabled(const QmpCommand *cmd)
     return cmd->enabled;
 }
 
+bool qmp_command_has_feature(const QmpCommand *cmd, unsigned feature)
+{
+    return cmd->special_features & feature;
+}
+
 const char *qmp_command_name(const QmpCommand *cmd)
 {
     return cmd->name;
-- 
2.45.1

[PATCH 03/14] qapi: cope with special feature names containing a '-'

[Daniel P. Berrangé]

When we shortly allow custom special feature names to be defined, it
will be valid to include a '-', which must be translated to a '_'
when generating code.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/qapi/gen.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 9c590a1c2e..650efc59ed 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -41,7 +41,7 @@
 
 
 def gen_special_features(features: Sequence[QAPISchemaFeature]) -> str:
-    special_features = [f"1u << QAPI_FEATURE_{feat.name.upper()}"
+    special_features = [f"1u << QAPI_FEATURE_{feat.name.upper().replace('-','_')}"
                         for feat in features if feat.is_special()]
     return ' | '.join(special_features) or '0'
 
-- 
2.45.1

Re: [PATCH 03/14] qapi: cope with special feature names containing a '-'

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> When we shortly allow custom special feature names to be defined, it
> will be valid to include a '-', which must be translated to a '_'
> when generating code.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/qapi/gen.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 9c590a1c2e..650efc59ed 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -41,7 +41,7 @@
>  
>  
>  def gen_special_features(features: Sequence[QAPISchemaFeature]) -> str:
> -    special_features = [f"1u << QAPI_FEATURE_{feat.name.upper()}"
> +    special_features = [f"1u << QAPI_FEATURE_{feat.name.upper().replace('-','_')}"
>                          for feat in features if feat.is_special()]
>      return ' | '.join(special_features) or '0'

I'd prefer something like

    f"1 << {c_enum_const('QAPI_FEATURE', feat.name)}"

[PATCH 04/14] qapi: add a 'command-features' pragma

[Daniel P. Berrangé]

The 'command-features' pragma allows for defining additional
special features that are unique to a particular QAPI schema
instance and its implementation.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/qapi/parser.py | 2 ++
 scripts/qapi/source.py | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 7b13a583ac..36a9046243 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -243,6 +243,8 @@ def check_list_str(name: str, value: object) -> List[str]:
             pragma.documentation_exceptions = check_list_str(name, value)
         elif name == 'member-name-exceptions':
             pragma.member_name_exceptions = check_list_str(name, value)
+        elif name == 'command-features':
+            pragma.command_features = check_list_str(name, value)
         else:
             raise QAPISemError(info, "unknown pragma '%s'" % name)
 
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index 7b379fdc92..07c2958ac4 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -28,6 +28,8 @@ def __init__(self) -> None:
         self.documentation_exceptions: List[str] = []
         # Types whose member names may violate case conventions
         self.member_name_exceptions: List[str] = []
+        # Arbitrary extra features recorded against commands
+        self.command_features: List[str] = []
 
 
 class QAPISourceInfo:
-- 
2.45.1

Re: [PATCH 04/14] qapi: add a 'command-features' pragma

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> The 'command-features' pragma allows for defining additional
> special features that are unique to a particular QAPI schema
> instance and its implementation.

So far, we have special features (predefined, known to the generator and
treated specially), and normal features (user-defined, not known to the
generator).  You create a new kind in between: user-defined, not known
to the generator, yet treated specially (I guess?).  Hmm.

Could you at least hint at indented use here?  What special treatment do
you have in mind?

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/qapi/parser.py | 2 ++
>  scripts/qapi/source.py | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 7b13a583ac..36a9046243 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -243,6 +243,8 @@ def check_list_str(name: str, value: object) -> List[str]:
>              pragma.documentation_exceptions = check_list_str(name, value)
>          elif name == 'member-name-exceptions':
>              pragma.member_name_exceptions = check_list_str(name, value)
> +        elif name == 'command-features':
> +            pragma.command_features = check_list_str(name, value)
>          else:
>              raise QAPISemError(info, "unknown pragma '%s'" % name)
>  
> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> index 7b379fdc92..07c2958ac4 100644
> --- a/scripts/qapi/source.py
> +++ b/scripts/qapi/source.py
> @@ -28,6 +28,8 @@ def __init__(self) -> None:
>          self.documentation_exceptions: List[str] = []
>          # Types whose member names may violate case conventions
>          self.member_name_exceptions: List[str] = []
> +        # Arbitrary extra features recorded against commands
> +        self.command_features: List[str] = []
>  
>  
>  class QAPISourceInfo:

Re: [PATCH 04/14] qapi: add a 'command-features' pragma

[Daniel P. Berrangé]

On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > The 'command-features' pragma allows for defining additional
> > special features that are unique to a particular QAPI schema
> > instance and its implementation.
> 
> So far, we have special features (predefined, known to the generator and
> treated specially), and normal features (user-defined, not known to the
> generator).  You create a new kind in between: user-defined, not known
> to the generator, yet treated specially (I guess?).  Hmm.
> 
> Could you at least hint at indented use here?  What special treatment do
> you have in mind?

Essentially, these features are a way to attach metadata to commands that
the server side impl can later query. This eliminates the need to hardcode
lists of commands, such as in QGA which hardcodes a list of commands which
are safe to use when filesystems are frozen. This is illustrated later in
this series.

> 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  scripts/qapi/parser.py | 2 ++
> >  scripts/qapi/source.py | 2 ++
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 7b13a583ac..36a9046243 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -243,6 +243,8 @@ def check_list_str(name: str, value: object) -> List[str]:
> >              pragma.documentation_exceptions = check_list_str(name, value)
> >          elif name == 'member-name-exceptions':
> >              pragma.member_name_exceptions = check_list_str(name, value)
> > +        elif name == 'command-features':
> > +            pragma.command_features = check_list_str(name, value)
> >          else:
> >              raise QAPISemError(info, "unknown pragma '%s'" % name)
> >  
> > diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> > index 7b379fdc92..07c2958ac4 100644
> > --- a/scripts/qapi/source.py
> > +++ b/scripts/qapi/source.py
> > @@ -28,6 +28,8 @@ def __init__(self) -> None:
> >          self.documentation_exceptions: List[str] = []
> >          # Types whose member names may violate case conventions
> >          self.member_name_exceptions: List[str] = []
> > +        # Arbitrary extra features recorded against commands
> > +        self.command_features: List[str] = []
> >  
> >  
> >  class QAPISourceInfo:
> 

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 :|

Re: [PATCH 04/14] qapi: add a 'command-features' pragma

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > The 'command-features' pragma allows for defining additional
>> > special features that are unique to a particular QAPI schema
>> > instance and its implementation.
>> 
>> So far, we have special features (predefined, known to the generator and
>> treated specially), and normal features (user-defined, not known to the
>> generator).  You create a new kind in between: user-defined, not known
>> to the generator, yet treated specially (I guess?).  Hmm.
>> 
>> Could you at least hint at indented use here?  What special treatment do
>> you have in mind?
>
> Essentially, these features are a way to attach metadata to commands that
> the server side impl can later query. This eliminates the need to hardcode
> lists of commands, such as in QGA which hardcodes a list of commands which
> are safe to use when filesystems are frozen. This is illustrated later in
> this series.

Please update docs/devel/qapi-code-gen.rst section "Pragma directives",
and maybe section "Features".

I'm not sure conflating the new kind of feature with existing special
features is a good idea.  I need to review more of the series before I
can make up my mind.

Re: [PATCH 04/14] qapi: add a 'command-features' pragma

[Daniel P. Berrangé]

On Fri, Jul 12, 2024 at 10:50:54AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > The 'command-features' pragma allows for defining additional
> >> > special features that are unique to a particular QAPI schema
> >> > instance and its implementation.
> >> 
> >> So far, we have special features (predefined, known to the generator and
> >> treated specially), and normal features (user-defined, not known to the
> >> generator).  You create a new kind in between: user-defined, not known
> >> to the generator, yet treated specially (I guess?).  Hmm.
> >> 
> >> Could you at least hint at indented use here?  What special treatment do
> >> you have in mind?
> >
> > Essentially, these features are a way to attach metadata to commands that
> > the server side impl can later query. This eliminates the need to hardcode
> > lists of commands, such as in QGA which hardcodes a list of commands which
> > are safe to use when filesystems are frozen. This is illustrated later in
> > this series.
> 
> Please update docs/devel/qapi-code-gen.rst section "Pragma directives",
> and maybe section "Features".
> 
> I'm not sure conflating the new kind of feature with existing special
> features is a good idea.  I need to review more of the series before I
> can make up my mind.

I originally implemented a completely separate 'tags' concept in the
QAPI parser, before deciding I was just re-inventing 'features' for
no obvious benefit.

The other nice thing about using features is that these are exposed
in the schema and docs. With the 'fsfreeze' restriction in code,
there's no formal docs of what commands are allowed when frozen, and
this is also not exposed in QAPI schema to apps. Using 'features'
we get all that as standard.

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 :|

Re: [PATCH 04/14] qapi: add a 'command-features' pragma

[Markus Armbruster]

Sorry for the delay; too many distractions, and I needed a good think.

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Jul 12, 2024 at 10:50:54AM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >> 
>> >> > The 'command-features' pragma allows for defining additional
>> >> > special features that are unique to a particular QAPI schema
>> >> > instance and its implementation.
>> >> 
>> >> So far, we have special features (predefined, known to the generator and
>> >> treated specially), and normal features (user-defined, not known to the
>> >> generator).  You create a new kind in between: user-defined, not known
>> >> to the generator, yet treated specially (I guess?).  Hmm.
>> >> 
>> >> Could you at least hint at indented use here?  What special treatment do
>> >> you have in mind?
>> >
>> > Essentially, these features are a way to attach metadata to commands that
>> > the server side impl can later query. This eliminates the need to hardcode
>> > lists of commands, such as in QGA which hardcodes a list of commands which
>> > are safe to use when filesystems are frozen. This is illustrated later in
>> > this series.
>> 
>> Please update docs/devel/qapi-code-gen.rst section "Pragma directives",
>> and maybe section "Features".

Second thoughts; see below.

>> I'm not sure conflating the new kind of feature with existing special
>> features is a good idea.  I need to review more of the series before I
>> can make up my mind.
>
> I originally implemented a completely separate 'tags' concept in the
> QAPI parser, before deciding I was just re-inventing 'features' for
> no obvious benefit.
>
> The other nice thing about using features is that these are exposed
> in the schema and docs. With the 'fsfreeze' restriction in code,
> there's no formal docs of what commands are allowed when frozen, and
> this is also not exposed in QAPI schema to apps. Using 'features'
> we get all that as standard.

When you need to tack a mark to one or more things for whatever purpose
*and* expose it to QMP clients, then features make sense.  This is the
case here.

Initially, features were strictly an external interface annotation, and
were not meant to be used within QEMU.  All features were user-defined.

This changed when I created configurable policy for deprecated and
unstable management interfaces: the policy engine needs to check for
features 'deprecated' and 'unstable'.  Since the policy engine is partly
implemented in generated code, these two features need to be baked into
the generator.  This makes them special.

You need less than that: a predicate "does <command> have <feature>" for
certain features, ideally without baking them into the generator.

The command registry already tracks each command's special features for
use by the policy engine.  Obvious idea: also track the features you
want to pass to the predicate.

Your series adds tracking for exactly the features you need:

* Enumerate them in the schema with new pragma command-features

  Missing: documentation for the pragma.

* Generate an extension QapiSpecialFeatureCustom of existing enum
  QapiSpecialFeature, which is not generated.  The latter is in
  qapi/util.h, the former in ${prefix}qapi-init-commands.h.

* Mark these features special for commands only, so existing registry
  machinery tracks them.  Do *not* make them special elsewhere, because
  that would break things.

  Feels like a hack.  Minor trap: if you use the same feature in
  multiple schemas, multiple generated headers will define the same enum
  constant, possibly with different values.  If you manage to include
  the wrong header *and* the value differs there, you'll likely lose
  hair.

* Missing: tests.

I think we can avoid supplying most of the missing bits.  The main QAPI
schema uses five features: deprecated, unstable,
allow-write-only-overlay, dynamic-auto-read-only, fdset.  The QGA QAPI
schema uses four, namely the four you add in this series.  Why not track
all features, and dispense with the pragma?  Like this:

* Change type of feature bitsets to uint64_t (it's unsigned now).

* Error out if a schema has more than 64 features.

* Move enum QapiSpecialFeature into a new generated header.

* Generate a member for each feature, not just the two predefined ones.

* Pass all command features to the registry, not just the special ones.

* Recommended: do the same elsewhere, i.e. replace
  gen_special_features() by gen_features().

Thoughts?


PS: I think I spotted a number of additional minor issues in your
series, but I won't describe them here, as I hope the simplification
will make most of them go away.

Re: [PATCH 04/14] qapi: add a 'command-features' pragma

[Daniel P. Berrangé]

On Tue, Jul 16, 2024 at 08:08:42PM +0200, Markus Armbruster wrote:
> Sorry for the delay; too many distractions, and I needed a good think.
> 
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Jul 12, 2024 at 10:50:54AM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote:
> >> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> >> 
> >> >> > The 'command-features' pragma allows for defining additional
> >> >> > special features that are unique to a particular QAPI schema
> >> >> > instance and its implementation.
> >> >> 
> >> >> So far, we have special features (predefined, known to the generator and
> >> >> treated specially), and normal features (user-defined, not known to the
> >> >> generator).  You create a new kind in between: user-defined, not known
> >> >> to the generator, yet treated specially (I guess?).  Hmm.
> >> >> 
> >> >> Could you at least hint at indented use here?  What special treatment do
> >> >> you have in mind?
> >> >
> >> > Essentially, these features are a way to attach metadata to commands that
> >> > the server side impl can later query. This eliminates the need to hardcode
> >> > lists of commands, such as in QGA which hardcodes a list of commands which
> >> > are safe to use when filesystems are frozen. This is illustrated later in
> >> > this series.
> >> 
> >> Please update docs/devel/qapi-code-gen.rst section "Pragma directives",
> >> and maybe section "Features".
> 
> Second thoughts; see below.
> 
> >> I'm not sure conflating the new kind of feature with existing special
> >> features is a good idea.  I need to review more of the series before I
> >> can make up my mind.
> >
> > I originally implemented a completely separate 'tags' concept in the
> > QAPI parser, before deciding I was just re-inventing 'features' for
> > no obvious benefit.
> >
> > The other nice thing about using features is that these are exposed
> > in the schema and docs. With the 'fsfreeze' restriction in code,
> > there's no formal docs of what commands are allowed when frozen, and
> > this is also not exposed in QAPI schema to apps. Using 'features'
> > we get all that as standard.
> 
> When you need to tack a mark to one or more things for whatever purpose
> *and* expose it to QMP clients, then features make sense.  This is the
> case here.
> 
> Initially, features were strictly an external interface annotation, and
> were not meant to be used within QEMU.  All features were user-defined.
> 
> This changed when I created configurable policy for deprecated and
> unstable management interfaces: the policy engine needs to check for
> features 'deprecated' and 'unstable'.  Since the policy engine is partly
> implemented in generated code, these two features need to be baked into
> the generator.  This makes them special.
> 
> You need less than that: a predicate "does <command> have <feature>" for
> certain features, ideally without baking them into the generator.
> 
> The command registry already tracks each command's special features for
> use by the policy engine.  Obvious idea: also track the features you
> want to pass to the predicate.
> 
> Your series adds tracking for exactly the features you need:
> 
> * Enumerate them in the schema with new pragma command-features
> 
>   Missing: documentation for the pragma.
> 
> * Generate an extension QapiSpecialFeatureCustom of existing enum
>   QapiSpecialFeature, which is not generated.  The latter is in
>   qapi/util.h, the former in ${prefix}qapi-init-commands.h.
> 
> * Mark these features special for commands only, so existing registry
>   machinery tracks them.  Do *not* make them special elsewhere, because
>   that would break things.
> 
>   Feels like a hack.  Minor trap: if you use the same feature in
>   multiple schemas, multiple generated headers will define the same enum
>   constant, possibly with different values.  If you manage to include
>   the wrong header *and* the value differs there, you'll likely lose
>   hair.
> 
> * Missing: tests.
> 
> I think we can avoid supplying most of the missing bits.  The main QAPI
> schema uses five features: deprecated, unstable,
> allow-write-only-overlay, dynamic-auto-read-only, fdset.  The QGA QAPI
> schema uses four, namely the four you add in this series.  Why not track
> all features, and dispense with the pragma?  Like this:
> 
> * Change type of feature bitsets to uint64_t (it's unsigned now).
> 
> * Error out if a schema has more than 64 features.
> 
> * Move enum QapiSpecialFeature into a new generated header.
> 
> * Generate a member for each feature, not just the two predefined ones.
> 
> * Pass all command features to the registry, not just the special ones.
> 
> * Recommended: do the same elsewhere, i.e. replace
>   gen_special_features() by gen_features().
> 
> Thoughts?

So basically the code would always have access to all features, and
we would have no notion of "special" features any more.

I'm happy to give that a try.

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 :|

Re: [PATCH 04/14] qapi: add a 'command-features' pragma

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> So basically the code would always have access to all features, and
> we would have no notion of "special" features any more.

"Special" gets reduced to "predefined / known to the generator".

> I'm happy to give that a try.

Thank you!

[PATCH 05/14] qapi: stop hardcoding list of special features

[Daniel P. Berrangé]

Use the additional list of special features for commands, previously
defined by a pragma, to augment the standard 'unstable' and 'deprecated'
ones.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/qapi/schema.py | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 721c470d2b..b83a9bdcb7 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -932,8 +932,18 @@ def connect_doc(self, doc: Optional[QAPIDoc]) -> None:
 class QAPISchemaFeature(QAPISchemaMember):
     role = 'feature'
 
+    def __init__(
+        self,
+        name: str,
+        info: Optional[QAPISourceInfo],
+        ifcond: Optional[QAPISchemaIfCond] = None,
+        special: bool = False,
+    ):
+        super().__init__(name, info, ifcond)
+        self.special = special
+
     def is_special(self) -> bool:
-        return self.name in ('deprecated', 'unstable')
+        return self.special
 
 
 class QAPISchemaObjectTypeMember(QAPISchemaMember):
@@ -1143,6 +1153,9 @@ def __init__(self, fname: str):
         self._predefining = True
         self._def_predefineds()
         self._predefining = False
+        self._custom_special_features: Dict[str, List[str]] = {
+            'command': parser.info.pragma.command_features
+        }
         self._def_exprs(exprs)
         self.check()
 
@@ -1254,12 +1267,21 @@ def _make_features(
         self,
         features: Optional[List[Dict[str, Any]]],
         info: Optional[QAPISourceInfo],
+        custom_special_features: Optional[List[str]] = [],
     ) -> List[QAPISchemaFeature]:
         if features is None:
             return []
-        return [QAPISchemaFeature(f['name'], info,
-                                  QAPISchemaIfCond(f.get('if')))
-                for f in features]
+        ret = []
+        for f in features:
+            name = f['name']
+            special = name in ["unstable", "deprecated"]
+            if custom_special_features is not None:
+                if name in custom_special_features:
+                    special = True
+            ret.append(QAPISchemaFeature(name, info,
+                                         QAPISchemaIfCond(f.get('if')),
+                                         special))
+        return ret
 
     def _make_enum_member(
         self,
@@ -1430,7 +1452,8 @@ def _def_command(self, expr: QAPIExpression) -> None:
         coroutine = expr.get('coroutine', False)
         ifcond = QAPISchemaIfCond(expr.get('if'))
         info = expr.info
-        features = self._make_features(expr.get('features'), info)
+        features = self._make_features(expr.get('features'), info,
+                                       self._custom_special_features['command'])
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
                 name, info, ifcond,
-- 
2.45.1

[PATCH 06/14] qapi: define enum for custom special features on commands

[Daniel P. Berrangé]

In order to register custom special features against a command,
they have to have enum constants defined. The defined constant
values start where the last built-in special feature stops.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qapi/util.h      |  2 ++
 scripts/qapi/commands.py | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/qapi/util.h b/include/qapi/util.h
index 7698e789a9..3c3c9e401c 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -14,6 +14,8 @@
 typedef enum {
     QAPI_FEATURE_DEPRECATED,
     QAPI_FEATURE_UNSTABLE,
+
+    QAPI_FEATURE_BUILT_IN_LAST,
 } QapiSpecialFeature;
 
 typedef struct QEnumLookup {
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 79951a841f..50a60968d4 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -347,7 +347,27 @@ def visit_begin(self, schema: QAPISchema) -> None:
         self._add_module('./init', ' * QAPI Commands initialization')
         self._genh.add(mcgen('''
 #include "qapi/qmp/dispatch.h"
+'''))
+
+        features = schema._custom_special_features['command']
+        if len(features) > 0:
+            self._genh.add(mcgen('''
+
+typedef enum {
+'''))
+            suffix = " = QAPI_FEATURE_BUILT_IN_LAST"
+            for f in features:
+                self._genh.add(mcgen('''
+    QAPI_FEATURE_%(name)s%(suffix)s,
+''', suffix=suffix, name=f.upper().replace('-', '_')))
+                suffix = ""
 
+            self._genh.add(mcgen('''
+} QapiSpecialFeatureCustom;
+
+'''))
+
+        self._genh.add(mcgen('''
 void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
 ''',
                              c_prefix=c_name(self._prefix, protect=False)))
-- 
2.45.1

[PATCH 07/14] qga: use special feature to mark those that can run when FS are frozen

[Daniel P. Berrangé]

Currently a list of commands which are safe to run when FS are frozen
is hardcoded in the source. Now that the QAPI schema allows custom
special features, a 'fs-frozen' feature can be added to track this
metadata.

This has the benefit that the restrictions on commands permitted when
frozen are now recorded in the QGA generated documentation.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qga/main.c           | 22 ++--------------------
 qga/qapi-schema.json | 44 +++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index c7b7b0a9bc..7bf5ec49ba 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -128,17 +128,6 @@ struct GAState {
 struct GAState *ga_state;
 QmpCommandList ga_commands;
 
-/* commands that are safe to issue while filesystems are frozen */
-static const char *ga_freeze_allowlist[] = {
-    "guest-ping",
-    "guest-info",
-    "guest-sync",
-    "guest-sync-delimited",
-    "guest-fsfreeze-status",
-    "guest-fsfreeze-thaw",
-    NULL
-};
-
 #ifdef _WIN32
 DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
                                   LPVOID ctx);
@@ -421,7 +410,6 @@ static gint ga_strcmp(gconstpointer str1, gconstpointer str2)
 
 static bool ga_command_is_allowed(const QmpCommand *cmd, GAState *state)
 {
-    int i = 0;
     GAConfig *config = state->config;
     const char *name = qmp_command_name(cmd);
     /* Fallback policy is allow everything */
@@ -453,15 +441,9 @@ static bool ga_command_is_allowed(const QmpCommand *cmd, GAState *state)
      * If frozen, this filtering must take priority over
      * absolutely everything
      */
-    if (state->frozen) {
+    if (state->frozen &&
+        !qmp_command_has_feature(cmd, QAPI_FEATURE_FS_FROZEN)) {
         allowed = false;
-
-        while (ga_freeze_allowlist[i] != NULL) {
-            if (strcmp(name, ga_freeze_allowlist[i]) == 0) {
-                allowed = true;
-            }
-            i++;
-        }
     }
 
     return allowed;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 571be3a914..8b1eff3abc 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -36,7 +36,11 @@
         'guest-sync-delimited' ],
     # Types and commands with undocumented members:
     'documentation-exceptions': [
-        'GuestNVMeSmart' ] } }
+        'GuestNVMeSmart' ],
+    'command-features': [
+        # Commands permitted while FS are frozen
+        'fs-frozen'
+    ] } }
 
 ##
 # @guest-sync-delimited:
@@ -67,11 +71,16 @@
 #
 # Returns: The unique integer id passed in by the client
 #
+# Features:
+#
+# @fs-frozen: permitted to execute when filesystems are frozen
+#
 # Since: 1.1
 ##
 { 'command': 'guest-sync-delimited',
   'data':    { 'id': 'int' },
-  'returns': 'int' }
+  'returns': 'int',
+  'features': [ 'fs-frozen'] }
 
 ##
 # @guest-sync:
@@ -104,20 +113,30 @@
 #
 # Returns: The unique integer id passed in by the client
 #
+# Features:
+#
+# @fs-frozen: permitted to execute when filesystems are frozen
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-sync',
   'data':    { 'id': 'int' },
-  'returns': 'int' }
+  'returns': 'int',
+  'features': [ 'fs-frozen'] }
 
 ##
 # @guest-ping:
 #
 # Ping the guest agent, a non-error return implies success
 #
+# Features:
+#
+# @fs-frozen: permitted to execute when filesystems are frozen
+#
 # Since: 0.15.0
 ##
-{ 'command': 'guest-ping' }
+{ 'command': 'guest-ping',
+  'features': [ 'fs-frozen'] }
 
 ##
 # @guest-get-time:
@@ -196,10 +215,15 @@
 #
 # Returns: @GuestAgentInfo
 #
+# Features:
+#
+# @fs-frozen: permitted when filesystems are frozen
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-info',
-  'returns': 'GuestAgentInfo' }
+  'returns': 'GuestAgentInfo',
+  'features': [ 'fs-frozen'] }
 
 ##
 # @guest-shutdown:
@@ -426,10 +450,15 @@
 # Note: This may fail to properly report the current state as a result
 #     of some other guest processes having issued an fs freeze/thaw.
 #
+# Features:
+#
+# @fs-frozen: permitted when filesystems are frozen
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-fsfreeze-status',
   'returns': 'GuestFsfreezeStatus',
+  'features': [ 'fs-frozen'],
   'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSFREEZE'] } }
 
 ##
@@ -488,10 +517,15 @@
 #     filesystems were unfrozen before this call, and that the
 #     filesystem state may have changed before issuing this command.
 #
+# Features:
+#
+# @fs-frozen: permitted when filesystems are frozen
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-fsfreeze-thaw',
   'returns': 'int',
+  'features': [ 'fs-frozen'],
   'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSFREEZE'] } }
 
 ##
-- 
2.45.1

[PATCH 08/14] qga: add command line to limit commands for confidential guests

[Daniel P. Berrangé]

When running in a confidential guest, a very large number of QGA
commands are unsafe to permit, since they can be used to violate
the privacy of the guest.

This introduces a new command line "--confidential" / "-i" which,
if set, will run the guest in confidential mode, which should
avoid leaking information to the host, while still allowing
important VM mgmt tasks to be performed.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qga/main.c           | 14 ++++++++++++++
 qga/qapi-schema.json |  5 ++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/qga/main.c b/qga/main.c
index 7bf5ec49ba..12b91eb713 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -86,6 +86,7 @@ struct GAConfig {
     gchar *aliststr; /* allowedrpcs may point to this string */
     GList *blockedrpcs;
     GList *allowedrpcs;
+    bool only_confidential;
     int daemonize;
     GLogLevelFlags log_level;
     int dumpconf;
@@ -415,6 +416,15 @@ static bool ga_command_is_allowed(const QmpCommand *cmd, GAState *state)
     /* Fallback policy is allow everything */
     bool allowed = true;
 
+    /*
+     * If running in confidential mode, block commands that
+     * would violate guest data privacy
+     */
+    if (config->only_confidential &&
+        !qmp_command_has_feature(cmd, QAPI_FEATURE_CONFIDENTIAL)) {
+        allowed = false;
+    }
+
     if (config->allowedrpcs) {
         /*
          * If an allow-list is given, this changes the fallback
@@ -1197,6 +1207,7 @@ static void config_parse(GAConfig *config, int argc, char **argv)
 #endif
         { "statedir", 1, NULL, 't' },
         { "retry-path", 0, NULL, 'r' },
+        { "confidential", 0, NULL, 'i' },
         { NULL, 0, NULL, 0 }
     };
 
@@ -1293,6 +1304,9 @@ static void config_parse(GAConfig *config, int argc, char **argv)
             }
             break;
 #endif
+        case 'i':
+            config->only_confidential = true;
+            break;
         case 'h':
             usage(argv[0]);
             exit(EXIT_SUCCESS);
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 8b1eff3abc..9a213dfc06 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -39,7 +39,10 @@
         'GuestNVMeSmart' ],
     'command-features': [
         # Commands permitted while FS are frozen
-        'fs-frozen'
+        'fs-frozen',
+        # Commands which do not violate privacy
+        # of a confidential guest
+        'confidential'
     ] } }
 
 ##
-- 
2.45.1

[PATCH 09/14] qga: define commands which can be run in confidential mode

[Daniel P. Berrangé]

This adds the 'confidential' feature tag to the commands which are
safe to permit in confidential VMs. In a confidential virt scenario,
the host must not be permitted to modify guest data, nor request
information that could compromise guest data.

This effectively limits the QGA to commands which either are part
of the QGA operation, or are related to modifying virtual hardware
to assist in a host mgmt tasks.

This results in the following being permitted

 * guest-sync
 * guest-sync-delimited
 * guest-ping
 * guest-get-time
 * guest-set-time
 * guest-info
 * guest-shutdown
 * guest-fsfreeze-status
 * guest-fsfreeze-freeze
 * guest-fsfreeze-freeze-list
 * guest-fsfreeze-thaw
 * guest-fstrim
 * guest-suspend-disk
 * guest-suspend-ram
 * guest-suspend-hybrid
 * guest-get-vcpus
 * guest-set-vcpus
 * guest-get-memory-blocks
 * guest-set-memory-blocks
 * guest-get-memory-block-info
 * guest-get-host-name
 * guest-get-cpustats

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qga/qapi-schema.json | 117 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 107 insertions(+), 10 deletions(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 9a213dfc06..48ea95cdba 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -78,12 +78,14 @@
 #
 # @fs-frozen: permitted to execute when filesystems are frozen
 #
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 1.1
 ##
 { 'command': 'guest-sync-delimited',
   'data':    { 'id': 'int' },
   'returns': 'int',
-  'features': [ 'fs-frozen'] }
+  'features': [ 'fs-frozen', 'confidential' ] }
 
 ##
 # @guest-sync:
@@ -120,12 +122,14 @@
 #
 # @fs-frozen: permitted to execute when filesystems are frozen
 #
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-sync',
   'data':    { 'id': 'int' },
   'returns': 'int',
-  'features': [ 'fs-frozen'] }
+  'features': [ 'fs-frozen', 'confidential' ] }
 
 ##
 # @guest-ping:
@@ -136,10 +140,12 @@
 #
 # @fs-frozen: permitted to execute when filesystems are frozen
 #
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-ping',
-  'features': [ 'fs-frozen'] }
+  'features': [ 'fs-frozen', 'confidential' ] }
 
 ##
 # @guest-get-time:
@@ -149,10 +155,15 @@
 #
 # Returns: Time in nanoseconds.
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 1.5
 ##
 { 'command': 'guest-get-time',
-  'returns': 'int' }
+  'returns': 'int',
+  'features': [ 'confidential' ] }
 
 ##
 # @guest-set-time:
@@ -175,10 +186,15 @@
 # @time: time of nanoseconds, relative to the Epoch of 1970-01-01 in
 #     UTC.
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 1.5
 ##
 { 'command': 'guest-set-time',
-  'data': { '*time': 'int' } }
+  'data': { '*time': 'int' },
+  'features': [ 'confidential' ] }
 
 ##
 # @GuestAgentCommandInfo:
@@ -222,11 +238,13 @@
 #
 # @fs-frozen: permitted when filesystems are frozen
 #
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-info',
   'returns': 'GuestAgentInfo',
-  'features': [ 'fs-frozen'] }
+  'features': [ 'fs-frozen', 'confidential' ] }
 
 ##
 # @guest-shutdown:
@@ -241,10 +259,15 @@
 # when running with --no-shutdown, by issuing the query-status QMP
 # command to confirm the VM status is "shutdown".
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-shutdown', 'data': { '*mode': 'str' },
-  'success-response': false }
+  'success-response': false,
+  'features': [ 'confidential' ] }
 
 ##
 # @guest-file-open:
@@ -457,11 +480,13 @@
 #
 # @fs-frozen: permitted when filesystems are frozen
 #
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-fsfreeze-status',
   'returns': 'GuestFsfreezeStatus',
-  'features': [ 'fs-frozen'],
+  'features': [ 'fs-frozen', 'confidential' ],
   'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSFREEZE'] } }
 
 ##
@@ -481,10 +506,15 @@
 #     Volume Shadow-copy Service DLL helper.  The frozen state is
 #     limited for up to 10 seconds by VSS.
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-fsfreeze-freeze',
   'returns': 'int',
+  'features': [ 'confidential' ],
   'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSFREEZE'] } }
 
 ##
@@ -501,11 +531,16 @@
 #
 # Returns: Number of file systems currently frozen.
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 2.2
 ##
 { 'command': 'guest-fsfreeze-freeze-list',
   'data':    { '*mountpoints': ['str'] },
   'returns': 'int',
+  'features': [ 'confidential' ],
   'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSFREEZE'] } }
 
 ##
@@ -524,11 +559,13 @@
 #
 # @fs-frozen: permitted when filesystems are frozen
 #
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-fsfreeze-thaw',
   'returns': 'int',
-  'features': [ 'fs-frozen'],
+  'features': [ 'fs-frozen', 'confidential' ],
   'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSFREEZE'] } }
 
 ##
@@ -576,11 +613,16 @@
 # Returns: A @GuestFilesystemTrimResponse which contains the status of
 #     all trimmed paths.  (since 2.4)
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 1.2
 ##
 { 'command': 'guest-fstrim',
   'data': { '*minimum': 'int' },
   'returns': 'GuestFilesystemTrimResponse',
+  'features': [ 'confidential' ],
   'if': { 'any': ['CONFIG_WIN32', 'CONFIG_FSTRIM'] } }
 
 ##
@@ -608,9 +650,14 @@
 # Notes: It's strongly recommended to issue the guest-sync command
 #     before sending commands when the guest resumes
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 1.1
 ##
 { 'command': 'guest-suspend-disk', 'success-response': false,
+  'features': [ 'confidential' ],
   'if': { 'any': ['CONFIG_LINUX', 'CONFIG_WIN32'] } }
 
 ##
@@ -645,9 +692,14 @@
 # Notes: It's strongly recommended to issue the guest-sync command
 #     before sending commands when the guest resumes
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 1.1
 ##
 { 'command': 'guest-suspend-ram', 'success-response': false,
+  'features': [ 'confidential' ],
   'if': { 'any': ['CONFIG_LINUX', 'CONFIG_WIN32'] } }
 
 ##
@@ -681,9 +733,14 @@
 # Notes: It's strongly recommended to issue the guest-sync command
 #     before sending commands when the guest resumes
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 1.1
 ##
 { 'command': 'guest-suspend-hybrid', 'success-response': false,
+  'features': [ 'confidential' ],
   'if': 'CONFIG_LINUX' }
 
 ##
@@ -815,10 +872,15 @@
 # Returns: The list of all VCPUs the guest knows about.  Each VCPU is
 #     put on the list exactly once, but their order is unspecified.
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 1.5
 ##
 { 'command': 'guest-get-vcpus',
   'returns': ['GuestLogicalProcessor'],
+  'features': [ 'confidential' ],
   'if': { 'any': ['CONFIG_LINUX', 'CONFIG_WIN32'] } }
 
 ##
@@ -857,11 +919,16 @@
 #     - If the reconfiguration of the first node in @vcpus failed.
 #       Guest state has not been changed.
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 1.5
 ##
 { 'command': 'guest-set-vcpus',
   'data':    {'vcpus': ['GuestLogicalProcessor'] },
   'returns': 'int',
+  'features': [ 'confidential' ],
   'if': 'CONFIG_LINUX' }
 
 ##
@@ -1180,10 +1247,15 @@
 #     memory block is put on the list exactly once, but their order is
 #     unspecified.
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 2.3
 ##
 { 'command': 'guest-get-memory-blocks',
   'returns': ['GuestMemoryBlock'],
+  'features': [ 'confidential' ],
   'if': 'CONFIG_LINUX' }
 
 ##
@@ -1254,11 +1326,16 @@
 #     empty on input, or there is an error, and in this case, guest
 #     state will not be changed.
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 2.3
 ##
 { 'command': 'guest-set-memory-blocks',
   'data':    {'mem-blks': ['GuestMemoryBlock'] },
   'returns': ['GuestMemoryBlockResponse'],
+  'features': [ 'confidential' ],
   'if': 'CONFIG_LINUX' }
 
 ##
@@ -1268,10 +1345,15 @@
 #     minimal units of memory block online/offline operations (also
 #     called Logical Memory Hotplug).
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 2.3
 ##
 { 'struct': 'GuestMemoryBlockInfo',
   'data': {'size': 'uint64'},
+  'features': [ 'confidential' ],
   'if': 'CONFIG_LINUX' }
 
 ##
@@ -1281,10 +1363,15 @@
 #
 # Returns: @GuestMemoryBlockInfo
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 2.3
 ##
 { 'command': 'guest-get-memory-block-info',
   'returns': 'GuestMemoryBlockInfo',
+  'features': [ 'confidential' ],
   'if': 'CONFIG_LINUX' }
 
 ##
@@ -1430,10 +1517,15 @@
 #
 # Returns: the host name of the machine
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 2.10
 ##
 { 'command': 'guest-get-host-name',
-  'returns': 'GuestHostName' }
+  'returns': 'GuestHostName',
+  'features': [ 'confidential' ] }
 
 
 ##
@@ -1882,9 +1974,14 @@
 #
 # Returns: List of CPU stats of guest.
 #
+# Features:
+#
+# @confidential: permitted when running inside a confidential VM
+#
 # Since: 7.1
 ##
 { 'command': 'guest-get-cpustats',
   'returns': ['GuestCpuStats'],
+  'features': [ 'confidential' ],
   'if': 'CONFIG_LINUX'
 }
-- 
2.45.1

[PATCH 10/14] qga: add command line to block unrestricted command/file access

[Daniel P. Berrangé]

Historically there has been no default policy on command usage in
the QEMU guest agent. A wide variety of commands have been added
for various purposes

 * Co-ordinating host mgmt tasks (FS freezing, CPU hotplug,
   memory block hotplug)
 * Guest information querying (CPU stats, mount info, etc)
 * Arbitrary file read/write and command execution
 * User account auth setup (passwords, SSH keys)

All of these have valid use cases, but they come with very different
levels of risk to the guest OS.

The commands supporting arbitrary file access / command exec though
are giving the guest agent client effectively unrestricted access to
do anything at all in the guest OS.

The guest agent client is the host OS, so in effect running the QEMU
guest agent gives the host admin a trivial direct backdoor into the
guest OS, with no authentication, authorization or auditing of what
they do.

In the absense of confidential computing, the host admin already has
to be considered largely trustworthy, as they will typically have
direct access to any guest RAM regardless.

None the less, to limit their exposure, guest OS admins may choose
to limit these commands by passing '--no-unrestricted' / '-u' to
QGA

The --allowedrpcs / --blockedrpcs arguments take precedence over the
--unrestricted arg (whether present or not), thus allowing fine tuning
the defaults further.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qga/main.c           | 15 +++++++++++++++
 qga/qapi-schema.json |  5 ++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/qga/main.c b/qga/main.c
index 12b91eb713..66068ad535 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -87,6 +87,7 @@ struct GAConfig {
     GList *blockedrpcs;
     GList *allowedrpcs;
     bool only_confidential;
+    bool no_unrestricted;
     int daemonize;
     GLogLevelFlags log_level;
     int dumpconf;
@@ -425,6 +426,16 @@ static bool ga_command_is_allowed(const QmpCommand *cmd, GAState *state)
         allowed = false;
     }
 
+    /*
+     * If unrestricted commands are not allowed that sets
+     * a new default, but an explicit allow/block list can
+     * override
+     */
+    if (config->no_unrestricted &&
+        qmp_command_has_feature(cmd, QAPI_FEATURE_UNRESTRICTED)) {
+        allowed = false;
+    }
+
     if (config->allowedrpcs) {
         /*
          * If an allow-list is given, this changes the fallback
@@ -1208,6 +1219,7 @@ static void config_parse(GAConfig *config, int argc, char **argv)
         { "statedir", 1, NULL, 't' },
         { "retry-path", 0, NULL, 'r' },
         { "confidential", 0, NULL, 'i' },
+        { "no-unrestricted", 0, NULL, 'u' },
         { NULL, 0, NULL, 0 }
     };
 
@@ -1307,6 +1319,9 @@ static void config_parse(GAConfig *config, int argc, char **argv)
         case 'i':
             config->only_confidential = true;
             break;
+        case 'u':
+            config->no_unrestricted = true;
+            break;
         case 'h':
             usage(argv[0]);
             exit(EXIT_SUCCESS);
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 48ea95cdba..de7c1de0b7 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -42,7 +42,10 @@
         'fs-frozen',
         # Commands which do not violate privacy
         # of a confidential guest
-        'confidential'
+        'confidential',
+        # Commands which allow unrestricted access to or
+        # modification of guest files or execute arbitrary commands
+        'unrestricted'
     ] } }
 
 ##
-- 
2.45.1

[PATCH 11/14] qga: mark guest-file-* commands with 'unrestricted' flag

[Daniel P. Berrangé]

This blocks use of all the 'guest-file-*' commands unless the QGA is run
with the --unrestricted command line argument.

These commands allow the host admin to read and write arbitrary guest
files and so directly compromise the guest OS.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qga/qapi-schema.json | 48 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index de7c1de0b7..2f80d89536 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -283,11 +283,17 @@
 #
 # Returns: Guest file handle
 #
+# Features:
+#
+# @unrestricted: not permitted if agent disables unrestricted
+#                resource access mode
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-file-open',
   'data':    { 'path': 'str', '*mode': 'str' },
-  'returns': 'int' }
+  'returns': 'int',
+  'features': [ 'unrestricted' ] }
 
 ##
 # @guest-file-close:
@@ -296,10 +302,16 @@
 #
 # @handle: filehandle returned by guest-file-open
 #
+# Features:
+#
+# @unrestricted: not permitted if agent disables unrestricted
+#                resource access mode
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-file-close',
-  'data': { 'handle': 'int' } }
+  'data': { 'handle': 'int' },
+  'features': [ 'unrestricted' ] }
 
 ##
 # @GuestFileRead:
@@ -332,11 +344,17 @@
 #
 # Returns: @GuestFileRead
 #
+# Features:
+#
+# @unrestricted: not permitted if agent disables unrestricted
+#                resource access mode
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-file-read',
   'data':    { 'handle': 'int', '*count': 'int' },
-  'returns': 'GuestFileRead' }
+  'returns': 'GuestFileRead',
+  'features': [ 'unrestricted' ] }
 
 ##
 # @GuestFileWrite:
@@ -367,11 +385,17 @@
 #
 # Returns: @GuestFileWrite
 #
+# Features:
+#
+# @unrestricted: not permitted if agent disables unrestricted
+#                resource access mode
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-file-write',
   'data':    { 'handle': 'int', 'buf-b64': 'str', '*count': 'int' },
-  'returns': 'GuestFileWrite' }
+  'returns': 'GuestFileWrite',
+  'features': [ 'unrestricted' ] }
 
 
 ##
@@ -434,12 +458,18 @@
 #
 # Returns: @GuestFileSeek
 #
+# Features:
+#
+# @unrestricted: not permitted if agent disables unrestricted
+#                resource access mode
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-file-seek',
   'data':    { 'handle': 'int', 'offset': 'int',
                'whence': 'GuestFileWhence' },
-  'returns': 'GuestFileSeek' }
+  'returns': 'GuestFileSeek',
+  'features': [ 'unrestricted' ] }
 
 ##
 # @guest-file-flush:
@@ -448,10 +478,16 @@
 #
 # @handle: filehandle returned by guest-file-open
 #
+# Features:
+#
+# @unrestricted: not permitted if agent disables unrestricted
+#                resource access mode
+#
 # Since: 0.15.0
 ##
 { 'command': 'guest-file-flush',
-  'data': { 'handle': 'int' } }
+  'data': { 'handle': 'int' },
+  'features': [ 'unrestricted' ] }
 
 ##
 # @GuestFsfreezeStatus:
-- 
2.45.1

[PATCH 12/14] qga: mark guest-exec-* commands with 'unrestricted' flag

[Daniel P. Berrangé]

This blocks use of all the 'guest-exec-*' commands unless the QGA is run
with the --unrestricted command line argument.

These commands allow the host admin to execute arbitrary programs and so
directly compromise the guest OS.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qga/qapi-schema.json | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 2f80d89536..a4f8653446 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1454,11 +1454,17 @@
 #
 # Returns: GuestExecStatus
 #
+# Features:
+#
+# @unrestricted: not permitted if agent disables unrestricted
+#                resource access mode
+#
 # Since: 2.5
 ##
 { 'command': 'guest-exec-status',
   'data':    { 'pid': 'int' },
-  'returns': 'GuestExecStatus' }
+  'returns': 'GuestExecStatus',
+  'features': [ 'unrestricted' ] }
 
 ##
 # @GuestExec:
@@ -1527,12 +1533,18 @@
 #
 # Returns: PID
 #
+# Features:
+#
+# @unrestricted: not permitted if agent disables unrestricted
+#                resource access mode
+#
 # Since: 2.5
 ##
 { 'command': 'guest-exec',
   'data':    { 'path': 'str', '*arg': ['str'], '*env': ['str'],
                '*input-data': 'str', '*capture-output': 'GuestExecCaptureOutput' },
-  'returns': 'GuestExec' }
+  'returns': 'GuestExec',
+  'features': [ 'unrestricted' ] }
 
 
 ##
-- 
2.45.1

[PATCH 13/14] qga: add command line to block user authentication commands

[Daniel P. Berrangé]

Historically there has been no default policy on command usage in
the QEMU guest agent. A wide variety of commands have been added
for various purposes

 * Co-ordinating host mgmt tasks (FS freezing, CPU hotplug,
   memory block hotplug)
 * Guest information querying (CPU stats, mount info, etc)
 * Arbitrary file read/write and command execution
 * User account auth setup (passwords, SSH keys)

All of these have valid use cases, but they come with very different
levels of risk to the guest OS.

The commands supporting alteration of user authentication credentials
are giving the guest agent client effectively unrestricted access to
do anything at all in the guest OS by enabling them to subsequently
access a user login shell.

The guest agent client is the host OS, so in effect running the QEMU
guest agent gives the host admin a trivial direct backdoor into the
guest OS.

In the absense of confidential computing, the host admin already has
to be considered largely trustworthy, as they will typically have
direct access to any guest RAM regardless.

None the less, to limit their exposure, guest OS admins may choose
to limit these commands by passing '--no-user-auth' / '-e' to
QGA

The --allowedrpcs / --blockedrpcs arguments take precedence over the
--unrestricted arg (whether present or not), thus allowing fine tuning
the defaults further.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qga/main.c           | 15 +++++++++++++++
 qga/qapi-schema.json |  5 ++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/qga/main.c b/qga/main.c
index 66068ad535..0d792cd92e 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -88,6 +88,7 @@ struct GAConfig {
     GList *allowedrpcs;
     bool only_confidential;
     bool no_unrestricted;
+    bool no_user_auth;
     int daemonize;
     GLogLevelFlags log_level;
     int dumpconf;
@@ -436,6 +437,16 @@ static bool ga_command_is_allowed(const QmpCommand *cmd, GAState *state)
         allowed = false;
     }
 
+    /*
+     * If user auth commands are not allowed that sets
+     * a new default, but an explicit allow/block list can
+     * override
+     */
+    if (config->no_user_auth &&
+        qmp_command_has_feature(cmd, QAPI_FEATURE_USER_AUTH)) {
+        allowed = false;
+    }
+
     if (config->allowedrpcs) {
         /*
          * If an allow-list is given, this changes the fallback
@@ -1220,6 +1231,7 @@ static void config_parse(GAConfig *config, int argc, char **argv)
         { "retry-path", 0, NULL, 'r' },
         { "confidential", 0, NULL, 'i' },
         { "no-unrestricted", 0, NULL, 'u' },
+        { "no-user-auth", 0, NULL, 'e' },
         { NULL, 0, NULL, 0 }
     };
 
@@ -1322,6 +1334,9 @@ static void config_parse(GAConfig *config, int argc, char **argv)
         case 'u':
             config->no_unrestricted = true;
             break;
+        case 'e':
+            config->no_user_auth = true;
+            break;
         case 'h':
             usage(argv[0]);
             exit(EXIT_SUCCESS);
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index a4f8653446..25068b8110 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -45,7 +45,10 @@
         'confidential',
         # Commands which allow unrestricted access to or
         # modification of guest files or execute arbitrary commands
-        'unrestricted'
+        'unrestricted',
+        # Commands which allow changes to user account
+        # authentication credentials (keys, passwords)
+        'user-auth'
     ] } }
 
 ##
-- 
2.45.1

[PATCH 14/14] qga: mark guest-ssh-* / guest-*-password commands with 'unrestricted' flag

[Daniel P. Berrangé]

This blocks use of all the 'guest-ssh-*' / 'guest-password' commands
unless the QGA is runwith the --unrestricted command line argument.

These commands allow the host admin to takeover user accounts and so
directly compromise the guest OS.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qga/qapi-schema.json | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 25068b8110..e7ce80a479 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1251,10 +1251,15 @@
 # transmission, even if already crypt()d, to ensure it is 8-bit safe
 # when passed as JSON.
 #
+# Features:
+#
+# @user-auth: not permitted if agent is limiting user auth
+#
 # Since: 2.3
 ##
 { 'command': 'guest-set-user-password',
   'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool' },
+  'features': [ 'user-auth' ],
   'if': { 'any': [ 'CONFIG_WIN32', 'CONFIG_LINUX', 'CONFIG_FREEBSD'] } }
 
 ##
@@ -1810,11 +1815,16 @@
 #
 # Returns: @GuestAuthorizedKeys
 #
+# Features:
+#
+# @user-auth: not permitted if agent is limiting user auth
+#
 # Since: 5.2
 ##
 { 'command': 'guest-ssh-get-authorized-keys',
   'data': { 'username': 'str' },
-  'returns': 'GuestAuthorizedKeys'
+  'returns': 'GuestAuthorizedKeys',
+  'features': [ 'user-auth' ]
 }
 
 ##
@@ -1830,10 +1840,15 @@
 #
 # @reset: ignore the existing content, set it with the given keys only
 #
+# Features:
+#
+# @user-auth: not permitted if agent is limiting user auth
+#
 # Since: 5.2
 ##
 { 'command': 'guest-ssh-add-authorized-keys',
-  'data': { 'username': 'str', 'keys': ['str'], '*reset': 'bool' }
+  'data': { 'username': 'str', 'keys': ['str'], '*reset': 'bool' },
+  'features': [ 'user-auth' ]
 }
 
 ##
@@ -1848,10 +1863,15 @@
 # @keys: the public keys to remove (in OpenSSH/sshd(8) authorized_keys
 #     format)
 #
+# Features:
+#
+# @user-auth: not permitted if agent is limiting user auth
+#
 # Since: 5.2
 ##
 { 'command': 'guest-ssh-remove-authorized-keys',
-  'data': { 'username': 'str', 'keys': ['str'] }
+  'data': { 'username': 'str', 'keys': ['str'] },
+  'features': [ 'user-auth' ]
 }
 
 ##
-- 
2.45.1

Re: [PATCH 00/14] Improve mechanism for configuring allowed commands

[Daniel P. Berrangé]

Ping: any review comments from QGA maintainers ?

On Tue, Jun 04, 2024 at 04:32:28PM +0100, Daniel P. Berrangé wrote:
> The QGA supports dynamically filtering what commands are enabled via a
> combination of allow lists and deny lists. This is very flexible, but
> at the same time very fragile.
> 
> Consider that a user wants to block all commands that allow unrestricted
> file access/command execution, so they set the deny list when starting
> QGA. Now their OS vendor issues a software update which includes a new
> version of QGA. This new QGA version is liable to contain new commands,
> some of which might undermine the intent of the user's configured deny
> list.
> 
> IOW, the generic deny list functionality is inherently dangerous as a
> mechanism for limiting risk exposure.
> 
> Using an allow list is much safer, but means on every update the user
> should check the list of new commands to decide which are safe or not,
> putting a burden on every user.
> 
> In the context of RHEL, there has been a long term deny list that blocks
> use of guest-file and guest-exec commands, since they give unrestricted
> access to the guest.
> 
> With the advent of confidential computing, a far greater number of QGA
> commands are very unsafe to permit, and it is unreasonable to expect
> each user and/or downstream vendor to repeat the work to figure out
> what commands are OK.
> 
> This is a similar problem seen in the "seccomp" world where new syscalls
> appear frequently and users can't be expected to understand all of them.
> Systemd pioneered the approach of defining "profiles"  which group
> together sets of syscalls, which we subsequently copied in QEMU.
> 
> This series applies this same conceptual idea to QGA command filtering,
> making use of the QAPI "features" facility to associate commands into
> one or more groups.
> 
> This grouping is then exposed via some new higher level command line
> arguments.
> 
> * --no-unrestricted / -u
> 
>   A flag to block all the guest-file and guest-exec commands
> 
>   This replicates the policy RHEL currently defines via a deny list.
> 
> * --no-user-auth / -e
> 
>   A flag to block all the commands for manipulating user account
>   authentication credentials.
> 
> * --confidential / -i
> 
>   A flag to block all commands, except for those which have been
>   explicitly marked as not violating guest owner data privacy
> 
> This feature mechanism is further utilized internally to track the
> commands which are safe to use while FS are frozen.
> 
> A key benefit of using the QAPI "features" facility is that these
> groupings are visible in the documentation of the QGA commands.
> 
> By using these high level command lines arguments, deployments will
> be safe wrt software upgrades, as long as QEMU maintainers apply
> appropriate tags to any new commands.
> 
> The allow/deny list command line flags can still be used to further
> refine the command lines, but ideally that would be rare.
> 
> A missing piece in this series is getting the --confidential flag to
> be automatically passed to QGA when running in a confidential VM. This
> is something that will likely be done via systemd unit files. My thought
> is that the existing 'qemu-guest-agent.service' would get a parameter
> 
>    ConditionSecurity=!cvm
> 
> while a new qemu-guest-agent-confidential.service' would have the same
> content but with ConditionSecurity=cvm instead, and would pass the
> --confidential flag.
> 
> This series depends on the one I sent earlier:
> 
>   https://lists.nongnu.org/archive/html/qemu-devel/2024-06/msg00743.html
> 
> Daniel P. Berrangé (14):
>   qapi: use "QAPI_FEATURE" as namespace for special features
>   qapi: add helper for checking if a command feature is set
>   qapi: cope with special feature names containing a '-'
>   qapi: add a 'command-features' pragma
>   qapi: stop hardcoding list of special features
>   qapi: define enum for custom special features on commands
>   qga: use special feature to mark those that can run when FS are frozen
>   qga: add command line to limit commands for confidential guests
>   qga: define commands which can be run in confidential mode
>   qga: add command line to block unrestricted command/file access
>   qga: mark guest-file-* commands with 'unrestricted' flag
>   qga: mark guest-exec-* commands with 'unrestricted' flag
>   qga: add command line to block user authentication commands
>   qga: mark guest-ssh-* / guest-*-password commands with 'unrestricted'
>     flag
> 
>  include/qapi/qmp/dispatch.h   |   1 +
>  include/qapi/util.h           |   6 +-
>  qapi/qapi-util.c              |   4 +-
>  qapi/qmp-registry.c           |   5 +
>  qapi/qobject-output-visitor.c |   4 +-
>  qga/main.c                    |  66 ++++++---
>  qga/qapi-schema.json          | 248 +++++++++++++++++++++++++++++++---
>  scripts/qapi/commands.py      |  20 +++
>  scripts/qapi/gen.py           |   2 +-
>  scripts/qapi/parser.py        |   2 +
>  scripts/qapi/schema.py        |  33 ++++-
>  scripts/qapi/source.py        |   2 +
>  12 files changed, 341 insertions(+), 52 deletions(-)
> 
> -- 
> 2.45.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 :|

Re: [PATCH 00/14] Improve mechanism for configuring allowed commands

[Markus Armbruster]

Hi Daniel, got a public branch I could pull?

Re: [PATCH 00/14] Improve mechanism for configuring allowed commands

[Daniel P. Berrangé]

On Mon, Jul 15, 2024 at 11:52:10AM +0200, Markus Armbruster wrote:
> Hi Daniel, got a public branch I could pull?

This particular v1 posting:

   https://gitlab.com/berrange/qemu/-/tags/qga-features-v1

Or latest git master rebase

   https://gitlab.com/berrange/qemu/-/tree/qga-features

NB, this is on top of my other qga patch series changing build time
conditions, so the above branch includes this branch:

   https://gitlab.com/berrange/qemu/-/tree/qga-conditions

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 :|