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

[Marcos Paulo de Souza]

On Tue, Aug 14, 2018 at 10:53:50AM -0400, John Ferlan wrote:
> 
> 
> 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.

Indeed, that was my plan, to land incremental patches to cleanup the virAuthGet*
interfaces, but it seems that you already did :) Now I will try to find another
places to fix and inprove inside libvirt!

> 
> >  }
> > @@ -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);
> >  }
> > 

-- 
Thanks,
	Marcos