Re: [libvirt] [PATCH 1/5] virauth.c: Check for valid auth callback
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
> > ---
> > 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() (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 (!( = virAuthGet*(...)))
> goto ;
>
> 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, ) < 0)
> > return NULL;
> >
> > +if (!auth || !auth->cb) {
> > +
Re: [libvirt] [PATCH 1/5] virauth.c: Check for valid auth callback
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
> ---
> 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() (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 (!( = virAuthGet*(...)))
goto ;
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, ) < 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
[libvirt] [PATCH 1/5] virauth.c: Check for valid auth callback
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
---
src/util/virauth.c | 12
1 file changed, 12 insertions(+)
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, ) < 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);
}
@@ -262,5 +268,11 @@ virAuthGetPassword(virConnectPtr conn,
if (virAuthGetConfigFilePath(conn, ) < 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);
}
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
