issue #337: ssh: disabling PTYs round 2: make it automatic.

docs/api.rst

@@ -713,11 +713,12 @@ Router Class
 
         Construct a remote context over an OpenSSH ``ssh`` invocation.
 
-        By default, the ``ssh`` process is started in a newly allocated
-        pseudo-terminal to support typing interactive passwords, however when
-        making many connections, this may be disabled by specifying
-        `batch_mode=True`, as most operating systems have a conservative upper
-        limit on the number of pseudo-terminals that may exist.
+        The ``ssh`` process is started in a newly allocated pseudo-terminal to
+        support typing interactive passwords and responding to prompts, if a
+        password is specified, or `check_host_keys=accept`. In other scenarios,
+        ``BatchMode`` is enabled and no PTY is allocated. For many-target
+        configurations, both options should be avoided as most systems have a
+        conservative limit on the number of pseudo-terminals that may exist.
 
         Accepts all parameters accepted by :meth:`local`, in addition to:
 
@@ -764,11 +765,6 @@ Router Class
             are already compressed, however it has a large effect on every
             remaining message in the otherwise uncompressed stream protocol,
             such as function call arguments and return values.
-        :param bool batch_mode:
-            If :data:`True`, disable pseudo-terminal allocation. When
-            :data:`True`, the `password=` parameter may not be used, since no
-            PTY exists to enter the password, and the `check_host_keys=`
-            parameter may not be set to `accept`.
         :param int ssh_debug_level:
             Optional integer `0..3` indicating the SSH client debug level.
         :raises mitogen.ssh.PasswordError:

mitogen/core.py

@@ -1103,8 +1103,9 @@ class Stream(BasicStream):
         )
 
         if msg_len > self._router.max_message_size:
-            LOG.error('Maximum message size exceeded (got %d, max %d)',
-                      msg_len, self._router.max_message_size)
+            LOG.error('Maximum message size exceeded (got %d, max %d) %r',
+                      msg_len, self._router.max_message_size,
+                      self._input_buf[0])
             self.on_disconnect(broker)
             return False
 

mitogen/doas.py

@@ -45,8 +45,8 @@ class Stream(mitogen.parent.Stream):
     create_child = staticmethod(mitogen.parent.hybrid_tty_create_child)
     child_is_immediate_subprocess = False
 
-    #: Once connected, points to the corresponding TtyLogStream, allowing it to
-    #: be disconnected at the same time this stream is being torn down.
+    #: Once connected, points to the corresponding DiagLogStream, allowing it
+    #: to be disconnected at the same time this stream is being torn down.
     tty_stream = None
 
     username = 'root'
@@ -89,7 +89,7 @@ class Stream(mitogen.parent.Stream):
     password_required_msg = 'doas password is required'
 
     def _connect_bootstrap(self, extra_fd):
-        self.tty_stream = mitogen.parent.TtyLogStream(extra_fd, self)
+        self.tty_stream = mitogen.parent.DiagLogStream(extra_fd, self)
 
         password_sent = False
         it = mitogen.parent.iter_read(

mitogen/parent.py

@@ -205,7 +205,7 @@ def detach_popen(*args, **kwargs):
     return proc.pid
 
 
-def create_child(args, merge_stdio=False, preexec_fn=None):
+def create_child(args, merge_stdio=False, stderr_pipe=False, preexec_fn=None):
     """
     Create a child process whose stdin/stdout is connected to a socket.
 
@@ -216,8 +216,13 @@ def create_child(args, merge_stdio=False, preexec_fn=None):
         socketpair, rather than inherited from the parent process. This may be
         necessary to ensure that not TTY is connected to any stdio handle, for
         instance when using LXC.
+    :param bool stderr_pipe:
+        If :data:`True` and `merge_stdio` is :data:`False`, arrange for
+        `stderr` to be connected to a separate pipe, to allow any ongoing debug
+        logs generated by e.g. SSH to be outpu as the session progresses,
+        without interfering with `stdout`.
     :returns:
-        `(pid, socket_obj, :data:`None`)`
+        `(pid, socket_obj, :data:`None` or pipe_fd)`
     """
     parentfp, childfp = create_socketpair()
     # When running under a monkey patches-enabled gevent, the socket module
@@ -228,8 +233,12 @@ def create_child(args, merge_stdio=False, preexec_fn=None):
 
     if merge_stdio:
         extra = {'stderr': childfp}
-    else:
-        extra = {}
+        stderr_r = None
+    elif stderr_pipe:
+        stderr_r, stderr_w = os.pipe()
+        mitogen.core.set_cloexec(stderr_r)
+        mitogen.core.set_cloexec(stderr_w)
+        extra = {'stderr': stderr_w}
 
     pid = detach_popen(
         args=args,
@@ -239,6 +248,8 @@ def create_child(args, merge_stdio=False, preexec_fn=None):
         preexec_fn=preexec_fn,
         **extra
     )
+    if stderr_pipe:
+        os.close(stderr_w)
     childfp.close()
     # Decouple the socket from the lifetime of the Python socket object.
     fd = os.dup(parentfp.fileno())
@@ -246,7 +257,7 @@ def create_child(args, merge_stdio=False, preexec_fn=None):
 
     LOG.debug('create_child() child %d fd %d, parent %d, cmd: %s',
               pid, fd, os.getpid(), Argv(args))
-    return pid, fd, None
+    return pid, fd, stderr_r
 
 
 def _acquire_controlling_tty():
@@ -792,7 +803,7 @@ PREFERRED_POLLER = POLLER_BY_SYSNAME.get(
 mitogen.core.Latch.poller_class = PREFERRED_POLLER
 
 
-class TtyLogStream(mitogen.core.BasicStream):
+class DiagLogStream(mitogen.core.BasicStream):
     """
     For "hybrid TTY/socketpair" mode, after a connection has been setup, a
     spare TTY file descriptor will exist that cannot be closed, and to which
@@ -802,18 +813,21 @@ class TtyLogStream(mitogen.core.BasicStream):
     termination signal to any processes whose controlling TTY is the TTY that
     has been closed.
 
-    TtyLogStream takes over this descriptor and creates corresponding log
+    DiagLogStream takes over this descriptor and creates corresponding log
     messages for anything written to it.
     """
 
-    def __init__(self, tty_fd, stream):
-        self.receive_side = mitogen.core.Side(self, tty_fd)
+    def __init__(self, fd, stream):
+        self.receive_side = mitogen.core.Side(self, fd)
         self.transmit_side = self.receive_side
         self.stream = stream
         self.buf = ''
 
     def __repr__(self):
-        return 'mitogen.parent.TtyLogStream(%r)' % (self.stream.name,)
+        return 'mitogen.parent.DiagLogStream(fd=%r, %r)' % (
+            self.receive_side.fd,
+            self.stream.name,
+        )
 
     def on_receive(self, broker):
         """

mitogen/ssh.py

@@ -120,7 +120,7 @@ class Stream(mitogen.parent.Stream):
     #: Number of -v invocations to pass on command line.
     ssh_debug_level = 0
 
-    #: If batch_mode=False, points to the corresponding TtyLogStream, allowing
+    #: If batch_mode=False, points to the corresponding DiagLogStream, allowing
     #: it to be disconnected at the same time this stream is being torn down.
     tty_stream = None
 
@@ -136,27 +136,15 @@ class Stream(mitogen.parent.Stream):
     ssh_args = None
 
     check_host_keys_msg = 'check_host_keys= must be set to accept, enforce or ignore'
-    batch_mode_check_host_keys_msg = (
-        'check_host_keys cannot be set to "accept" when batch mode is '
-        'enabled, since batch mode disables PTY allocation.'
-    )
-    batch_mode_password_msg = (
-        'A password cannot be set when batch mode is enabled, '
-        'since batch mode disables PTY allocation.'
-    )
 
     def construct(self, hostname, username=None, ssh_path=None, port=None,
                   check_host_keys='enforce', password=None, identity_file=None,
                   compression=True, ssh_args=None, keepalive_enabled=True,
-                  keepalive_count=3, keepalive_interval=15, batch_mode=False,
+                  keepalive_count=3, keepalive_interval=15,
                   identities_only=True, ssh_debug_level=None, **kwargs):
         super(Stream, self).construct(**kwargs)
         if check_host_keys not in ('accept', 'enforce', 'ignore'):
             raise ValueError(self.check_host_keys_msg)
-        if check_host_keys == 'accept' and batch_mode:
-            raise ValueError(self.batch_mode_check_host_keys_msg)
-        if password is not None and batch_mode:
-            raise ValueError(self.batch_mode_password_msg)
 
         self.hostname = hostname
         self.username = username
@@ -169,14 +157,6 @@ class Stream(mitogen.parent.Stream):
         self.keepalive_enabled = keepalive_enabled
         self.keepalive_count = keepalive_count
         self.keepalive_interval = keepalive_interval
-        self.batch_mode = batch_mode
-        if self.batch_mode:
-            self.create_child = mitogen.parent.create_child
-            self.create_child_args = {
-                'merge_stdio': True,
-            }
-        else:
-            self.create_child = mitogen.parent.hybrid_tty_create_child
         if ssh_path:
             self.ssh_path = ssh_path
         if ssh_args:
@@ -184,6 +164,30 @@ class Stream(mitogen.parent.Stream):
         if ssh_debug_level:
             self.ssh_debug_level = ssh_debug_level
 
+        self._init_create_child()
+
+    def _requires_pty(self):
+        """
+        Return :data:`True` if the configuration requires a PTY to be
+        allocated. This is only true if we must interactively accept host keys,
+        or type a password.
+        """
+        return (self.check_host_keys == 'accept' or
+                self.password is not None)
+
+    def _init_create_child(self):
+        """
+        Initialize the base class :attr:`create_child` and
+        :attr:`create_child_args` according to whether we need a PTY or not.
+        """
+        if self._requires_pty():
+            self.create_child = mitogen.parent.hybrid_tty_create_child
+        else:
+            self.create_child = mitogen.parent.create_child
+            self.create_child_args = {
+                'stderr_pipe': True,
+            }
+
     def on_disconnect(self, broker):
         if self.tty_stream is not None:
             self.tty_stream.on_disconnect(broker)
@@ -213,15 +217,12 @@ class Stream(mitogen.parent.Stream):
                 '-o', 'ServerAliveInterval %s' % (self.keepalive_interval,),
                 '-o', 'ServerAliveCountMax %s' % (self.keepalive_count,),
             ]
-        if self.batch_mode:
+        if not self._requires_pty():
             bits += ['-o', 'BatchMode yes']
         if self.check_host_keys == 'enforce':
             bits += ['-o', 'StrictHostKeyChecking yes']
         if self.check_host_keys == 'accept':
-            if self.batch_mode:
-                bits += ['-o', 'StrictHostKeyChecking no']
-            else:
-                bits += ['-o', 'StrictHostKeyChecking ask']
+            bits += ['-o', 'StrictHostKeyChecking ask']
         elif self.check_host_keys == 'ignore':
             bits += [
                 '-o', 'StrictHostKeyChecking no',
@@ -273,7 +274,7 @@ class Stream(mitogen.parent.Stream):
     def _connect_bootstrap(self, extra_fd):
         fds = [self.receive_side.fd]
         if extra_fd is not None:
-            self.tty_stream = mitogen.parent.TtyLogStream(extra_fd, self)
+            self.tty_stream = mitogen.parent.DiagLogStream(extra_fd, self)
             fds.append(extra_fd)
 
         it = mitogen.parent.iter_read(fds=fds, deadline=self.connect_deadline)
@@ -294,6 +295,9 @@ class Stream(mitogen.parent.Stream):
                 # it at the start of the line.
                 if self.password is not None and password_sent:
                     raise PasswordError(self.password_incorrect_msg)
+                elif 'password' in buf and self.password is None:
+                    # Permission denied (password,pubkey)
+                    raise PasswordError(self.password_required_msg)
                 else:
                     raise PasswordError(self.auth_incorrect_msg)
             elif partial and PASSWORD_PROMPT in buf.lower():

mitogen/su.py

@@ -49,7 +49,7 @@ class Stream(mitogen.parent.Stream):
     create_child = staticmethod(mitogen.parent.tty_create_child)
     child_is_immediate_subprocess = False
 
-    #: Once connected, points to the corresponding TtyLogStream, allowing it to
+    #: Once connected, points to the corresponding DiagLogStream, allowing it to
     #: be disconnected at the same time this stream is being torn down.
 
     username = 'root'

mitogen/sudo.py

@@ -107,7 +107,7 @@ class Stream(mitogen.parent.Stream):
     create_child = staticmethod(mitogen.parent.hybrid_tty_create_child)
     child_is_immediate_subprocess = False
 
-    #: Once connected, points to the corresponding TtyLogStream, allowing it to
+    #: Once connected, points to the corresponding DiagLogStream, allowing it to
     #: be disconnected at the same time this stream is being torn down.
     tty_stream = None
 
@@ -165,7 +165,7 @@ class Stream(mitogen.parent.Stream):
     password_required_msg = 'sudo password is required'
 
     def _connect_bootstrap(self, extra_fd):
-        self.tty_stream = mitogen.parent.TtyLogStream(extra_fd, self)
+        self.tty_stream = mitogen.parent.DiagLogStream(extra_fd, self)
 
         password_sent = False
         it = mitogen.parent.iter_read(

tests/data/fakessh.py

@@ -45,6 +45,13 @@ if os.getenv('FAKESSH_MODE') == 'strict':
     sys.exit(255)
 
 
+#
+# Set an env var if stderr was a TTY to make ssh_test tests easier to write.
+#
+if os.isatty(2):
+    os.environ['STDERR_WAS_TTY'] = '1'
+
+
 parser = optparse.OptionParser()
 parser.add_option('--user', '-l', action='store')
 parser.add_option('-o', dest='options', action='append')

tests/ssh_test.py

@@ -124,20 +124,9 @@ class BannerTest(testlib.DockerMixin, unittest2.TestCase):
         self.assertEquals(name, context.name)
 
 
-class BatchModeTest(testlib.DockerMixin, testlib.TestCase):
+class RequirePtyTest(testlib.DockerMixin, testlib.TestCase):
     stream_class = mitogen.ssh.Stream
-    #
-    # Test that:
-    # 
-    # - batch_mode=false, host_key_checking=accept
-    # - batch_mode=false, host_key_checking=enforce
-    # - batch_mode=false, host_key_checking=ignore
-    #
-    # - batch_mode=true, host_key_checking=accept
-    # - batch_mode=true, host_key_checking=enforce
-    # - batch_mode=true, host_key_checking=ignore
-    # - batch_mode=true, password is not None
-    # 
+
     def fake_ssh(self, FAKESSH_MODE=None, **kwargs):
         os.environ['FAKESSH_MODE'] = str(FAKESSH_MODE)
         try:
@@ -150,59 +139,25 @@ class BatchModeTest(testlib.DockerMixin, testlib.TestCase):
         finally:
             del os.environ['FAKESSH_MODE']
 
-    def test_false_accept(self):
-        # Should succeed.
-        self.fake_ssh(FAKESSH_MODE='ask', check_host_keys='accept')
-
-    def test_false_enforce(self):
-        # Should succeed.
-        self.fake_ssh(check_host_keys='enforce')
-
-    def test_false_ignore(self):
-        # Should succeed.
-        self.fake_ssh(check_host_keys='ignore')
-
-    def test_false_password(self):
-        # Should succeed.
-        self.docker_ssh(username='mitogen__has_sudo_nopw',
-                        password='has_sudo_nopw_password')
-
-    def test_true_accept(self):
-        e = self.assertRaises(ValueError,
-            lambda: self.fake_ssh(check_host_keys='accept', batch_mode=True)
-        )
-        self.assertEquals(e.args[0],
-            self.stream_class.batch_mode_check_host_keys_msg)
-
-    def test_true_enforce(self):
-        e = self.assertRaises(mitogen.ssh.HostKeyError,
-            lambda: self.docker_ssh(
-                batch_mode=True,
-                check_host_keys='enforce',
-                ssh_args=['-o', 'UserKnownHostsFile /dev/null'],
-            )
-        )
-        self.assertEquals(e.args[0], self.stream_class.hostkey_failed_msg)
-
-    def test_true_ignore(self):
-        e = self.assertRaises(mitogen.ssh.HostKeyError,
-            lambda: self.fake_ssh(
-                FAKESSH_MODE='strict',
-                batch_mode=True,
-                check_host_keys='ignore',
-            )
-        )
-        self.assertEquals(e.args[0], self.stream_class.hostkey_failed_msg)
-
-    def test_true_password(self):
-        e = self.assertRaises(ValueError,
-            lambda: self.fake_ssh(
-                password='nope',
-                batch_mode=True,
-            )
-        )
-        self.assertEquals(e.args[0], self.stream_class.batch_mode_password_msg)
-
+    def test_check_host_keys_accept(self):
+        # required=true, host_key_checking=accept
+        context = self.fake_ssh(FAKESSH_MODE='ask', check_host_keys='accept')
+        self.assertEquals('1', context.call(os.getenv, 'STDERR_WAS_TTY'))
+
+    def test_check_host_keys_enforce(self):
+        # required=false, host_key_checking=enforce
+        context = self.fake_ssh(check_host_keys='enforce')
+        self.assertEquals(None, context.call(os.getenv, 'STDERR_WAS_TTY'))
+
+    def test_check_host_keys_ignore(self):
+        # required=false, host_key_checking=ignore
+        context = self.fake_ssh(check_host_keys='ignore')
+        self.assertEquals(None, context.call(os.getenv, 'STDERR_WAS_TTY'))
+
+    def test_password_present(self):
+        # required=true, password is not None
+        context = self.fake_ssh(check_host_keys='ignore', password='willick')
+        self.assertEquals('1', context.call(os.getenv, 'STDERR_WAS_TTY'))
 
 
 if __name__ == '__main__':