Re: [Qemu-block] [PATCH 08/13] qcrypto: add the plumbing for encryption management
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
> > > ---
> > > 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
Re: [Qemu-block] [PATCH 08/13] qcrypto: add the plumbing for encryption management
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
> > ---
> > 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
Re: [Qemu-block] [PATCH 08/13] qcrypto: add the plumbing for encryption management
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
> ---
> 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
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-
[Qemu-block] [PATCH 08/13] qcrypto: add the plumbing for encryption management
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
---
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
+ *
+ */
+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 */
--
2.17.2
