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

On 12/12/18 9:29 AM, Michal Privoznik wrote: > 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 > Someone used an "iscsi" usage type secret by to create their luks encrypted volume. When using vol-dumpxml after creation the secret by usage existed. Then a pool-refresh caused the secret to disappear because on pool refresh the secret for a volume is refreshed by "volume" secret usage type. When I posted this patch, the bz didn't exist, but it does now, see: https://bugzilla.redhat.com/show_bug.cgi?id=1656255