mirror of
https://git.tartarus.org/simon/putty.git
synced 2025-01-10 09:58:01 +00:00
e66e1ebeae
7 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Simon Tatham
|
a2ff884512 |
Richer data type for interactive prompt results.
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!) |
||
|
Simon Tatham
|
f00c72cc2a |
Framework for announcing which Interactor is talking.
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. |
||
|
Simon Tatham
|
efa89573ae |
Reorganise host key checking and confirmation.
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. |
||
|
Simon Tatham
|
3037258808 |
Localise user_input to SSH connection layers.
Now that the SSH backend's user_input bufchain is no longer needed for handling userpass input, it doesn't have to be awkwardly shared between all the packet protocol layers any more. So we can turn the want_user_input and got_user_input methods of PacketProtocolLayer into methods of ConnectionLayer, and then only the two connection layers have to bother implementing them, or store a pointer to the bufchain they read from. |
||
|
Simon Tatham
|
cd8a7181fd |
Complete rework of terminal userpass input system.
The system for handling seat_get_userpass_input has always been structured differently between GUI PuTTY and CLI tools like Plink. In the CLI tools, password input is read directly from the OS terminal/console device by console_get_userpass_input; this means that you need to ensure the same terminal input data _hasn't_ already been consumed by the main event loop and sent on to the backend. This is achieved by the backend_sendok() method, which tells the event loop when the backend has finished issuing password prompts, and hence, when it's safe to start passing standard input to backend_send(). But in the GUI tools, input generated by the terminal window has always been sent straight to backend_send(), regardless of whether backend_sendok() says it wants it. So the terminal-based implementation of username and password prompts has to work by consuming input data that had _already_ been passed to the backend - hence, any backend that needs to do that must keep its input on a bufchain, and pass that bufchain to seat_get_userpass_input. It's awkward that these two totally different systems coexist in the first place. And now that SSH proxying needs to present interactive prompts of its own, it's clear which one should win: the CLI style is the Right Thing. So this change reworks the GUI side of the mechanism to be more similar: terminal data now goes into a queue in the Ldisc, and is not sent on to the backend until the backend says it's ready for it via backend_sendok(). So terminal-based userpass prompts can now consume data directly from that queue during the connection setup stage. As a result, the 'bufchain *' parameter has vanished from all the userpass_input functions (both the official implementations of the Seat trait method, and term_get_userpass_input() to which some of those implementations delegate). The only function that actually used that bufchain, namely term_get_userpass_input(), now instead reads from the ldisc's input queue via a couple of new Ldisc functions. (Not _trivial_ functions, since input buffered by Ldisc can be a mixture of raw bytes and session specials like SS_EOL! The input queue inside Ldisc is a bufchain containing a fiddly binary encoding that can represent an arbitrary interleaving of those things.) This greatly simplifies the calls to seat_get_userpass_input in backends, which now don't have to mess about with passing their own user_input bufchain around, or toggling their want_user_input flag back and forth to request data to put on to that bufchain. But the flip side is that now there has to be some _other_ method for notifying the terminal when there's more input to be consumed during an interactive prompt, and for notifying the backend when prompt input has finished so that it can proceed to the next stage of the protocol. This is done by a pair of extra callbacks: when more data is put on to Ldisc's input queue, it triggers a call to term_get_userpass_input, and when term_get_userpass_input finishes, it calls a callback function provided in the prompts_t. Therefore, any use of a prompts_t which *might* be asynchronous must fill in the latter callback when setting up the prompts_t. In SSH, the callback is centralised into a common PPL helper function, which reinvokes the same PPL's process_queue coroutine; in rlogin we have to set it up ourselves. I'm sorry for this large and sprawling patch: I tried fairly hard to break it up into individually comprehensible sub-patches, but I just couldn't tease out any part of it that would stand sensibly alone. |
||
|
Simon Tatham
|
2fd2f4715d |
Squash shift warnings in ssh2_bpp_check_unimplemented.
I did a horrible thing with a list macro which builds up a 256-bit bitmap of known SSH-2 message types at compile time, by means of evaluating a conditional expression per known message type and per bitmap word which boils down to (in pseudocode) (shift count in range ? 1 << shift count : 0) I think this is perfectly valid C. If the shift count is out of range, then the use of the << operator in the true branch of the ?: would have undefined behaviour if it were executed - but that's OK, because in that situation, the safe false branch is executed instead. But when the whole thing is a compile-time evaluated constant expression, the compiler can prove statically that the << in the true branch is an out-of-range shift, and at least some compilers will warn about it verbosely. The same compiler *could* also prove statically that that branch isn't taken, and use that to suppress the warning - but at least clang does not. The solution is the same one I used in shift_right_by_one_word and shift_left_by_one_word in mpint.c: inside the true branch, nest a second conditional expression which coerces the shift count to always be in range, by setting it to 0 if it's not. This doesn't affect the output, because the only cases in which the output of the true branch is altered by this transformation are the ones in which the true branch wasn't taken anyway. So this change should make no difference to the output of this macro construction, but it suppresses about 350 pointless warnings from clang. |
||
|
Simon Tatham
|
83fa43497f |
Move the SSH implementation into its own subdirectory.
This clears up another large pile of clutter at the top level, and in the process, allows me to rename source files to things that don't all have that annoying 'ssh' prefix at the top. |