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@xxxxxxxxx>
> > ---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
