src/secret/secret_util.c | 17 +++++++++++++++++ tests/qemuxml2argvtest.c | 4 +++- 2 files changed, 20 insertions(+), 1 deletion(-)
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;
+ }
+
*secret = conn->secretDriver->secretGetValue(sec, secret_size, 0,
VIR_SECRET_GET_VALUE_INTERNAL_CALL);
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index e17709e7e1..700868ca0b 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -77,7 +77,9 @@ static virSecretPtr
fakeSecretLookupByUUID(virConnectPtr conn,
const unsigned char *uuid)
{
- return virGetSecret(conn, uuid, 0, "");
+ /* NB: This mocked value could be "tls" or "volume" depending on
+ * which test is being run, we'll leave at NONE (or 0) */
+ return virGetSecret(conn, uuid, VIR_SECRET_USAGE_TYPE_NONE, "");
}
static virSecretDriver fakeSecretDriver = {
--
2.17.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
ping?
Thanks,
John
On 12/4/18 3: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;
> + }
> +
> *secret = conn->secretDriver->secretGetValue(sec, secret_size, 0,
> VIR_SECRET_GET_VALUE_INTERNAL_CALL);
>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index e17709e7e1..700868ca0b 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -77,7 +77,9 @@ static virSecretPtr
> fakeSecretLookupByUUID(virConnectPtr conn,
> const unsigned char *uuid)
> {
> - return virGetSecret(conn, uuid, 0, "");
> + /* NB: This mocked value could be "tls" or "volume" depending on
> + * which test is being run, we'll leave at NONE (or 0) */
> + return virGetSecret(conn, uuid, VIR_SECRET_USAGE_TYPE_NONE, "");
> }
>
> static virSecretDriver fakeSecretDriver = {
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/13/18 8:12 AM, Michal Privoznik wrote:
> 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
>
I responded to your needsinfo in the bz, but to more or less replay it
here for those following along at home or work is the vol-create[-as]
was incorrectly allowed to use an 'iscsi' secret (e.g. one meant as the
passphrase to the iSCSI server) when creating the volume because the
vol-create-as XML used the UUID to lookup the secret that was being used
to create the volume instead of the "proper" action of making sure that
the secret to be used was a "volume" secret.
An example perhaps helps a lot... Assume a virsh secret-list entry having:
...
2a682ffe-2c95-43c3-8cdb-e690d407371e iscsi notPrivateSecret
1a2e881d-2e88-411c-b817-fd02bfa30bd5 volume /home/vm-images/encrypt1.img
...
The output is:
UUID usageType usageID
where usageType is an enum type and usageID is a char* used along with
UUID as one of the ways to lookup secrets. The unfortunate closeness of
usageType and usageID has bitten me in the past too.
Anyway, currently the code allows
<encryption format='luks'>
<secret type='passphrase'
uuid='2a682ffe-2c95-43c3-8cdb-e690d407371e'/>
</encryption>
to be used as input in the volume XML when creating the volume when it
should have been using:
<encryption format='luks'>
<secret type='passphrase'
uuid='1a2e881d-2e88-411c-b817-fd02bfa30bd5'/>
</encryption>
to create the LUKS volume secret for /home/vm-images/encrypt1.img
This patch "fixes" that by checking the "usageType" (e.g. that iscsi or
volume field in the output) of the returned lookup by UUID matches the
usageType "expected" used to create the volume. Creating a volume
expects a usageType volume when calling virSecretGetSecretString to get
the secret (see storageBackendCreateQemuImgSecretPath).
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/13/18 3:18 PM, John Ferlan wrote:
>
>
> On 12/13/18 8:12 AM, Michal Privoznik wrote:
>> 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
>>
>
> I responded to your needsinfo in the bz, but to more or less replay it
> here for those following along at home or work is the vol-create[-as]
> was incorrectly allowed to use an 'iscsi' secret (e.g. one meant as the
> passphrase to the iSCSI server) when creating the volume because the
> vol-create-as XML used the UUID to lookup the secret that was being used
> to create the volume instead of the "proper" action of making sure that
> the secret to be used was a "volume" secret.
>
> An example perhaps helps a lot... Assume a virsh secret-list entry having:
>
> ...
> 2a682ffe-2c95-43c3-8cdb-e690d407371e iscsi notPrivateSecret
> 1a2e881d-2e88-411c-b817-fd02bfa30bd5 volume /home/vm-images/encrypt1.img
> ...
>
> The output is:
>
> UUID usageType usageID
>
> where usageType is an enum type and usageID is a char* used along with
> UUID as one of the ways to lookup secrets. The unfortunate closeness of
> usageType and usageID has bitten me in the past too.
>
> Anyway, currently the code allows
>
> <encryption format='luks'>
> <secret type='passphrase'
> uuid='2a682ffe-2c95-43c3-8cdb-e690d407371e'/>
> </encryption>
>
> to be used as input in the volume XML when creating the volume when it
> should have been using:
>
> <encryption format='luks'>
> <secret type='passphrase'
> uuid='1a2e881d-2e88-411c-b817-fd02bfa30bd5'/>
> </encryption>
>
> to create the LUKS volume secret for /home/vm-images/encrypt1.img
>
> This patch "fixes" that by checking the "usageType" (e.g. that iscsi or
> volume field in the output) of the returned lookup by UUID matches the
> usageType "expected" used to create the volume. Creating a volume
> expects a usageType volume when calling virSecretGetSecretString to get
> the secret (see storageBackendCreateQemuImgSecretPath).
Ah, this helped. Thanks. ACK then.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.