In the previous few commits I noticed some repeated work in the form
of pointless empty implementations of Plug's log method, plus some
existing (and some new) empty cases of Socket's endpoint_info. As a
cleanup, I'm replacing as many as I can find with uses of a central
null implementation in the stubs directory.
This enables plug_log to run query methods on the socket in order to
find out useful information to log. I don't expect it's sensible to do
anything else with it.
The peer_info method in the Socket vtable is replaced with
endpoint_info, which takes a boolean indicating which end you're
asking about.
sk_peer_info still exists, as a wrapper on the new sk_endpoint_info.
I'm preparing to be able to ask about the other end of the connection
too, so the first step is to give this data structure a neutral name
that can refer to either. No functional change yet.
This involved a trivial merge conflict fix in terminal.c because of
the way the cherry-pick 73b41feba5 differed from its original
bdbd5f429c.
But a more significant rework was needed in windows/console.c, because
the updates to confirm_weak_* conflicted with the changes on main to
abstract out the ConsoleIO system.
This centralises the messages for weak crypto algorithms (general, and
host keys in particular, the latter including a list of all the other
available host key types) into ssh/common.c, in much the same way as
we previously did for ordinary host key warnings.
The reason is the same too: I'm about to want to vary the text in one
of those dialog boxes, so it's convenient to start by putting it
somewhere that I can modify just once.
This is like the seat-independent nonfatal(), but specifies a Seat,
which allows the GUI dialog box to have the right terminal window as
its parent (if there are multiple ones).
Changed over all the nonfatal() calls in the code base that could be
localised to a Seat, which means all the ones that come up if
something goes horribly wrong in host key storage. To make that
possible, I've added a 'seat' parameter to store_host_key(); it turns
out that all its call sites had one available already.
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.
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.
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!)
(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.
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.
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.
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.
All this Interactor business has been gradually working towards being
able to inform the user _which_ network connection is currently
presenting them with a password prompt (or whatever), in situations
where more than one of them might be, such as an SSH connection being
used as a proxy for another SSH connection when neither one has
one-touch login configured.
At some point, we have to arrange that any attempt to do a user
interaction during connection setup - be it a password prompt, a host
key confirmation dialog, or just displaying an SSH login banner -
makes it clear which host it's come from. That's going to mean calling
some kind of announcement function before doing any of those things.
But there are several of those functions in the Seat API, and calls to
them are scattered far and wide across the SSH backend. (And not even
just there - the Rlogin backend also uses seat_get_userpass_input).
How can we possibly make sure we don't forget a vital call site on
some obscure little-tested code path, and leave the user confused in
just that one case which nobody might notice for years?
Today I thought of a trick to solve that problem. We can use the C
type system to enforce it for us!
The plan is: we invent a new struct type which contains nothing but a
'Seat *'. Then, for every Seat method which does a thing that ought to
be clearly identified as relating to a particular Interactor, we
adjust the API for that function to take the new struct type where it
previously took a plain 'Seat *'. Or rather - doing less violence to
the existing code - we only need to adjust the API of the dispatch
functions inline in putty.h.
How does that help? Because the way you _get_ one of these
struct-wrapped Seat pointers is by calling interactor_announce() on
your Interactor, which will in turn call interactor_get_seat(), and
wrap the returned pointer into one of these structs.
The effect is that whenever the SSH (or Rlogin) code wants to call one
of those particular Seat methods, it _has_ to call
interactor_announce() just beforehand, which (once I finish all of
this) will make sure the user is aware of who is presenting the prompt
or banner or whatever. And you can't forget to call it, because if you
don't call it, then you just don't have a struct of the right type to
give to the Seat method you wanted to call!
(Of course, there's nothing stopping code from _deliberately_ taking a
Seat * it already has and wrapping it into the new struct. In fact
SshProxy has to do that, in order to forward these requests up the
chain of Seats. But the point is that you can't do it _by accident_,
just by forgetting to make a vital function call - when you do that,
you _know_ you're doing it on purpose.)
No functional change: the new interactor_announce() function exists,
and the type-system trick ensures it's called in all the right places,
but it doesn't actually _do_ anything yet.
Thanks to the previous commit, this new parameter can replace two of
the existing ones: instead of passing a LogPolicy and a Seat, we now
pass just an Interactor, from which any proxy implementation can
extract the LogPolicy and the Seat anyway if they need it.
Previously, SSH authentication banners were displayed by calling the
ordinary seat_output function, and passing it a special value in the
SeatOutputType enumeration indicating an auth banner.
The awkwardness of this was already showing a little in SshProxy's
implementation of seat_output, where it had to check for that special
value and do totally different things for SEAT_OUTPUT_AUTH_BANNER and
everything else. Further work in that area is going to make it more
and more awkward if I keep the two output systems unified.
So let's split them up. Now, Seat has separate output() and banner()
methods, which each implementation can override differently if it
wants to.
All the 'end user' Seat implementations use the centralised
implementation function nullseat_banner_to_stderr(), which turns
banner text straight back into SEAT_OUTPUT_STDERR and passes it on to
seat_output. So I didn't have to tediously implement a boring version
of this function in GTK, Windows GUI, consoles, file transfer etc.
There are quite a few of them already, and I'm about to make another
one, so let's start with a bit of tidying up.
The CMake build organisation is unchanged: I haven't put the proxy
object files into a separate library, just moved the locations of the
source files. (Organising proxying as a library would be tricky
anyway, because of the various overrides for tools that want to avoid
cryptography.)
