[libvirt] [PATCH] remote: Forbid default "/session" connections when using ssh transport

[Peter Krempa]

On 06/12/13 21:34, Laine Stump wrote:
> On 06/10/2013 08:44 AM, Peter Krempa wrote:
>
>
> Can you include in the commit log a link to the BZ describing this
> problem? It helps *immensely* when trying to trace things months/years
> later.
>

There isn't any publicly available BZ describing this problem so I chose 
not to put the private link in the commit. I can do it if you insist but 
having private links in a public repo doesn't seem right to me.

>
>> Without the socket path explicitly specified, the remote driver tried to
>> connect to the "/system" instance socket even if "/session" was
>> specified in the uri. With this patch this configuration now produces an
>> error.
>>
>> It is still possible to initiate a session connection with specifying
>> the path to the socket manually and also manually starting the session
>> daemon. This was also possible prior to this patch,
>>
>> This is a minimal fix. We may decide to support remote session
>> connections using ssh but this will require changes to the remote driver
>> code so this fix shouldn't cause regressions in the case we decide to do
>> that.
>> ---
>>   src/remote/remote_driver.c | 40 ++++++++++++++++++++++++++++++----------
>>   1 file changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
>> index fcf45d3..bd5646a 100644
>> --- a/src/remote/remote_driver.c
>> +++ b/src/remote/remote_driver.c
>> @@ -631,11 +631,21 @@ doRemoteOpen(virConnectPtr conn,
>>           break;
>>
>>       case trans_libssh2:
>> -        if (!sockname &&
>> -            VIR_STRDUP(sockname,
>> -                       flags & VIR_DRV_OPEN_REMOTE_RO ?
>> -                       LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET) < 0)
>> -            goto failed;
>> +        if (!sockname) {
>> +            /* Right now we don't support default session connections */
>> +            if (STREQ(conn->uri->path, "/session")) {
>> +                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>> +                               _("Connecting to session instance without "
>> +                                 "socket path is not supported by the libssh2 "
>> +                                 "connection driver"));
>> +                goto failed;
>> +            }
>
> A totally naive question: do we want to only allow "/system"? or
> 'anything except "/session"'?

We want to forbid only "/session" with the default socket path which 
won't work right now. A user is able to start a session daemon and 
successfully connect to it even with this patch. The user has to 
manually specify a socket path.

Rejecting everything except "/system" would break drivers that use 
different path. A (stupid) example:

$ virsh -c test+ssh://root@localhost/default
Welcome to virsh, the virtualization interactive terminal.

Type:  'help' for help with commands
        'quit' to quit

virsh # list --all
  Id    Name                           State
----------------------------------------------------
  1     test                           running

>
>
>> +
>> +            if (VIR_STRDUP(sockname,
>> +                           flags & VIR_DRV_OPEN_REMOTE_RO ?
>> +                           LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET) < 0)
>> +                goto failed;
>> +        }
>>
>>           VIR_DEBUG("Starting LibSSH2 session");
>>
>> @@ -698,11 +708,21 @@ doRemoteOpen(virConnectPtr conn,
>>           if (!command && VIR_STRDUP(command, "ssh") < 0)
>>               goto failed;
>>
>> -        if (!sockname &&
>> -            VIR_STRDUP(sockname,
>> -                       flags & VIR_DRV_OPEN_REMOTE_RO ?
>> -                       LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET) < 0)
>> -            goto failed;
>> +        if (!sockname) {
>> +            /* Right now we don't support default session connections */
>> +            if (STREQ(conn->uri->path, "/session")) {
>> +                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>> +                               _("Connecting to session instance without "
>> +                                 "socket path is not supported by the ssh "
>> +                                 "connection driver"));
>> +                goto failed;
>> +            }
>> +
>> +            if (VIR_STRDUP(sockname,
>> +                           flags & VIR_DRV_OPEN_REMOTE_RO ?
>> +                           LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET) < 0)
>> +                goto failed;
>> +        }
>>
>>           if (!(priv->client = virNetClientNewSSH(priv->hostname,
>>                                                   port,
>
> ACK once the BZ link is included in the commit log (and pending my
> question about whether we want to check for == "/session" or != "/system").
>

Peter