Re: [PULL 02/14] qapi: add 'insecure-types' option for -compat argument

[Markus Armbruster <armbru@redhat.com>]

This has long been merged.  I couldn't review it at the time, and
merging it without my review was clearly appropriate.

I have questions / doubts about the external interface I'd like to
discuss now.

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

> This introduces a new 'insecure-types' option for the 'compat'
> argument that accepts three values
>
>  * accept: Allow any usage
>  * reject: Reject with an error reported
>  * warn: Allow any usage, with a warning reported
>
> For historical compatibility it defaults to 'accept'.
>
> The 'reject' and 'warn' values will take effect for any type
> that has been explicitly marked insecure, or is lacking an
> explicit declaration of its security status.
>
> This new command line option is currently a no-op, but will
> become functional as following patches enable the checks.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/qapi/compat-policy.h |  5 +++++
>  qapi/compat.json             | 23 ++++++++++++++++++++++-
>  qapi/qapi-util.c             | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/include/qapi/compat-policy.h b/include/qapi/compat-policy.h
> index ea65e10744a..f5af2090692 100644
> --- a/include/qapi/compat-policy.h
> +++ b/include/qapi/compat-policy.h
> @@ -24,6 +24,11 @@ bool compat_policy_input_ok(uint64_t features,
>                              const char *kind, const char *name,
>                              Error **errp);
>  
> +bool compat_policy_check_security(const CompatPolicy *policy,
> +                                  const char *typename,
> +                                  bool is_secure,
> +                                  Error **errp);
> +
>  /*
>   * Create a QObject input visitor for @obj for use with QMP
>   *
> diff --git a/qapi/compat.json b/qapi/compat.json
> index 90b8d51cf27..b54f8eb3713 100644
> --- a/qapi/compat.json
> +++ b/qapi/compat.json
> @@ -37,6 +37,23 @@
>  { 'enum': 'CompatPolicyOutput',
>    'data': [ 'accept', 'hide' ] }
>  
> +##
> +# @CompatPolicySecurity:
> +#
> +# Policy for handling any devices or backends which do not provide a
> +# security boundary to protect against untrusted environments
> +#
> +# @accept: Allow any usage
> +#
> +# @reject: Reject with an error reported
> +#
> +# @warn: Allow any usage, with a warning reported
> +#
> +# Since: 11.2
> +##
> +{ 'enum': 'CompatPolicySecurity',
> +  'data': [ 'accept', 'reject', 'warn' ] }
> +
>  ##
>  # @CompatPolicy:
>  #
> @@ -62,10 +79,14 @@
   # Policy for handling deprecated management interfaces.
   #
   # This is intended for testing users of the management interfaces.
   #
   # Limitation: covers only syntactic aspects of QMP, i.e. stuff tagged
   # with feature 'deprecated' or 'unstable'.  We may want to extend it
   # to cover semantic aspects and CLI.
   #
   # Limitation: deprecated-output policy @hide is not implemented for
   # enumeration values.  They behave the same as with policy @accept.
   #
   # @deprecated-input: how to handle deprecated input (default 'accept')
   #
   # @deprecated-output: how to handle deprecated output (default
   #     'accept')
   #
   # @unstable-input: how to handle unstable input (default 'accept')
   #     (since 6.2)
   #
>  # @unstable-output: how to handle unstable output (default 'accept')
>  #     (since 6.2)
>  #
> +# @insecure-types: how to handle types that are not declared secure
> +#     (default 'accept') (since 11.2)

Minor issue: "types" is ambiguous without context.  We call lots of
things "types" in our external interfaces.  It's actually QOM types
here.  Let's say so.  I can take care of it.

Similar confusion exists elsewhere[*], but let's not digress.

> +#
>  # Since: 6.0
>  ##
>  { 'struct': 'CompatPolicy',
>    'data': { '*deprecated-input': 'CompatPolicyInput',
>              '*deprecated-output': 'CompatPolicyOutput',
>              '*unstable-input': 'CompatPolicyInput',
> -            '*unstable-output': 'CompatPolicyOutput' } }
> +            '*unstable-output': 'CompatPolicyOutput',
> +            '*insecure-types': 'CompatPolicySecurity' } }

The existing members are all named "wide", and the documentation narrows
them.  For instance, the name @deprecated-input could cover *any* input,
but its documentation narrows it to "only syntactic aspects of QMP".  I
did it this way to let us extend coverage without having to change the
external interface.

New member @insecure-types is named "narrow".  Why different?  Why not
@insecure-input?

The existing @FOO-input support CompatPolicyInput: "accept", "reject",
"crash".  New insecure-types supports CompatPolicySecurity: "accept",
"reject", "warn".  Why different?

My rationale for "crash" is in commit dbb675c19aa (qapi: New -compat
deprecated-input=crash):

    Bugs in integration tests may mask the error from policy "reject".
    Provide a larger hammer: crash outright.  Masking that seems unlikely.

If "crash" can be useful for testing a management application's non-use
of deprecated and unstable interfaces, surely it's just as useful for
insecure ones.

Likewise, "warn".  I didn't provide "warn" at the time, because QMP
lacks support for warnings.  Do management applications find warnings on
stderr in response to QMP commands useful?

-util.c b/qapi/qapi-util.c
> index 3d849fe0347..38b1cec7a41 100644
> --- a/qapi/qapi-util.c
> +++ b/qapi/qapi-util.c
> @@ -14,6 +14,7 @@
>  #include "qapi/compat-policy.h"
>  #include "qapi/error.h"
>  #include "qemu/ctype.h"
> +#include "qemu/error-report.h"
>  #include "qapi/qmp/qerror.h"
>  
>  CompatPolicy compat_policy;
> @@ -58,6 +59,35 @@ bool compat_policy_input_ok(uint64_t features,
>      return true;
>  }
>  
> +bool compat_policy_check_security(const CompatPolicy *policy,
> +                                  const char *typename,
> +                                  bool is_secure,
> +                                  Error **errp)

More minor issues.

The policy functions are named compat_policy_FOO_ok().  Why different?

Their arguments are "relevant facts" (@features), @policy, "stuff for
error message" (@error_class, @kind, @name), @errp.  This one has the
same things in different order.  Why?

> +{
> +    if (is_secure) {
> +        return true;
> +    }
> +
> +    switch (policy->insecure_types) {
> +    case COMPAT_POLICY_SECURITY_ACCEPT:
> +        return true;
> +
> +    case COMPAT_POLICY_SECURITY_REJECT:
> +        error_setg(errp, "Type '%s' does not provide a security boundary "
> +                   "to protect against untrusted data or actions", typename);
> +        return false;
> +
> +    case COMPAT_POLICY_SECURITY_WARN:
> +        warn_report("Type '%s' does not provide a security boundary "
> +                    "to protect against untrusted data or actions", typename);
> +        return true;
> +
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +
>  const char *qapi_enum_lookup(const QEnumLookup *lookup, int val)
>  {
>      assert(val >= 0 && val < lookup->size);


[*] So I digress in a footnote.  Sue me ;)

While examining the QAPI schema, I found near object-add:

    ##
    # @qom-list-properties:
    #     List properties associated with a QOM object.
    #
    # @typename: the type name of an object
    [...]
    ##
    # @object-add:
    #     Create a QOM object.
    #
    # Errors:
    #     - If @qom-type is not a valid class name
    [...]
    ##
    # @ObjectOptions:
    #     Describes the options of a user creatable QOM object.
    #
    # @qom-type: the class name for the object to be created

We use "QOM type" and "QOM class".  This is pretty bad; we should pick
one and stick with it.

And near device_add:

    ##
    # @device-list-properties:
    #     List properties associated with a device.
    #
    # @typename: the type name of a device
    [...]
    ##
    # @device_add:
    #     Add a device.
    #
    # @driver: the name of the new device's driver

We use "driver" and "device type".  Similarly bad.

Not today.