[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] crypto: make loaded property read-only
|
From: |
Daniel P . Berrangé |
|
Subject: |
Re: [PATCH] crypto: make loaded property read-only |
|
Date: |
Mon, 9 May 2022 11:36:02 +0100 |
|
User-agent: |
Mutt/2.1.5 (2021-12-30) |
On Mon, May 09, 2022 at 12:19:07PM +0200, Paolo Bonzini wrote:
> The ``loaded=on`` option in the command line or QMP ``object-add`` either had
> no effect (if ``loaded`` was the last option) or caused options to be
> effectively ignored as if they were not given. The property is therefore
> useless and was deprecated in 6.0; make it read-only now.
Why read-only, as opposed to deleting it entirely ? Unless I'm missing
something, nothing will read the property either
>
> The patch is best reviewed with "-b".
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> crypto/secret_common.c | 84 ++++++++++++++-------------------
> crypto/tlscredsanon.c | 20 ++------
> crypto/tlscredspsk.c | 20 ++------
> crypto/tlscredsx509.c | 20 ++------
> docs/about/deprecated.rst | 10 ----
> docs/about/removed-features.rst | 8 ++++
> 6 files changed, 55 insertions(+), 107 deletions(-)
>
> diff --git a/crypto/secret_common.c b/crypto/secret_common.c
> index 714a15d5e5..3441c44ca8 100644
> --- a/crypto/secret_common.c
> +++ b/crypto/secret_common.c
> @@ -138,36 +138,44 @@ static void qcrypto_secret_decode(const uint8_t *input,
>
>
> static void
> -qcrypto_secret_prop_set_loaded(Object *obj,
> - bool value,
> - Error **errp)
> +qcrypto_secret_complete(UserCreatable *uc, Error **errp)
> {
> - QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(obj);
> + QCryptoSecretCommon *secret = QCRYPTO_SECRET_COMMON(uc);
> QCryptoSecretCommonClass *sec_class
> - = QCRYPTO_SECRET_COMMON_GET_CLASS(obj);
> + = QCRYPTO_SECRET_COMMON_GET_CLASS(uc);
>
> - if (value) {
> - Error *local_err = NULL;
> - uint8_t *input = NULL;
> - size_t inputlen = 0;
> - uint8_t *output = NULL;
> - size_t outputlen = 0;
> + Error *local_err = NULL;
> + uint8_t *input = NULL;
> + size_t inputlen = 0;
> + uint8_t *output = NULL;
> + size_t outputlen = 0;
>
> - if (sec_class->load_data) {
> - sec_class->load_data(secret, &input, &inputlen, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - return;
> - }
> - } else {
> - error_setg(errp, "%s provides no 'load_data' method'",
> - object_get_typename(obj));
> + if (sec_class->load_data) {
> + sec_class->load_data(secret, &input, &inputlen, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> return;
> }
> + } else {
> + error_setg(errp, "%s provides no 'load_data' method'",
> + object_get_typename(OBJECT(uc)));
> + return;
> + }
>
> - if (secret->keyid) {
> - qcrypto_secret_decrypt(secret, input, inputlen,
> - &output, &outputlen, &local_err);
> + if (secret->keyid) {
> + qcrypto_secret_decrypt(secret, input, inputlen,
> + &output, &outputlen, &local_err);
> + g_free(input);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + input = output;
> + inputlen = outputlen;
> + } else {
> + if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) {
> + qcrypto_secret_decode(input, inputlen,
> + &output, &outputlen, &local_err);
> g_free(input);
> if (local_err) {
> error_propagate(errp, local_err);
> @@ -175,26 +183,11 @@ qcrypto_secret_prop_set_loaded(Object *obj,
> }
> input = output;
> inputlen = outputlen;
> - } else {
> - if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) {
> - qcrypto_secret_decode(input, inputlen,
> - &output, &outputlen, &local_err);
> - g_free(input);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - return;
> - }
> - input = output;
> - inputlen = outputlen;
> - }
> }
> -
> - secret->rawdata = input;
> - secret->rawlen = inputlen;
> - } else if (secret->rawdata) {
> - error_setg(errp, "Cannot unload secret");
> - return;
> }
> +
> + secret->rawdata = input;
> + secret->rawlen = inputlen;
> }
>
>
> @@ -268,13 +261,6 @@ qcrypto_secret_prop_get_keyid(Object *obj,
> }
>
>
> -static void
> -qcrypto_secret_complete(UserCreatable *uc, Error **errp)
> -{
> - object_property_set_bool(OBJECT(uc), "loaded", true, errp);
> -}
> -
> -
> static void
> qcrypto_secret_finalize(Object *obj)
> {
> @@ -294,7 +280,7 @@ qcrypto_secret_class_init(ObjectClass *oc, void *data)
>
> object_class_property_add_bool(oc, "loaded",
> qcrypto_secret_prop_get_loaded,
> - qcrypto_secret_prop_set_loaded);
> + NULL);
> object_class_property_add_enum(oc, "format",
> "QCryptoSecretFormat",
> &QCryptoSecretFormat_lookup,
> diff --git a/crypto/tlscredsanon.c b/crypto/tlscredsanon.c
> index 6fb83639ec..c0d23a0ef3 100644
> --- a/crypto/tlscredsanon.c
> +++ b/crypto/tlscredsanon.c
> @@ -119,16 +119,11 @@ qcrypto_tls_creds_anon_unload(QCryptoTLSCredsAnon
> *creds G_GNUC_UNUSED)
>
>
> static void
> -qcrypto_tls_creds_anon_prop_set_loaded(Object *obj,
> - bool value,
> - Error **errp)
> +qcrypto_tls_creds_anon_complete(UserCreatable *uc, Error **errp)
> {
> - QCryptoTLSCredsAnon *creds = QCRYPTO_TLS_CREDS_ANON(obj);
> + QCryptoTLSCredsAnon *creds = QCRYPTO_TLS_CREDS_ANON(uc);
>
> - qcrypto_tls_creds_anon_unload(creds);
> - if (value) {
> - qcrypto_tls_creds_anon_load(creds, errp);
> - }
> + qcrypto_tls_creds_anon_load(creds, errp);
> }
>
>
> @@ -163,13 +158,6 @@ qcrypto_tls_creds_anon_prop_get_loaded(Object *obj
> G_GNUC_UNUSED,
> #endif /* ! CONFIG_GNUTLS */
>
>
> -static void
> -qcrypto_tls_creds_anon_complete(UserCreatable *uc, Error **errp)
> -{
> - object_property_set_bool(OBJECT(uc), "loaded", true, errp);
> -}
> -
> -
> static void
> qcrypto_tls_creds_anon_finalize(Object *obj)
> {
> @@ -188,7 +176,7 @@ qcrypto_tls_creds_anon_class_init(ObjectClass *oc, void
> *data)
>
> object_class_property_add_bool(oc, "loaded",
> qcrypto_tls_creds_anon_prop_get_loaded,
> - qcrypto_tls_creds_anon_prop_set_loaded);
> + NULL);
> }
>
>
> diff --git a/crypto/tlscredspsk.c b/crypto/tlscredspsk.c
> index 752f2d92be..a4f9891274 100644
> --- a/crypto/tlscredspsk.c
> +++ b/crypto/tlscredspsk.c
> @@ -188,16 +188,11 @@ qcrypto_tls_creds_psk_unload(QCryptoTLSCredsPSK *creds
> G_GNUC_UNUSED)
>
>
> static void
> -qcrypto_tls_creds_psk_prop_set_loaded(Object *obj,
> - bool value,
> - Error **errp)
> +qcrypto_tls_creds_psk_complete(UserCreatable *uc, Error **errp)
> {
> - QCryptoTLSCredsPSK *creds = QCRYPTO_TLS_CREDS_PSK(obj);
> + QCryptoTLSCredsPSK *creds = QCRYPTO_TLS_CREDS_PSK(uc);
>
> - qcrypto_tls_creds_psk_unload(creds);
> - if (value) {
> - qcrypto_tls_creds_psk_load(creds, errp);
> - }
> + qcrypto_tls_creds_psk_load(creds, errp);
> }
>
>
> @@ -232,13 +227,6 @@ qcrypto_tls_creds_psk_prop_get_loaded(Object *obj
> G_GNUC_UNUSED,
> #endif /* ! CONFIG_GNUTLS */
>
>
> -static void
> -qcrypto_tls_creds_psk_complete(UserCreatable *uc, Error **errp)
> -{
> - object_property_set_bool(OBJECT(uc), "loaded", true, errp);
> -}
> -
> -
> static void
> qcrypto_tls_creds_psk_finalize(Object *obj)
> {
> @@ -276,7 +264,7 @@ qcrypto_tls_creds_psk_class_init(ObjectClass *oc, void
> *data)
>
> object_class_property_add_bool(oc, "loaded",
> qcrypto_tls_creds_psk_prop_get_loaded,
> - qcrypto_tls_creds_psk_prop_set_loaded);
> + NULL);
> object_class_property_add_str(oc, "username",
> qcrypto_tls_creds_psk_prop_get_username,
> qcrypto_tls_creds_psk_prop_set_username);
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 32948a6bdc..d14313925d 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -687,16 +687,11 @@ qcrypto_tls_creds_x509_unload(QCryptoTLSCredsX509
> *creds G_GNUC_UNUSED)
>
>
> static void
> -qcrypto_tls_creds_x509_prop_set_loaded(Object *obj,
> - bool value,
> - Error **errp)
> +qcrypto_tls_creds_x509_complete(UserCreatable *uc, Error **errp)
> {
> - QCryptoTLSCredsX509 *creds = QCRYPTO_TLS_CREDS_X509(obj);
> + QCryptoTLSCredsX509 *creds = QCRYPTO_TLS_CREDS_X509(uc);
>
> - qcrypto_tls_creds_x509_unload(creds);
> - if (value) {
> - qcrypto_tls_creds_x509_load(creds, errp);
> - }
> + qcrypto_tls_creds_x509_load(creds, errp);
> }
>
>
> @@ -814,13 +809,6 @@ qcrypto_tls_creds_x509_reload(QCryptoTLSCreds *creds,
> Error **errp)
> #endif /* ! CONFIG_GNUTLS */
>
>
> -static void
> -qcrypto_tls_creds_x509_complete(UserCreatable *uc, Error **errp)
> -{
> - object_property_set_bool(OBJECT(uc), "loaded", true, errp);
> -}
> -
> -
> static void
> qcrypto_tls_creds_x509_init(Object *obj)
> {
> @@ -852,7 +840,7 @@ qcrypto_tls_creds_x509_class_init(ObjectClass *oc, void
> *data)
>
> object_class_property_add_bool(oc, "loaded",
> qcrypto_tls_creds_x509_prop_get_loaded,
> - qcrypto_tls_creds_x509_prop_set_loaded);
> + NULL);
> object_class_property_add_bool(oc, "sanity-check",
> qcrypto_tls_creds_x509_prop_get_sanity,
> qcrypto_tls_creds_x509_prop_set_sanity);
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 896e5a97ab..2feb0c506c 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -99,16 +99,6 @@ other options have been processed. This will either have
> no effect (if
> ``opened`` was the last option) or cause errors. The property is therefore
> useless and should not be specified.
>
> -``loaded`` property of ``secret`` and ``secret_keyring`` objects (since 6.0)
> -''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> -
> -The only effect of specifying ``loaded=on`` in the command line or QMP
> -``object-add`` is that the secret is loaded immediately, possibly before all
> -other options have been processed. This will either have no effect (if
> -``loaded`` was the last option) or cause options to be effectively ignored as
> -if they were not given. The property is therefore useless and should not be
> -specified.
> -
> ``-display sdl,window_close=...`` (since 6.1)
> '''''''''''''''''''''''''''''''''''''''''''''
>
> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
> index 4a0b270296..2032608314 100644
> --- a/docs/about/removed-features.rst
> +++ b/docs/about/removed-features.rst
> @@ -741,6 +741,14 @@ aware that there are already potential security risks to
> blindly using
> ``qemu-img info`` to probe the format of an untrusted backing image,
> when deciding what format to add into an existing image.
>
> +``loaded`` property of ``secret`` and ``secret_keyring`` objects (removed in
> 7.1)
> +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +The ``loaded=on`` option in the command line or QMP ``object-add`` either had
> +no effect (if ``loaded`` was the last option) or caused options to be
> +effectively ignored as if they were not given. The property is therefore
> +useless and should simply be removed.
> +
> Block devices
> -------------
>
> --
> 2.35.1
>
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 :|