There's a small cluster of related work building up on SSH keys:
See PHI135. See T13006. We don't validate that private keys in Passphrase are really usable private keys.
See PHI500. We don't validate that public keys in Auth are really usable public keys.
See PHI269. An install would like @cert-authority support, although this potentially makes key revocation complicated.
We don't validate that private keys in Passphrase are really usable private keys.
It's not immediately clear how we can do this. Ideally, there may be some ssh-keygen --is-this-a-valid-private-key <key>. However, we currently detect "does the key have a passphrase" by looking for the literal text "ENCRYPTED" in the key, which is apparently the canonical way to perform this test. We may need to write a PhutilSSHPrivateKeyParser chunk of code and actually parse/decode the key material ourselves, or adapt this from elsewhere, if the ssh-keygen-family command line tools don't reasonably support some kind of key inspection command.
One reasonable approach might be to try extracting the public key from the private key with ssh-keygen -y .... If we can't, the private key is (probably?) garbage. However, in PHI135/T13006 it looked like other ssh tools had difficulty distinguishing between a bad key and an encrypted key which requires a passphrase. It would be nice if we could be specific in raising an error message to the user and distinguish between "you uploaded garbage" and "you gave us the wrong keyphrase".
We don't validate that public keys in Auth are really usable public keys.
Likewise, it's not immediately clear how we can do this. There may be some ssh-keygen command which can be used as a test, or we may need to write a PhutilSSHPublicKeyParser.
An install would like @cert-authority support, although this potentially makes key revocation complicated.
Ideally, it should be possible to revoke a key signed by a cert-authority.
Currently, we just don't write revoked keys into the output of bin/ssh-auth. That won't work if we need to be able to revoke a particular key, since writing the authority but not writing the key will result in the key working.
One possibility is that we can write a line like this:
cert-authority,command="bin/ssh-exec",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty ssh-rsa <authority key material> command="bin/this-key-has-been-revoked",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty ssh-rsa <derived key material>
That is, specify that the revoked key should run bin/this-key-has-been-revoked, and the generic authority key should run bin/ssh-exec normally.
man 8 sshd says:
If both certificate restrictions and key options are present, the most restrictive union of the two is applied.
It's not clear what this means when both specify a command. We could determine this by examining the sshd source or (perhaps) empirically.
Another approach might be to add a directive like from="nowhere" to the revoked key, so that the key may never connect. Since this restriction would only exist on the specific key, it would presumably always be the "most restrictive" option in the union of the options. However, this would produce a less helpful error than running a dedicated bin/this-key-has-been-revoked command would.
It's also difficult for us to identify which revoked keys have been signed by non-revoked certificate authority keys. Ideally, we would be able to do this. If we can't, we can put every revoked key in the file, but this will mean that bin/ssh-auth tends to grow faster than your userbase does and regularly cycling keys may negatively impact ssh performance forever. It is technically very difficult to get away from listing every key in bin/ssh-auth (see PHI500 for discussion).
There may be some ssh-keygen --is-this-key derived.key --signed-by-that-key parent.key or similar, or we could implement PhutilThisClassIsJustOpenSSHWrittenInPHP.
Given the complexity and limited outstanding interest, we could also decline to support revocation of individual derived keys and just say "if you trust an authority, you're trusting all the keys, and can not revoke keys individually: revoke the authority if any of the keys leak". This limits the value of cert-authority but you still get expiry so maybe that's something?
sshd supports explicit key revocation with RevokedKeys, but this must be a file on disk and there's no, e.g., RevokedKeysCommand option, so this isn't generally suitable for use with Phabricator.
See PHI500 and T13179. Recent versions of SSH support passing the key fingerprint to the AuthorizedKeysCommand by specifying it like this:
AuthorizedKeysCommand /path/to/command.sh %f
There are some other % escapes, too.
This would potentially let us scale this workflow better by generating an output file with only the matching key, instead of needing to generate every key. We can do this fairly seamlessly by checking if we get an argument or not, and then saying "Add the %f argument if your OpenSSH is version X.Y.Z or better".
This is much more attractive than "run a version of OpenSSH that I sort of patched by copying someone else's patch".
Although this doesn't help much with validation, it does help with "making sshd not spew a bunch of errors if you have garbage keys" and with overall SSH performance.
See https://hackerone.com/reports/474897, which suggests that ssh-keygen -l ... ("Show fingerprint of specified public key file.") is probably a pretty good starting point for ssh --is-this-a-valid-public-key:
epriestley@orbital ~ $ ssh-keygen -lf ~/dev/core/conf/keys/client.pub 2048 SHA256:MsAA07YPZaoEJrfYbHigIVhnfbFzemP3hW7n5CH1pv4 Generated (RSA)
epriestley@orbital ~ $ ssh-keygen -lf ~/dev/core/lib/phabricator/README.md /Users/epriestley/dev/core/lib/phabricator/README.md is not a public key file. epriestley@orbital ~ $ echo $? 255
...but note that it gives similar output for private keys.
The first piece of output is the number of bits in the key. We should likely reject keys that are not at least 2048-bit, since factoring these is relatively practical. Modern ssh-keygen won't let you generate keys shorter than 1024 bits, but you could reasonably have one from the distant past.
The other component of that report is that there are 32,768 low-entropy "2048-bit" RSA keys which Debian systems generated until ~2008:
https://github.com/g0tmi1k/debian-ssh
I think we can blocklist all the fingerprints for those keys practically as part of a general builtin revocation list, similar to the revocation list we have for the most common passwords.
This thread suggests that some version of ssh-keygen is sensitive to trailing whitespace in private keys:
I'm highly suspicious that this report is substantive, but it might merit 30 seconds of evaluation.