Now we offer the OpenSSH certificate key types in our KEXINIT host key
algorithm list, so that if the server has a certificate, they can send
it to us.
There's a new storage.h abstraction for representing a list of trusted
host CAs, and which ones are trusted to certify hosts for what
domains. This is stored outside the normal saved session data, because
the whole point of host certificates is to avoid per-host faffing.
Configuring this set of trusted CAs is done via a new GUI dialog box,
separate from the main PuTTY config box (because it modifies a single
set of settings across all saved sessions), which you can launch by
clicking a button in the 'Host keys' pane. The GUI is pretty crude for
the moment, and very much at a 'just about usable' stage right now. It
will want some polishing.
If we have no CA configured that matches the hostname, we don't offer
to receive certified host keys in the first place. So for existing
users who haven't set any of this up yet, nothing will immediately
change.
Currently, if we do offer to receive certified host keys and the
server presents one signed by a CA we don't trust, PuTTY will bomb out
unconditionally with an error, instead of offering a confirmation box.
That's an unfinished part which I plan to fix before this goes into a
release.
This makes room to add more entries without the Proxy panel
overflowing. It also means we can put in a bit more explanation in
some of the more cryptic one-word names!
I got a pterm into a stuck state this morning by an accidental mouse
action. I'd intended to press Ctrl + right-click to pop up the context
menu, but I accidentally pressed down the left button first, starting
a selection drag, and then while the left button was still held down,
pressed down the right button as well, triggering the menu.
The effect was that the context menu appeared while term->selstate was
set to DRAGGING, in which state terminal output is suppressed, and
which is only unset by a mouse-button release event. But then that
release event went to the popup menu, and the terminal window never
got it. So the terminal stayed stuck forever - or rather, until I
guessed the cause and did another selection drag to reset it.
This happened to me on GTK, but once I knew how I'd done it, I found I
could reproduce the same misbehaviour on Windows by the same method.
Added a simplistic fix, on both platforms, that cancels a selection
drag if the popup menu is summoned part way through it.
Checking various implementations of these functions against each
other, I noticed by eyeball review that some of the special cases in
mb_to_wc() never check the buffer limit at all. Yikes!
Fortunately, I think there's no vulnerability, because these special
cases are ones that write out at most one wide char per multibyte
char, and at all the call sites (including dup_mb_to_wc) we allocate
that much even for the first attempt. The only exception to that is
the call in key_event() in unix/window.c, which uses a fixed-size
output buffer, but its input will always be the data generated by an X
keystroke event. So that one can only overrun the buffer if an X key
event manages to translate into more than 32 wide characters of text -
and even if that does come up in some exotic edge case, it will at
least not be happening under _enemy_ control.
When the window can't be resized for any reason, there will be extra
space inside the drawing area that's not part of our standard
width*font_width+2*window_border. We should include that in the
backing surface and make sure we erase it to the background colour,
otherwise it can end up containing unwanted visual junk.
An example is the same case described in the previous commit: maximise
the window and then start playing about with the font size. If you do
this while running a full-screen application that displays text in the
bottom line, it's easy to see that part of the previous display is
left over and not cleared when the new font size leaves more space at
the bottom than the old one.
If you maximise the terminal window and then press Ctrl-> or Ctrl-< to
change the font size, then the maximised window can't change size, so
what _should_ happen instead is that the terminal adjusts the number
of character cells to whatever the new font size will now permit in
the same size of window as before.
But in fact, the terminal size wasn't changing at all, because the
call to gtkwin_request_resize (called from change_font_size) detected
the maximised window and went straight to gtkwin_deny_term_resize,
which immediately called term_size() to tell the terminal it still had
the same size as before.
This commit switches gtkwin_deny_term_resize so that instead it calls
drawing_area_setup_simple(), which re-runs drawing_area_setup with the
same size the drawing area already had. This should work out the same
in the case where we're _not_ changing the font size, but now also
does the right thing when we are.
Unix Pageant is in a tricky position as a hybrid CLI/GUI application.
It has uses even in a purely CLI environment, but it won't build
without libgtk-3-dev and friends.
The solution, of course - enabled by the migration to cmake - is to
allow it to build without GTK, leaving out just the GTK askpass
functionality. That way you can still use it in any of its CLI modes,
either as a non-graphical SSH agent or as a client for an agent
elsewhere.
(You can still even use it in X lifetime mode, because its connection
to the X server is done using PuTTY's built-in X authentication and
connection setup code. It's only putting up the password prompt window
that you lose in this configuration - so you're still fine as long as
you don't try to add any encrypted keys.)
Correcting a source file name in the docs just now reminded me that
I've seen a lot of outdated source file names elsewhere in the code,
due to all the reorganisation since we moved to cmake. Here's a giant
pass of trying to make them all accurate again.
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.
Previously, a setup function returning one of these socket types (such
as platform_new_connection) had to do all its setup synchronously,
because if it was going to call make_fd_socket or make_handle_socket,
it had to have the actual fds or HANDLEs ready-made. If some kind of
asynchronous operation were needed before those fds become available,
there would be no way the function could achieve it, except by
becoming a whole extra permanent Socket wrapper layer.
Now there is, because you can make an FdSocket when you don't yet have
the fds, or a HandleSocket without the HANDLEs. Instead, you provide
an instance of the new trait 'DeferredSocketOpener', which is
responsible for setting in motion whatever asynchronous setup
procedure it needs, and when that finishes, calling back to
setup_fd_socket / setup_handle_socket to provide the missing pieces.
In the meantime, the FdSocket or HandleSocket will sit there inertly,
buffering any data the client might eagerly hand it via sk_write(),
and waiting for its setup to finish. When it does finish, buffered
data will be released.
In FdSocket, this is easy enough, because we were doing our own
buffering anyway - we called the uxsel system to find out when the fds
were readable/writable, and then wrote to them from our own bufchain.
So more or less all I had to do was make the try_send function do
nothing if the setup phase wasn't finished yet.
In HandleSocket, on the other hand, we're passing all our data to the
underlying handle-io.c system, and making _that_ deferrable in the
same way would be much more painful, because that's the place where
the scary threads live. So instead I've arranged it by replacing the
whole vtable, so that a deferred HandleSocket and a normal
HandleSocket are effectively separate trait implementations that can
share their state structure. And in fact that state struct itself now
contains a big anonymous union, containing one branch to go with each
vtable.
Nothing yet uses this system, but the next commit will do so.
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, when you resize the Event Log window, the list box grows in both
directions. Previously, as a side effect of the Columns-based layout,
it grew only horizontally.
I've arranged this by adding an extra wrinkle to the Columns layout
system, which allows you to tag _exactly one_ widget in the whole
container as the 'vexpand' widget. When the Columns is size-allocated
taller than its minimum height, the vexpand widget is given all the
extra space.
This technique ports naturally across all versions of GTK (since the
hard part is done in my own code). But it's limited: you can't set
more than one widget to be vexpand (which saves having to figure out
whether they're side by side and can expand in parallel, or vertically
separated and each have to get half the available extra space, etc).
And in complex layouts where the widget you really want to expand is
in a sub-Columns, there's no system for recursively searching down to
find it.
In other words, this is a one-shot bodge for the Event Log, and it
will want more work if we ever plan to extend it to list boxes in the
main config dialog.
While re-testing on Wayland after all this churn of the window
resizing code, I discovered that the window constantly came out a few
pixels too small, losing a character cell in width and height. This
stopped happening once I experimentally stopped setting geometry
hints.
Source-diving in GTK, it turns out that this is because the GDK
Wayland backend is applying the geometry hints to the size of the
window including 'margins', which are a very large extra space around
a window beyond even the visible 'non-client-area' furniture like the
title bar. And I have no idea how you find out the size of those
margins, so I can't allow for them in the geometry hints.
I also noticed that gtk_window_set_geometry_hints is removed in GTK 4,
which suggests that GTK upstream are probably not interested in
fiddling with them until they work more usefully (even if they would
agree with me that this is a bug in the first place, which I have no
idea). A simpler workaround is to avoid setting geometry hints at all
on any GDK backend other than X11.
So, that's what this commit does. On Wayland (or other backends), the
window can now be resized a pixel at a time, and if its size doesn't
work out to a whole number of character cells, then you just get some
dead space at the edges. Not especially nice, but better than the
alternatives I can see.
One other job the geometry hints were doing was to forbid resizing if
the backend sets the BACKEND_RESIZE_FORBIDDEN flag (which SUPDUP
does). That's now done at window creation time, via
gtk_window_set_resizable.
The window size set in the geometry hints when the backend has the
BACKEND_RESIZE_FORBIDDEN flag was computed in a simplistic way that
forgot to take account of window furniture like scrollbars and menu
bars. Now it's computed based on the rest of the geometry hints, which
are more accurate.
If the Seat that the pty backend is talking to starts to back up, then
we ought to temporarily stop reading from the pty device, to pass that
back-pressure on to whatever's running in the terminal.
Previously, this didn't matter because a Seat running a GUI terminal
never backed up anyway. But now it does, so we should support it all
the way through the system.
Normally, the GTK code runs toplevel callbacks from a GTK 'idle
function'. But those mean what they say: they are considered
low-priority, to be run _only_ when the system is idle - so they can
fail to run at all in conditions of a steady stream of higher-priority
things, e.g. something is throwing data at the application so fast
that every main-loop iteration finds a readable fd.
And that's not good, because _we_ don't think our callbacks are
low-priority: they do a lot of really important work like redrawing
the window. So if they never get round to happening, PuTTY or pterm
can appear to lock up.
Simple solution to that one: whenever we process a select notification
on any fd, we _also_ call run_toplevel_callbacks(). Then our callbacks
are bound to happen reasonably regularly.
The return value of term_data() is used as the return value from the
GUI-terminal versions of the Seat output method, which means backends
will take it to be the amount of standard-output data currently
buffered, and exert back-pressure on the remote peer if it gets too
big (e.g. by ceasing to extend the window in that particular SSH-2
channel).
Historically, as a comment in term_data() explained, we always just
returned 0 from that function, on the basis that we were processing
all the terminal data through our terminal emulation code immediately,
and never retained any of it in the buffer at all. If the terminal
emulation code were to start running slowly, then it would slow down
the _whole_ PuTTY system, due to single-threadedness, and
back-pressure of a sort would be exerted on the remote by it simply
failing to get round to reading from the network socket. But by the
time we got back to the top level of term_data(), we'd have finished
reading all the data we had, so it was still appropriate to return 0.
That comment is still correct if you're thinking about the limiting
factor on terminal data processing being the CPU usage in term_out().
But now that's no longer the whole story, because sometimes we leave
data in term->inbuf without having processed it: during drag-selects
in the terminal window, and (just introduced) while waiting for the
response to a pending window resize request. For both those reasons,
we _don't_ always have a buffer size of zero when we return from
term_data().
So now that hole in our buffer size management is filled in:
term_data() returns the true size of the remaining unprocessed
terminal output, so that back-pressure will be exerted if the terminal
is currently not consuming it. And when processing resumes and we
start to clear our backlog, we call backend_unthrottle to let the
backend know it can relax the back-pressure if necessary.
When the terminal asks its TermWin for a resize, the resize operation
happens asynchronously (or can do), and sooner or later, the terminal
will see a term_size() telling it the resize has actually taken
effect.
If the resize _doesn't_ take effect for any reason - e.g. because the
window is maximised, or because the X11 window manager is a tiling one
which will refuse requests to change the window size anyway - then the
terminal never got any explicit notification of refusal to resize. Now
it should, in as many cases as I can arrange.
One obvious case of this is the early exit I recently added to
gtkwin_request_resize() when the window is known to be in a maximised
or tiled state preventing a resize: in that situation, when our own
code knows we're not even attempting the resize, we also queue a
toplevel callback to tell the terminal so.
The more interesting case is when the request is refused for a reason
GTK _didn't_ know in advance, e.g. because the user is running an X11
tiling window manager such as i3, which generally refuses windows'
resize requests. In X11, if a window manager refuses an attempt to
change the window's size via ConfigureWindow, ICCCM says it should
respond by sending a synthetic ConfigureNotify event restating the
same size. Such no-op configure events do reach the "configure_event"
handler in a GTK program, but they weren't previously getting as far
as term_size(), because the call to term_size() was triggered from the
GTK "size_allocate" event on the GtkDrawingArea inside the window (via
drawing_area_setup()), so GTK would detect that nothing had changed.
Now we queue a last-ditch toplevel callback which ensures that if the
configure event doesn't also cause a size_allocate and a call to
drawing_area_setup(), then we cause one of our own once the dust has
settled. And drawing_area_setup(), in turn, now unconditionally calls
term_size() even if the size is the same as it was last time, instead
of taking an early exit. (It still does take an early exit to avoid
unnecessary faffing with Cairo surfaces etc, but _after_ term_size()).
This won't be 100% reliable, because it's the window manager's
responsibility to send those synthetic ConfigureNotify events, and a
window manager is a fallible process which could get into a stuck
state. But it covers all the cases I know of that _can_ be sensibly
covered - so now, when terminal.c asks the front end to resize the
window, it ought to find out in a timely manner whether or not that
has happened, in _almost_ all cases.
This is another thing that seems harmless on X11 but causes window
redraws to semipermanently stop happening on Wayland: if we try to
gtk_window_resize() a window that is maximised at the time, then
something mysterious goes wrong and we stop ever getting "draw" events.
The event should return a 'gboolean', indicating whether the event
needs propagating any further. We were returning void, which meant
that the default handling might be accidentally suppressed.
On Wayland, this had the particularly nasty effect that window redraws
would stop completely if you maximised the terminal window.
(By trial and error I found that this stopped happening if I removed
GDK_HINT_RESIZE_INC from the geometry hints, from which I guess that
the default window-state-event handler is doing something important
relating to that hint, or would have been if we hadn't accidentally
suppressed it. But the bug is clearly here.)
These alternate frontends using the GtkApplication class don't work
before GTK3, because the GtkApplication class didn't exist. In the old
mkfiles.pl system, the simplest way to prevent a build failure was to
just compile them anyway but make them reduce to a stub main(). But
now, with the new library-based code organisation, library search
order issues mean that these applications won't build at all.
Happily, with cmake, it's also easy to simply omit these binaries from
the build completely depending on our GTK version.
It's unquestionably a test program, and I'm generally clearing those
out of the top level. I only missed it in the last clearout because I
was looking for things with 'test' in the name.
This commit replaces all those fiddly little linking modules
(be_all.c, be_none.c, be_ssh.c etc) with a single source file
controlled by ifdefs, and introduces a function be_list() in
setup.cmake that makes it easy to compile a version of it appropriate
to each application.
This is a net reduction in code according to 'git diff --stat', even
though I've introduced more comments. It also gets rid of another pile
of annoying little source files in the top-level directory that didn't
deserve to take up so much room in 'ls'.
More concretely, doing this has some maintenance advantages.
Centralisation means less to maintain (e.g. n_ui_backends is worked
out once in a way that makes sense everywhere), and also, 'appname'
can now be reliably set per program. Previously, some programs got the
wrong appname due to sharing the same linking module (e.g. Plink had
appname="PuTTY"), which was a latent bug that would have manifested if
I'd wanted to reuse the same string in another context.
One thing I've changed in this rework is that Windows pterm no longer
has the ConPTY backend in its backends[]: it now has an empty one. The
special be_conpty.c module shouldn't really have been there in the
first place: it was used in the very earliest uncommitted drafts of
the ConPTY work, where I was using another method of selecting that
backend, but now that Windows pterm has a dedicated
backend_vt_from_conf() that refers to conpty_backend by name, it has
no need to live in backends[] at all, just as it doesn't have to in
Unix pterm.
I'm actually quite surprised there was only _one_ copy of each of
these standard macros in the code base, given my general habit of
casually redefining them anywhere I need them! But each one was in a
silly place. Moved them up to the top level where they're available
globally.
While I'm in the mood for cleaning up the top-level directory here:
all the 'nostuff.c' files have moved into a new 'stubs' directory, and
I broke up be_misc.c into smaller modules that can live in 'utils'.
Now testcrypt has _two_ header files, that's more files than I want at
the top level, so I decided to move it.
It has a good claim to live in either 'test' or 'crypto', but in the
end I decided it wasn't quite specific enough to crypto (it already
also tests things in keygen and proxy), and also, the Python half of
the mechanism already lives in 'test', so it can live alongside that.
Having done that, it seemed silly to leave testsc and testzlib at the
top level: those have 'test' in the names as well, so they can go in
the test subdir as well.
While I'm renaming, also renamed testcrypt.h to testcrypt-func.h to
distinguish it from the new testcrypt-enum.h.
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.
When I wanted to append an ordinary C string to a BinarySink, without
any prefix length field or suffix terminator, I was using the idiom
put_datapl(bs, ptrlen_from_asciz(string));
but I've finally decided that's too cumbersome, and it deserves a
shorter name. put_dataz(bs, string) now does the same thing - in fact
it's a macro expanding to exactly the above.
While I'm at it, I've also added put_datalit(), which is the same
except that it expects a C string literal (and will enforce that at
compile time, via PTRLEN_LITERAL which it calls in turn). You can use
that where possible to avoid the run-time cost of the strlen.
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.
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.
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.)
Now every struct that implements the Backend trait is completely
cleared before we start initialising any of its fields. This will mean
I can add new fields that default to 0 or NULL, without having to mess
around initialising them explicitly everywhere.
After this change, the cmake setup now works even on Debian stretch
(oldoldstable), which runs cmake 3.7.
In order to support a version that early I had to:
- write a fallback implementation of 'add_compile_definitions' for
older cmakes, which is easy, because add_compile_definitions(FOO)
is basically just add_compile_options(-DFOO)
- stop using list(TRANSFORM) and string(JOIN), of which I had one
case each, and they were easily replaced with simple foreach loops
- stop putting OBJECT libraries in the target_link_libraries command
for executable targets, in favour of adding $<TARGET_OBJECTS:foo>
to the main sources list for the same target. That matches what I
do with library targets, so it's probably more sensible anyway.
I tried going back by another Debian release and getting this cmake
setup to work on jessie, but that runs CMake 3.0.1, and in _that_
version of cmake the target_sources command is missing, and I didn't
find any alternative way to add extra sources to a target after having
first declared it. Reorganising to cope with _that_ omission would be
too much upheaval without a very good reason.
Previously, checking the host key against the persistent cache managed
by the storage.h API was done as part of the seat_verify_ssh_host_key
method, i.e. separately by each Seat.
Now that check is done by verify_ssh_host_key(), which is a new
function in ssh/common.c that centralises all the parts of host key
checking that don't need an interactive prompt. It subsumes the
previous verify_ssh_manual_host_key() that checked against the Conf,
and it does the check against the storage API that each Seat was
previously doing separately. If it can't confirm or definitively
reject the host key by itself, _then_ it calls out to the Seat, once
an interactive prompt is definitely needed.
The main point of doing this is so that when SshProxy forwards a Seat
call from the proxy SSH connection to the primary Seat, it won't print
an announcement of which connection is involved unless it's actually
going to do something interactive. (Not that we're printing those
announcements _yet_ anyway, but this is a piece of groundwork that
works towards doing so.)
But while I'm at it, I've also taken the opportunity to clean things
up a bit by renaming functions sensibly. Previously we had three very
similarly named functions verify_ssh_manual_host_key(), SeatVtable's
'verify_ssh_host_key' method, and verify_host_key() in storage.h. Now
the Seat method is called 'confirm' rather than 'verify' (since its
job is now always to print an interactive prompt, so it looks more
like the other confirm_foo methods), and the storage.h function is
called check_stored_host_key(), which goes better with store_host_key
and avoids having too many functions with similar names. And the
'manual' function is subsumed into the new centralised code, so
there's now just *one* host key function with 'verify' in the name.
Several functions are reindented in this commit. Best viewed with
whitespace changes ignored.
The current 'displayname' field is designed for presenting in the
config UI, so it starts with a capital letter even when it's not a
proper noun. If I want to name the backend in the middle of a
sentence, I'll need a version that starts with lowercase where
appropriate.
The old field is renamed displayname_tc, to avoid ambiguity.
It was totally unused. No implementation of the 'closing' method in a
Plug vtable was checking it for any reason at all, except for
ProxySocket which captured it from its client in order to pass on to
its server (which, perhaps after further iterations of ProxySocket,
would have ended up ignoring it similarly). And every caller of
plug_closing set it to 0 (aka false), except for the one in sshproxy.c
which passed true (but it would have made no difference to anyone).
The comment in network.h refers to a FIXME comment which was in
try_send() when that code was written (see winnet.c in commit
7b0e082700). That FIXME is long gone, replaced by a use of a
toplevel callback. So I think the aim must have been to avoid
re-entrancy when sk_write called try_send which encountered a socket
error and called back to plug_closing - but that's long since fixed by
other means now.
