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 @@ def _receive_one(self, broker):
         )
 
         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 @@ def get_boot_command(self):
     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

@@ -206,7 +206,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.
 
@@ -217,8 +217,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
@@ -229,8 +234,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,
@@ -240,14 +249,16 @@ 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())
     parentfp.close()
 
     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():
@@ -794,7 +805,7 @@ def poll(self, timeout=None):
 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
@@ -804,18 +815,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,21 +157,37 @@ def construct(self, hostname, username=None, ssh_path=None, port=None,
         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:
             self.ssh_args = ssh_args
         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 @@ def get_boot_command(self):
                 '-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 @@ def _ec0_received(self):
     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 @@ def _connect_bootstrap(self, extra_fd):
                 # 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 @@ def get_boot_command(self):
     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 @@ def confirm(msg):
     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')