Re: [PATCH v4 4/6] machine/nitro-enclave: Add built-in Nitro Secure Module device

[Daniel P . Berrangé]

On Mon, Aug 19, 2024 at 10:07:02PM +0600, Dorjoy Chowdhury wrote:
> On Mon, Aug 19, 2024 at 9:53 PM Daniel P. Berrangé <berrange@redhat.com> 
> wrote:
> >
> > On Mon, Aug 19, 2024 at 09:32:55PM +0600, Dorjoy Chowdhury wrote:
> > > On Mon, Aug 19, 2024 at 4:13 PM Alexander Graf <graf@amazon.com> wrote:
> > > >
> > > > Hey Dorjoy,
> > > >
> > > > On 18.08.24 13:42, Dorjoy Chowdhury wrote:
> > > > > AWS Nitro Enclaves have built-in Nitro Secure Module (NSM) device 
> > > > > which
> > > > > is used for stripped down TPM functionality like attestation. This 
> > > > > commit
> > > > > adds the built-in NSM device in the nitro-enclave machine type.
> > > > >
> > > > > In Nitro Enclaves, all the PCRs start in a known zero state and the 
> > > > > first
> > > > > 16 PCRs are locked from boot and reserved. The PCR0, PCR1, PCR2 and 
> > > > > PCR8
> > > > > contain the SHA384 hashes related to the EIF file used to boot the
> > > > > VM for validation.
> > > > >
> > > > > Some optional nitro-enclave machine options have been added:
> > > > >      - 'id': Enclave identifier, reflected in the module-id of the NSM
> > > > > device. If not provided, a default id will be set.
> > > > >      - 'parent-role': Parent instance IAM role ARN, reflected in PCR3
> > > > > of the NSM device.
> > > > >      - 'parent-id': Parent instance identifier, reflected in PCR4 of 
> > > > > the
> > > > > NSM device.
> > > > >
> > > > > Signed-off-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>
> > > > > ---
> > > > >   crypto/meson.build              |   2 +-
> > > > >   crypto/x509-utils.c             |  73 +++++++++++
> > > >
> > > >
> > > > Can you please put this new API into its own patch file?
> > > >
> > > >
> > > > >   hw/core/eif.c                   | 225 
> > > > > +++++++++++++++++++++++++++++---
> > > > >   hw/core/eif.h                   |   5 +-
> > > >
> > > >
> > > > These changes to eif.c should ideally already be part of the patch that
> > > > introduces eif.c (patch 1), no? In fact, do you think you can make the
> > > > whole eif logic its own patch file?
> > > >
> > > >
> > > > >   hw/core/meson.build             |   4 +-
> > > > >   hw/i386/Kconfig                 |   1 +
> > > > >   hw/i386/nitro_enclave.c         | 141 +++++++++++++++++++-
> > > > >   include/crypto/x509-utils.h     |  22 ++++
> > > > >   include/hw/i386/nitro_enclave.h |  26 ++++
> > > > >   9 files changed, 479 insertions(+), 20 deletions(-)
> > > > >   create mode 100644 crypto/x509-utils.c
> > > > >   create mode 100644 include/crypto/x509-utils.h
> > > > >
> > > > > diff --git a/crypto/meson.build b/crypto/meson.build
> > > > > index c46f9c22a7..09633194ed 100644
> > > > > --- a/crypto/meson.build
> > > > > +++ b/crypto/meson.build
> > > > > @@ -62,7 +62,7 @@ endif
> > > > >   if gcrypt.found()
> > > > >     util_ss.add(gcrypt, files('random-gcrypt.c'))
> > > > >   elif gnutls.found()
> > > > > -  util_ss.add(gnutls, files('random-gnutls.c'))
> > > > > +  util_ss.add(gnutls, files('random-gnutls.c', 'x509-utils.c'))
> > > >
> > > >
> > > > What if we don't have gnutls. Will everything still compile or do we
> > > > need to add any dependencies?
> > > >
> > > >
> > >
> > > [...]
> > >
> > > > >
> > > > > diff --git a/hw/core/meson.build b/hw/core/meson.build
> > > > > index f32d1ad943..8dc4552e35 100644
> > > > > --- a/hw/core/meson.build
> > > > > +++ b/hw/core/meson.build
> > > > > @@ -12,6 +12,8 @@ hwcore_ss.add(files(
> > > > >     'qdev-clock.c',
> > > > >   ))
> > > > >
> > > > > +libcbor = dependency('libcbor', version: '>=0.7.0')
> > > > > +
> > > > >   common_ss.add(files('cpu-common.c'))
> > > > >   common_ss.add(files('machine-smp.c'))
> > > > >   system_ss.add(when: 'CONFIG_FITLOADER', if_true: 
> > > > > files('loader-fit.c'))
> > > > > @@ -24,7 +26,7 @@ system_ss.add(when: 'CONFIG_REGISTER', if_true: 
> > > > > files('register.c'))
> > > > >   system_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: 
> > > > > files('split-irq.c'))
> > > > >   system_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
> > > > >   system_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: 
> > > > > files('sysbus-fdt.c'))
> > > > > -system_ss.add(when: 'CONFIG_NITRO_ENCLAVE', if_true: 
> > > > > [files('eif.c'), zlib])
> > > > > +system_ss.add(when: 'CONFIG_NITRO_ENCLAVE', if_true: 
> > > > > [files('eif.c'), zlib, libcbor, gnutls])
> > > >
> > > >
> > > > Ah, you add the gnutls dependency here. Great! However, this means we
> > > > now make gnutls (and libcbor) a mandatory dependency for the default
> > > > configuration. Does configure know about that? I believe before gnutls
> > > > was optional, right?
> > > >
> > >
> > > I see gnutls is not a required dependency in the root meson.build. I
> > > am not sure what we should do here.
> > >
> > > Hey Daniel, do you have any suggestions about how this dependency
> > > should be included?
> >
> > Unconditionally build the crypto/x509-utils.c file, but in that put
> > file #ifdef CONFIG_GNUTLS, and in the #else put a stub impl of the
> > method that just calls error_setg().
> >
> > That way you can compile everything without any hard dep on gnutls,
> > but if someone tries to use it they'll get a runtime error when
> > gnutls is not built
> >
> 
> Understood. Thanks! What should I do about libcbor? That one is
> required for building nitro-enclave and virtio-nsm. Should I make that
> required in the root meson.build file?

No, we can't introduce new mandatory dependancies for a such a niche
use case.

In retrospect, I think you'll need to conditionalize meson.build to
avoid building any of the nitro-enclave & virtio-nsm code, when
either gnutls or cbor are missing.

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