Suggested by Jacob: if this dialog box is going to pop up
_unexpectedly_ - perhaps when people have momentarily forgotten
they're even running Pageant, or at least forgotten they added a key
encrypted,, or maybe haven't found out yet that their IT installed it
- then it could usefully come with a help button that pops up further
explanation of what the dialog box means, and from which you can find
your way to the rest of the help.
I continue to believe that there's nothing I can (or should) do about
the fact that on Windows, Pageant's async passphrase prompt dialog box
doesn't automatically get the input focus when it pops up in response
to a request received via invisible IPC.
However, one thing I can do is add some text to the box that _warns_
people about it, so that at least there's some kind of suggestion that
you should get into the habit of clicking on the passphrase prompt
before typing your passphrase into it.
(I would be less concerned about all of this if it weren't for the
fact that focus is surprisingly non-obvious on Windows 10, at least on
the machine I have here. When the window doesn't have focus, the title
bar has the same background colour, and only the text is fainter. And
perhaps more confusingly, the cursor in the edit box still flashes!
That fooled _me_ a few times to begin with.)
This code base has always been a bit confused about which spelling it
likes to use to refer to that signature algorithm. The SSH protocol id
is "ssh-dss". But everyone I know refers to it as the Digital
Signature _Algorithm_, not the Digital Signature _Standard_.
When I moved everything down into the crypto subdir, I took the
opportunity to rename sshdss.c to dsa.c. Now I'm doing the rest of the
job: all internal identifiers and code comments refer to DSA, and the
spelling "dss" only survives in externally visible identifiers that
have to remain constant.
(Such identifiers include the SSH protocol id, and also the string id
used to identify the key type in PuTTY's own host key cache. We can't
change the latter without causing everyone a backwards-compatibility
headache, and if we _did_ ever decide to do that, we'd surely want to
do a much more thorough job of making the cache format more sensible!)
This is the last of the subdirectory creations I had planned. This one
is almost too footling to bother with (it hardly declutters the top
level very much).
One useful side effect is that I've included testback.c (containing
the null and loopback backends) in the otherbackends library, which
means it will now actually be _compiled_ even when nothing's using it,
and we'll spot bit-rot promptly when internal APIs change.
(And, to prove the point, I've immediately had to fix some bit-rot.)
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.
sshblowf.h (as was) is 100% internal to that directory. And mpint_i.h
and ecc.h are specialist enough that it's reasonable to ask clients
outside the crypto directory to include them with a subdirectory path,
to hint that it's an unusual thing to be doing.
I hadn't actually realised until now that the SSH server code is now
being compiled on Windows! It happened because I've been using static
libraries internally to the build organisation: of course, CMake has
no way of knowing that those libraries are only needed _within_ the
build, and for all it knows they might be end products shipped to
users to link their own applications with. So all the objects in the
'sshserver' library will now be compiled, even on Windows, where no
applications actually link with it.
And in that context, the use of snprintf caused a compiler warning
from the w32old build, because there, snprintf doesn't exist in the
older version of the C library.
Of course, it's currently benign, because no application in the w32old
build (or any other Windows build) is actually linking again the
sshserver library. But I don't want to rule it out in future, or at
least not for a trivial reason like this. So I've fixed the warning in
the simplest way, by switching to our own dupprintf, which is
available everywhere.
When preparing commit fca13a17b1, I redesigned the cmake test
function at the last minute, and apparently didn't quite get all the
call sites correctly rewritten. This one still omitted some of the
argument-type keywords, and had an obsolete parameter giving an
explicit name for a sub-library, which I later decided wasn't needed.
This merge was done with '-s ours', and doesn't change the contents of
main. It just records the relationship so that the next 0.75 -> main
merge (if any) shouldn't have trouble.
It's no longer a hard requirement, because now we're on cmake rather
than mkfiles.pl, we _can_ compile the same source file multiple times
with different ifdefs.
I still think it's a better idea not to: I'd prefer that most of this
code base remained in the form of libraries reused between
applications, with parametrisation done by choice of what other
objects to link them to rather than by recompiling the library modules
themselves with different settings. But the latter is now a
possibility at need.
This applies to all of AES, SHA-1, SHA-256 and SHA-512. All those
source files previously contained multiple implementations of the
algorithm, enabled or disabled by ifdefs detecting whether they would
work on a given compiler. And in order to get advanced machine
instructions like AES-NI or NEON crypto into the output file when the
compile flags hadn't enabled them, we had to do nasty stuff with
compiler-specific pragmas or attributes.
Now we can do the detection at cmake time, and enable advanced
instructions in the more sensible way, by compile-time flags. So I've
broken up each of these modules into lots of sub-pieces: a file called
(e.g.) 'foo-common.c' containing common definitions across all
implementations (such as round constants), one called 'foo-select.c'
containing the top-level vtable(s), and a separate file for each
implementation exporting just the vtable(s) for that implementation.
One advantage of this is that it depends a lot less on compiler-
specific bodgery. My particular least favourite part of the previous
setup was the part where I had to _manually_ define some Arm ACLE
feature macros before including <arm_neon.h>, so that it would define
the intrinsics I wanted. Now I'm enabling interesting architecture
features in the normal way, on the compiler command line, there's no
need for that kind of trick: the right feature macros are already
defined and <arm_neon.h> does the right thing.
Another change in this reorganisation is that I've stopped assuming
there's just one hardware implementation per platform. Previously, the
accelerated vtables were called things like sha256_hw, and varied
between FOO-NI and NEON depending on platform; and the selection code
would simply ask 'is hw available? if so, use hw, else sw'. Now, each
HW acceleration strategy names its vtable its own way, and the
selection vtable has a whole list of possibilities to iterate over
looking for a supported one. So if someone feels like writing a second
accelerated implementation of something for a given platform - for
example, I've heard you can use plain NEON to speed up AES somewhat
even without the crypto extension - then it will now have somewhere to
drop in alongside the existing ones.
Similarly to 'utils', I've moved all the stuff in the crypto
build-time library into a source directory of its own, and while I'm
at it, split up the monolithic sshauxcrypt.c into its various
unrelated parts.
This is also an opportunity to remove the annoying 'ssh' prefix from
the front of the file names, and give several of them less cryptic
names.
The old behaviour is still present under an ifdef based on _MSC_VER,
so it should still appear in the w32old builds we're still making.
(cherry picked from commit 49b91bc128)
This set of rules should cover make and ninja on Linux, and all of
nmake, ninja and vcxproj on Windows, so that if someone follows the
README build instructions (by doing 'cmake .' in-tree), it should
generate no debris that .gitignore can't filter out.
Now there's a utils/CMakeLists.txt, which contains the huge list of
source files in that directory, so that the top-level file does a
better job of showing the overview.
The definition of HAVE_CMAKE_H is now at the very top of the main
CMakeLists.txt, so that it applies to all objects. And the consequent
include of cmake.h is at the very top of defs.h, so that it should be
included first by everything. This way, I don't have to worry any more
that the HAVE_FOO definitions in cmake.h might accidentally have
failed to reach some part of the code.
add_platform_sources_to_library() is now called
add_sources_from_current_dir(), so that it will make sense when I use
it in subdirectories that aren't for a particular platform.
PuTTYgen and its documentation are pretty consistent about calling their
encryption key a 'passphrase', as opposed to a 'password' supplied
directly to a server; but the Argon2 parameters UI reverted to
'password hash', which seemed unecessarily confusing.
I think it's better to use the term 'passphrase' consistently in the UI.
(People who are used to Argon2 being called a 'password hash' can
probably deal.)
This required tweaking the coordinates of the Windows PuTTYgen UI.
I've finally got round to updating this system for the fixed
(post-VS7) command-line splitting. That means I need to regenerate the
table in the big comment. So here's an automated method of doing it
that doesn't require me to read off the output of -generate in an
error-prone manual way.
Something weird was happening in the string handling which caused the
output to be full of the kind of gibberish you expect to see from
unterminated strings. Rather than debug it in detail, I've taken
advantage of now having the utils library conveniently available, and
simply used a strbuf, which I _know_ works sensibly.
One bit shift inside mp_divmod_into gave me "warning C4334: '<<':
result of 32-bit shift implicitly converted to 64 bits". In this case
it was actually on purpose: I intentionally did a shift that I
expected to always fit in a BignumInt, and then I passed the result to
mp_add_integer_into_shifted_by_words() which is general enough that it
wants to accept the biggest integer type it can think of.
It's easy to squelch the warning by using a temporary variable of the
right type. Now I get a warning-clean build on VS2017, for both 64-
and 32-bit.
On Windows, configure-style checks are a bit slow, so it's worth
avoiding unnecessary ones if possible. I was testing for three
different header file names that are alternatives to each other, so it
makes sense to stop as soon as we find a usable one.
It was there because of a limitation of mkfiles.pl, which had a single
list of include directories that it used on all platforms. CMake does
not. So now there's an easier and more sensible way to have a
different header file included on Windows and Unix: call it the same
name in the two subdirectories, and rely on CMake having put the right
one of those subdirs on the include path.
I found these while going through the code, and decided if we're going
to have them then we should compile them. They didn't all compile
first time, proving my point :-)
I've enhanced the tree234 test so that it has a verbose option, which
by default is off.
This new implementation uses the same optimisation-barrier technique
that I used in various places in testsc: have a no-op function, and a
volatile function pointer pointing at it, and then call through the
function pointer, so that nothing actually happens (apart from the
physical call and return) but the compiler has to assume that
_anything_ might have happened.
Doing this just after a memset enforces that the compiler can't have
thrown away the memset, because the called function might (for
example) check that all the memory really is zero and abort if not.
I've been turning this over in my mind ever since coming up with the
technique for testsc. I think it's far more robust than the previous
smemclr technique: so much so that I'm switching to using it
_everywhere_, and no longer using platform alternatives like Windows's
SecureZeroMemory().
This is the start of the payoff for all that reorganisation (and
perhaps also from having moved to a library-based build structure in
the first place): a collection of pointless stub functions in outlying
programs, which were only there to prevent link failures, now no
longer need to be there even for that purpose.
