Re: [libvirt] [PATCH] secret: Add check/validation for correct usage when LookupByUUID

On 12/4/18 9:32 PM, John Ferlan wrote: > If virSecretGetSecretString is using by secretLookupByUUID, > then it's possible the found sec->usageType doesn't match the > desired @secretUsageType. If this occurs for the encrypted > volume creation processing and a subsequent pool refresh is > executed, then the secret used to create the volume will not > be found by the storageBackendLoadDefaultSecrets which expects > to find secrets by VIR_SECRET_USAGE_TYPE_VOLUME. > > Add a check to virSecretGetSecretString to avoid the possibility > along with an error indicating the incorrect matched types. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > > If someone has an idea regarding how the usage could be filled > in "properly" for the qemuxml2argvtest, I'm willing to give it > a shot. However, fair warning trying to "mock" for tls, volume, > iscsi, and ceph could be rather painful. Thus the NONE was the > well, easiest way to go since the stored secret (ahem) shouldn't > be of usageType "none" (famous last words). > > src/secret/secret_util.c | 17 +++++++++++++++++ > tests/qemuxml2argvtest.c | 4 +++- > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/src/secret/secret_util.c b/src/secret/secret_util.c > index 16e43ab2cc..27e164a425 100644 > --- a/src/secret/secret_util.c > +++ b/src/secret/secret_util.c > @@ -71,6 +71,23 @@ virSecretGetSecretString(virConnectPtr conn, > if (!sec) > goto cleanup; > > + /* NB: NONE is a byproduct of the qemuxml2argvtest test mocking > + * for UUID lookups. Normal secret XML processing would fail if > + * the usage type was NONE and since we have no way to set the > + * expected usage in that environment, let's just accept NONE */ > + if (sec->usageType != VIR_SECRET_USAGE_TYPE_NONE && > + sec->usageType != secretUsageType) { > + char uuidstr[VIR_UUID_STRING_BUFLEN]; > + > + virUUIDFormat(seclookupdef->u.uuid, uuidstr); > + virReportError(VIR_ERR_INVALID_ARG, > + _("secret with uuid %s is of type '%s' not " > + "expected '%s' type"), > + uuidstr, virSecretUsageTypeToString(sec->usageType), > + virSecretUsageTypeToString(secretUsageType)); > + goto cleanup; > + } I don't quite understand why this is needed. I mean, if seclookupdef->type == VIR_SECRET_LOOKUP_TYPE_USAGE then this check is done while looking the secret up. If seclookupdef->type == VIR_SECRET_LOOKUP_TYPE_UUID then this check feels odd; the caller wants the secret by UUID and they don't care about the usage type. Is there an actual error you're seeing? Michal