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)

But this does not really fly with the way callback is called. So
far, we have three places where the callback is called:

  1) remoteAuthInteract()
  2) virAuthGetUsernamePath()
  3) virAuthAskCredential()

Now, 1) just grabs whatever the client side provided and sends it
to the other side via RPC. All interesting work takes place at 2)
and 3). And the usual pattern in which these two are called is:

  if (!(cred = wrapper()))
      return -1;

where wrapper() is a function that firstly tries to get data from
a config file or URI itself. The wrappers are named
virAuthGetUsername() for virAuthGetUsernamePath() and
virAuthGetPassword() for virAuthAskCredential().

All wrappers do report error on failure. Except, when the user
provided callback set the struct member to NULL. Then they both
return NULL without setting any error. This then leads to the
generic error message:

  error : An error occurred, but the cause is unknown

Since we failed to get desired credential data, we can't proceed
(what data should we pass down to the auth layer, say ssh?) and
have to fail. But, we can produce an error message that'll
hopefully point users in the right direction.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2181235
Signed-off-by: Michal Privoznik <mprivozn@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

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

>
>Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2181235
>Signed-off-by: Michal Privoznik <mprivozn@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
>