[libvirt] [PATCH 00/11] More virAuthGet* cleanups
In a case of review collision, Michal pushed Marcos Paulo de Souza's series after I had reviewed, but before seeing my review (oh well): https://www.redhat.com/archives/libvir-list/2018-August/msg00874.html Rather than worry about it, here's a series of changes that I described in my review: https://www.redhat.com/archives/libvir-list/2018-August/msg00860.html based on top of Marcos' now pushed weries. The only thing I didn't cover in this was my comment regarding xenapiUtil_RequestPassword - I just left it as is. John Ferlan (11): util: Alter virAuthGet*Path API to check valid parameters util: Alter virAuthGet*Path API to check valid callback util: Remove invalid parameter checks from virAuthGet{Username|Password} util: Alter virAuthGet*Path API return processing util: Alter virAuthGet*Path API to generate auth->cb error esx: Don't overwrite virAuthGet{Username|Password} errors hyperv: Don't overwrite virAuthGet{Username|Password} errors phyp: Don't overwrite virAuthGet{Username|Password} errors xenapi: Don't overwrite virAuthGet{Username|Password} errors test: Don't overwrite virAuthGet{Username|Password} errors rpc: Don't overwrite virAuthGet{Username|Password}Path errors src/esx/esx_driver.c | 27 ++++------------ src/hyperv/hyperv_driver.c | 16 +++------ src/phyp/phyp_driver.c | 16 +++------ src/rpc/virnetlibsshsession.c | 2 -- src/rpc/virnetsshsession.c | 5 +-- src/test/test_driver.c | 15 +++------ src/util/virauth.c | 61 ++++++++++++++++++++++++----------- src/xenapi/xenapi_driver.c | 16 +++------ 8 files changed, 67 insertions(+), 91 deletions(-) -- 2.17.1
Before trying to dereference @auth, let's ensure it's valid. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virauth.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/util/virauth.c b/src/util/virauth.c index 759b8f0cd3..1b9e4b6704 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -153,6 +153,12 @@ virAuthGetUsernamePath(const char *path, if (ret != NULL) return ret; + if (!auth) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Missing authentication credentials")); + return NULL; + } + memset(&cred, 0, sizeof(virConnectCredential)); if (defaultUsername != NULL) { @@ -226,6 +232,12 @@ virAuthGetPasswordPath(const char *path, if (ret != NULL) return ret; + if (!auth) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Missing authentication credentials")); + return NULL; + } + memset(&cred, 0, sizeof(virConnectCredential)); if (virAsprintf(&prompt, _("Enter %s's password for %s"), username, -- 2.17.1
Before trying to call @auth->cb, let's ensure it exists. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virauth.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/util/virauth.c b/src/util/virauth.c index 1b9e4b6704..7e7098317d 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -175,6 +175,12 @@ virAuthGetUsernamePath(const char *path, if (auth->credtype[ncred] != VIR_CRED_AUTHNAME) continue; + if (!auth->cb) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Missing authentication callback")); + return NULL; + } + cred.type = VIR_CRED_AUTHNAME; cred.prompt = prompt; cred.challenge = hostname; @@ -251,6 +257,12 @@ virAuthGetPasswordPath(const char *path, continue; } + if (!auth->cb) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Missing authentication callback")); + return NULL; + } + cred.type = auth->credtype[ncred]; cred.prompt = prompt; cred.challenge = hostname; -- 2.17.1
Now that the virAuthGet*Path helpers make the checks, we can remove them from here. Reviewed-by: John Ferlan <jferlan@redhat.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virauth.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/src/util/virauth.c b/src/util/virauth.c index 7e7098317d..a04abb613b 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -210,14 +210,8 @@ 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); + defaultUsername, hostname); } @@ -292,11 +286,5 @@ 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); } -- 2.17.1
If we never find the valid credtype in the list, then we'd return NULL without an error signaled forcing the caller to generate one that will probably be incorrect. Let's be specific. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virauth.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/util/virauth.c b/src/util/virauth.c index a04abb613b..d706658135 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -191,10 +191,12 @@ virAuthGetUsernamePath(const char *path, if ((*(auth->cb))(&cred, 1, auth->cbdata) < 0) VIR_FREE(cred.result); - break; + return cred.result; } - return cred.result; + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("Missing VIR_CRED_AUTHNAME credential type")); + return NULL; } @@ -267,10 +269,13 @@ virAuthGetPasswordPath(const char *path, if ((*(auth->cb))(&cred, 1, auth->cbdata) < 0) VIR_FREE(cred.result); - break; + return cred.result; } - return cred.result; + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("Missing VIR_CRED_PASSPHRASE or VIR_CRED_NOECHOPROMPT " + "credential type")); + return NULL; } -- 2.17.1
Rather than forcing the caller to generate an error, let's generate the Username or Password error message failure if the auth->cb fails. This is the last error path that needs a specific message for various callers. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virauth.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/util/virauth.c b/src/util/virauth.c index d706658135..5f1515e113 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -188,8 +188,11 @@ 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) { + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("Username request failed")); VIR_FREE(cred.result); + } return cred.result; } @@ -266,8 +269,11 @@ virAuthGetPasswordPath(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) { + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("Password request failed")); VIR_FREE(cred.result); + } return cred.result; } -- 2.17.1
Now that the virAuthGet*Path API's generate all the error messages we can remove them from the callers. This means that we will no longer overwrite the error from the API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/esx/esx_driver.c | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index ef258a2ce2..b1af646155 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -635,20 +635,14 @@ esxConnectToHost(esxPrivate *priv, if (VIR_STRDUP(username, conn->uri->user) < 0) goto cleanup; } else { - username = virAuthGetUsername(conn, auth, "esx", "root", conn->uri->server); - - if (!username) { - virReportError(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); + if (!(username = virAuthGetUsername(conn, auth, "esx", "root", + conn->uri->server))) goto cleanup; - } } - password = virAuthGetPassword(conn, auth, "esx", username, conn->uri->server); - - if (!password) { - virReportError(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); + if (!(password = virAuthGetPassword(conn, auth, "esx", username, + conn->uri->server))) goto cleanup; - } if (virAsprintf(&url, "%s://%s:%d/sdk", priv->parsedUri->transport, conn->uri->server, conn->uri->port) < 0) @@ -733,20 +727,13 @@ esxConnectToVCenter(esxPrivate *priv, if (VIR_STRDUP(username, conn->uri->user) < 0) goto cleanup; } else { - username = virAuthGetUsername(conn, auth, "esx", "administrator", hostname); - - if (!username) { - virReportError(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); + if (!(username = virAuthGetUsername(conn, auth, "esx", "administrator", + hostname))) goto cleanup; - } } - password = virAuthGetPassword(conn, auth, "esx", username, hostname); - - if (!password) { - virReportError(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); + if (!(password = virAuthGetPassword(conn, auth, "esx", username, hostname))) goto cleanup; - } if (virAsprintf(&url, "%s://%s:%d/sdk", priv->parsedUri->transport, hostname, conn->uri->port) < 0) -- 2.17.1
Now that the virAuthGet*Path API's generate all the error messages we can remove them from the callers. This means that we will no longer overwrite the error from the API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/hyperv/hyperv_driver.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 7ca145aef5..f41cd1c939 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -152,21 +152,15 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, if (VIR_STRDUP(username, conn->uri->user) < 0) goto cleanup; } else { - username = virAuthGetUsername(conn, auth, "hyperv", "administrator", conn->uri->server); - - if (username == NULL) { - virReportError(VIR_ERR_AUTH_FAILED, "%s", _("Username request failed")); + if (!(username = virAuthGetUsername(conn, auth, "hyperv", + "administrator", + conn->uri->server))) goto cleanup; - } } - password = virAuthGetPassword(conn, auth, "hyperv", username, conn->uri->server); - - if (password == NULL) { - virReportError(VIR_ERR_AUTH_FAILED, "%s", _("Password request failed")); + if (!(password = virAuthGetPassword(conn, auth, "hyperv", username, + conn->uri->server))) goto cleanup; - } - if (hypervInitConnection(conn, priv, username, password) < 0) goto cleanup; -- 2.17.1
Now that the virAuthGet*Path API's generate all the error messages we can remove them from the callers. This means that we will no longer overwrite the error from the API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/phyp/phyp_driver.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 5b17508dae..28a1fa3c26 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -957,13 +957,9 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, if (VIR_STRDUP(username, conn->uri->user) < 0) goto err; } else { - username = virAuthGetUsername(conn, auth, "ssh", NULL, conn->uri->server); - - if (username == NULL) { - virReportError(VIR_ERR_AUTH_FAILED, "%s", - _("Username request failed")); + if (!(username = virAuthGetUsername(conn, auth, "ssh", NULL, + conn->uri->server))) goto err; - } } memset(&hints, 0, sizeof(hints)); @@ -1034,13 +1030,9 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, || rc == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED || rc == LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED) { - password = virAuthGetPassword(conn, auth, "ssh", username, conn->uri->server); - - if (password == NULL) { - virReportError(VIR_ERR_AUTH_FAILED, "%s", - _("Password request failed")); + if (!(password = virAuthGetPassword(conn, auth, "ssh", username, + conn->uri->server))) goto disconnect; - } while ((rc = libssh2_userauth_password(session, username, -- 2.17.1
Now that the virAuthGet*Path API's generate all the error messages we can remove them from the callers. This means that we will no longer overwrite the error from the API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenapi/xenapi_driver.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 3af5eeafcf..96cad99e4e 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -156,22 +156,14 @@ xenapiConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, if (VIR_STRDUP(username, conn->uri->user) < 0) goto error; } else { - username = virAuthGetUsername(conn, auth, "xen", NULL, conn->uri->server); - - if (username == NULL) { - xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED, - _("Username request failed")); + if (!(username = virAuthGetUsername(conn, auth, "xen", NULL, + conn->uri->server))) goto error; - } } - password = virAuthGetPassword(conn, auth, "xen", username, conn->uri->server); - - if (password == NULL) { - xenapiSessionErrorHandler(conn, VIR_ERR_AUTH_FAILED, - _("Password request failed")); + if (!(password = virAuthGetPassword(conn, auth, "xen", username, + conn->uri->server))) goto error; - } if (VIR_ALLOC(privP) < 0) goto error; -- 2.17.1
Now that the virAuthGet*Path API's generate all the error messages we can remove them from the callers. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/test/test_driver.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index dfca95c981..6697a7dfe6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1374,12 +1374,9 @@ testConnectAuthenticate(virConnectPtr conn, /* Authentication is required because the test XML contains a * non-empty <auth/> section. First we must ask for a username. */ - username = virAuthGetUsername(conn, auth, "test", NULL, "localhost"/*?*/); - if (!username) { - virReportError(VIR_ERR_AUTH_FAILED, "%s", - _("authentication failed when asking for username")); + if (!(username = virAuthGetUsername(conn, auth, "test", NULL, + "localhost"/*?*/))) goto cleanup; - } /* Does the username exist? */ for (i = 0; i < privconn->numAuths; ++i) { @@ -1391,13 +1388,9 @@ testConnectAuthenticate(virConnectPtr conn, found_user: /* Even if we didn't find the user, we still ask for a password. */ if (i == -1 || privconn->auths[i].password != NULL) { - password = virAuthGetPassword(conn, auth, "test", - username, "localhost"); - if (password == NULL) { - virReportError(VIR_ERR_AUTH_FAILED, "%s", - _("authentication failed when asking for password")); + if (!(password = virAuthGetPassword(conn, auth, "test", username, + "localhost"))) goto cleanup; - } } if (i == -1 || -- 2.17.1
Now that the virAuthGet*Path API's generate all the error messages we can remove them from the callers. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/virnetlibsshsession.c | 2 -- src/rpc/virnetsshsession.c | 5 +---- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 21eb9f2500..7c5f158f4d 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -637,8 +637,6 @@ virNetLibsshAuthenticatePassword(virNetLibsshSessionPtr sess, if (!(password = virAuthGetPasswordPath(sess->authPath, sess->cred, "ssh", sess->username, sess->hostname))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to retrieve password")); ret = SSH_AUTH_ERROR; goto cleanup; } diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 6566f36ce0..35dc6c5356 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -726,11 +726,8 @@ virNetSSHAuthenticatePassword(virNetSSHSessionPtr sess, while (true) { if (!(password = virAuthGetPasswordPath(sess->authPath, sess->cred, "ssh", priv->username, - sess->hostname))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to retrieve password")); + sess->hostname))) goto cleanup; - } /* tunelled password authentication */ if ((rc = libssh2_userauth_password(sess->session, -- 2.17.1
On Tue, Aug 14, 2018 at 01:07:45PM -0400, John Ferlan wrote:
In a case of review collision, Michal pushed Marcos Paulo de Souza's series after I had reviewed, but before seeing my review (oh well):
https://www.redhat.com/archives/libvir-list/2018-August/msg00874.html
Rather than worry about it, here's a series of changes that I described in my review:
https://www.redhat.com/archives/libvir-list/2018-August/msg00860.html
based on top of Marcos' now pushed weries.
The only thing I didn't cover in this was my comment regarding xenapiUtil_RequestPassword - I just left it as is.
John Ferlan (11): util: Alter virAuthGet*Path API to check valid parameters util: Alter virAuthGet*Path API to check valid callback util: Remove invalid parameter checks from virAuthGet{Username|Password} util: Alter virAuthGet*Path API return processing util: Alter virAuthGet*Path API to generate auth->cb error esx: Don't overwrite virAuthGet{Username|Password} errors hyperv: Don't overwrite virAuthGet{Username|Password} errors phyp: Don't overwrite virAuthGet{Username|Password} errors xenapi: Don't overwrite virAuthGet{Username|Password} errors test: Don't overwrite virAuthGet{Username|Password} errors rpc: Don't overwrite virAuthGet{Username|Password}Path errors
src/esx/esx_driver.c | 27 ++++------------ src/hyperv/hyperv_driver.c | 16 +++------ src/phyp/phyp_driver.c | 16 +++------ src/rpc/virnetlibsshsession.c | 2 -- src/rpc/virnetsshsession.c | 5 +-- src/test/test_driver.c | 15 +++------ src/util/virauth.c | 61 ++++++++++++++++++++++++----------- src/xenapi/xenapi_driver.c | 16 +++------ 8 files changed, 67 insertions(+), 91 deletions(-)
Nice cleanup. Reviewed-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com> Thanks,
-- 2.17.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Thanks, Marcos
participants (2)
-
John Ferlan -
Marcos Paulo de Souza