12:07 p.m.
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
12:07 p.m.
New subject: [libvirt] [PATCH 01/11] util: Alter virAuthGet*Path API to check valid parameters
Before trying to dereference @auth, let's ensure it's valid.
Signed-off-by: John Ferlan <jferlan(a)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
12:07 p.m.
New subject: [libvirt] [PATCH 02/11] util: Alter virAuthGet*Path API to check valid callback
Before trying to call @auth->cb, let's ensure it exists.
Signed-off-by: John Ferlan <jferlan(a)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
12:07 p.m.
New subject: [libvirt] [PATCH 03/11] util: Remove invalid parameter checks from virAuthGet{Username|Password}
Now that the virAuthGet*Path helpers make the checks, we can remove
them from here.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Signed-off-by: John Ferlan <jferlan(a)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
12:07 p.m.
New subject: [libvirt] [PATCH 04/11] util: Alter virAuthGet*Path API return processing
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(a)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
12:07 p.m.
New subject: [libvirt] [PATCH 05/11] util: Alter virAuthGet*Path API to generate auth->cb error
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(a)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
12:07 p.m.
New subject: [libvirt] [PATCH 06/11] esx: Don't overwrite virAuthGet{Username|Password} errors
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(a)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
12:07 p.m.
New subject: [libvirt] [PATCH 07/11] hyperv: Don't overwrite virAuthGet{Username|Password} errors
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(a)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
12:07 p.m.
New subject: [libvirt] [PATCH 08/11] phyp: Don't overwrite virAuthGet{Username|Password} errors
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(a)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
12:07 p.m.
New subject: [libvirt] [PATCH 09/11] xenapi: Don't overwrite virAuthGet{Username|Password} errors
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(a)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
12:07 p.m.
New subject: [libvirt] [PATCH 10/11] test: Don't overwrite virAuthGet{Username|Password} errors
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(a)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
12:07 p.m.
New subject: [libvirt] [PATCH 11/11] rpc: Don't overwrite virAuthGet{Username|Password}Path errors
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(a)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
5:24 p.m.
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(a)gmail.com>
Thanks,
--
2.17.1
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Thanks,
Marcos
2293
days inactive
2293
days old
12 comments
2 participants
participants (2)
-
John Ferlan -
Marcos Paulo de Souza