Re: [PATCH v3 3/6] libssh_transport: add new libssh-based transport

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tuesday, 1 November 2016 13:20:28 CET Peter Krempa wrote:
> On Wed, Oct 19, 2016 at 14:40:36 +0200, Pino Toscano wrote:
> > Implement a new libssh transport, which uses libssh to communicate with
> > remote hosts, and add all the build system stuff (search of libssh,
> > private symbols, etc) to built it.
> > 
> > This new transport supports all the common ssh authentication methods,
> > making use of libvirt's auth callbacks for interaction with the user.
> > ---
> >  config-post.h                 |    2 +
> >  configure.ac                  |    3 +
> >  m4/virt-libssh.m4             |   26 +
> >  po/POTFILES.in                |    1 +
> >  src/Makefile.am               |   21 +-
> >  src/libvirt_libssh.syms       |   22 +
> >  src/rpc/virnetlibsshsession.c | 1458 +++++++++++++++++++++++++++++++++++++++++
> >  src/rpc/virnetlibsshsession.h |   80 +++
> >  8 files changed, 1611 insertions(+), 2 deletions(-)
> >  create mode 100644 m4/virt-libssh.m4
> >  create mode 100644 src/libvirt_libssh.syms
> >  create mode 100644 src/rpc/virnetlibsshsession.c
> >  create mode 100644 src/rpc/virnetlibsshsession.h
> 
> 
> > diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c
> > new file mode 100644
> > index 0000000..13dd52c
> > --- /dev/null
> > +++ b/src/rpc/virnetlibsshsession.c
> > @@ -0,0 +1,1458 @@
> 
> [...]
> 
> > +static void
> > +virNetLibsshSessionAuthMethodsFree(virNetLibsshSessionPtr sess)
> > +{
> > +    size_t i;
> > +
> > +    for (i = 0; i < sess->nauths; i++) {
> > +        VIR_FREE(sess->auths[i]->password);
> 
> This should be turned into VIR_DISPOSE_STR.

Right. (Note the same fix should be done in the libssh2 transport too.)

> > +
> > +/* string representation of public key of remote server */
> > +static char *
> > +virSshServerKeyAsString(virNetLibsshSessionPtr sess)
> > +{
> > +    int ret;
> > +    ssh_key key;
> > +    unsigned char *keyhash;
> > +    size_t keyhashlen;
> > +    char *str;
> > +
> > +    if (ssh_get_publickey(sess->session, &key) != SSH_OK) {
> > +        virReportError(VIR_ERR_LIBSSH, "%s",
> > +                       _("failed to get the key of the current "
> > +                         "session"));
> > +        return NULL;
> > +    }
> > +
> > +    /* calculate remote key hash, using MD5 algorithm that is
> > +     * usual in OpenSSH. The returned value must be freed */
> > +    ret = ssh_get_publickey_hash(key, SSH_PUBLICKEY_HASH_MD5,
> > +                                 &keyhash, &keyhashlen);
> 
> Openssh currently is using SHA256 with base64 encoding. I think we
> should at least for libssh switch to this newer approach.

Unfortunately the libssh API only allows SHA1 and MD5:

enum ssh_publickey_hash_type {
    SSH_PUBLICKEY_HASH_SHA1,
    SSH_PUBLICKEY_HASH_MD5
};

libssh2 uses MD5, which is why I chose to do the same in this case.
I will change it to SHA1, should be slightly better.

> > +void
> > +virNetLibsshSessionAuthReset(virNetLibsshSessionPtr sess)
> 
> This function is not used anywhere in this series.

Neither is virNetSSHSessionAuthReset -- removed.

> > +/* perform keyboard interactive authentication
> > + *
> > + * returns SSH_AUTH_* values
> > + */
> > +static int
> > +virNetLibsshAuthenticateKeyboardInteractive(virNetLibsshSessionPtr sess,
> > +                                            virNetLibsshAuthMethodPtr priv)
> > +{
> > +    int ret;
> > +    const char *errmsg;
> > +    int try = 0;
> > +
> > +    /* request user's key password */
> > +    if (!sess->cred || !sess->cred->cb) {
> > +        virReportError(VIR_ERR_LIBSSH, "%s",
> > +                       _("No user interaction callback provided: "
> > +                         "Can't get input from keyboard interactive "
> > +                         "authentication"));
> > +        return SSH_AUTH_ERROR;
> > +    }
> > +
> > + again:
> 
> Please use a while loop for this purpose.
> 
> > +    ret = ssh_userauth_kbdint(sess->session, NULL, NULL);
> > +    while (ret == SSH_AUTH_INFO) {
> > +        const char *name, *instruction;
> > +        int nprompts, iprompt;
> > +        virBuffer buff = VIR_BUFFER_INITIALIZER;
> > +
> > +        name = ssh_userauth_kbdint_getname(sess->session);
> > +        instruction = ssh_userauth_kbdint_getinstruction(sess->session);
> > +        nprompts = ssh_userauth_kbdint_getnprompts(sess->session);
> > +
> > +        /* compose the main buffer with name and instruction, if present */
> > +        if (name && name[0])
> > +            virBufferAddStr(&buff, name);
> > +        if (instruction && instruction[0]) {
> > +            if (virBufferUse(&buff) > 0)
> > +                virBufferAddChar(&buff, '\n');
> > +            virBufferAddStr(&buff, instruction);
> > +        }
> > +
> > +        if (virBufferCheckError(&buff) < 0)
> > +            return -1;
> > +
> > +        for (iprompt = 0; iprompt < nprompts; ++iprompt) {
> > +            virConnectCredential retr_passphrase;
> > +            const char *promptStr;
> > +            char echo;
> > +            virBuffer prompt_buff = VIR_BUFFER_INITIALIZER;
> > +            char *prompt = NULL;
> > +            int cred_type;
> > +
> > +            /* get the prompt, stripping the trailing newlines and spaces */
> > +            promptStr = ssh_userauth_kbdint_getprompt(sess->session, iprompt,
> > +                                                      &echo);
> > +
> > +            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;
> > +            }
> > +
> > +            /* compose the instruction buffer with this prompt */
> > +            if (virBufferUse(&buff) > 0) {
> > +                virBufferAddBuffer(&prompt_buff, &buff);
> > +                virBufferAddChar(&prompt_buff, '\n');
> > +            }
> > +            virBufferAdd(&prompt_buff, promptStr,
> > +                         virLengthForPromptString(promptStr));
> > +
> > +            if (virBufferCheckError(&prompt_buff) < 0)
> > +                goto prompt_error;
> > +
> > +            prompt = virBufferContentAndReset(&prompt_buff);
> > +
> > +            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;
> > +            }
> > +
> > +            VIR_FREE(prompt);
> > +
> > +            ret = ssh_userauth_kbdint_setanswer(sess->session, iprompt,
> > +                                                retr_passphrase.result);
> > +            VIR_DISPOSE_STRING(retr_passphrase.result);
> > +            if (ret < 0) {
> > +                errmsg = ssh_get_error(sess->session);
> > +                virReportError(VIR_ERR_AUTH_FAILED,
> > +                               _("authentication failed: %s"), errmsg);
> > +                goto prompt_error;
> > +            }
> 
> Maybe it would be better to extract this into a function since this
> looks rather complex.

How "granular" should this be split?

Thanks,
-- 
Pino Toscano

Attachment: signature.asc
Description: This is a digitally signed message part.

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]