[PATCH 00/15] Clean up credential asking code in SSH transport
Peter Krempa (15): virNetLibsshSessionAuthAddPrivKeyAuth: Drop 'password' argument virNetLibsshAuthMethod: Drop 'password' field util: authconfig: Use automatic pointer clearing for virAuthConfig util: authconfig: Use conteporary and consistent header style virNetSSHSessionAuthAddPrivKeyAuth: Remove unused 'password' argument virNetSSHSessionAuthAddPrivKeyAuth: Refactor cleanup virNetSSHAuthMethod: Remove unused 'password' field virnetsshsession: Pass in username via virNetSSHSessionNew rather than auth functions util: auth: Introduce virAuthAskCredential virNetLibsshAuthenticateKeyboardInteractive: Use virAuthAskCredential virNetLibsshAuthenticatePrivkeyCb: Use virAuthAskCredential util: virauth: Export virAuthGetCredential virNetLibsshCheckHostKey: Use virAuthAskCredential virNetLibsshAuthenticatePassword: Use virAuthAskPassword instead of virAuthGetPasswordPath virAuthGetPasswordPath: Use virAuthAskCredential for callback interaction src/libvirt_private.syms | 3 + src/rpc/virnetlibsshsession.c | 211 ++++++++++------------------------ src/rpc/virnetlibsshsession.h | 3 +- src/rpc/virnetsocket.c | 19 +-- src/rpc/virnetsshsession.c | 162 ++++++++------------------ src/rpc/virnetsshsession.h | 13 +-- src/util/virauth.c | 107 +++++++++++------ src/util/virauth.h | 12 ++ src/util/virauthconfig.c | 52 ++++----- 9 files changed, 223 insertions(+), 359 deletions(-) -- 2.38.1
The only caller doesn't actually populate it. Remove it to simplify internals. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 4 +--- src/rpc/virnetlibsshsession.h | 3 +-- src/rpc/virnetsocket.c | 4 +--- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index b1420bea2c..bbc5d54386 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -988,8 +988,7 @@ virNetLibsshSessionAuthAddAgentAuth(virNetLibsshSession *sess) int virNetLibsshSessionAuthAddPrivKeyAuth(virNetLibsshSession *sess, - const char *keyfile, - const char *password) + const char *keyfile) { virNetLibsshAuthMethod *auth; @@ -1006,7 +1005,6 @@ virNetLibsshSessionAuthAddPrivKeyAuth(virNetLibsshSession *sess, return -1; } - auth->password = g_strdup(password); auth->filename = g_strdup(keyfile); auth->method = VIR_NET_LIBSSH_AUTH_PRIVKEY; auth->ssh_flags = SSH_AUTH_METHOD_PUBLICKEY; diff --git a/src/rpc/virnetlibsshsession.h b/src/rpc/virnetlibsshsession.h index c3b5f3e80d..7f94fd15dc 100644 --- a/src/rpc/virnetlibsshsession.h +++ b/src/rpc/virnetlibsshsession.h @@ -46,8 +46,7 @@ int virNetLibsshSessionAuthAddPasswordAuth(virNetLibsshSession *sess, int virNetLibsshSessionAuthAddAgentAuth(virNetLibsshSession *sess); int virNetLibsshSessionAuthAddPrivKeyAuth(virNetLibsshSession *sess, - const char *keyfile, - const char *password); + const char *keyfile); int virNetLibsshSessionAuthAddKeyboardAuth(virNetLibsshSession *sess, int tries); diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 8280bda007..8fbc69d51c 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -1080,9 +1080,7 @@ virNetSocketNewConnectLibssh(const char *host, } else if (STRCASEEQ(authMethod, "password")) { ret = virNetLibsshSessionAuthAddPasswordAuth(sess, uri); } else if (STRCASEEQ(authMethod, "privkey")) { - ret = virNetLibsshSessionAuthAddPrivKeyAuth(sess, - privkey, - NULL); + ret = virNetLibsshSessionAuthAddPrivKeyAuth(sess, privkey); } else if (STRCASEEQ(authMethod, "agent")) { ret = virNetLibsshSessionAuthAddAgentAuth(sess); } else { -- 2.38.1
The field was never populated so we can remove it and all the associated logic. Both for password authentication and fetching the password for the public key we still can use the authentication callbacks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 61 ++++++++++++++--------------------- 1 file changed, 25 insertions(+), 36 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index bbc5d54386..084224b3f8 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -69,7 +69,6 @@ struct _virNetLibsshAuthMethod { virNetLibsshAuthMethods method; int ssh_flags; /* SSH_AUTH_METHOD_* for this auth method */ - char *password; char *filename; int tries; @@ -129,8 +128,6 @@ virNetLibsshSessionDispose(void *obj) } for (i = 0; i < sess->nauths; i++) { - virSecureEraseString(sess->auths[i]->password); - g_free(sess->auths[i]->password); g_free(sess->auths[i]->filename); g_free(sess->auths[i]); } @@ -456,7 +453,7 @@ virNetLibsshImportPrivkey(virNetLibsshSession *sess, * failed or libssh did. */ virResetLastError(); - ret = ssh_pki_import_privkey_file(priv->filename, priv->password, + ret = ssh_pki_import_privkey_file(priv->filename, NULL, virNetLibsshAuthenticatePrivkeyCb, sess, &key); if (ret == SSH_EOF) { @@ -564,47 +561,39 @@ virNetLibsshAuthenticatePrivkey(virNetLibsshSession *sess, * returns SSH_AUTH_* values */ static int -virNetLibsshAuthenticatePassword(virNetLibsshSession *sess, - virNetLibsshAuthMethod *priv) +virNetLibsshAuthenticatePassword(virNetLibsshSession *sess) { const char *errmsg; int rc = SSH_AUTH_ERROR; VIR_DEBUG("sess=%p", sess); - if (priv->password) { - /* tunnelled password authentication */ - if ((rc = ssh_userauth_password(sess->session, NULL, - priv->password)) == 0) - return SSH_AUTH_SUCCESS; - } else { - /* password authentication with interactive password request */ - if (!sess->cred || !sess->cred->cb) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("Can't perform authentication: " - "Authentication callback not provided")); - return SSH_AUTH_ERROR; - } + /* password authentication with interactive password request */ + if (!sess->cred || !sess->cred->cb) { + virReportError(VIR_ERR_LIBSSH, "%s", + _("Can't perform authentication: " + "Authentication callback not provided")); + return SSH_AUTH_ERROR; + } - /* Try the authenticating the set amount of times. The server breaks the - * connection if maximum number of bad auth tries is exceeded */ - while (true) { - g_autofree char *password = NULL; + /* Try the authenticating the set amount of times. The server breaks the + * connection if maximum number of bad auth tries is exceeded */ + while (true) { + g_autofree char *password = NULL; - if (!(password = virAuthGetPasswordPath(sess->authPath, sess->cred, - "ssh", sess->username, - sess->hostname))) - return SSH_AUTH_ERROR; + if (!(password = virAuthGetPasswordPath(sess->authPath, sess->cred, + "ssh", sess->username, + sess->hostname))) + return SSH_AUTH_ERROR; - /* tunnelled password authentication */ - rc = ssh_userauth_password(sess->session, NULL, password); - virSecureEraseString(password); + /* tunnelled password authentication */ + rc = ssh_userauth_password(sess->session, NULL, password); + virSecureEraseString(password); - if (rc == 0) - return SSH_AUTH_SUCCESS; - else if (rc != SSH_AUTH_DENIED) - break; - } + if (rc == 0) + return SSH_AUTH_SUCCESS; + else if (rc != SSH_AUTH_DENIED) + break; } /* error path */ @@ -809,7 +798,7 @@ virNetLibsshAuthenticate(virNetLibsshSession *sess) break; case VIR_NET_LIBSSH_AUTH_PASSWORD: /* try to authenticate with password */ - ret = virNetLibsshAuthenticatePassword(sess, auth); + ret = virNetLibsshAuthenticatePassword(sess); break; } -- 2.38.1
Fix and clean up the error paths in virAuthConfigNew*. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virauthconfig.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c index dabd7cd217..983ac47f6b 100644 --- a/src/util/virauthconfig.c +++ b/src/util/virauthconfig.c @@ -37,23 +37,17 @@ VIR_LOG_INIT("util.authconfig"); virAuthConfig *virAuthConfigNew(const char *path) { - virAuthConfig *auth; - - auth = g_new0(virAuthConfig, 1); + g_autoptr(virAuthConfig) auth = g_new0(virAuthConfig, 1); auth->path = g_strdup(path); if (!(auth->keyfile = g_key_file_new())) - goto error; + return NULL; if (!g_key_file_load_from_file(auth->keyfile, path, 0, NULL)) - goto error; - - return auth; + return NULL; - error: - virAuthConfigFree(auth); - return NULL; + return g_steal_pointer(&auth); } @@ -61,23 +55,17 @@ virAuthConfig *virAuthConfigNewData(const char *path, const char *data, size_t len) { - virAuthConfig *auth; - - auth = g_new0(virAuthConfig, 1); + g_autoptr(virAuthConfig) auth = g_new0(virAuthConfig, 1); auth->path = g_strdup(path); if (!(auth->keyfile = g_key_file_new())) - goto error; + return NULL; if (!g_key_file_load_from_data(auth->keyfile, data, len, 0, NULL)) - goto error; - - return auth; + return NULL; - error: - virAuthConfigFree(auth); - return NULL; + return g_steal_pointer(&auth); } -- 2.38.1
On 1/17/23 10:20 AM, Peter Krempa wrote:
Fix and clean up the error paths in virAuthConfigNew*.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virauthconfig.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c index dabd7cd217..983ac47f6b 100644 --- a/src/util/virauthconfig.c +++ b/src/util/virauthconfig.c @@ -37,23 +37,17 @@ VIR_LOG_INIT("util.authconfig");
virAuthConfig *virAuthConfigNew(const char *path) { - virAuthConfig *auth; - - auth = g_new0(virAuthConfig, 1); + g_autoptr(virAuthConfig) auth = g_new0(virAuthConfig, 1);
auth->path = g_strdup(path);
if (!(auth->keyfile = g_key_file_new())) - goto error; + return NULL;
Unrelated to your changes, but as far as I know, g_key_file_new() cannot actually fail so this check is pointless.
if (!g_key_file_load_from_file(auth->keyfile, path, 0, NULL)) - goto error; - - return auth; + return NULL;
- error: - virAuthConfigFree(auth); - return NULL; + return g_steal_pointer(&auth); }
@@ -61,23 +55,17 @@ virAuthConfig *virAuthConfigNewData(const char *path, const char *data, size_t len) { - virAuthConfig *auth; - - auth = g_new0(virAuthConfig, 1); + g_autoptr(virAuthConfig) auth = g_new0(virAuthConfig, 1);
auth->path = g_strdup(path);
if (!(auth->keyfile = g_key_file_new())) - goto error; + return NULL;
again
if (!g_key_file_load_from_data(auth->keyfile, data, len, 0, NULL)) - goto error; - - return auth; + return NULL;
- error: - virAuthConfigFree(auth); - return NULL; + return g_steal_pointer(&auth); }
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virauthconfig.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c index 983ac47f6b..0363a1bef9 100644 --- a/src/util/virauthconfig.c +++ b/src/util/virauthconfig.c @@ -35,7 +35,8 @@ struct _virAuthConfig { VIR_LOG_INIT("util.authconfig"); -virAuthConfig *virAuthConfigNew(const char *path) +virAuthConfig * +virAuthConfigNew(const char *path) { g_autoptr(virAuthConfig) auth = g_new0(virAuthConfig, 1); @@ -51,9 +52,10 @@ virAuthConfig *virAuthConfigNew(const char *path) } -virAuthConfig *virAuthConfigNewData(const char *path, - const char *data, - size_t len) +virAuthConfig * +virAuthConfigNewData(const char *path, + const char *data, + size_t len) { g_autoptr(virAuthConfig) auth = g_new0(virAuthConfig, 1); @@ -69,7 +71,8 @@ virAuthConfig *virAuthConfigNewData(const char *path, } -void virAuthConfigFree(virAuthConfig *auth) +void +virAuthConfigFree(virAuthConfig *auth) { if (!auth) return; @@ -80,11 +83,12 @@ void virAuthConfigFree(virAuthConfig *auth) } -int virAuthConfigLookup(virAuthConfig *auth, - const char *service, - const char *hostname, - const char *credname, - char **value) +int +virAuthConfigLookup(virAuthConfig *auth, + const char *service, + const char *hostname, + const char *credname, + char **value) { g_autofree char *authgroup = NULL; g_autofree char *credgroup = NULL; -- 2.38.1
The only caller doesn't pass the password. Remove the argument. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetsocket.c | 3 +-- src/rpc/virnetsshsession.c | 7 +------ src/rpc/virnetsshsession.h | 3 +-- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 8fbc69d51c..b9b7328f87 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -954,8 +954,7 @@ virNetSocketNewConnectLibSSH2(const char *host, } else if (STRCASEEQ(authMethod, "privkey")) { ret = virNetSSHSessionAuthAddPrivKeyAuth(sess, username, - privkey, - NULL); + privkey); } else if (STRCASEEQ(authMethod, "agent")) { ret = virNetSSHSessionAuthAddAgentAuth(sess, username); } else { diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 485318d09b..08f246be61 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -1056,13 +1056,11 @@ virNetSSHSessionAuthAddAgentAuth(virNetSSHSession *sess, int virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess, const char *username, - const char *keyfile, - const char *password) + const char *keyfile) { virNetSSHAuthMethod *auth; char *user = NULL; - char *pass = NULL; char *file = NULL; if (!username || !keyfile) { @@ -1076,13 +1074,11 @@ virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess, user = g_strdup(username); file = g_strdup(keyfile); - pass = g_strdup(password); if (!(auth = virNetSSHSessionAuthMethodNew(sess))) goto error; auth->username = user; - auth->password = pass; auth->filename = file; auth->method = VIR_NET_SSH_AUTH_PRIVKEY; @@ -1091,7 +1087,6 @@ virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess, error: VIR_FREE(user); - VIR_FREE(pass); VIR_FREE(file); virObjectUnlock(sess); return -1; diff --git a/src/rpc/virnetsshsession.h b/src/rpc/virnetsshsession.h index 7a056df37f..8d6c99c547 100644 --- a/src/rpc/virnetsshsession.h +++ b/src/rpc/virnetsshsession.h @@ -56,8 +56,7 @@ int virNetSSHSessionAuthAddAgentAuth(virNetSSHSession *sess, int virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess, const char *username, - const char *keyfile, - const char *password); + const char *keyfile); int virNetSSHSessionAuthAddKeyboardAuth(virNetSSHSession *sess, const char *username, -- 2.38.1
With g_strdup not failing we can remove all of the 'error' section. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetsshsession.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 08f246be61..9f2aa17131 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -1060,9 +1060,6 @@ virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess, { virNetSSHAuthMethod *auth; - char *user = NULL; - char *file = NULL; - if (!username || !keyfile) { virReportError(VIR_ERR_SSH, "%s", _("Username and key file path must be provided " @@ -1072,24 +1069,15 @@ virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess, virObjectLock(sess); - user = g_strdup(username); - file = g_strdup(keyfile); - if (!(auth = virNetSSHSessionAuthMethodNew(sess))) - goto error; + return -1; - auth->username = user; - auth->filename = file; + auth->username = g_strdup(username); + auth->filename = g_strdup(keyfile); auth->method = VIR_NET_SSH_AUTH_PRIVKEY; virObjectUnlock(sess); return 0; - - error: - VIR_FREE(user); - VIR_FREE(file); - virObjectUnlock(sess); - return -1; } int -- 2.38.1
On 1/17/23 10:20 AM, Peter Krempa wrote:
With g_strdup not failing we can remove all of the 'error' section.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetsshsession.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 08f246be61..9f2aa17131 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -1060,9 +1060,6 @@ virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess, { virNetSSHAuthMethod *auth;
- char *user = NULL; - char *file = NULL; - if (!username || !keyfile) { virReportError(VIR_ERR_SSH, "%s", _("Username and key file path must be provided " @@ -1072,24 +1069,15 @@ virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess,
virObjectLock(sess);
- user = g_strdup(username); - file = g_strdup(keyfile); - if (!(auth = virNetSSHSessionAuthMethodNew(sess))) - goto error; + return -1;
In this path, you no longer unlock the session.
- auth->username = user; - auth->filename = file; + auth->username = g_strdup(username); + auth->filename = g_strdup(keyfile); auth->method = VIR_NET_SSH_AUTH_PRIVKEY;
virObjectUnlock(sess); return 0; - - error: - VIR_FREE(user); - VIR_FREE(file); - virObjectUnlock(sess); - return -1; }
int
None of the callers actually set it. Remove the field and corresponding logic. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetsshsession.c | 57 +++++++++++++++----------------------- 1 file changed, 22 insertions(+), 35 deletions(-) diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 9f2aa17131..0454deec16 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -71,7 +71,6 @@ typedef struct _virNetSSHAuthMethod virNetSSHAuthMethod; struct _virNetSSHAuthMethod { virNetSSHAuthMethods method; char *username; - char *password; char *filename; int tries; @@ -117,7 +116,6 @@ virNetSSHSessionAuthMethodsClear(virNetSSHSession *sess) for (i = 0; i < sess->nauths; i++) { VIR_FREE(sess->auths[i]->username); - VIR_FREE(sess->auths[i]->password); VIR_FREE(sess->auths[i]->filename); VIR_FREE(sess->auths[i]); } @@ -580,12 +578,11 @@ virNetSSHAuthenticatePrivkey(virNetSSHSession *sess, priv->username, NULL, priv->filename, - priv->password)) == 0) + NULL)) == 0) return 0; /* success */ VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR - if (priv->password || - ret == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED || + if (ret == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED || ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED) { VIR_WARNINGS_RESET libssh2_session_last_error(sess->session, &errmsg, NULL, 0); @@ -681,44 +678,34 @@ virNetSSHAuthenticatePassword(virNetSSHSession *sess, VIR_DEBUG("sess=%p", sess); - if (priv->password) { + /* password authentication with interactive password request */ + if (!sess->cred || !sess->cred->cb) { + virReportError(VIR_ERR_SSH, "%s", + _("Can't perform authentication: " + "Authentication callback not provided")); + goto cleanup; + } + + /* Try the authenticating the set amount of times. The server breaks the + * connection if maximum number of bad auth tries is exceeded */ + while (true) { + if (!(password = virAuthGetPasswordPath(sess->authPath, sess->cred, + "ssh", priv->username, + sess->hostname))) + goto cleanup; + /* tunnelled password authentication */ if ((rc = libssh2_userauth_password(sess->session, priv->username, - priv->password)) == 0) { + password)) == 0) { ret = 0; goto cleanup; } - } else { - /* password authentication with interactive password request */ - if (!sess->cred || !sess->cred->cb) { - virReportError(VIR_ERR_SSH, "%s", - _("Can't perform authentication: " - "Authentication callback not provided")); - goto cleanup; - } - /* Try the authenticating the set amount of times. The server breaks the - * connection if maximum number of bad auth tries is exceeded */ - while (true) { - if (!(password = virAuthGetPasswordPath(sess->authPath, sess->cred, - "ssh", priv->username, - sess->hostname))) - goto cleanup; - - /* tunnelled password authentication */ - if ((rc = libssh2_userauth_password(sess->session, - priv->username, - password)) == 0) { - ret = 0; - goto cleanup; - } - - if (rc != LIBSSH2_ERROR_AUTHENTICATION_FAILED) - break; + if (rc != LIBSSH2_ERROR_AUTHENTICATION_FAILED) + break; - VIR_FREE(password); - } + VIR_FREE(password); } /* error path */ -- 2.38.1
We only ever allow one username so there's no point passing it to each authentication registration function. Additionally the only caller (virNetClientNewLibSSH2) always passes a username so all the checks were pointless. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetsocket.c | 14 +++---- src/rpc/virnetsshsession.c | 84 ++++++++++---------------------------- src/rpc/virnetsshsession.h | 10 ++--- 3 files changed, 29 insertions(+), 79 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index b9b7328f87..b248ce24dc 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -909,7 +909,7 @@ virNetSocketNewConnectLibSSH2(const char *host, } /* create ssh session context */ - if (!(sess = virNetSSHSessionNew())) + if (!(sess = virNetSSHSessionNew(username))) goto error; /* set ssh session parameters */ @@ -946,17 +946,13 @@ virNetSocketNewConnectLibSSH2(const char *host, const char *authMethod = *authMethodNext; if (STRCASEEQ(authMethod, "keyboard-interactive")) { - ret = virNetSSHSessionAuthAddKeyboardAuth(sess, username, -1); + ret = virNetSSHSessionAuthAddKeyboardAuth(sess, -1); } else if (STRCASEEQ(authMethod, "password")) { - ret = virNetSSHSessionAuthAddPasswordAuth(sess, - uri, - username); + ret = virNetSSHSessionAuthAddPasswordAuth(sess, uri); } else if (STRCASEEQ(authMethod, "privkey")) { - ret = virNetSSHSessionAuthAddPrivKeyAuth(sess, - username, - privkey); + ret = virNetSSHSessionAuthAddPrivKeyAuth(sess, privkey); } else if (STRCASEEQ(authMethod, "agent")) { - ret = virNetSSHSessionAuthAddAgentAuth(sess, username); + ret = virNetSSHSessionAuthAddAgentAuth(sess); } else { virReportError(VIR_ERR_INVALID_ARG, _("Invalid authentication method: '%s'"), diff --git a/src/rpc/virnetsshsession.c b/src/rpc/virnetsshsession.c index 0454deec16..8f59906b4a 100644 --- a/src/rpc/virnetsshsession.c +++ b/src/rpc/virnetsshsession.c @@ -70,7 +70,6 @@ typedef struct _virNetSSHAuthMethod virNetSSHAuthMethod; struct _virNetSSHAuthMethod { virNetSSHAuthMethods method; - char *username; char *filename; int tries; @@ -93,6 +92,7 @@ struct _virNetSSHSession { int port; /* authentication stuff */ + char *username; virConnectAuthPtr cred; char *authPath; virNetSSHAuthCallbackError authCbErr; @@ -115,7 +115,6 @@ virNetSSHSessionAuthMethodsClear(virNetSSHSession *sess) size_t i; for (i = 0; i < sess->nauths; i++) { - VIR_FREE(sess->auths[i]->username); VIR_FREE(sess->auths[i]->filename); VIR_FREE(sess->auths[i]); } @@ -151,6 +150,7 @@ virNetSSHSessionDispose(void *obj) g_free(sess->hostname); g_free(sess->knownHostsFile); g_free(sess->authPath); + g_free(sess->username); } static virClass *virNetSSHSessionClass; @@ -488,8 +488,7 @@ virNetSSHCheckHostKey(virNetSSHSession *sess) * -1 on error */ static int -virNetSSHAuthenticateAgent(virNetSSHSession *sess, - virNetSSHAuthMethod *priv) +virNetSSHAuthenticateAgent(virNetSSHSession *sess) { struct libssh2_agent_publickey *agent_identity = NULL; bool no_identity = true; @@ -515,7 +514,7 @@ virNetSSHAuthenticateAgent(virNetSSHSession *sess, agent_identity))) { no_identity = false; if (!(ret = libssh2_agent_userauth(sess->agent, - priv->username, + sess->username, agent_identity))) return 0; /* key accepted */ @@ -575,7 +574,7 @@ virNetSSHAuthenticatePrivkey(virNetSSHSession *sess, /* try open the key with no password */ if ((ret = libssh2_userauth_publickey_fromfile(sess->session, - priv->username, + sess->username, NULL, priv->filename, NULL)) == 0) @@ -634,7 +633,7 @@ virNetSSHAuthenticatePrivkey(virNetSSHSession *sess, VIR_FREE(tmp); ret = libssh2_userauth_publickey_fromfile(sess->session, - priv->username, + sess->username, NULL, priv->filename, retr_passphrase.result); @@ -668,8 +667,7 @@ virNetSSHAuthenticatePrivkey(virNetSSHSession *sess, * -1 on error */ static int -virNetSSHAuthenticatePassword(virNetSSHSession *sess, - virNetSSHAuthMethod *priv) +virNetSSHAuthenticatePassword(virNetSSHSession *sess) { char *password = NULL; char *errmsg; @@ -690,13 +688,13 @@ virNetSSHAuthenticatePassword(virNetSSHSession *sess, * connection if maximum number of bad auth tries is exceeded */ while (true) { if (!(password = virAuthGetPasswordPath(sess->authPath, sess->cred, - "ssh", priv->username, + "ssh", sess->username, sess->hostname))) goto cleanup; /* tunnelled password authentication */ if ((rc = libssh2_userauth_password(sess->session, - priv->username, + sess->username, password)) == 0) { ret = 0; goto cleanup; @@ -751,7 +749,7 @@ virNetSSHAuthenticateKeyboardInteractive(virNetSSHSession *sess, * connection if maximum number of bad auth tries is exceeded */ while (priv->tries < 0 || priv->tries-- > 0) { ret = libssh2_userauth_keyboard_interactive(sess->session, - priv->username, + sess->username, virNetSSHKbIntCb); /* check for errors while calling the callback */ @@ -817,9 +815,8 @@ virNetSSHAuthenticate(virNetSSHSession *sess) } /* obtain list of supported auth methods */ - auth_list = libssh2_userauth_list(sess->session, - sess->auths[0]->username, - strlen(sess->auths[0]->username)); + auth_list = libssh2_userauth_list(sess->session, sess->username, + strlen(sess->username)); if (!auth_list) { /* unlikely event, authentication succeeded with NONE as method */ if (libssh2_userauth_authenticated(sess->session) == 1) @@ -845,7 +842,7 @@ virNetSSHAuthenticate(virNetSSHSession *sess) break; case VIR_NET_SSH_AUTH_AGENT: if (strstr(auth_list, "publickey")) - ret = virNetSSHAuthenticateAgent(sess, auth); + ret = virNetSSHAuthenticateAgent(sess); break; case VIR_NET_SSH_AUTH_PRIVKEY: if (strstr(auth_list, "publickey")) @@ -853,7 +850,7 @@ virNetSSHAuthenticate(virNetSSHSession *sess) break; case VIR_NET_SSH_AUTH_PASSWORD: if (strstr(auth_list, "password")) - ret = virNetSSHAuthenticatePassword(sess, auth); + ret = virNetSSHAuthenticatePassword(sess); break; } @@ -969,11 +966,9 @@ virNetSSHSessionAuthReset(virNetSSHSession *sess) int virNetSSHSessionAuthAddPasswordAuth(virNetSSHSession *sess, - virURI *uri, - const char *username) + virURI *uri) { virNetSSHAuthMethod *auth; - char *user = NULL; if (uri) { VIR_FREE(sess->authPath); @@ -982,75 +977,50 @@ virNetSSHSessionAuthAddPasswordAuth(virNetSSHSession *sess, goto error; } - if (!username) { - if (!(user = virAuthGetUsernamePath(sess->authPath, sess->cred, - "ssh", NULL, sess->hostname))) - goto error; - } else { - user = g_strdup(username); - } - virObjectLock(sess); if (!(auth = virNetSSHSessionAuthMethodNew(sess))) goto error; - auth->username = user; auth->method = VIR_NET_SSH_AUTH_PASSWORD; virObjectUnlock(sess); return 0; error: - VIR_FREE(user); virObjectUnlock(sess); return -1; } int -virNetSSHSessionAuthAddAgentAuth(virNetSSHSession *sess, - const char *username) +virNetSSHSessionAuthAddAgentAuth(virNetSSHSession *sess) { virNetSSHAuthMethod *auth; - char *user = NULL; - - if (!username) { - virReportError(VIR_ERR_SSH, "%s", - _("Username must be provided " - "for ssh agent authentication")); - return -1; - } virObjectLock(sess); - user = g_strdup(username); - if (!(auth = virNetSSHSessionAuthMethodNew(sess))) goto error; - auth->username = user; auth->method = VIR_NET_SSH_AUTH_AGENT; virObjectUnlock(sess); return 0; error: - VIR_FREE(user); virObjectUnlock(sess); return -1; } int virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess, - const char *username, const char *keyfile) { virNetSSHAuthMethod *auth; - if (!username || !keyfile) { + if (!keyfile) { virReportError(VIR_ERR_SSH, "%s", - _("Username and key file path must be provided " - "for private key authentication")); + _("Key file path must be provided for private key authentication")); return -1; } @@ -1059,7 +1029,6 @@ virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess, if (!(auth = virNetSSHSessionAuthMethodNew(sess))) return -1; - auth->username = g_strdup(username); auth->filename = g_strdup(keyfile); auth->method = VIR_NET_SSH_AUTH_PRIVKEY; @@ -1069,27 +1038,15 @@ virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess, int virNetSSHSessionAuthAddKeyboardAuth(virNetSSHSession *sess, - const char *username, int tries) { virNetSSHAuthMethod *auth; - char *user = NULL; - - if (!username) { - virReportError(VIR_ERR_SSH, "%s", - _("Username must be provided " - "for ssh agent authentication")); - return -1; - } virObjectLock(sess); - user = g_strdup(username); - if (!(auth = virNetSSHSessionAuthMethodNew(sess))) goto error; - auth->username = user; auth->tries = tries; auth->method = VIR_NET_SSH_AUTH_KEYBOARD_INTERACTIVE; @@ -1097,7 +1054,6 @@ virNetSSHSessionAuthAddKeyboardAuth(virNetSSHSession *sess, return 0; error: - VIR_FREE(user); virObjectUnlock(sess); return -1; @@ -1170,7 +1126,7 @@ virNetSSHSessionSetHostKeyVerification(virNetSSHSession *sess, } /* allocate and initialize a ssh session object */ -virNetSSHSession *virNetSSHSessionNew(void) +virNetSSHSession *virNetSSHSessionNew(const char *username) { virNetSSHSession *sess = NULL; @@ -1180,6 +1136,8 @@ virNetSSHSession *virNetSSHSessionNew(void) if (!(sess = virObjectLockableNew(virNetSSHSessionClass))) goto error; + sess->username = g_strdup(username); + /* initialize session data, use the internal data for callbacks * and stick to default memory management functions */ if (!(sess->session = libssh2_session_init_ex(NULL, diff --git a/src/rpc/virnetsshsession.h b/src/rpc/virnetsshsession.h index 8d6c99c547..8187346000 100644 --- a/src/rpc/virnetsshsession.h +++ b/src/rpc/virnetsshsession.h @@ -25,7 +25,7 @@ typedef struct _virNetSSHSession virNetSSHSession; -virNetSSHSession *virNetSSHSessionNew(void); +virNetSSHSession *virNetSSHSessionNew(const char *username); void virNetSSHSessionFree(virNetSSHSession *sess); typedef enum { @@ -48,18 +48,14 @@ int virNetSSHSessionAuthSetCallback(virNetSSHSession *sess, virConnectAuthPtr auth); int virNetSSHSessionAuthAddPasswordAuth(virNetSSHSession *sess, - virURI *uri, - const char *username); + virURI *uri); -int virNetSSHSessionAuthAddAgentAuth(virNetSSHSession *sess, - const char *username); +int virNetSSHSessionAuthAddAgentAuth(virNetSSHSession *sess); int virNetSSHSessionAuthAddPrivKeyAuth(virNetSSHSession *sess, - const char *username, const char *keyfile); int virNetSSHSessionAuthAddKeyboardAuth(virNetSSHSession *sess, - const char *username, int tries); int virNetSSHSessionSetHostKeyVerification(virNetSSHSession *sess, -- 2.38.1
The helper uses the user-provided auth callbacks to ask the user. The helper encapsulates the steps we do to query the user in few places into a common helper which can be then used further. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virauth.c | 66 ++++++++++++++++++++++++++++++++++++++++ src/util/virauth.h | 7 +++++ 3 files changed, 75 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 576ec8f95f..5616c0d44c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1850,6 +1850,8 @@ virAuditSend; # util/virauth.h +virAuthAskCredential; +virAuthConnectCredentialFree; virAuthGetConfigFilePath; virAuthGetConfigFilePathURI; virAuthGetPassword; diff --git a/src/util/virauth.c b/src/util/virauth.c index b9c2ae3ed1..aa1da80266 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -31,6 +31,7 @@ #include "virerror.h" #include "configmake.h" #include "virauthconfig.h" +#include "virsecureerase.h" #define VIR_FROM_THIS VIR_FROM_AUTH @@ -283,3 +284,68 @@ virAuthGetPassword(virConnectPtr conn, return virAuthGetPasswordPath(path, auth, servicename, username, hostname); } + + +void +virAuthConnectCredentialFree(virConnectCredential *cred) +{ + if (cred->result) { + virSecureErase(cred->result, cred->resultlen); + g_free(cred->result); + } + g_free(cred); +} + + +/** + * virAuthAskCredential: + * @auth: authentication callback data + * @prompt: question string to ask the user + * @echo: true if user's reply should be considered sensitive and not echoed + * + * Invoke the authentication callback for the connection @auth and ask the user + * the question in @prompt. If @echo is true user's reply should be collected + * as sensitive (user's input not printed on screen). + */ +virConnectCredential * +virAuthAskCredential(virConnectAuthPtr auth, + const char *prompt, + bool echo) +{ + g_autoptr(virConnectCredential) ret = g_new0(virConnectCredential, 1); + size_t i; + + ret->type = -1; + + for (i = 0; i < auth->ncredtype; ++i) { + int type = auth->credtype[i]; + if (echo) { + if (type == VIR_CRED_ECHOPROMPT) { + ret->type = type; + break; + } + } else { + if (type == VIR_CRED_PASSPHRASE || + type == VIR_CRED_NOECHOPROMPT) { + ret->type = type; + break; + } + } + } + + if (ret->type == -1) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("no suitable callback authentication callback was found")); + return NULL; + } + + ret->prompt = prompt; + + if (auth->cb(ret, 1, auth->cbdata)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to retrieve user response for authentication callback")); + return NULL; + } + + return g_steal_pointer(&ret); +} diff --git a/src/util/virauth.h b/src/util/virauth.h index a0fd84962b..3eaf40c626 100644 --- a/src/util/virauth.h +++ b/src/util/virauth.h @@ -52,3 +52,10 @@ char * virAuthGetPasswordPath(const char *path, const char *servicename, const char *username, const char *hostname); + +virConnectCredential *virAuthAskCredential(virConnectAuthPtr auth, + const char *prompt, + bool echo); + +void virAuthConnectCredentialFree(virConnectCredential *cred); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virConnectCredential, virAuthConnectCredentialFree); -- 2.38.1
On 1/17/23 10:20 AM, Peter Krempa wrote:
The helper uses the user-provided auth callbacks to ask the user. The helper encapsulates the steps we do to query the user in few places into a common helper which can be then used further.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virauth.c | 66 ++++++++++++++++++++++++++++++++++++++++ src/util/virauth.h | 7 +++++ 3 files changed, 75 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 576ec8f95f..5616c0d44c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1850,6 +1850,8 @@ virAuditSend;
# util/virauth.h +virAuthAskCredential; +virAuthConnectCredentialFree; virAuthGetConfigFilePath; virAuthGetConfigFilePathURI; virAuthGetPassword; diff --git a/src/util/virauth.c b/src/util/virauth.c index b9c2ae3ed1..aa1da80266 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -31,6 +31,7 @@ #include "virerror.h" #include "configmake.h" #include "virauthconfig.h" +#include "virsecureerase.h"
#define VIR_FROM_THIS VIR_FROM_AUTH
@@ -283,3 +284,68 @@ virAuthGetPassword(virConnectPtr conn,
return virAuthGetPasswordPath(path, auth, servicename, username, hostname); } + + +void +virAuthConnectCredentialFree(virConnectCredential *cred) +{ + if (cred->result) { + virSecureErase(cred->result, cred->resultlen); + g_free(cred->result); + } + g_free(cred); +}
As long as you're introducing this function, I'd personally like to see the open-coded versions replaced with a call to this function in other locations (such as remoteAuthInteractStateClear()).
+ + +/** + * virAuthAskCredential: + * @auth: authentication callback data + * @prompt: question string to ask the user + * @echo: true if user's reply should be considered sensitive and not echoed
The description of the @echo parameter seems inverted.
+ * + * Invoke the authentication callback for the connection @auth and ask the user + * the question in @prompt. If @echo is true user's reply should be collected + * as sensitive (user's input not printed on screen).
again
+ */ +virConnectCredential * +virAuthAskCredential(virConnectAuthPtr auth, + const char *prompt, + bool echo) +{ + g_autoptr(virConnectCredential) ret = g_new0(virConnectCredential, 1); + size_t i; + + ret->type = -1; + + for (i = 0; i < auth->ncredtype; ++i) { + int type = auth->credtype[i]; + if (echo) { + if (type == VIR_CRED_ECHOPROMPT) { + ret->type = type; + break; + } + } else { + if (type == VIR_CRED_PASSPHRASE || + type == VIR_CRED_NOECHOPROMPT) { + ret->type = type; + break; + } + } + } + + if (ret->type == -1) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("no suitable callback authentication callback was found")); + return NULL; + } + + ret->prompt = prompt; + + if (auth->cb(ret, 1, auth->cbdata)) {
I suppose this works since negative values evaluate to true, but it *looks* like the logic is incorrect. It would be more readable as "if (cb() < 0)"
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to retrieve user response for authentication callback")); + return NULL; + } + + return g_steal_pointer(&ret); +} diff --git a/src/util/virauth.h b/src/util/virauth.h index a0fd84962b..3eaf40c626 100644 --- a/src/util/virauth.h +++ b/src/util/virauth.h @@ -52,3 +52,10 @@ char * virAuthGetPasswordPath(const char *path, const char *servicename, const char *username, const char *hostname); + +virConnectCredential *virAuthAskCredential(virConnectAuthPtr auth, + const char *prompt, + bool echo); + +void virAuthConnectCredentialFree(virConnectCredential *cred); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virConnectCredential, virAuthConnectCredentialFree);
Rework the code to use the new helper instead of open coding the auth callback interaction. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 47 ++++++----------------------------- 1 file changed, 7 insertions(+), 40 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 084224b3f8..942f8526c2 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -647,26 +647,17 @@ virNetLibsshAuthenticateKeyboardInteractive(virNetLibsshSession *sess, virBufferAddChar(&buff, '\n'); for (iprompt = 0; iprompt < nprompts; ++iprompt) { - virConnectCredential retr_passphrase; const char *promptStr; int promptStrLen; char echo; - char *prompt = NULL; - int cred_type; + g_autofree char *prompt = NULL; + g_autoptr(virConnectCredential) cred = NULL; /* get the prompt */ promptStr = ssh_userauth_kbdint_getprompt(sess->session, iprompt, &echo); promptStrLen = virLengthForPromptString(promptStr); - cred_type = virCredTypeForPrompt(sess->cred, echo); - if (cred_type == -1) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("no suitable callback for input of keyboard " - "response")); - goto prompt_error; - } - /* create the prompt for the user, using the instruction * buffer if specified */ @@ -681,42 +672,18 @@ virNetLibsshAuthenticateKeyboardInteractive(virNetLibsshSession *sess, prompt = g_strndup(promptStr, promptStrLen); } - memset(&retr_passphrase, 0, sizeof(virConnectCredential)); - retr_passphrase.type = cred_type; - retr_passphrase.prompt = prompt; - - if (retr_passphrase.type == -1) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("no suitable callback for input of key " - "passphrase")); - goto prompt_error; - } - - if (sess->cred->cb(&retr_passphrase, 1, sess->cred->cbdata)) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("failed to retrieve keyboard interactive " - "result: callback has failed")); - goto prompt_error; - } + if (!(cred = virAuthAskCredential(sess->cred, prompt, echo))) + return SSH_AUTH_ERROR; - VIR_FREE(prompt); - - ret = ssh_userauth_kbdint_setanswer(sess->session, iprompt, - retr_passphrase.result); - virSecureEraseString(retr_passphrase.result); - g_free(retr_passphrase.result); - if (ret < 0) { + if (ssh_userauth_kbdint_setanswer(sess->session, iprompt, + cred->result) < 0) { errmsg = ssh_get_error(sess->session); virReportError(VIR_ERR_AUTH_FAILED, _("authentication failed: %s"), errmsg); - goto prompt_error; + return SSH_AUTH_ERROR; } continue; - - prompt_error: - VIR_FREE(prompt); - return SSH_AUTH_ERROR; } ret = ssh_userauth_kbdint(sess->session, NULL, NULL); -- 2.38.1
I personally would've preferred to have this patch combined with the previous one so that it's easier to verify that the refactored code maintains the same functionality as the replaced code. But I understand that you're refactoring more cases in the upcoming commits, so I guess there's not much point making this one any different than the others. Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com> On 1/17/23 10:20 AM, Peter Krempa wrote:
Rework the code to use the new helper instead of open coding the auth callback interaction.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 47 ++++++----------------------------- 1 file changed, 7 insertions(+), 40 deletions(-)
diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 084224b3f8..942f8526c2 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -647,26 +647,17 @@ virNetLibsshAuthenticateKeyboardInteractive(virNetLibsshSession *sess, virBufferAddChar(&buff, '\n');
for (iprompt = 0; iprompt < nprompts; ++iprompt) { - virConnectCredential retr_passphrase; const char *promptStr; int promptStrLen; char echo; - char *prompt = NULL; - int cred_type; + g_autofree char *prompt = NULL; + g_autoptr(virConnectCredential) cred = NULL;
/* get the prompt */ promptStr = ssh_userauth_kbdint_getprompt(sess->session, iprompt, &echo); promptStrLen = virLengthForPromptString(promptStr);
- cred_type = virCredTypeForPrompt(sess->cred, echo); - if (cred_type == -1) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("no suitable callback for input of keyboard " - "response")); - goto prompt_error; - } - /* create the prompt for the user, using the instruction * buffer if specified */ @@ -681,42 +672,18 @@ virNetLibsshAuthenticateKeyboardInteractive(virNetLibsshSession *sess, prompt = g_strndup(promptStr, promptStrLen); }
- memset(&retr_passphrase, 0, sizeof(virConnectCredential)); - retr_passphrase.type = cred_type; - retr_passphrase.prompt = prompt; - - if (retr_passphrase.type == -1) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("no suitable callback for input of key " - "passphrase")); - goto prompt_error; - } - - if (sess->cred->cb(&retr_passphrase, 1, sess->cred->cbdata)) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("failed to retrieve keyboard interactive " - "result: callback has failed")); - goto prompt_error; - } + if (!(cred = virAuthAskCredential(sess->cred, prompt, echo))) + return SSH_AUTH_ERROR;
- VIR_FREE(prompt); - - ret = ssh_userauth_kbdint_setanswer(sess->session, iprompt, - retr_passphrase.result); - virSecureEraseString(retr_passphrase.result); - g_free(retr_passphrase.result); - if (ret < 0) { + if (ssh_userauth_kbdint_setanswer(sess->session, iprompt, + cred->result) < 0) { errmsg = ssh_get_error(sess->session); virReportError(VIR_ERR_AUTH_FAILED, _("authentication failed: %s"), errmsg); - goto prompt_error; + return SSH_AUTH_ERROR; }
continue; - - prompt_error: - VIR_FREE(prompt); - return SSH_AUTH_ERROR; }
ret = ssh_userauth_kbdint(sess->session, NULL, NULL);
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 942f8526c2..748c1ed569 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -394,10 +394,8 @@ virNetLibsshAuthenticatePrivkeyCb(const char *prompt, void *userdata) { virNetLibsshSession *sess = userdata; - virConnectCredential retr_passphrase; - int cred_type; g_autofree char *actual_prompt = NULL; - int p; + g_autoptr(virConnectCredential) cred = NULL; /* request user's key password */ if (!sess->cred || !sess->cred->cb) { @@ -407,30 +405,12 @@ virNetLibsshAuthenticatePrivkeyCb(const char *prompt, return -1; } - cred_type = virCredTypeForPrompt(sess->cred, echo); - if (cred_type == -1) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("no suitable callback for input of key passphrase")); - return -1; - } - actual_prompt = g_strndup(prompt, virLengthForPromptString(prompt)); - memset(&retr_passphrase, 0, sizeof(virConnectCredential)); - retr_passphrase.type = cred_type; - retr_passphrase.prompt = actual_prompt; - - if (sess->cred->cb(&retr_passphrase, 1, sess->cred->cbdata)) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("failed to retrieve private key passphrase: " - "callback has failed")); + if (!(cred = virAuthAskCredential(sess->cred, actual_prompt, echo))) return -1; - } - p = virStrcpy(buf, retr_passphrase.result, len); - virSecureEraseString(retr_passphrase.result); - g_free(retr_passphrase.result); - if (p < 0) { + if (virStrcpy(buf, cred->result, len) < 0) { virReportError(VIR_ERR_LIBSSH, "%s", _("passphrase is too long for the buffer")); return -1; -- 2.38.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virauth.c | 2 +- src/util/virauth.h | 5 +++++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5616c0d44c..59ae5c2720 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1854,6 +1854,7 @@ virAuthAskCredential; virAuthConnectCredentialFree; virAuthGetConfigFilePath; virAuthGetConfigFilePathURI; +virAuthGetCredential; virAuthGetPassword; virAuthGetPasswordPath; virAuthGetUsername; diff --git a/src/util/virauth.c b/src/util/virauth.c index aa1da80266..e33658d356 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -99,7 +99,7 @@ virAuthGetConfigFilePath(virConnectPtr conn, } -static int +int virAuthGetCredential(const char *servicename, const char *hostname, const char *credname, diff --git a/src/util/virauth.h b/src/util/virauth.h index 3eaf40c626..589f3df6b7 100644 --- a/src/util/virauth.h +++ b/src/util/virauth.h @@ -42,6 +42,11 @@ char *virAuthGetPassword(virConnectPtr conn, const char *servicename, const char *username, const char *hostname); +int virAuthGetCredential(const char *servicename, + const char *hostname, + const char *credname, + const char *path, + char **value); char * virAuthGetUsernamePath(const char *path, virConnectAuthPtr auth, const char *servicename, -- 2.38.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 57 +++++------------------------------ 1 file changed, 7 insertions(+), 50 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 748c1ed569..ecee30e5df 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -218,27 +218,6 @@ virLibsshServerKeyAsString(virNetLibsshSession *sess) return str; } -static int -virCredTypeForPrompt(virConnectAuthPtr cred, char echo) -{ - size_t i; - - for (i = 0; i < cred->ncredtype; ++i) { - int type = cred->credtype[i]; - if (echo) { - if (type == VIR_CRED_ECHOPROMPT) - return type; - } else { - if (type == VIR_CRED_PASSPHRASE || - type == VIR_CRED_NOECHOPROMPT) { - return type; - } - } - } - - return -1; -} - static int virLengthForPromptString(const char *str) { @@ -296,9 +275,8 @@ virNetLibsshCheckHostKey(virNetLibsshSession *sess) case SSH_SERVER_NOT_KNOWN: /* key was not found, query to add it to database */ if (sess->hostKeyVerify == VIR_NET_LIBSSH_HOSTKEY_VERIFY_NORMAL) { - virConnectCredential askKey; - int cred_type; - char *tmp; + g_autoptr(virConnectCredential) cred = NULL; + g_autofree char *prompt = NULL; /* ask to add the key */ if (!sess->cred || !sess->cred->cb) { @@ -308,48 +286,27 @@ virNetLibsshCheckHostKey(virNetLibsshSession *sess) return -1; } - cred_type = virCredTypeForPrompt(sess->cred, 1 /* echo */); - if (cred_type == -1) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("no suitable callback for host key " - "verification")); - return -1; - } - - /* prepare data for the callback */ - memset(&askKey, 0, sizeof(virConnectCredential)); - askKey.type = cred_type; - keyhashstr = virLibsshServerKeyAsString(sess); if (!keyhashstr) return -1; - tmp = g_strdup_printf(_("Accept SSH host key with hash '%s' for " "host '%s:%d' (%s/%s)?"), - keyhashstr, sess->hostname, sess->port, "y", "n"); - askKey.prompt = tmp; + prompt = g_strdup_printf(_("Accept SSH host key with hash '%s' for " "host '%s:%d' (%s/%s)?"), + keyhashstr, sess->hostname, sess->port, "y", "n"); - if (sess->cred->cb(&askKey, 1, sess->cred->cbdata)) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("failed to retrieve decision to accept " - "host key")); - VIR_FREE(tmp); + if (!(cred = virAuthAskCredential(sess->cred, prompt, false))) { ssh_string_free_char(keyhashstr); return -1; } - VIR_FREE(tmp); - - if (!askKey.result || - STRCASENEQ(askKey.result, "y")) { + if (!cred->result || + STRCASENEQ(cred->result, "y")) { virReportError(VIR_ERR_LIBSSH, _("SSH host key for '%s' (%s) was not accepted"), sess->hostname, keyhashstr); ssh_string_free_char(keyhashstr); - VIR_FREE(askKey.result); return -1; } ssh_string_free_char(keyhashstr); - VIR_FREE(askKey.result); } /* write the host key file, if specified */ -- 2.38.1
On 1/17/23 10:20 AM, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 57 +++++------------------------------ 1 file changed, 7 insertions(+), 50 deletions(-)
diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 748c1ed569..ecee30e5df 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -218,27 +218,6 @@ virLibsshServerKeyAsString(virNetLibsshSession *sess) return str; }
-static int -virCredTypeForPrompt(virConnectAuthPtr cred, char echo) -{ - size_t i; - - for (i = 0; i < cred->ncredtype; ++i) { - int type = cred->credtype[i]; - if (echo) { - if (type == VIR_CRED_ECHOPROMPT) - return type; - } else { - if (type == VIR_CRED_PASSPHRASE || - type == VIR_CRED_NOECHOPROMPT) { - return type; - } - } - } - - return -1; -} - static int virLengthForPromptString(const char *str) { @@ -296,9 +275,8 @@ virNetLibsshCheckHostKey(virNetLibsshSession *sess) case SSH_SERVER_NOT_KNOWN: /* key was not found, query to add it to database */ if (sess->hostKeyVerify == VIR_NET_LIBSSH_HOSTKEY_VERIFY_NORMAL) { - virConnectCredential askKey; - int cred_type; - char *tmp; + g_autoptr(virConnectCredential) cred = NULL; + g_autofree char *prompt = NULL;
/* ask to add the key */ if (!sess->cred || !sess->cred->cb) { @@ -308,48 +286,27 @@ virNetLibsshCheckHostKey(virNetLibsshSession *sess) return -1; }
- cred_type = virCredTypeForPrompt(sess->cred, 1 /* echo */);
Here `echo` was 1
- if (cred_type == -1) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("no suitable callback for host key " - "verification")); - return -1; - } - - /* prepare data for the callback */ - memset(&askKey, 0, sizeof(virConnectCredential)); - askKey.type = cred_type; - keyhashstr = virLibsshServerKeyAsString(sess); if (!keyhashstr) return -1;
- tmp = g_strdup_printf(_("Accept SSH host key with hash '%s' for " "host '%s:%d' (%s/%s)?"), - keyhashstr, sess->hostname, sess->port, "y", "n"); - askKey.prompt = tmp; + prompt = g_strdup_printf(_("Accept SSH host key with hash '%s' for " "host '%s:%d' (%s/%s)?"), + keyhashstr, sess->hostname, sess->port, "y", "n");
- if (sess->cred->cb(&askKey, 1, sess->cred->cbdata)) { - virReportError(VIR_ERR_LIBSSH, "%s", - _("failed to retrieve decision to accept " - "host key")); - VIR_FREE(tmp); + if (!(cred = virAuthAskCredential(sess->cred, prompt, false))) {
Here you're passing `false` for `echo`.
ssh_string_free_char(keyhashstr); return -1; }
- VIR_FREE(tmp); - - if (!askKey.result || - STRCASENEQ(askKey.result, "y")) { + if (!cred->result || + STRCASENEQ(cred->result, "y")) { virReportError(VIR_ERR_LIBSSH, _("SSH host key for '%s' (%s) was not accepted"), sess->hostname, keyhashstr); ssh_string_free_char(keyhashstr); - VIR_FREE(askKey.result); return -1; } ssh_string_free_char(keyhashstr); - VIR_FREE(askKey.result); }
/* write the host key file, if specified */
virAuthGetPasswordPath can return the same password over and over if it's configured in the config. We rather want to try that only the first time and then ask the user instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index ecee30e5df..7da7a90985 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -500,6 +500,7 @@ virNetLibsshAuthenticatePrivkey(virNetLibsshSession *sess, static int virNetLibsshAuthenticatePassword(virNetLibsshSession *sess) { + g_autofree char *password = NULL; const char *errmsg; int rc = SSH_AUTH_ERROR; @@ -513,19 +514,34 @@ virNetLibsshAuthenticatePassword(virNetLibsshSession *sess) return SSH_AUTH_ERROR; } + /* first try to get password from config */ + if (virAuthGetCredential("ssh", sess->hostname, "password", sess->authPath, + &password) < 0) + return SSH_AUTH_ERROR; + + if (password) { + rc = ssh_userauth_password(sess->session, NULL, password); + virSecureEraseString(password); + + if (rc == 0) + return SSH_AUTH_SUCCESS; + else if (rc != SSH_AUTH_DENIED) + goto error; + } + /* Try the authenticating the set amount of times. The server breaks the * connection if maximum number of bad auth tries is exceeded */ while (true) { - g_autofree char *password = NULL; + g_autoptr(virConnectCredential) cred = NULL; + g_autofree char *prompt = NULL; + + prompt = g_strdup_printf(_("Enter %s's password for %s"), + sess->username, sess->hostname); - if (!(password = virAuthGetPasswordPath(sess->authPath, sess->cred, - "ssh", sess->username, - sess->hostname))) + if (!(cred = virAuthAskCredential(sess->cred, prompt, false))) return SSH_AUTH_ERROR; - /* tunnelled password authentication */ - rc = ssh_userauth_password(sess->session, NULL, password); - virSecureEraseString(password); + rc = ssh_userauth_password(sess->session, NULL, cred->result); if (rc == 0) return SSH_AUTH_SUCCESS; @@ -533,7 +549,7 @@ virNetLibsshAuthenticatePassword(virNetLibsshSession *sess) break; } - /* error path */ + error: errmsg = ssh_get_error(sess->session); virReportError(VIR_ERR_AUTH_FAILED, _("authentication failed: %s"), errmsg); -- 2.38.1
Replace the open-coded variant by the new helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virauth.c | 39 ++++----------------------------------- 1 file changed, 4 insertions(+), 35 deletions(-) diff --git a/src/util/virauth.c b/src/util/virauth.c index e33658d356..14c48f7e25 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -215,8 +215,7 @@ virAuthGetPasswordPath(const char *path, const char *username, const char *hostname) { - unsigned int ncred; - virConnectCredential cred; + g_autoptr(virConnectCredential) cred = NULL; g_autofree char *prompt = NULL; char *ret = NULL; @@ -231,42 +230,12 @@ virAuthGetPasswordPath(const char *path, return NULL; } - memset(&cred, 0, sizeof(virConnectCredential)); - prompt = g_strdup_printf(_("Enter %s's password for %s"), username, hostname); - for (ncred = 0; ncred < auth->ncredtype; ncred++) { - if (auth->credtype[ncred] != VIR_CRED_PASSPHRASE && - auth->credtype[ncred] != VIR_CRED_NOECHOPROMPT) { - 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; - cred.defresult = NULL; - cred.result = NULL; - cred.resultlen = 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; - } + if (!(cred = virAuthAskCredential(auth, prompt, false))) + return NULL; - virReportError(VIR_ERR_AUTH_FAILED, "%s", - _("Missing VIR_CRED_PASSPHRASE or VIR_CRED_NOECHOPROMPT " - "credential type")); - return NULL; + return g_steal_pointer(&cred->result); } -- 2.38.1
On 1/17/23 10:20 AM, Peter Krempa wrote:
Peter Krempa (15): virNetLibsshSessionAuthAddPrivKeyAuth: Drop 'password' argument virNetLibsshAuthMethod: Drop 'password' field util: authconfig: Use automatic pointer clearing for virAuthConfig util: authconfig: Use conteporary and consistent header style virNetSSHSessionAuthAddPrivKeyAuth: Remove unused 'password' argument virNetSSHSessionAuthAddPrivKeyAuth: Refactor cleanup virNetSSHAuthMethod: Remove unused 'password' field virnetsshsession: Pass in username via virNetSSHSessionNew rather than auth functions util: auth: Introduce virAuthAskCredential virNetLibsshAuthenticateKeyboardInteractive: Use virAuthAskCredential virNetLibsshAuthenticatePrivkeyCb: Use virAuthAskCredential util: virauth: Export virAuthGetCredential virNetLibsshCheckHostKey: Use virAuthAskCredential virNetLibsshAuthenticatePassword: Use virAuthAskPassword instead of virAuthGetPasswordPath virAuthGetPasswordPath: Use virAuthAskCredential for callback interaction
src/libvirt_private.syms | 3 + src/rpc/virnetlibsshsession.c | 211 ++++++++++------------------------ src/rpc/virnetlibsshsession.h | 3 +- src/rpc/virnetsocket.c | 19 +-- src/rpc/virnetsshsession.c | 162 ++++++++------------------ src/rpc/virnetsshsession.h | 13 +-- src/util/virauth.c | 107 +++++++++++------ src/util/virauth.h | 12 ++ src/util/virauthconfig.c | 52 ++++----- 9 files changed, 223 insertions(+), 359 deletions(-)
See separate emails for a few comments on individual patches Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
participants (2)
-
Jonathon Jongsma -
Peter Krempa