I think a lot of these were inserted by a prior run through GNU indent
many years ago. I noticed in a more recent experiment that that tool
doesn't always correctly distinguish which instances of 'id * id' are
pointer variable declarations and which are multiplications, so it
spaces some of the former as if they were the latter.
My bulk indentation check also turned up a lot of cases where a run-on
function call or if statement didn't have its later lines aligned
correctly relative to the open paren.
I think this is quite easy to do by getting things out of
sync (editing the first line of the function call and forgetting to
update the rest, perhaps even because you never _saw_ the rest during
a search-replace). But a few didn't quite fit into that pattern, in
particular an outright misleading case in unix/askpass.c where the
second line of a call was aligned neatly below the _wrong_ one of the
open parens on the opening line.
Restored as many alignments as I could easily find.
In several pieces of development recently I've run across the
occasional code block in the middle of a function which suddenly
switched to 2-space indent from this code base's usual 4. I decided I
was tired of it, so I ran the whole code base through a re-indenter,
which made a huge mess, and then manually sifted out the changes that
actually made sense from that pass.
Indeed, this caught quite a few large sections with 2-space indent
level, a couple with 8, and a handful of even weirder things like 3
spaces or 12. This commit fixes them all.
The text of the host key warnings was replicated in three places: the
Windows rc file, the GTK dialog setup function, and the console.c
shared between both platforms' CLI tools. Now it lives in just one
place, namely ssh/common.c where the rest of the centralised host-key
checking is done, so it'll be easier to adjust the wording in future.
This comes with some extra automation. Paragraph wrapping is no longer
done by hand in any version of these prompts. (Previously we let GTK
do the wrapping on GTK, but on Windows the resource file contained a
bunch of pre-wrapped LTEXT lines, and console.c had pre-wrapped
terminal messages.) And the dialog heights in Windows are determined
automatically based on the amount of stuff in the window.
The main idea of all this is that it'll be easier to set up more
elaborate kinds of host key prompt that deal with certificates (if,
e.g., a server sends us a certified host key which we don't trust the
CA for). But there are side benefits of this refactoring too: each
tool now reliably inserts its own appname in the prompts, and also, on
Windows the entire prompt text is copy-pastable.
Details of implementation: there's a new type SeatDialogText which
holds a set of (type, string) pairs describing the contents of a
prompt. Type codes distinguish ordinary text paragraphs, paragraphs to
be displayed prominently (like key fingerprints), the extra-bold scary
title at the top of the 'host key changed' version of the dialog, and
the various information that lives in the subsidiary 'more info' box.
ssh/common.c constructs this, and passes it to the Seat to present the
actual prompt.
In order to deal with the different UI for answering the prompt, I've
added an extra Seat method 'prompt_descriptions' which returns some
snippets of text to interpolate into the messages. ssh/common.c calls
that while it's still constructing the text, and incorporates the
resulting snippets into the SeatDialogText.
For the moment, this refactoring only affects the host key prompts.
The warnings about outmoded crypto are still done the old-fashioned
way; they probably ought to be similarly refactored to use this new
SeatDialogText system, but it's not immediately critical for the
purpose I have right now.
When talking to a web proxy which requires a password, our HTTP proxy
code sends an initial attempt to connect without authentication,
receives the 407 status indicating that authentication was required
(and which kind), and then closes and reopens the connection (if given
"Connection: close"). Then, on the next attempt, we try again with
authentication, and expect the first thing in the input bufchain to be
the HTTP status response to the revised request.
But what happened about the error document that followed those HTTP
headers? It - or at least some of it - would already have been in the
input bufchain.
With an HTTP/1.1 proxy, we already read it and discarded it (either
via a Content-length header or chunked transfer encoding), before we
set the 'reconnect' flag. So, assuming the proxy HTTP server is
behaving sensibly, our input bufchain ought to be empty at the point
when we start the fresh connection.
But if the proxy only speaks HTTP/1.0 (which does still happen -
'tinyproxy' is a still-current example), then we didn't get a
Content-length header either, so we didn't run any of the code that
skips over the error document. (And HTTP/1.0 implicitly has
"Connection: close" semantics, which is why that doesn't matter.) As a
result, some of it would still be in the input bufchain, and never got
cleared out, and we'd try to parse _that_ as if it was the HTTP
response from the second network connection.
The simple solution is that when we close and reopen the proxy network
connection, we also clear the input bufchain, so that the fresh
connection starts from a clean slate.
prepare_session() is the function that takes a Conf in the form it was
loaded from the configuration, and normalises it into the form that
backends can make sense of easily. In particular, if CONF_host
contains something like "user@hostname", this is the place where the
"user@" is stripped off the hostname field and moved into
CONF_username where the backend will expect to find it.
Therefore, you're _always_ supposed to call prepare_session() before
launching a backend from a Conf you (potentially) got from a saved
session. But the SSH proxy code forgot to.
As a result, if you had a saved session with "user@hostname" in the
hostname field, it would work fine to launch that session directly in
PuTTY, but trying to use the same saved session as an SSH proxy would
fail mysteriously after trying to pass "user@hostname" to
getaddrinfo() and getting nothing useful back.
(On Windows, the error message is especially opaque: an invalid string
of that kind generates an error code that we weren't even tranlsating.
And even if we _do_ translate it, it wouldn't be very meaningful,
because the error in question is WSANO_RECOVERY, which FormatMessage
renders as "A non-recoverable error occurred during a database
lookup." I guess the error in question was that Windows couldn't even
figure out how to translate that string into the format of a DNS
request message!)
In the initial version of SSH proxying that only opened direct-tcpip
channels, this wasn't important. But as of commit 6f7c52dcce, we
now support invoking a command or subsystem on the proxy SSH server,
and those _can_ generate stderr data which we must now separate from
stdout.
Happily, we have a perfectly sensible thing to _do_ with it: the same
thing we'd do with stderr coming from a local proxy subprocess, to
wit, pass it to log_proxy_stderr so that it can appear in the terminal
window (if configured to) and the Event Log.
This is a simple tweak to the existing in-process SSH jump host
support, where instead of opening a direct-tcpip channel to the
destination host, we open a session channel and run a process in it to
make the connection to the destination.
So, where the existing jump host support replaced a local proxy
command along the lines of "plink %proxyhost -nc %host %port", this
one replaces "plink %proxyhost run-some-command".
Also added a corresponding option to use a subsystem to make the
connection. (Someone could configure an SSH server to support specific
subsystem names for particular destinations, or a general schema of
subsystem names that include the destination address in some standard
format.)
To avoid overflowing the already-full Proxy config panel with an extra
subtype selector, I've put these in as additional top-level proxy
types, so that instead of just PROXY_SSH we now have three
PROXY_SSH_foo.
If an SSH proxy socket is frozen for long enough, and the SSH server
continues to send, then sooner or later the proxy SSH connection will
end up having to freeze its underlying physical socket too. When the
proxy socket is later unfrozen, it needs to pass that unfreezing on in
turn.
The way this should happen is that when the SshProxy begins to clear
the backlog of data passed to it from the proxy SSH connection via
seat_stdout, it should call backend_unthrottle to inform that proxy
connection that the backlog is clearing.
But there was no backlog_unthrottle call in the whole of sshproxy.c.
Now there is.
While testing the unrelated pile of commits just past, I accidentally
started a Cygwin saved session I hadn't run in ages which used the old
Telnet-based cygtermd as a local proxy command, and found that it
presented the Cygwin prompt with a trust sigil. Oops!
It turns out that this is because interactor_return_seat does two
things that can change the real seat's trust status, and it does them
in the wrong order: it defaults the status back to trusted (as if the
seat was brand new, because that's how they start out), and it calls
tempseat_flush which may have buffered a trust-status reset while the
seat was borrowed. The former should not override the latter!
FreeProxy sends 'algorithm="MD5"' instead of 'algorithm=MD5.' I'm
actually not sure whether that's legal by RFC 7616, but it's certainly
no trouble to parse if we see it.
With all these changes, PuTTY now _can_ successfully make connections
through FreeProxy again, whether it's in Basic or Digest mode.
Another awkward thing that FreeProxy does is to slam the connection
shut after sending its 407 response, at least in Basic auth mode. (It
keeps the connection alive in digest mode, which makes sense to me,
because that's a more stateful system.)
It was surprisingly easy to make the proxy code able to tolerate this!
I've set it up so that a ProxyNegotiator can just set its 'reconnect'
flag on return from the negotiation coroutine, and the effect will be
that proxy.c makes a new connection to the same proxy server before
doing anything else. In particular, you can set that flag _and_ put
data in the output bufchain, and there's no problem - the output data
will be queued directly into the new socket.
FreeProxy sends this as a substitute for the standard 'Connection'
header (with the same contents, i.e. 'keep-alive' or 'close' depending
on whether the TCP connection is going to continue afterwards). The
Internet reckons it's not standard, but it's easy to recognise as an
ad-hoc synonym for 'Connection'.
I had a report that the Windows free-as-in-beer proxy tool 'FreeProxy'
didn't work with the new HTTP proxy code, and it turns out that the
first reason why not is that the error-document in its 407 response is
sent via chunked transfer encoding, which is to say, instead of an
up-front Content-length header, you receive a sequence of chunks each
prefixed with a hex length.
(In 0.76, before the rewritten proxy support, we never even noticed
this because we sent Basic auth details up front in our first attempt,
rather than attempting a no-auth connection first and waiting to see
what kind of auth the proxy asks us for. So we'd only ever see a 407
if the auth details were refused - and since 0.76 didn't have
interactive proxy auth prompts, there was nothing we could do at that
point but abort immediately, without attempting to parse the rest of
the 407 at all.)
Now we spot the Transfer-encoding header and successfully parse
chunked transfers. Happily, we don't need to worry about the further
transfer-encodings such as 'gzip', because we're not actually _using_
the error document - we only have to skip over it to find the end of
the HTTP response.
This still doesn't make PuTTY work with FreeProxy, because there are
further problems hiding behind that one, which I'll fix in following
commits.
If you try to use a saved session for SSH proxying which specifies a
protocol that is not SSH or bare-SSH-connection, you get a clean error
return from the proxy setup code - *provided* it's at least a protocol
known to this particular build of PuTTY. If it's one so outlandish
that backend_vt_from_proto returns NULL, there'd have been a crash.
I don't think any such protocol currently exists, but if in the next
version of PuTTY some additional protocol becomes supported, it will
trip this error in the current version.
Spotted by Coverity.
I was just writing the documentation for the new proxy type, which
caused me to realise that the thing I obviously wanted to write in the
documentation was not actually true. Let's make it true, and then I
can document it!
All the seat functions that request an interactive prompt of some kind
to the user - both the main seat_get_userpass_input and the various
confirmation dialogs for things like host keys - were using a simple
int return value, with the general semantics of 0 = "fail", 1 =
"proceed" (and in the case of seat_get_userpass_input, answers to the
prompts were provided), and -1 = "request in progress, wait for a
callback".
In this commit I change all those functions' return types to a new
struct called SeatPromptResult, whose primary field is an enum
replacing those simple integer values.
The main purpose is that the enum has not three but _four_ values: the
"fail" result has been split into 'user abort' and 'software abort'.
The distinction is that a user abort occurs as a result of an
interactive UI action, such as the user clicking 'cancel' in a dialog
box or hitting ^D or ^C at a terminal password prompt - and therefore,
there's no need to display an error message telling the user that the
interactive operation has failed, because the user already knows,
because they _did_ it. 'Software abort' is from any other cause, where
PuTTY is the first to know there was a problem, and has to tell the
user.
We already had this 'user abort' vs 'software abort' distinction in
other parts of the code - the SSH backend has separate termination
functions which protocol layers can call. But we assumed that any
failure from an interactive prompt request fell into the 'user abort'
category, which is not true. A couple of examples: if you configure a
host key fingerprint in your saved session via the SSH > Host keys
pane, and the server presents a host key that doesn't match it, then
verify_ssh_host_key would report that the user had aborted the
connection, and feel no need to tell the user what had gone wrong!
Similarly, if a password provided on the command line was not
accepted, then (after I fixed the semantics of that in the previous
commit) the same wrong handling would occur.
So now, those Seat prompt functions too can communicate whether the
user or the software originated a connection abort. And in the latter
case, we also provide an error message to present to the user. Result:
in those two example cases (and others), error messages should no
longer go missing.
Implementation note: to avoid the hassle of having the error message
in a SeatPromptResult being a dynamically allocated string (and hence,
every recipient of one must always check whether it's non-NULL and
free it on every exit path, plus being careful about copying the
struct around), I've instead arranged that the structure contains a
function pointer and a couple of parameters, so that the string form
of the message can be constructed on demand. That way, the only users
who need to free it are the ones who actually _asked_ for it in the
first place, which is a much smaller set.
(This is one of the rare occasions that I regret not having C++'s
extra features available in this code base - a unique_ptr or
shared_ptr to a string would have been just the thing here, and the
compiler would have done all the hard work for me of remembering where
to insert the frees!)
This fills in the remaining gap in the interactive prompt rework of
the proxy system in general. If you used the Telnet proxy with a
command containing %user or %pass, and hadn't filled in those
variables in the PuTTY config, then proxy/telnet.c would prompt you at
run time to enter the proxy auth details. But the local proxy command,
which uses the same format_telnet_command function, would not do that.
Now it does!
I've implemented this by moving the formatting of the proxy command
into a new module proxy/local.c, shared between both the Unix and
Windows local-proxy implementations. That module implements a
DeferredSocketOpener, which constructs the proxy command (prompting
first if necessary), and once it's constructed, hands it to a
per-platform function platform_setup_local_proxy().
So each platform-specific proxy function, instead of starting a
subprocess there and then and passing its details to make_fd_socket or
make_handle_socket, now returns a _deferred_ version of one of those
sockets, with the DeferredSocketOpener being the thing in
proxy/local.c. When that calls back to platform_setup_local_proxy(),
we actually start the subprocess and pass the resulting fds/handles to
the deferred socket to un-defer it.
A side effect of the rewrite is that when proxy commands are logged in
the Event Log, they now get the same amenities as in the Telnet proxy
type: the proxy password is sanitised out, and any difficult
characters are escaped.
This will mean that platform-specific proxy types will also be able to
set themselves up as child Interactors and prompt the user
interactively for passwords and the like.
NFC: nothing uses the new parameter yet.
Now it's done using the same code as in write_c_string_literal(), by
means of factoring the latter into a version that targets any old
BinarySink and a convenience wrapper taking a FILE *.
This is a piece I forgot in the initial implementation of HTTP Digest:
an HTTP server can send _more than one_ authentication request header
(WWW-Authenticate for normal servers, Proxy-Authenticate for proxies),
and if it does, they're supposed to be treated as alternatives to each
other, so that the client chooses one to reply to.
I suppose that technically we were 'complying' with that spec already,
in that HttpProxyNegotiator would have read each new header and
overwritten all the fields set by the previous one, so we'd always
have gone with the last header presented by the server. But that seems
inelegant: better to choose the one we actually like best.
So now we do that. All the details of an auth header are moved out of
the main HttpProxyNegotiator struct into a sub-struct we can have
multiple copies of. Each new header is parsed into a fresh struct of
that kind, and then we can compare it with the previous one and decide
which we prefer.
The preference order, naturally, is 'more secure is better': Digest
beats Basic, and between two Digest headers, SHA-256 beats MD5. (And
anything beats a header we can't make sense of at all.)
Another side effect of this change is that a 407 response which
contains _no_ Proxy-Authenticate headers will trigger an error message
saying so, instead of just going with whatever happened to be left in
the relevant variables from the previous attempt.
I was dubious about it to begin with, when I found that RFC 7616's
example seemed to be treating it as a 256-bit truncation of SHA-512,
and not the thing FIPS 180-4 section 6.7 specifies as "SHA-512/256"
(which also changes the initial hash state). Having failed to get a
clarifying response from the RFC authors, I had the idea this morning
of testing other HTTP clients to see what _they_ thought that hash
function meant, and then at least I could go with an existing
in-practice consensus.
There is no in-practice consensus. Firefox doesn't support that
algorithm at all (but they do support SHA-256); wget doesn't support
anything that RFC 7616 added to the original RFC 2617. But the prize
for weirdness goes to curl, which does accept the name "SHA-512-256"
and ... treats it as an alias for SHA-256!
So I think the situation among real clients is too confusing to even
try to work with, and I'm going to stop adding to it. PuTTY will
follow Firefox's policy: if a proxy server asks for SHA-256 digests
we'll happily provide them, but if they ask for SHA-512-256 we'll
refuse on the grounds that it's not clear enough what it means.
In http.c, this drops in reasonably neatly alongside the existing
support for Basic, now that we're waiting for an initial 407 response
from the proxy to tell us which auth mechanism it would prefer to use.
The rest of this patch is mostly contriving to add testcrypt support
for the function in cproxy.c that generates the complicated output
header to go in the HTTP request: you need about a dozen assorted
parameters, the actual response hash has two more hashes in its
preimage, and there's even an option to hash the username as well if
necessary. Much more complicated than CHAP (which is just plain
HMAC-MD5), so it needs testing!
Happily, RFC 7616 comes with some reasonably useful test cases, and
I've managed to transcribe them directly into cryptsuite.py and
demonstrate that my response-generator agrees with them.
End-to-end testing of the whole system was done against Squid 4.13
(specifically, the squid package in Debian bullseye, version 4.13-10).
Now, we always try an initial CONNECT request with no auth at all, and
wait for the proxy to reject it before sending a second try with
auth.
That way, we can wait to see what _kind_ of authentication the proxy
requests, which will enable us to support something more secure than
Basic, such as HTTP Digest.
(I mean, it would _work_ to try Basic in request #1 and then retrying
with Digest in #2 when the proxy asks for it. But if the aim of using
Digest is to avoid sending the password in cleartext, it defeats the
entire purpose to have sent it in cleartext anyway by the time you
realise the server is prepared to do something better!)
In HTTP proxying, we can (and do) send the username and password
immediately in the form of HTTP Basic, if we have them in the Conf.
But if they get rejected, or if we never sent them in the first place
and the server won't let us in without auth, then we get back an HTTP
407 response with a full set of headers and an error-document.
Assuming the HTTP connection doesn't close after that (which in
sensible HTTP/1.1 proxies it won't), this gives us the opportunity to
respond by sending a second CONNECT request, containing a fresh
username and password we just requested interactively from the user.
Probably should have done this a long time ago: when we write the
formatted command into the log file, we now base it on a version in
which CONF_proxy_password has been reset to "*password*", to avoid
writing the actual password (if any) into log files.
The Telnet proxy system is not a proper network protocol - we have no
reliable way to receive communication from the proxy telling us
whether a password is even required. However, we _do_ know (a) whether
the keywords '%user' or '%pass' appeared in the format string stored
in the Conf, and (b) whether we actually had a username or a password
to substitute into them. So that's how we know whether to ask for a
username or a password: if the format string asks for them and the
Conf doesn't provide them, we prompt for them at startup.
This involved turning TelnetProxyNegotiator into a coroutine (matching
all the other proxy types, but previously, it was the only one simple
enough not to need to be one), so that it can wait until a response
arrives to that prompt. (And also, as it turned out, so that it can
wait until setup is finished before even presenting the prompt!)
It also involves having format_telnet_command grow an extra output
parameter, in the form of 'unsigned *flags', with which it can
communicate back to the caller that a username or password was wanted
but not found. The other clients of that function (the local proxy
implementations) don't use those flags, but if necessary, they could.
This fixes the Telnet proxy, which was the only one of the proxy types
I forgot to test when I pushed the previous patch series, and
therefore, naturally, the one I left a bug in: if a ProxyNegotiator
returns both some output to be transmitted _and_ the 'done' flag, we
were forgetting to do anything with the former. So the proxy command
was being carefully constructed by TelnetProxyNegotiator, and then
promptly dropped on the floor by the owning ProxySocket.
This is the first of the ProxyNegotiator implementations to use the
new interaction system. The other two both need more work than just
inserting a prompt and using the result.
This lays all the groundwork for ProxyNegotiators to be able to issue
username and password prompts: ProxySocket now implements the
Interactor trait, it will borrow and return a Seat if one is
available, and it will present an Interactor of its own to the
ProxyNegotiator which can use it (via interactor_announce as usual) to
get a Seat to send prompts to. Also, proxy.c provides a centralised
system for making a prompts_t with an appropriate callback in it, and
dealing with the results of that callback.
No actual ProxyNegotiator implementation uses it yet, though.
Previously, the proxy negotiation functions were written as explicit
state machines, with ps->state being manually set to a sequence of
positive integer values which would be tested by if statements in the
next call to the same negotiation function.
That's not how this code base likes to do things! We have a coroutine
system to allow those state machines to be implicit rather than
explicit, so that we can use ordinary control flow statements like
while loops. Reorganised each proxy negotiation function into a
coroutine-based system like that.
While I'm at it, I've also moved each proxy negotiator out into its
own source file, to make proxy.c less overcrowded and monolithic. And
_that_ gave me the opportunity to define each negotiator as an
implementation of a trait rather than as a single function - which
means now each one can define its own local variables and have its own
cleanup function, instead of all of them having to share the variables
inside the main ProxySocket struct.
In the new coroutine system, negotiators don't have to worry about the
mechanics of actually sending data down the underlying Socket any
more. The negotiator coroutine just appends to a bufchain (via a
provided bufchain_sink), and after every call to the coroutine,
central code in proxy.c transfers the data to the Socket itself. This
avoids a lot of intermediate allocations within the negotiators, which
previously kept having to make temporary strbufs or arrays in order to
have something to point an sk_write() at; now they can just put
formatted data directly into the output bufchain via the marshal.h
interface.
In this version of the code, I've also moved most of the SOCKS5 CHAP
implementation from cproxy.c into socks5.c, so that it can sit in the
same coroutine as the rest of the proxy negotiation control flow.
That's because calling a sub-coroutine (co-subroutine?) is awkward to
set up (though it is _possible_ - we do SSH-2 kex that way), and
there's no real need to bother in this case, since the only thing that
really needs to go in cproxy.c is the actual cryptography plus a flag
to tell socks5.c whether to offer CHAP authentication in the first
place.
These were just boilerplate in all the proxy negotiation functions:
every negotiator had to contain a handler for each of these events,
and they all handled them in exactly the same way. Remove them and
centralise the handling in the shared code.
A long time ago, some of these event codes were added with purpose in
mind. PROXY_CHANGE_CLOSING was there to anticipate the possibility
that you might need to make multiple TCP connections to the proxy
server (e.g. retrying with different authentication) before
successfully getting a connection you could use to talk to the
ultimate destination. And PROXY_CHANGE_ACCEPTING was there so that we
could use the listening side of SOCKS (where you ask the proxy to open
a listening socket on your behalf). But neither of them has ever been
used, and now that the code has evolved, I think probably if we do
ever need to do either of those things then they'll want to be done
differently.
This seemed like a worthwhile cleanup to do while I was working on
this code anyway. Now all the magic numbers are defined in a header
file by macro names indicating their meaning, and used by both the
SOCKS client code in the proxy subdirectory and the SOCKS server code
in portfwd.c.
It's variously 'ps' and 'p' in functions that already receive one, and
it's 'ret' in the main function that initially constructs one. Let's
call it 'ps' consistently, so that the code idioms are the same
everywhere.
marshal.h now provides a macro put_fmt() which allows you to write
arbitrary printf-formatted data to an arbitrary BinarySink.
We already had this facility for strbufs in particular, in the form of
strbuf_catf(). That was able to take advantage of knowing the inner
structure of a strbuf to minimise memory allocation (it would snprintf
directly into the strbuf's existing buffer if possible). For a general
black-box BinarySink we can't do that, so instead we dupvprintf into a
temporary buffer.
For consistency, I've removed strbuf_catf, and converted all uses of
it into the new put_fmt - and I've also added an extra vtable method
in the BinarySink API, so that put_fmt can still use strbuf_catf's
more efficient memory management when talking to a strbuf, and fall
back to the simpler strategy when that's not available.
(TL;DR: to suppress redundant 'Press Return to begin session' prompts
in between hops of a jump-host configuration, in Plink.)
This new query method directly asks the Seat the question: is the same
stream of input used to provide responses to interactive login
prompts, and the session input provided after login concludes?
It's used to suppress the last-ditch anti-spoofing defence in Plink of
interactively asking 'Access granted. Press Return to begin session',
on the basis that any such spoofing attack works by confusing the user
about what's a legit login prompt before the session begins and what's
sent by the server after the main session begins - so if those two
things take input from different places, the user can't be confused.
This doesn't change the existing behaviour of Plink, which was already
suppressing the antispoof prompt in cases where its standard input was
redirected from something other than a terminal. But previously it was
doing it within the can_set_trust_status() seat query, and I've now
moved it out into a separate query function.
The reason why these need to be separate is for SshProxy, which needs
to give an unusual combination of answers when run inside Plink. For
can_set_trust_status(), it needs to return whatever the parent Seat
returns, so that all the login prompts for a string of proxy
connections in session will be antispoofed the same way. But you only
want that final 'Access granted' prompt to happen _once_, after all
the proxy connection setup phases are done, because up until then
you're still in the safe hands of PuTTY itself presenting an unbroken
sequence of legit login prompts (even if they come from a succession
of different servers). Hence, SshProxy unconditionally returns 'no' to
the query of whether it has a single mixed input stream, because
indeed, it never does - for purposes of session input it behaves like
an always-redirected Plink, no matter what kind of real Seat it ends
up sending its pre-session login prompts to.
I'd forgotten that the text-only branch of seat_antispoof_msg()
constructs a string from its input in the expectation that it's a
one-line message. So it was a mistake to put a \n at the start of the
string in interactor_announce() to get a blank line first.
Now interactor_announce() makes an extra call to seat_antispoof_msg to
show its blank line, and seat_antispoof_msg itself handles the
blank-line case specially.
This is generated when setup of a network connection is cancelled by
deliberate user action, namely, pressing ^C or ^D or the like at a
get_userpass_input prompt presented during proxy setup.
It's handled just like normal socket setup errors, except that it
omits the call to seat_connection_fatal, on the grounds that in this
one case of connection-setup failure, the user doesn't need to be
_informed_ that the connection failed - they already know, because
they failed it themself on purpose.
Passing an operating-system-specific error code to plug_closing(),
such as errno or GetLastError(), was always a bit weird, given that it
generally had to be handled by cross-platform receiving code in
backends. I had the platform.h implementations #define any error
values that the cross-platform code would have to handle specially,
but that's still not a great system, because it also doesn't leave
freedom to invent error representations of my own that don't
correspond to any OS code. (For example, the ones I just removed from
proxy.h.)
So now, the OS error code is gone from the plug_closing API, and in
its place is a custom enumeration of closure types: normal, error, and
the special case BROKEN_PIPE which is the only OS error code we have
so far needed to handle specially. (All others just mean 'abandon the
connection and print the textual message'.)
Having already centralised the handling of OS error codes in the
previous commit, we've now got a convenient place to add any further
type codes for errors needing special handling: each of Unix
plug_closing_errno(), Windows plug_closing_system_error(), and Windows
plug_closing_winsock_error() can easily grow extra special cases if
need be, and each one will only have to live in one place.
Having a single plug_closing() function covering various kinds of
closure is reasonably convenient from the point of view of Plug
implementations, but it's annoying for callers, who all have to fill
in pointless NULL and 0 parameters in the cases where they're not
used.
Added some inline helper functions in network.h alongside the main
plug_closing() dispatch wrappers, so that each kind of connection
closure can present a separate API for the Socket side of the
interface, without complicating the vtable for the Plug side.
Also, added OS-specific extra helpers in the Unix and Windows
directories, which centralise the job of taking an OS error code (of
whatever kind) and translating it into its error message.
In passing, this removes the horrible ad-hoc made-up error codes in
proxy.h, which is OK, because nothing checked for them anyway, and
also I'm about to do an API change to plug_closing proper that removes
the need for them.
Now we always respond to backend disconnection or connection_fatal by
calling plug_closing. And we always do it in a toplevel callback, so
that when the Plug responds by calling our Socket close method (which
frees us), nothing re-entrant happens.
Also, the handling of notify_remote_disconnect is brought into line
with the spec in putty.h, which says it can be sent redundantly (when
already disconnected) or spuriously (when not even disconnected at
all), so the toplevel callback queued by that method will check first.
After this change, failures during connection_setup are now handled
_mostly_ sensibly: if the proxy connection fails, then the main
connection gets enough information to pass a sensible connection_fatal
on to the real front end.
This also fixes the assertion failure mentioned in the TODO comment,
replacing it with a reasonably sensible connection_fatal() - although
I still think that in that situation it might be better not to have a
dialog box at all.
In interactor_return_seat, I wrote a comment saying that we should
call interactor_announce when handing over to the next Interactor in
the chain, *if* any Interactor had already made any kind of
announcement.
But, having written that comment, I didn't actually *implement* the
'if' clause, and called interactor_announce unconditionally! Now
fixed.
Removed the FIXMEs in various Seat passthrough functions that were
there to remind me to say which SSH connection they referred to: that
is now done by the interactor_announce mechanism, as of commit
215b9d1775. (Though not by modifying the actual passthrough
functions, as it turned out, which is how I didn't find and remove the
FIXMEs when I did all that.)
Also, added a comment in wrap() explaining *why* it's allowed to (in
fact, must) cheat by making an InteractionReadySeat without going
through interactor_announce().
Finally, the payoff from all of this refactoring: now, when a proxy
prompts interactively during connection setup, you get a message in
advance telling you which Interactor is originating the following
messages.
To achieve this, I've arranged to link Interactors together into a
list, so that any Interactor created by a proxy has a 'parent' pointer
pointing to the Interactor its client passed to new_connection().
This allows interactor_announce() to follow the links back up the
chain and count them, so that it knows whether it's a primary
connection, or a proxy, or a proxy-for-a-proxy, or more generally an
nth-order proxy, and can include that in its announcement.
And secondly, once interactor_announce() reaches the top of the chain,
it can use that as a storage location agreed on by all Interactors in
the whole setup, to tell each other which one of them was the last to
do anything interactive. Then, whenever there's a change of
Interactor, a message can be printed to indicate it to the user; and
when the same Interactor does multiple things in succession, you don't
get a slew of pointless messages in between them all.
Previously, SshProxy dealt with creating a TempSeat to wrap the one it
was borrowing from its client, and then each client in turn dealt with
detecting when it had had its seat borrowed and finishing up with the
TempSeat. The latter involved a lot of code duplication; the former
didn't involve code duplication _yet_ (since SshProxy was the only
thing doing this job), but would have once we started wanting to do
interactive password prompting for other types of network proxy.
Now all of that functionality is centralised into two new Interactor
helper functions: interactor_borrow_seat and interactor_return_seat.
