From a08f953bd6c1a3d19e125c7958243b64a0136823 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 13 Sep 2021 17:17:20 +0100 Subject: [PATCH] sshproxy: share the caller's LogPolicy. Now new_connection() takes an optional LogPolicy * argument, and passes it on to the SshProxy setup. This means that SshProxy's implementation of the LogPolicy trait can answer queries like askappend() and logging_error() by passing them on to the same LogPolicy used by the main backend. Not all callers of new_connection have a LogPolicy, so we still have to fall back to the previous conservative default behaviour if SshProxy doesn't have a LogPolicy it can ask. The main backend implementations didn't _quite_ have access to a LogPolicy already, but they do have a LogContext, which has a LogPolicy vtable pointer inside it; so I've added a query function log_get_policy() which allows them to extract that pointer to pass to new_connection. This is the first step of fixing the non-interactivity limitations of SshProxy. But it's also the easiest step: the next ones will be more involved. --- logging.c | 5 +++ network.h | 20 ++++++++--- noproxy.c | 2 +- nosshproxy.c | 3 +- otherbackends/raw.c | 3 +- otherbackends/rlogin.c | 3 +- otherbackends/supdup.c | 3 +- otherbackends/telnet.c | 3 +- proxy.c | 4 +-- putty.h | 1 + ssh/portfwd.c | 3 +- ssh/ssh.c | 3 +- ssh/x11fwd.c | 2 +- sshproxy.c | 81 +++++++++++++++++++++++++----------------- unix/pageant.c | 3 +- unix/sharing.c | 2 +- 16 files changed, 90 insertions(+), 51 deletions(-) diff --git a/logging.c b/logging.c index 31cbccfb..e065f1a4 100644 --- a/logging.c +++ b/logging.c @@ -81,6 +81,11 @@ void logflush(LogContext *ctx) fflush(ctx->lgfp); } +LogPolicy *log_get_policy(LogContext *ctx) +{ + return ctx->lp; +} + static void logfopen_callback(void *vctx, int mode) { LogContext *ctx = (LogContext *)vctx; diff --git a/network.h b/network.h index 05b71279..067775e4 100644 --- a/network.h +++ b/network.h @@ -110,13 +110,22 @@ struct PlugVtable { */ }; -/* proxy indirection layer */ -/* NB, control of 'addr' is passed via new_connection, which takes - * responsibility for freeing it */ +/* Proxy indirection layer. + * + * Calling new_connection transfers ownership of 'addr': the proxy + * layer is now responsible for freeing it, and the caller shouldn't + * assume it exists any more. + * + * You can optionally pass a LogPolicy to this function, which will be + * passed on in turn to proxy types that can use one (e.g. SSH jump + * host proxy). If you don't have one, all proxy types are required to + * be able to manage without (and will just degrade their logging + * control). + */ Socket *new_connection(SockAddr *addr, const char *hostname, int port, bool privport, bool oobinline, bool nodelay, bool keepalive, - Plug *plug, Conf *conf); + Plug *plug, Conf *conf, LogPolicy *lp); Socket *new_listener(const char *srcaddr, int port, Plug *plug, bool local_host_only, Conf *conf, int addressfamily); SockAddr *name_lookup(const char *host, int port, char **canonicalname, @@ -134,7 +143,8 @@ Socket *platform_new_connection(SockAddr *addr, const char *hostname, Socket *sshproxy_new_connection(SockAddr *addr, const char *hostname, int port, bool privport, bool oobinline, bool nodelay, bool keepalive, - Plug *plug, Conf *conf); + Plug *plug, Conf *conf, + LogPolicy *clientlp); /* socket functions */ diff --git a/noproxy.c b/noproxy.c index 1d372932..6b5832ab 100644 --- a/noproxy.c +++ b/noproxy.c @@ -20,7 +20,7 @@ SockAddr *name_lookup(const char *host, int port, char **canonicalname, Socket *new_connection(SockAddr *addr, const char *hostname, int port, bool privport, bool oobinline, bool nodelay, bool keepalive, - Plug *plug, Conf *conf) + Plug *plug, Conf *conf, LogPolicy *lp) { return sk_new(addr, port, privport, oobinline, nodelay, keepalive, plug); } diff --git a/nosshproxy.c b/nosshproxy.c index e47a860c..cce33231 100644 --- a/nosshproxy.c +++ b/nosshproxy.c @@ -10,7 +10,8 @@ const bool ssh_proxy_supported = false; Socket *sshproxy_new_connection(SockAddr *addr, const char *hostname, int port, bool privport, bool oobinline, bool nodelay, bool keepalive, - Plug *plug, Conf *conf) + Plug *plug, Conf *conf, + LogPolicy *clientlp) { return NULL; } diff --git a/otherbackends/raw.c b/otherbackends/raw.c index 9f1655d8..638e2a50 100644 --- a/otherbackends/raw.c +++ b/otherbackends/raw.c @@ -167,7 +167,8 @@ static char *raw_init(const BackendVtable *vt, Seat *seat, * Open socket. */ raw->s = new_connection(addr, *realhost, port, false, true, nodelay, - keepalive, &raw->plug, conf); + keepalive, &raw->plug, conf, + log_get_policy(logctx)); if ((err = sk_socket_error(raw->s)) != NULL) return dupstr(err); diff --git a/otherbackends/rlogin.c b/otherbackends/rlogin.c index 555d91fb..3748e536 100644 --- a/otherbackends/rlogin.c +++ b/otherbackends/rlogin.c @@ -204,7 +204,8 @@ static char *rlogin_init(const BackendVtable *vt, Seat *seat, * Open socket. */ rlogin->s = new_connection(addr, *realhost, port, true, false, - nodelay, keepalive, &rlogin->plug, conf); + nodelay, keepalive, &rlogin->plug, conf, + log_get_policy(logctx)); if ((err = sk_socket_error(rlogin->s)) != NULL) return dupstr(err); diff --git a/otherbackends/supdup.c b/otherbackends/supdup.c index 69e931c5..cfa1b2ad 100644 --- a/otherbackends/supdup.c +++ b/otherbackends/supdup.c @@ -712,7 +712,8 @@ static char *supdup_init(const BackendVtable *x, Seat *seat, * Open socket. */ supdup->s = new_connection(addr, *realhost, port, false, true, - nodelay, keepalive, &supdup->plug, supdup->conf); + nodelay, keepalive, &supdup->plug, supdup->conf, + log_get_policy(logctx)); if ((err = sk_socket_error(supdup->s)) != NULL) return dupstr(err); diff --git a/otherbackends/telnet.c b/otherbackends/telnet.c index ec65d400..b006305d 100644 --- a/otherbackends/telnet.c +++ b/otherbackends/telnet.c @@ -739,7 +739,8 @@ static char *telnet_init(const BackendVtable *vt, Seat *seat, * Open socket. */ telnet->s = new_connection(addr, *realhost, port, false, true, nodelay, - keepalive, &telnet->plug, telnet->conf); + keepalive, &telnet->plug, telnet->conf, + log_get_policy(logctx)); if ((err = sk_socket_error(telnet->s)) != NULL) return dupstr(err); diff --git a/proxy.c b/proxy.c index 4fa8f16f..770f3d02 100644 --- a/proxy.c +++ b/proxy.c @@ -395,7 +395,7 @@ static const PlugVtable ProxySocket_plugvt = { Socket *new_connection(SockAddr *addr, const char *hostname, int port, bool privport, bool oobinline, bool nodelay, bool keepalive, - Plug *plug, Conf *conf) + Plug *plug, Conf *conf, LogPolicy *lp) { int type = conf_get_int(conf, CONF_proxy_type); @@ -411,7 +411,7 @@ Socket *new_connection(SockAddr *addr, const char *hostname, if (type == PROXY_SSH && (sret = sshproxy_new_connection(addr, hostname, port, privport, oobinline, nodelay, keepalive, - plug, conf)) != NULL) + plug, conf, lp)) != NULL) return sret; if ((sret = platform_new_connection(addr, hostname, port, privport, diff --git a/putty.h b/putty.h index 2042d43a..8538bd52 100644 --- a/putty.h +++ b/putty.h @@ -1962,6 +1962,7 @@ void logfopen(LogContext *logctx); void logfclose(LogContext *logctx); void logtraffic(LogContext *logctx, unsigned char c, int logmode); void logflush(LogContext *logctx); +LogPolicy *log_get_policy(LogContext *logctx); void logevent(LogContext *logctx, const char *event); void logeventf(LogContext *logctx, const char *fmt, ...) PRINTF_LIKE(2, 3); void logeventvf(LogContext *logctx, const char *fmt, va_list ap); diff --git a/ssh/portfwd.c b/ssh/portfwd.c index f96fe31a..a67268a4 100644 --- a/ssh/portfwd.c +++ b/ssh/portfwd.c @@ -1160,7 +1160,8 @@ char *portfwdmgr_connect(PortFwdManager *mgr, Channel **chan_ret, pf->socks_state = SOCKS_NONE; pf->s = new_connection(addr, dummy_realhost, port, - false, true, false, false, &pf->plug, mgr->conf); + false, true, false, false, &pf->plug, mgr->conf, + NULL); sfree(dummy_realhost); if ((err = sk_socket_error(pf->s)) != NULL) { char *err_ret = dupstr(err); diff --git a/ssh/ssh.c b/ssh/ssh.c index aee04db9..8b7dd92c 100644 --- a/ssh/ssh.c +++ b/ssh/ssh.c @@ -789,7 +789,8 @@ static char *connect_to_host( ssh->s = new_connection(addr, *realhost, port, false, true, nodelay, keepalive, - &ssh->plug, ssh->conf); + &ssh->plug, ssh->conf, + log_get_policy(ssh->logctx)); if ((err = sk_socket_error(ssh->s)) != NULL) { ssh->s = NULL; seat_notify_remote_exit(ssh->seat); diff --git a/ssh/x11fwd.c b/ssh/x11fwd.c index 49dca95a..eb4d4584 100644 --- a/ssh/x11fwd.c +++ b/ssh/x11fwd.c @@ -564,7 +564,7 @@ static size_t x11_send( xconn->s = new_connection(sk_addr_dup(xconn->disp->addr), xconn->disp->realhost, xconn->disp->port, false, true, false, false, &xconn->plug, - sshfwd_get_conf(xconn->c)); + sshfwd_get_conf(xconn->c), NULL); if ((err = sk_socket_error(xconn->s)) != NULL) { char *err_message = dupprintf("unable to connect to" " forwarded X server: %s", err); diff --git a/sshproxy.c b/sshproxy.c index feb9b077..46ef9d6c 100644 --- a/sshproxy.c +++ b/sshproxy.c @@ -16,15 +16,15 @@ const bool ssh_proxy_supported = true; /* * TODO for future work: * - * At present, this use of SSH as a proxy is 100% noninteractive. In - * our implementations of the Seat and LogPolicy traits, every method - * that involves interactively prompting the user is implemented by - * pretending the user gave a safe default answer. So the effect is - * very much as if you'd used 'plink -batch' as a proxy subprocess - - * password prompts are cancelled and any dubious host key or crypto - * primitive is unconditionally rejected - except that it all happens - * in-process, making it mildly more convenient to set up, perhaps a - * hair faster, and you get all the Event Log data in one place. + * At present, this use of SSH as a proxy is mostly noninteractive. In + * our implementations of the Seat trait, every method that involves + * interactively prompting the user is implemented by pretending the + * user gave a safe default answer. So the effect is very much as if + * you'd used 'plink -batch' as a proxy subprocess - password prompts + * are cancelled and any dubious host key or crypto primitive is + * unconditionally rejected - except that it all happens in-process, + * making it mildly more convenient to set up, perhaps a hair faster, + * and you get all the Event Log data in one place. * * But the biggest benefit of in-process SSH proxying would be that * the interactive prompts from the sub-SSH can be passed through to @@ -32,24 +32,21 @@ const bool ssh_proxy_supported = true; * both require password authentication, you should be able to type * both password in sequence into the PuTTY terminal window; if you're * running a session of this kind for the first time, you should be - * able to confirm both host keys one after another; if you need to - * store SSH packet logs from both SSH connections, you should be able - * to respond in turn to two askappend() prompts if necessary. And in - * the current state of the code, none of that is yet implemented. + * able to confirm both host keys one after another. In the current + * state of the code, none of that is yet implemented: we're borrowing + * the client LogPolicy for things like askappend(), but not the Seat, + * which is where all the really important stuff lives. * * To fix that, we'd have to start by arranging for this proxy - * implementation to get hold of the 'real' (outer) Seat and LogPolicy - * objects, which probably means that they'd have to be passed to - * new_connection. Then, each method in this file that receives an + * implementation to get hold of the 'real' (outer) Seat object, which + * means passing it to new_connection as we're already doing with + * LogPolicy. Then, each method in this file that receives an * interactive prompt request would handle it by passing it on to the - * outer Seat or LogPolicy, with some kind of tweak that would allow + * client Seat if present, with some kind of tweak that would allow * the end user to see clearly that the prompt had come from the proxy - * SSH connection rather than the primary one. - * - * One problem here is that not all uses of new_connection _have_ a - * Seat or a LogPolicy available. So we'd also have to check if those - * pointers are NULL, and if so, fall back to the existing behaviour - * of behaving as if in batch mode. + * SSH connection rather than the primary one. If no client Seat was + * present, we'd have no choice but to fall back to the existing + * behaviour of behaving as if in batch mode. */ typedef struct SshProxy { @@ -57,6 +54,7 @@ typedef struct SshProxy { Conf *conf; LogContext *logctx; Backend *backend; + LogPolicy *clientlp; ProxyStderrBuf psb; Plug *plug; @@ -171,10 +169,17 @@ static int sshproxy_askappend(LogPolicy *lp, Filename *filename, void (*callback)(void *ctx, int result), void *ctx) { + SshProxy *sp = container_of(lp, SshProxy, logpolicy); + /* - * TODO: if we had access to the outer LogPolicy, we could pass on - * this request to the end user. (But we'd still have to have this - * code as a fallback in case there isn't a LogPolicy available.) + * If we have access to the outer LogPolicy, pass on this request + * to the end user. + */ + if (sp->clientlp) + return lp_askappend(sp->clientlp, filename, callback, ctx); + + /* + * Otherwise, fall back to the safe noninteractive assumption. */ char *msg = dupprintf("Log file \"%s\" already exists; logging cancelled", filename_to_str(filename)); @@ -185,12 +190,20 @@ static int sshproxy_askappend(LogPolicy *lp, Filename *filename, static void sshproxy_logging_error(LogPolicy *lp, const char *event) { + SshProxy *sp = container_of(lp, SshProxy, logpolicy); + /* - * TODO: if we had access to the outer LogPolicy, we could pass on - * this request to _its_ logging_error method, where it would be - * more prominent than just dumping it in the outer SSH - * connection's Event Log. (But we'd still have to have this code - * as a fallback in case there isn't a LogPolicy available.) + * If we have access to the outer LogPolicy, pass on this request + * to it. + */ + if (sp->clientlp) { + lp_logging_error(sp->clientlp, event); + return; + } + + /* + * Otherwise, the best we can do is to put it in the outer SSH + * connection's Event Log. */ char *msg = dupprintf("Logging error: %s", event); sshproxy_eventlog(lp, msg); @@ -459,13 +472,13 @@ static const SeatVtable SshProxy_seat_vt = { .verbose = nullseat_verbose_no, .interactive = nullseat_interactive_no, .get_cursor_position = nullseat_get_cursor_position, - }; Socket *sshproxy_new_connection(SockAddr *addr, const char *hostname, int port, bool privport, bool oobinline, bool nodelay, bool keepalive, - Plug *plug, Conf *clientconf) + Plug *plug, Conf *clientconf, + LogPolicy *clientlp) { SshProxy *sp = snew(SshProxy); memset(sp, 0, sizeof(*sp)); @@ -575,5 +588,7 @@ Socket *sshproxy_new_connection(SockAddr *addr, const char *hostname, sfree(realhost); + sp->clientlp = clientlp; + return &sp->sock; } diff --git a/unix/pageant.c b/unix/pageant.c index f204ea9d..b68b829d 100644 --- a/unix/pageant.c +++ b/unix/pageant.c @@ -1189,7 +1189,8 @@ void run_agent(FILE *logfp, const char *symlink_path) conn->plug.vt = &X11Connection_plugvt; s = new_connection(sk_addr_dup(disp->addr), disp->realhost, disp->port, - false, true, false, false, &conn->plug, conf); + false, true, false, false, &conn->plug, conf, + NULL); if ((err = sk_socket_error(s)) != NULL) { fprintf(stderr, "pageant: unable to connect to X server: %s", err); exit(1); diff --git a/unix/sharing.c b/unix/sharing.c index f1ef2019..268b8df1 100644 --- a/unix/sharing.c +++ b/unix/sharing.c @@ -297,7 +297,7 @@ int platform_ssh_share(const char *pi_name, Conf *conf, if (can_downstream) { retsock = new_connection(unix_sock_addr(sockname), "", 0, false, true, false, false, - downplug, conf); + downplug, conf, NULL); if (sk_socket_error(retsock) == NULL) { sfree(*logtext); *logtext = sockname;