Re: [Qemu-devel] [PATCH v5 1/3] hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()

[Daniel P . Berrangé]

On Thu, Jun 20, 2019 at 02:21:30PM +0200, Philippe Mathieu-Daudé wrote:
> The Edk2Crypto object is used to hold configuration values specific
> to EDK2.
> 
> The edk2_add_host_crypto_policy() function loads crypto policies
> from the host, and register them as fw_cfg named file items.
> So far only the 'https' policy is supported.
> 
> A usercase example is the 'HTTPS Boof' feature of OVMF [*].
> 
> Usage example:
> 
> - via the command line:
> 
>   $ qemu-system-x86_64 \
>       --object edk2_crypto,id=https,\
>               ciphers=/etc/crypto-policies/back-ends/openssl.config,\
>               cacerts=/etc/pki/ca-trust/extracted/edk2/cacerts.bin
> 
> - via QMP:
> 
>   {
>     "execute": "object-add",
>     "arguments": {
>       "qom-type": "edk2_crypto",
>       "id": "https",
>       "props": {
>         "ciphers": "/etc/crypto-policies/back-ends/openssl.config",
>         "cacerts": "/etc/pki/ca-trust/extracted/edk2/cacerts.bin"
>       }
>     }
>   }

These commands / args create an object with ID "https" but you are
not telling anything to use this object, which makes me fear that
you have some code somewhere hardcoded to mandate an object with
an ID of "https".....


> +static void edk2_add_host_crypto_policy_https(FWCfgState *fw_cfg)
> +{
> +    Edk2Crypto *s;
> +
> +    s = edk2_crypto_by_policy_id("https", NULL);

....indeed you have hardcoded use of a particular object ID.

This is not good - choice of what strings to use for object IDs is something
for the the management app - QEMU must not be dictating certain magic IDs.

There needs to be a command line arg somewhere to tell the firmware what
object ID provides the data.

Given that you're triggering load of this object from the machine type
initializer code, having an arg to the -machine option is a natural
choice.

ie something like this:

   $ qemu-system-x86_64 \
       --object edk2_crypto,id=edkpolicy0,\
               ciphers=/etc/crypto-policies/back-ends/openssl.config,\
               cacerts=/etc/pki/ca-trust/extracted/edk2/cacerts.bin \
       --machine q35,edk2_crypto_policy=edkpolicy0

I don't see a compelling reason to separate https policy out as an
explicit tunable, distinct from other, yet to be invented, needs for
crypto policy even if EDK2 itself is fine grained in this way.

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