diff --git a/docs/api.rst b/docs/api.rst index 008ae247..e823b877 100644 --- a/docs/api.rst +++ b/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: diff --git a/mitogen/core.py b/mitogen/core.py index 12983071..9df0fa1d 100644 --- a/mitogen/core.py +++ b/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 diff --git a/mitogen/doas.py b/mitogen/doas.py index 1d9d04eb..cdcee0b0 100644 --- a/mitogen/doas.py +++ b/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( diff --git a/mitogen/parent.py b/mitogen/parent.py index 67b7bf1d..d7177dde 100644 --- a/mitogen/parent.py +++ b/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): """ diff --git a/mitogen/ssh.py b/mitogen/ssh.py index 2650d32f..670294f1 100644 --- a/mitogen/ssh.py +++ b/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(): diff --git a/mitogen/su.py b/mitogen/su.py index 45229d6d..7e2e5f08 100644 --- a/mitogen/su.py +++ b/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' diff --git a/mitogen/sudo.py b/mitogen/sudo.py index 402d8549..c410dac9 100644 --- a/mitogen/sudo.py +++ b/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( diff --git a/tests/data/fakessh.py b/tests/data/fakessh.py index 415425af..69d47339 100755 --- a/tests/data/fakessh.py +++ b/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') diff --git a/tests/ssh_test.py b/tests/ssh_test.py index a305ec70..efca057d 100644 --- a/tests/ssh_test.py +++ b/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__':