[PATCH] close stderr of persistent proxy command if not in debug mode

[Steffen Prohaska]

Hello,

This is my first patch to OpenSSH. Apologies if the format is not as expected. Let me know if I should submit in a different format.

---
From 48393827a9d335a77c6c9bc96d33cc7aa234bbd3 Mon Sep 17 00:00:00 2001
From: Steffen Prohaska <***@zib.de>
Date: Sat, 27 Oct 2018 16:52:57 +0200
Subject: [PATCH] close stderr of persistent proxy command if not in debug mode

The patch should likely be first applied to upstream.

If the parent becomes a new persistent connection master daemon, stderr
of the proxy command should be detached, too, similar to the master
daemon's stderr, as changed in
openssh-***@d2d6bf864e52af8491a60dd507f85b74361f5da3,
***@4fb726f0fdcb155ad419913cea10dc4afd409d24 and discussed in
bz#1988.

Signed-off-by: Steffen Prohaska <***@zib.de>
---

You can view a signed commit at GitHub:

https://github.com/sprohaska/openssh-portable/tree/pr/proxy-detach-stderr
https://github.com/sprohaska/openssh-portable/commit/48393827a9d335a77c6c9bc96d33cc7aa234bbd3

or pull with:

git pull https://github.com/sprohaska/openssh-portable.git pr/proxy-detach-stderr


sshconnect.c | 38 ++++++++++++++++++++++++++++++++------
1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/sshconnect.c b/sshconnect.c
index 52c32811..5266b2af 100644
--- a/sshconnect.c
+++ b/sshconnect.c
@@ -78,6 +78,7 @@ static int matching_host_key_dns = 0;
static pid_t proxy_command_pid = 0;

/* import */
+extern int debug_flag;
extern Options options;
extern char *__progname;

@@ -99,6 +100,33 @@ expand_proxy_command(const char *proxy_command, const char *user,
return ret;
}

+/*
+ * If the parent may become a new master daemon in `control_persist_detach()`,
+ * keep stderr of the proxy command in debug mode, so that error messages get
+ * printed on the user's terminal. But detach stderr in non-debug mode,
+ * because the proxy command will run as a daemon.
+ */
+static void
+prepare_proxy_stderr()
+{
+ int devnull;
+
+ if (!options.control_persist || debug_flag) {
+ return;
+ }
+
+ if ((devnull = open(_PATH_DEVNULL, O_RDWR)) == -1) {
+ error("%s: open(\"/dev/null\"): %s", __func__,
+ strerror(errno));
+ return;
+ }
+
+ if (dup2(devnull, STDERR_FILENO) == -1)
+ error("%s: dup2: %s", __func__, strerror(errno));
+ if (devnull > STDERR_FILENO)
+ close(devnull);
+}
+
/*
* Connect to the given ssh server using a proxy command that passes a
* a connected fd back to us.
@@ -140,10 +168,8 @@ ssh_proxy_fdpass_connect(struct ssh *ssh, const char *host, u_short port,
if (sp[0] >= 2)
close(sp[0]);

- /*
- * Stderr is left as it is so that error messages get
- * printed on the user's terminal.
- */
+ prepare_proxy_stderr();
+
argv[0] = shell;
argv[1] = "-c";
argv[2] = command_string;
@@ -219,8 +245,8 @@ ssh_proxy_connect(struct ssh *ssh, const char *host, u_short port,
/* Cannot be 1 because pin allocated two descriptors. */
close(pout[1]);

- /* Stderr is left as it is so that error messages get
- printed on the user's terminal. */
+ prepare_proxy_stderr();
+
argv[0] = shell;
argv[1] = "-c";
argv[2] = command_string;
--
2.19.0.212.gdb50a52598

[Steffen Prohaska]

Post by Steffen Prohaska
Date: Sat, 27 Oct 2018 16:52:57 +0200
Subject: [PATCH] close stderr of persistent proxy command if not in debug mode
If the parent becomes a new persistent connection master daemon, stderr
of the proxy command should be detached, too, similar to the master
daemon's stderr, as changed in
bz#1988.

Since I haven't received any comments, I'd like to explain in more detail why I think that the patch is relevant.

The problem that the patch solves is a variant of the problem discussed in bz#1988 <https://bugzilla.mindrot.org/show_bug.cgi?id=1988>, i.e. scripts that use stderr unexpectedly hang. A specific example with Python:

subprocess.run(['ssh', ...'], capture_output=True, ...)

will hang if ssh starts a new master connection that uses as proxy command. Python waits for EOF on the stderr child pipe. But it does not receive EOF if the proxy command keeps stderr open. With the proposed patch, it works as expected.

The current behavior is confusing and difficult to debug. The direct child process terminates successfully, but sometimes `run()` does not return. It returns if the master connection has already been active, but it hangs if ssh opens a new master connection. One must understand that ssh spawns additional daemon processes, and one of those keeps the stderr pipe open, which is the reason why `run()` does not see EOF and thus does not return.

One possible workaround until the patch has been applied is to not use ProxyJump and use ProxyCommand as follows:

Host internal.example.org
ProxyCommand 2>/dev/null ssh -W %h:%p login.example.org

This works because ProxyCommand is executed via a shell `exec ...`, so that `2>/dev/null` closes stderr in the same way as the proposed patch.

Steffen