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

On 12/12/18 3:36 PM, John Ferlan wrote: > > > 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 Okay, this prevents users from defining a volume with mismatching secret usage type. But I think the problem is also elsewhere - in the bug, after the pool is refreshed the secret is lost from the volume XML and therefore vol-create-from looks up multiple secrets and is confused. Michal