[libvirt] [PATCH] remote: Forbid default "/session" connections when using ssh transport
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; + } + + 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, -- 1.8.2.1
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.
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"'?
+ + 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").
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
On 06/13/2013 05:46 AM, Peter Krempa wrote:
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.
Sigh. No, I agree that private BZ's shouldn't be linked in a public repo, as it just causes frustration for those without access to the private BZ. It's unfortunate that the BZ was private, though (although there's nothing that we could do about it other than filing a parallel BZ)
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.
Okay, I just wanted to make sure. So ACK as it is then.
On 06/13/13 16:42, Laine Stump wrote:
On 06/13/2013 05:46 AM, Peter Krempa wrote:
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.
Sigh.
No, I agree that private BZ's shouldn't be linked in a public repo, as it just causes frustration for those without access to the private BZ. It's unfortunate that the BZ was private, though (although there's nothing that we could do about it other than filing a parallel BZ)
Indeed. Open source could be a bit more open sometimes.
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.
Okay, I just wanted to make sure. So ACK as it is then.
Thanks for the review. Pushed. Peter
participants (2)
-
Laine Stump -
Peter Krempa