[libvirt] [PATCH] rpc: always pass "-T -e none" args to ssh

Daniel P. Berrangé posted 1 patch 5 years, 4 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190709171559.3372-1-berrange@redhat.com
src/rpc/virnetsocket.c   |  4 ++--
tests/virnetsockettest.c | 34 ++++++++++++++++++----------------
2 files changed, 20 insertions(+), 18 deletions(-)
[libvirt] [PATCH] rpc: always pass "-T -e none" args to ssh
Posted by Daniel P. Berrangé 5 years, 4 months ago
Way back in the past, the "no_tty=1" option was added for the remote
driver to disable local password prompting by disabling use of the local
tty:

  commit b32f42984994a397441a1c48f1a002e906624c51
  Author: Daniel P. Berrange <berrange@redhat.com>
  Date:   Fri Sep 21 20:17:09 2007 +0000

    Added a no_tty param to remote URIs to stop SSH prompting for password

This was done by adding "-T -o BatchMode=yes -e none" args to ssh. This
achieved the desired results but is none the less semantically flawed
because it is mixing up config parameters for the local tty vs the
remote tty.

The "-T" arg stops allocation of a TTY on the remote host. This is good
for all libvirt SSH tunnels as we never require a TTY for our usage
model, so we should have just passed this unconditionally.

The "-e none" option disables the escape character for sessions with a
TTY. If we pass "-T" this is not required, but it also not harmful to
add it, so we should just pass it unconditionally too.

Only the "-o BatchMode=yes" option is related to disabling local
password prompts and thus needs control via the no_tty URI param.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/rpc/virnetsocket.c   |  4 ++--
 tests/virnetsockettest.c | 34 ++++++++++++++++++----------------
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index bfa1952989..aa46b83da6 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -854,9 +854,9 @@ int virNetSocketNewConnectSSH(const char *nodename,
         virCommandAddArgList(cmd, "-l", username, NULL);
     if (keyfile)
         virCommandAddArgList(cmd, "-i", keyfile, NULL);
+    virCommandAddArgList(cmd, "-T", "-e", "none", NULL);
     if (noTTY)
-        virCommandAddArgList(cmd, "-T", "-o", "BatchMode=yes",
-                             "-e", "none", NULL);
+        virCommandAddArgList(cmd, "-o", "BatchMode=yes", NULL);
     if (noVerify)
         virCommandAddArgList(cmd, "-o", "StrictHostKeyChecking=no", NULL);
 
diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c
index 9c14989287..bb8357f7cd 100644
--- a/tests/virnetsockettest.c
+++ b/tests/virnetsockettest.c
@@ -571,12 +571,13 @@ mymain(void)
     struct testSSHData sshData1 = {
         .nodename = "somehost",
         .path = "/tmp/socket",
-        .expectOut = "-- somehost sh -c 'if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
-                                         "ARG=-q0;"
-                                     "else "
-                                         "ARG=;"
-                                     "fi;"
-                                     "'nc' $ARG -U /tmp/socket'\n",
+        .expectOut = "-T -e none -- somehost sh -c '"
+                     "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
+                         "ARG=-q0;"
+                     "else "
+                         "ARG=;"
+                     "fi;"
+                     "'nc' $ARG -U /tmp/socket'\n",
     };
     if (virTestRun("SSH test 1", testSocketSSH, &sshData1) < 0)
         ret = -1;
@@ -589,7 +590,7 @@ mymain(void)
         .noTTY = true,
         .noVerify = false,
         .path = "/tmp/socket",
-        .expectOut = "-p 9000 -l fred -T -o BatchMode=yes -e none -- somehost sh -c '"
+        .expectOut = "-p 9000 -l fred -T -e none -o BatchMode=yes -- somehost sh -c '"
                      "if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
                          "ARG=-q0;"
                      "else "
@@ -608,7 +609,7 @@ mymain(void)
         .noTTY = false,
         .noVerify = true,
         .path = "/tmp/socket",
-        .expectOut = "-p 9000 -l fred -o StrictHostKeyChecking=no -- somehost sh -c '"
+        .expectOut = "-p 9000 -l fred -T -e none -o StrictHostKeyChecking=no -- somehost sh -c '"
                      "if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
                          "ARG=-q0;"
                      "else "
@@ -630,7 +631,7 @@ mymain(void)
     struct testSSHData sshData5 = {
         .nodename = "crashyhost",
         .path = "/tmp/socket",
-        .expectOut = "-- crashyhost sh -c "
+        .expectOut = "-T -e none -- crashyhost sh -c "
                      "'if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
                          "ARG=-q0;"
                      "else "
@@ -647,7 +648,7 @@ mymain(void)
         .path = "/tmp/socket",
         .keyfile = "/root/.ssh/example_key",
         .noVerify = true,
-        .expectOut = "-i /root/.ssh/example_key -o StrictHostKeyChecking=no -- example.com sh -c '"
+        .expectOut = "-i /root/.ssh/example_key -T -e none -o StrictHostKeyChecking=no -- example.com sh -c '"
                      "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
                          "ARG=-q0;"
                      "else "
@@ -662,12 +663,13 @@ mymain(void)
         .nodename = "somehost",
         .netcat = "nc -4",
         .path = "/tmp/socket",
-        .expectOut = "-- somehost sh -c 'if ''nc -4'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
-                                         "ARG=-q0;"
-                                     "else "
-                                         "ARG=;"
-                                     "fi;"
-                                     "''nc -4'' $ARG -U /tmp/socket'\n",
+        .expectOut = "-T -e none -- somehost sh -c '"
+                     "if ''nc -4'' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
+                         "ARG=-q0;"
+                     "else "
+                         "ARG=;"
+                     "fi;"
+                     "''nc -4'' $ARG -U /tmp/socket'\n",
     };
     if (virTestRun("SSH test 7", testSocketSSH, &sshData7) < 0)
         ret = -1;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: always pass "-T -e none" args to ssh
Posted by Andrea Bolognani 5 years, 4 months ago
On Tue, 2019-07-09 at 18:15 +0100, Daniel P. Berrangé wrote:
> Way back in the past, the "no_tty=1" option was added for the remote
> driver to disable local password prompting by disabling use of the local
> tty:
> 
>   commit b32f42984994a397441a1c48f1a002e906624c51
>   Author: Daniel P. Berrange <berrange@redhat.com>
>   Date:   Fri Sep 21 20:17:09 2007 +0000
> 
>     Added a no_tty param to remote URIs to stop SSH prompting for password
> 
> This was done by adding "-T -o BatchMode=yes -e none" args to ssh. This
> achieved the desired results but is none the less semantically flawed
> because it is mixing up config parameters for the local tty vs the
> remote tty.
> 
> The "-T" arg stops allocation of a TTY on the remote host. This is good
> for all libvirt SSH tunnels as we never require a TTY for our usage
> model, so we should have just passed this unconditionally.
> 
> The "-e none" option disables the escape character for sessions with a
> TTY. If we pass "-T" this is not required, but it also not harmful to
> add it, so we should just pass it unconditionally too.
> 
> Only the "-o BatchMode=yes" option is related to disabling local
> password prompts and thus needs control via the no_tty URI param.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/rpc/virnetsocket.c   |  4 ++--
>  tests/virnetsockettest.c | 34 ++++++++++++++++++----------------
>  2 files changed, 20 insertions(+), 18 deletions(-)

Great commit message!

The change makes sense, and the ssh documentation confirms the
options you mention indeed mean what you claim they do :)

Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

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