Instead of hardcoding the TLS creds alias in
qemuBuildGraphicsVNCCommandLine, store it
in the domain private data.
Given that we only support one VNC graphics
and thus have only one alias per-domain,
this is overengineered, but it will allow us
to prepare the secret upfront when we start
supporting encrypted server TLS keys.
Note that the alias is not formatted anywhere
since we won't need to access it after domain
startup.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
src/qemu/qemu_command.c | 8 ++++----
src/qemu/qemu_domain.c | 44 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 822d5f8669..d130d0463c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8035,18 +8035,18 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
virBufferAddLit(&opt, ",password");
if (cfg->vncTLS) {
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509)) {
- const char *alias = "vnc-tls-creds0";
+ qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics);
+ if (gfxPriv->tlsAlias) {
if (qemuBuildTLSx509CommandLine(cmd,
cfg->vncTLSx509certdir,
true,
cfg->vncTLSx509verify,
NULL,
- alias,
+ gfxPriv->tlsAlias,
qemuCaps) < 0)
goto error;
- virBufferAsprintf(&opt, ",tls-creds=%s", alias);
+ virBufferAsprintf(&opt, ",tls-creds=%s", gfxPriv->tlsAlias);
} else {
virBufferAddLit(&opt, ",tls");
if (cfg->vncTLSx509verify) {
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 63e739b778..6960f0569b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1741,6 +1741,42 @@ qemuDomainSecretChardevPrepare(virQEMUDriverConfigPtr cfg,
}
+static void
+qemuDomainSecretGraphicsDestroy(virDomainGraphicsDefPtr graphics)
+{
+ qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics);
+
+ if (!gfxPriv)
+ return;
+
+ VIR_FREE(gfxPriv->tlsAlias);
+}
+
+
+static int
+qemuDomainSecretGraphicsPrepare(virQEMUDriverConfigPtr cfg,
+ qemuDomainObjPrivatePtr priv,
+ virDomainGraphicsDefPtr graphics)
+{
+ virQEMUCapsPtr qemuCaps = priv->qemuCaps;
+ qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics);
+
+ if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC)
+ return 0;
+
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509))
+ return 0;
+
+ if (!cfg->vncTLS)
+ return 0;
+
+ if (VIR_STRDUP(gfxPriv->tlsAlias, "vnc-tls-creds0") < 0)
+ return -1;
+
+ return 0;
+}
+
+
/* qemuDomainSecretDestroy:
* @vm: Domain object
*
@@ -1782,6 +1818,9 @@ qemuDomainSecretDestroy(virDomainObjPtr vm)
for (i = 0; i < vm->def->nredirdevs; i++)
qemuDomainSecretChardevDestroy(vm->def->redirdevs[i]->source);
+
+ for (i = 0; i < vm->def->ngraphics; i++)
+ qemuDomainSecretGraphicsDestroy(vm->def->graphics[i]);
}
@@ -1865,6 +1904,11 @@ qemuDomainSecretPrepare(virQEMUDriverPtr driver,
goto cleanup;
}
+ for (i = 0; i < vm->def->ngraphics; i++) {
+ if (qemuDomainSecretGraphicsPrepare(cfg, priv, vm->def->graphics[i]) < 0)
+ goto cleanup;
+ }
+
ret = 0;
cleanup:
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 1/16/19 2:41 AM, Ján Tomko wrote:
> Instead of hardcoding the TLS creds alias in
> qemuBuildGraphicsVNCCommandLine, store it
> in the domain private data.
>
> Given that we only support one VNC graphics
> and thus have only one alias per-domain,
> this is overengineered, but it will allow us
> to prepare the secret upfront when we start
> supporting encrypted server TLS keys.
>
> Note that the alias is not formatted anywhere
> since we won't need to access it after domain
> startup.
>
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> ---
> src/qemu/qemu_command.c | 8 ++++----
> src/qemu/qemu_domain.c | 44 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 48 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 822d5f8669..d130d0463c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8035,18 +8035,18 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
> virBufferAddLit(&opt, ",password");
>
> if (cfg->vncTLS) {
> - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509)) {
> - const char *alias = "vnc-tls-creds0";
> + qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics);
> + if (gfxPriv->tlsAlias) {
> if (qemuBuildTLSx509CommandLine(cmd,
> cfg->vncTLSx509certdir,
> true,
> cfg->vncTLSx509verify,
> NULL,
> - alias,
> + gfxPriv->tlsAlias,
> qemuCaps) < 0)
> goto error;
>
> - virBufferAsprintf(&opt, ",tls-creds=%s", alias);
> + virBufferAsprintf(&opt, ",tls-creds=%s", gfxPriv->tlsAlias);
> } else {
> virBufferAddLit(&opt, ",tls");
> if (cfg->vncTLSx509verify) {
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 63e739b778..6960f0569b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1741,6 +1741,42 @@ qemuDomainSecretChardevPrepare(virQEMUDriverConfigPtr cfg,
> }
>
>
> +static void
> +qemuDomainSecretGraphicsDestroy(virDomainGraphicsDefPtr graphics)
> +{
> + qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics);
> +
> + if (!gfxPriv)
> + return;
> +
> + VIR_FREE(gfxPriv->tlsAlias);
Free'ing of tlsAlias is handled by qemuDomainGraphicsPrivateDispose, so
this would be
virObjectUnref(gfxPriv);
QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics) = NULL;
The second to avoid the virDomainGraphicsDefFree doing this as well.
Still this is "unusual" (so to speak) for a qemuDomainSecret*Destroy
function. Typically they just clear/destroy the *Secinfo data.
Since you don't have that yet until patch 7, this could wait until then.
On the other hand, I don't "foresee" anything wrong with properly
free'ing the graphics def privateData now.
> +}
> +
> +
> +static int
> +qemuDomainSecretGraphicsPrepare(virQEMUDriverConfigPtr cfg,
> + qemuDomainObjPrivatePtr priv,
> + virDomainGraphicsDefPtr graphics)
> +{
> + virQEMUCapsPtr qemuCaps = priv->qemuCaps;
> + qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics);
> +
> + if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC)
> + return 0;
> +
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509))
> + return 0;
> +
> + if (!cfg->vncTLS)
> + return 0;
> +
Just a note/thought while reviewing... nothing all that important...
Seems to be a bit of overkill w/ graphics->privateData only being used
for VNC private data in this very specialized case. Still weighed vs the
then need for "gfxPriv && gfxPriv->...", who am I to complain ;-)
> + if (VIR_STRDUP(gfxPriv->tlsAlias, "vnc-tls-creds0") < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> /* qemuDomainSecretDestroy:
> * @vm: Domain object
> *
> @@ -1782,6 +1818,9 @@ qemuDomainSecretDestroy(virDomainObjPtr vm)
>
> for (i = 0; i < vm->def->nredirdevs; i++)
> qemuDomainSecretChardevDestroy(vm->def->redirdevs[i]->source);
> +
> + for (i = 0; i < vm->def->ngraphics; i++)
> + qemuDomainSecretGraphicsDestroy(vm->def->graphics[i]);
Interesting placement until one reads patch 7.
I think if patch 5 and 6 were placed before this one, then it'd be
clearer what the steps are. I'm OK with this here now since eventually
it'd be added.
With a couple of adjustments...
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
> }
>
>
> @@ -1865,6 +1904,11 @@ qemuDomainSecretPrepare(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
> + for (i = 0; i < vm->def->ngraphics; i++) {
> + if (qemuDomainSecretGraphicsPrepare(cfg, priv, vm->def->graphics[i]) < 0)
> + goto cleanup;
> + }
> +
> ret = 0;
>
> cleanup:
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jan 17, 2019 at 11:08:03AM -0500, John Ferlan wrote:
>
>
>On 1/16/19 2:41 AM, Ján Tomko wrote:
>> Instead of hardcoding the TLS creds alias in
>> qemuBuildGraphicsVNCCommandLine, store it
>> in the domain private data.
>>
>> Given that we only support one VNC graphics
>> and thus have only one alias per-domain,
>> this is overengineered, but it will allow us
>> to prepare the secret upfront when we start
>> supporting encrypted server TLS keys.
>>
>> Note that the alias is not formatted anywhere
>> since we won't need to access it after domain
>> startup.
>>
>> Signed-off-by: Ján Tomko <jtomko@redhat.com>
>> ---
>> src/qemu/qemu_command.c | 8 ++++----
>> src/qemu/qemu_domain.c | 44 +++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 48 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 822d5f8669..d130d0463c 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -8035,18 +8035,18 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
>> virBufferAddLit(&opt, ",password");
>>
>> if (cfg->vncTLS) {
>> - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509)) {
>> - const char *alias = "vnc-tls-creds0";
>> + qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics);
>> + if (gfxPriv->tlsAlias) {
>> if (qemuBuildTLSx509CommandLine(cmd,
>> cfg->vncTLSx509certdir,
>> true,
>> cfg->vncTLSx509verify,
>> NULL,
>> - alias,
>> + gfxPriv->tlsAlias,
>> qemuCaps) < 0)
>> goto error;
>>
>> - virBufferAsprintf(&opt, ",tls-creds=%s", alias);
>> + virBufferAsprintf(&opt, ",tls-creds=%s", gfxPriv->tlsAlias);
>> } else {
>> virBufferAddLit(&opt, ",tls");
>> if (cfg->vncTLSx509verify) {
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 63e739b778..6960f0569b 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -1741,6 +1741,42 @@ qemuDomainSecretChardevPrepare(virQEMUDriverConfigPtr cfg,
>> }
>>
>>
>> +static void
>> +qemuDomainSecretGraphicsDestroy(virDomainGraphicsDefPtr graphics)
>> +{
>> + qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics);
>> +
>> + if (!gfxPriv)
>> + return;
>> +
>> + VIR_FREE(gfxPriv->tlsAlias);
>
>Free'ing of tlsAlias is handled by qemuDomainGraphicsPrivateDispose, so
>this would be
>
> virObjectUnref(gfxPriv);
> QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics) = NULL;
>
>The second to avoid the virDomainGraphicsDefFree doing this as well.
>
It would be more unusual for a Secret*Destroy function to free the
private data.
IIUC the point is to not have the temporary data allocated during the
whole time the domain is running.
>Still this is "unusual" (so to speak) for a qemuDomainSecret*Destroy
>function. Typically they just clear/destroy the *Secinfo data.
>
>Since you don't have that yet until patch 7, this could wait until then.
>On the other hand, I don't "foresee" anything wrong with properly
>free'ing the graphics def privateData now.
>
>> +}
>> +
>> +
>> +static int
>> +qemuDomainSecretGraphicsPrepare(virQEMUDriverConfigPtr cfg,
>> + qemuDomainObjPrivatePtr priv,
>> + virDomainGraphicsDefPtr graphics)
>> +{
>> + virQEMUCapsPtr qemuCaps = priv->qemuCaps;
>> + qemuDomainGraphicsPrivatePtr gfxPriv = QEMU_DOMAIN_GRAPHICS_PRIVATE(graphics);
>> +
>> + if (graphics->type != VIR_DOMAIN_GRAPHICS_TYPE_VNC)
>> + return 0;
>> +
>> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_TLS_CREDS_X509))
>> + return 0;
>> +
>> + if (!cfg->vncTLS)
>> + return 0;
>> +
>
>Just a note/thought while reviewing... nothing all that important...
>
>Seems to be a bit of overkill w/ graphics->privateData only being used
>for VNC private data in this very specialized case. Still weighed vs the
>then need for "gfxPriv && gfxPriv->...", who am I to complain ;-)
>
I could convert it so that privateData is only allocated when needed,
but I considered the marginal memory usage increase worth the considency
and not forgetting to allocate/clean it up in some other case
(especially if we decide to use privateData for something else too).
So I'd rather not Unref the whole privateData in the Secret.*Dispose
function.
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.