[PATCH] virauth: Report error on empty auth result

[Martin Kletzander]

On Mon, Mar 27, 2023 at 01:21:51PM +0200, Michal Privoznik wrote:
>When opening a connection, it may be necessary to provide user
>credentials, or some additional info (e.g. whether to trust an
>ssh key). We have a special API for that: virConnectOpenAuth()
>where and additional callback can be passed. This callback is
>then called with _virConnectCredential struct filled partially
>and it's callback's responsibility to get desired data (e.g. by
>prompting user) and store it into .result member of the struct.
>
>But we document the callback behaviour as:
>
>   If an interaction cannot be filled, fill in NULL and 0.
>
>(meaning fill in NULL and return 0)
>

No, not really, the whole docstring is:

  * When authentication requires one or more interactions, this callback
  * is invoked. For each interaction supplied, data must be gathered
  * from the user and filled in to the 'result' and 'resultlen' fields.
  * If an interaction cannot be filled, fill in NULL and 0.
  *
  * Returns 0 if all interactions were filled, or -1 upon error

hence if interaction cannot be filled, then:
.result = NULL;
.resultlen = 0;

and since interaction could not be filled, then the callback should not
return 0 as that is the case only if all interactions were filled.

However the fix is, I believe, actually correct.  If we need some
credentials and they were not provided (for example even the default one
was not used -- that is also job of the callback) then we should still
error out.  We essentially do even before this patch, we just don't
report an error.

So I would scrap the rest of the commit and just say that we should also
work with "buggy" auth callbacks.

So with the commit message shortened:

Reviewed-by: Martin Kletzander <mkletzan at redhat.com>

>
>Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2181235
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> src/util/virauth.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/src/util/virauth.c b/src/util/virauth.c
>index 7b4a1bd8a5..d1f32f8e6d 100644
>--- a/src/util/virauth.c
>+++ b/src/util/virauth.c
>@@ -176,7 +176,8 @@ virAuthGetUsernamePath(const char *path,
>         cred.result = NULL;
>         cred.resultlen = 0;
>
>-        if ((*(auth->cb))(&cred, 1, auth->cbdata) < 0) {
>+        if ((*(auth->cb))(&cred, 1, auth->cbdata) < 0 ||
>+            !cred.result) {
>             virReportError(VIR_ERR_AUTH_FAILED, "%s",
>                            _("Username request failed"));
>             VIR_FREE(cred.result);
>@@ -310,7 +311,8 @@ virAuthAskCredential(virConnectAuthPtr auth,
>
>     ret->prompt = prompt;
>
>-    if (auth->cb(ret, 1, auth->cbdata) < 0) {
>+    if (auth->cb(ret, 1, auth->cbdata) < 0 ||
>+        !ret->result) {
>         virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>                        _("failed to retrieve user response for authentication callback"));
>         return NULL;
>-- 
>2.39.2
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20230328/3cc3a7a5/attachment-0001.sig>