[libvirt] [PATCH 1/5] virauth.c: Check for valid auth callback

[John Ferlan]

On 08/02/2018 08:27 PM, Marcos Paulo de Souza wrote:
> Instead of adding the same check for every drivers, execute the checks
> in virAuthGetUsername and virAuthGetPassword. These funtions are called
> when user is not set in the URI.
> 
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org at gmail.com>
> ---
>  src/util/virauth.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

I believe the virAuthGet{Username|Password}Path API's should be adjusted
instead.

Although not possible today as shown by the subsequent patches, if @auth
or !auth->cb were NULL calling virAuthGetCredential and returning
something is still possible since neither auth nor auth->cb is
referenced. They are only referenced when it's required to ask via some
mechanism. It's at those points we should generate the errors.

Furthermore, the callers that generate their own errors based on where
in the code they fail; however, those errors would hide memory
allocation failures from say virAsprintf or VIR_STRDUP calls from other
virAuth* APIs that are called.

For example, calling virAuthGetConfigFilePathURI and failing to allocate
memory would generate a memory allocation failure; however, the caller
could overwrite that with a "{Username|Password} request failed" error
message.

Interestingly, the virAuthGetUsernamePath caller expects the error to be
generated and doesn't generate it's own, but virAuthGetPasswordPath
callers will generate (and overwrite) their own error.

So, I think all of the error generation can be done in the *Path API's
and a lot more cleanup is possible...

First, just before the memset(&cred) (in both UsernamePath and
PasswordPath) add the logic that would check and error for "if (!auth)
{" with an error message such as "Missing authentication credentials"
using VIR_ERR_INVALID_ARG for virReportError and return NULL.

Next, checking and erroring for !auth->cb would only be necessary if we
ever get past the credtype "continue;" condition. In that case the error
message would be "Missing authentication callback" using
VIR_ERR_INVALID_ARG for virReportError and return NULL.

With those errors in place, there would be two more reasons that the
caller would need to generate an error... First if there was no expected
credtype for the API or if the (*cred->cb) returns < 0.

Since the for loop breaks once the auth->cb is called, if we change the
"break;" to a return cred.result and then instead of the bare return
cred.result at the bottom we turn that into an error "Missing XXX
credential type" (where XXX would be replaced by "VIR_CRED_AUTHNAME" and
"VIR_CRED_PASSPHRASE or VIR_CRED_NOECHOPROMPT" using VIR_ERR_AUTH_FAILED
for virReportError followed by return NULL.

With that in place, add error processing to the auth->cb, either:

           virReportError(VIR_ERR_AUTH_FAILED, "%s",
                           _("Username request failed"));

or

            virReportError(VIR_ERR_AUTH_FAILED, "%s",
                           _("Password request failed"));

when (*auth->cb) < 0

Once the above is all in place, then the callers could be adjusted to
not generate any errors for !auth, !auth->cb, and NULL return from the
function. Take the opportunity to clean up the callers a bit to alter
long lines and the calls to be just:

    if (!(xxxx = virAuthGet*(...)))
        goto xxxx;

where the virAuthGet* call could be across multiple lines if it goes
beyond the 80 cols, but the open/close parens {} aren't needed.

The commit messages for the subsequent patches would need to be adjusted
to note that you're removing the need to generate error messages for the
virAuthGet{Username|Password}[Path] callers since the API's handle that.
The test_driver and rpc/virnet*session.c callers do have different
messages on NULL failures now, but (famous last words) that shouldn't be
a problem.

NB: The rpc/virnet[lib]sshsession.c consumers already do the !auth ||
!auth->cb checks in the form of "if (!sess->cred || !sess->cred->cb) {".
I would just keep those as is.

Finally, not sure how it's called, but xenapiUtil_RequestPassword would
perhaps need similar adjustments. If it's caller still overwrites the
message, then at least the message is logged in a domain log file somewhere.

Hopefully this makes sense. This one patch is blossoming into many more
and the subsequent patches get a bit more code removal using the common
error messages.

John

> diff --git a/src/util/virauth.c b/src/util/virauth.c
> index 8c450b6b31..759b8f0cd3 100644
> --- a/src/util/virauth.c
> +++ b/src/util/virauth.c
> @@ -198,6 +198,12 @@ virAuthGetUsername(virConnectPtr conn,
>      if (virAuthGetConfigFilePath(conn, &path) < 0)
>          return NULL;
>  
> +    if (!auth || !auth->cb) {
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                       _("Missing or invalid auth pointer"));
> +        return NULL;
> +    }
> +
>      return virAuthGetUsernamePath(path, auth, servicename,
>                                   defaultUsername, hostname);

Could have taken the opportunity to fix the indention issue caused by
commit '12614e7e2' (e.g. s/defaultUsername/ defaultUsername/), but with
the above suggestions that would need a separate/unnecessary patch.
We'll just have to wait until someone comes this way again.

>  }
> @@ -262,5 +268,11 @@ virAuthGetPassword(virConnectPtr conn,
>      if (virAuthGetConfigFilePath(conn, &path) < 0)
>          return NULL;
>  
> +    if (!auth || !auth->cb) {
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                       _("Missing or invalid auth pointer"));
> +        return NULL;
> +    }
> +
>      return virAuthGetPasswordPath(path, auth, servicename, username, hostname);
>  }
>