diff mbox series

[08/13] qcrypto: add the plumbing for encryption management

Message ID 20190814202219.1870-9-mlevitsk@redhat.com
State New
Headers show
Series RFC: luks/encrypted qcow2 key management | expand

Commit Message

Maxim Levitsky Aug. 14, 2019, 8:22 p.m. UTC 
This adds qcrypto_block_manage_encryption, which
 is thin wrapper around manage_encryption of the crypto driver
 which is also added

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 crypto/block.c         | 29 +++++++++++++++++++++++++++++
 crypto/blockpriv.h     |  9 +++++++++
 include/crypto/block.h | 27 +++++++++++++++++++++++++++
 3 files changed, 65 insertions(+)

Comments

Daniel P. Berrangé Aug. 22, 2019, 11:16 a.m. UTC | #1 
On Wed, Aug 14, 2019 at 11:22:14PM +0300, Maxim Levitsky wrote:
> This adds qcrypto_block_manage_encryption, which
>  is thin wrapper around manage_encryption of the crypto driver
>  which is also added
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  crypto/block.c         | 29 +++++++++++++++++++++++++++++
>  crypto/blockpriv.h     |  9 +++++++++
>  include/crypto/block.h | 27 +++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+)
> 
> diff --git a/crypto/block.c b/crypto/block.c
> index ee96759f7d..5916e49aba 100644
> --- a/crypto/block.c
> +++ b/crypto/block.c
> @@ -20,6 +20,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +
>  #include "blockpriv.h"
>  #include "block-qcow.h"
>  #include "block-luks.h"
> @@ -282,6 +283,34 @@ void qcrypto_block_free(QCryptoBlock *block)
>  }
>  
>  
> +int qcrypto_block_setup_encryption(QCryptoBlock *block,
> +                                   QCryptoBlockReadFunc readfunc,
> +                                   QCryptoBlockWriteFunc writefunc,
> +                                   void *opaque,
> +                                   enum BlkSetupEncryptionAction action,
> +                                   QCryptoEncryptionSetupOptions *options,
> +                                   bool force,
> +                                   Error **errp)
> +{
> +    if (!block->driver->setup_encryption) {
> +        error_setg(errp,
> +                "Crypto format %s doesn't support management of encryption keys",
> +                QCryptoBlockFormat_str(block->format));
> +        return -1;
> +    }
> +
> +    return block->driver->setup_encryption(block,
> +                                           readfunc,
> +                                           writefunc,
> +                                           opaque,
> +                                           action,
> +                                           options,
> +                                           force,
> +                                           errp);
> +}
> +
> +
> +
>  typedef int (*QCryptoCipherEncDecFunc)(QCryptoCipher *cipher,
>                                          const void *in,
>                                          void *out,
> diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
> index 71c59cb542..804965dca3 100644
> --- a/crypto/blockpriv.h
> +++ b/crypto/blockpriv.h
> @@ -81,6 +81,15 @@ struct QCryptoBlockDriver {
>  
>      bool (*has_format)(const uint8_t *buf,
>                         size_t buflen);
> +
> +    int (*setup_encryption)(QCryptoBlock *block,
> +                            QCryptoBlockReadFunc readfunc,
> +                            QCryptoBlockWriteFunc writefunc,
> +                            void *opaque,
> +                            enum BlkSetupEncryptionAction action,
> +                            QCryptoEncryptionSetupOptions *options,
> +                            bool force,
> +                            Error **errp);
>  };
>  
>  
> diff --git a/include/crypto/block.h b/include/crypto/block.h
> index fe12899831..60d46e3efc 100644
> --- a/include/crypto/block.h
> +++ b/include/crypto/block.h
> @@ -23,6 +23,7 @@
>  
>  #include "crypto/cipher.h"
>  #include "crypto/ivgen.h"
> +#include "block/block.h"
>  
>  typedef struct QCryptoBlock QCryptoBlock;
>  
> @@ -268,4 +269,30 @@ uint64_t qcrypto_block_get_sector_size(QCryptoBlock *block);
>   */
>  void qcrypto_block_free(QCryptoBlock *block);
>  
> +
> +/**
> + * qcrypto_block_setup_encryption:
> + * @block: the block encryption object
> + *
> + * @readfunc: callback for reading data from the volume header
> + * @writefunc: callback for writing data to the volume header
> + * @opaque: data to pass to @readfunc and @writefunc
> + * @action: tell the driver the setup action (add/erase currently)
> + * @options: driver specific options, that specify
> + *           what encryption settings to manage
> + * @force: hint for the driver to allow unsafe operation
> + * @errp: error pointer
> + *
> + * Adds/Erases a new encryption key using @options

I'd prefer to see separate APIs for add + erase instead
of overloading. It'll lead to a clearer API from callers
POV to see exactly which parameters are for each action.

> + *
> + */
> +int qcrypto_block_setup_encryption(QCryptoBlock *block,
> +                                   QCryptoBlockReadFunc readfunc,
> +                                   QCryptoBlockWriteFunc writefunc,
> +                                   void *opaque,
> +                                   enum BlkSetupEncryptionAction action,
> +                                   QCryptoEncryptionSetupOptions *options,
> +                                   bool force,
> +                                   Error **errp);
> +

Regards,
Daniel
Maxim Levitsky Aug. 22, 2019, 11:47 a.m. UTC | #2 
On Thu, 2019-08-22 at 12:16 +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 14, 2019 at 11:22:14PM +0300, Maxim Levitsky wrote:
> > This adds qcrypto_block_manage_encryption, which
> >  is thin wrapper around manage_encryption of the crypto driver
> >  which is also added
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  crypto/block.c         | 29 +++++++++++++++++++++++++++++
> >  crypto/blockpriv.h     |  9 +++++++++
> >  include/crypto/block.h | 27 +++++++++++++++++++++++++++
> >  3 files changed, 65 insertions(+)
> > 
> > diff --git a/crypto/block.c b/crypto/block.c
> > index ee96759f7d..5916e49aba 100644
> > --- a/crypto/block.c
> > +++ b/crypto/block.c
> > @@ -20,6 +20,7 @@
> >  
> >  #include "qemu/osdep.h"
> >  #include "qapi/error.h"
> > +
> >  #include "blockpriv.h"
> >  #include "block-qcow.h"
> >  #include "block-luks.h"
> > @@ -282,6 +283,34 @@ void qcrypto_block_free(QCryptoBlock *block)
> >  }
> >  
> >  
> > +int qcrypto_block_setup_encryption(QCryptoBlock *block,
> > +                                   QCryptoBlockReadFunc readfunc,
> > +                                   QCryptoBlockWriteFunc writefunc,
> > +                                   void *opaque,
> > +                                   enum BlkSetupEncryptionAction action,
> > +                                   QCryptoEncryptionSetupOptions *options,
> > +                                   bool force,
> > +                                   Error **errp)
> > +{
> > +    if (!block->driver->setup_encryption) {
> > +        error_setg(errp,
> > +                "Crypto format %s doesn't support management of encryption keys",
> > +                QCryptoBlockFormat_str(block->format));
> > +        return -1;
> > +    }
> > +
> > +    return block->driver->setup_encryption(block,
> > +                                           readfunc,
> > +                                           writefunc,
> > +                                           opaque,
> > +                                           action,
> > +                                           options,
> > +                                           force,
> > +                                           errp);
> > +}
> > +
> > +
> > +
> >  typedef int (*QCryptoCipherEncDecFunc)(QCryptoCipher *cipher,
> >                                          const void *in,
> >                                          void *out,
> > diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
> > index 71c59cb542..804965dca3 100644
> > --- a/crypto/blockpriv.h
> > +++ b/crypto/blockpriv.h
> > @@ -81,6 +81,15 @@ struct QCryptoBlockDriver {
> >  
> >      bool (*has_format)(const uint8_t *buf,
> >                         size_t buflen);
> > +
> > +    int (*setup_encryption)(QCryptoBlock *block,
> > +                            QCryptoBlockReadFunc readfunc,
> > +                            QCryptoBlockWriteFunc writefunc,
> > +                            void *opaque,
> > +                            enum BlkSetupEncryptionAction action,
> > +                            QCryptoEncryptionSetupOptions *options,
> > +                            bool force,
> > +                            Error **errp);
> >  };
> >  
> >  
> > diff --git a/include/crypto/block.h b/include/crypto/block.h
> > index fe12899831..60d46e3efc 100644
> > --- a/include/crypto/block.h
> > +++ b/include/crypto/block.h
> > @@ -23,6 +23,7 @@
> >  
> >  #include "crypto/cipher.h"
> >  #include "crypto/ivgen.h"
> > +#include "block/block.h"
> >  
> >  typedef struct QCryptoBlock QCryptoBlock;
> >  
> > @@ -268,4 +269,30 @@ uint64_t qcrypto_block_get_sector_size(QCryptoBlock *block);
> >   */
> >  void qcrypto_block_free(QCryptoBlock *block);
> >  
> > +
> > +/**
> > + * qcrypto_block_setup_encryption:
> > + * @block: the block encryption object
> > + *
> > + * @readfunc: callback for reading data from the volume header
> > + * @writefunc: callback for writing data to the volume header
> > + * @opaque: data to pass to @readfunc and @writefunc
> > + * @action: tell the driver the setup action (add/erase currently)
> > + * @options: driver specific options, that specify
> > + *           what encryption settings to manage
> > + * @force: hint for the driver to allow unsafe operation
> > + * @errp: error pointer
> > + *
> > + * Adds/Erases a new encryption key using @options
> 
> I'd prefer to see separate APIs for add + erase instead
> of overloading. It'll lead to a clearer API from callers
> POV to see exactly which parameters are for each action.


The downside of this is some duplication of code in the middle layers.
I don't mind doing that if you are OK with that.

Also note that if we go with amend options, we will have to use the single
API.


Best regards,
	Maxim Levitsky
Daniel P. Berrangé Aug. 22, 2019, 11:49 a.m. UTC | #3 
On Thu, Aug 22, 2019 at 02:47:03PM +0300, Maxim Levitsky wrote:
> On Thu, 2019-08-22 at 12:16 +0100, Daniel P. Berrangé wrote:
> > On Wed, Aug 14, 2019 at 11:22:14PM +0300, Maxim Levitsky wrote:
> > > This adds qcrypto_block_manage_encryption, which
> > >  is thin wrapper around manage_encryption of the crypto driver
> > >  which is also added
> > > 
> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > ---
> > >  crypto/block.c         | 29 +++++++++++++++++++++++++++++
> > >  crypto/blockpriv.h     |  9 +++++++++
> > >  include/crypto/block.h | 27 +++++++++++++++++++++++++++
> > >  3 files changed, 65 insertions(+)
> > > 
> > > diff --git a/crypto/block.c b/crypto/block.c
> > > index ee96759f7d..5916e49aba 100644
> > > --- a/crypto/block.c
> > > +++ b/crypto/block.c
> > > @@ -20,6 +20,7 @@
> > >  
> > >  #include "qemu/osdep.h"
> > >  #include "qapi/error.h"
> > > +
> > >  #include "blockpriv.h"
> > >  #include "block-qcow.h"
> > >  #include "block-luks.h"
> > > @@ -282,6 +283,34 @@ void qcrypto_block_free(QCryptoBlock *block)
> > >  }
> > >  
> > >  
> > > +int qcrypto_block_setup_encryption(QCryptoBlock *block,
> > > +                                   QCryptoBlockReadFunc readfunc,
> > > +                                   QCryptoBlockWriteFunc writefunc,
> > > +                                   void *opaque,
> > > +                                   enum BlkSetupEncryptionAction action,
> > > +                                   QCryptoEncryptionSetupOptions *options,
> > > +                                   bool force,
> > > +                                   Error **errp)
> > > +{
> > > +    if (!block->driver->setup_encryption) {
> > > +        error_setg(errp,
> > > +                "Crypto format %s doesn't support management of encryption keys",
> > > +                QCryptoBlockFormat_str(block->format));
> > > +        return -1;
> > > +    }
> > > +
> > > +    return block->driver->setup_encryption(block,
> > > +                                           readfunc,
> > > +                                           writefunc,
> > > +                                           opaque,
> > > +                                           action,
> > > +                                           options,
> > > +                                           force,
> > > +                                           errp);
> > > +}
> > > +
> > > +
> > > +
> > >  typedef int (*QCryptoCipherEncDecFunc)(QCryptoCipher *cipher,
> > >                                          const void *in,
> > >                                          void *out,
> > > diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
> > > index 71c59cb542..804965dca3 100644
> > > --- a/crypto/blockpriv.h
> > > +++ b/crypto/blockpriv.h
> > > @@ -81,6 +81,15 @@ struct QCryptoBlockDriver {
> > >  
> > >      bool (*has_format)(const uint8_t *buf,
> > >                         size_t buflen);
> > > +
> > > +    int (*setup_encryption)(QCryptoBlock *block,
> > > +                            QCryptoBlockReadFunc readfunc,
> > > +                            QCryptoBlockWriteFunc writefunc,
> > > +                            void *opaque,
> > > +                            enum BlkSetupEncryptionAction action,
> > > +                            QCryptoEncryptionSetupOptions *options,
> > > +                            bool force,
> > > +                            Error **errp);
> > >  };
> > >  
> > >  
> > > diff --git a/include/crypto/block.h b/include/crypto/block.h
> > > index fe12899831..60d46e3efc 100644
> > > --- a/include/crypto/block.h
> > > +++ b/include/crypto/block.h
> > > @@ -23,6 +23,7 @@
> > >  
> > >  #include "crypto/cipher.h"
> > >  #include "crypto/ivgen.h"
> > > +#include "block/block.h"
> > >  
> > >  typedef struct QCryptoBlock QCryptoBlock;
> > >  
> > > @@ -268,4 +269,30 @@ uint64_t qcrypto_block_get_sector_size(QCryptoBlock *block);
> > >   */
> > >  void qcrypto_block_free(QCryptoBlock *block);
> > >  
> > > +
> > > +/**
> > > + * qcrypto_block_setup_encryption:
> > > + * @block: the block encryption object
> > > + *
> > > + * @readfunc: callback for reading data from the volume header
> > > + * @writefunc: callback for writing data to the volume header
> > > + * @opaque: data to pass to @readfunc and @writefunc
> > > + * @action: tell the driver the setup action (add/erase currently)
> > > + * @options: driver specific options, that specify
> > > + *           what encryption settings to manage
> > > + * @force: hint for the driver to allow unsafe operation
> > > + * @errp: error pointer
> > > + *
> > > + * Adds/Erases a new encryption key using @options
> > 
> > I'd prefer to see separate APIs for add + erase instead
> > of overloading. It'll lead to a clearer API from callers
> > POV to see exactly which parameters are for each action.
> 
> 
> The downside of this is some duplication of code in the middle layers.
> I don't mind doing that if you are OK with that.
> 
> Also note that if we go with amend options, we will have to use the single
> API.

That should not be the case. The interface used at the qemu-img/QMP
layer is something for the block layer to be concerned with. THis
code is in the crypto layer and shouldn't depend on stuff defined
in the block layer QAPI schema. The block layer ammend options impl
should translate to the relevant API calls needed at the crypto layer.


Regards,
Daniel
diff mbox series

Patch

diff --git a/crypto/block.c b/crypto/block.c
index ee96759f7d..5916e49aba 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -20,6 +20,7 @@ 
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+
 #include "blockpriv.h"
 #include "block-qcow.h"
 #include "block-luks.h"
@@ -282,6 +283,34 @@  void qcrypto_block_free(QCryptoBlock *block)
 }
 
 
+int qcrypto_block_setup_encryption(QCryptoBlock *block,
+                                   QCryptoBlockReadFunc readfunc,
+                                   QCryptoBlockWriteFunc writefunc,
+                                   void *opaque,
+                                   enum BlkSetupEncryptionAction action,
+                                   QCryptoEncryptionSetupOptions *options,
+                                   bool force,
+                                   Error **errp)
+{
+    if (!block->driver->setup_encryption) {
+        error_setg(errp,
+                "Crypto format %s doesn't support management of encryption keys",
+                QCryptoBlockFormat_str(block->format));
+        return -1;
+    }
+
+    return block->driver->setup_encryption(block,
+                                           readfunc,
+                                           writefunc,
+                                           opaque,
+                                           action,
+                                           options,
+                                           force,
+                                           errp);
+}
+
+
+
 typedef int (*QCryptoCipherEncDecFunc)(QCryptoCipher *cipher,
                                         const void *in,
                                         void *out,
diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 71c59cb542..804965dca3 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -81,6 +81,15 @@  struct QCryptoBlockDriver {
 
     bool (*has_format)(const uint8_t *buf,
                        size_t buflen);
+
+    int (*setup_encryption)(QCryptoBlock *block,
+                            QCryptoBlockReadFunc readfunc,
+                            QCryptoBlockWriteFunc writefunc,
+                            void *opaque,
+                            enum BlkSetupEncryptionAction action,
+                            QCryptoEncryptionSetupOptions *options,
+                            bool force,
+                            Error **errp);
 };
 
 
diff --git a/include/crypto/block.h b/include/crypto/block.h
index fe12899831..60d46e3efc 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -23,6 +23,7 @@ 
 
 #include "crypto/cipher.h"
 #include "crypto/ivgen.h"
+#include "block/block.h"
 
 typedef struct QCryptoBlock QCryptoBlock;
 
@@ -268,4 +269,30 @@  uint64_t qcrypto_block_get_sector_size(QCryptoBlock *block);
  */
 void qcrypto_block_free(QCryptoBlock *block);
 
+
+/**
+ * qcrypto_block_setup_encryption:
+ * @block: the block encryption object
+ *
+ * @readfunc: callback for reading data from the volume header
+ * @writefunc: callback for writing data to the volume header
+ * @opaque: data to pass to @readfunc and @writefunc
+ * @action: tell the driver the setup action (add/erase currently)
+ * @options: driver specific options, that specify
+ *           what encryption settings to manage
+ * @force: hint for the driver to allow unsafe operation
+ * @errp: error pointer
+ *
+ * Adds/Erases a new encryption key using @options
+ *
+ */
+int qcrypto_block_setup_encryption(QCryptoBlock *block,
+                                   QCryptoBlockReadFunc readfunc,
+                                   QCryptoBlockWriteFunc writefunc,
+                                   void *opaque,
+                                   enum BlkSetupEncryptionAction action,
+                                   QCryptoEncryptionSetupOptions *options,
+                                   bool force,
+                                   Error **errp);
+
 #endif /* QCRYPTO_BLOCK_H */