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@redhat.com> ---
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