Support for SSH key signing

[dnaka91]

I tried implementing the new commit signing of Git with the git2 and ssh-key crate. This crate was a great help and contained almost all the pieces I needed to create working signatures.

Took me a few hours of searching around and fiddling with the signature format, but got a working implementation of the signing part. Definitely needs tons of tests and more verification, but it's a start.

The main part that is missing, is this SSH signature scheme: https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.sshsig

I was wondering if this is something that would make sense to include into ssh-key, or may it be out of scope? Would try to start integrating my current solution into the crate and open a PR, if there is interest.

[tarcieri]

@dnaka91 good suggestion! I'll see if I can get it into the next release

[dnaka91]

Glad to hear!

Should I start working on this or do you prefer to implement it yourself?

Currently only have the sign variant, not the verify part yet, but there are definitely some details that are not obvious from the protocol docs. Took me a while, looking at the signature with a hex editor, to find out how it's actually structured.

[tarcieri]

I plan on doing a pass on ssh-key soon to cut another release after we release rsa v0.7.0.

If you want to push up a PR before that though, I can use that or work off of it.

[tarcieri]

@dnaka91 what did you find difficult or ambiguous about that spec? It looks fairly straightforward to me.

FYI, there will be a part of this which is a bit tricky, namely how to abstract over all of the various algorithm-specific backends in a way that can accommodate both the existing support for OpenSSH certificate signing / CAs as well as "sshsig".

That will likely require some refactoring of the current implementation.

[dnaka91]

I guess it was rather lack of knowledge about OpenSSH.

One, rather easy, pitfall was that I read string and assumed C-style strings with trailing \0 byte. But instead, they're an u32 length + content.

The other one was, what they meant by the encoding for publickey, which turned out to be the content of PublicKey::to_bytes.

So in the end not really something missing from the spec, but rather me not fully understanding some common things in OpenSSH 😅.

Sorry, have been distracted by another project, so didn't try to integrate my code into ssh-key yet. Maybe I just throw it in a gist and reference here, for the time being.

[tarcieri]

Yeah, this crate already includes an implementation of SSH's serialization formats, including one-pass decoding/encoding to/from PEM, e.g.

https://github.com/RustCrypto/SSH/blob/master/ssh-key/src/decode.rs

I'll take a look at trying to implement support this weekend, and after that, this crate should be ready for the next release.

[kim]

How is this coming along? I'd be interested in taking over if it has stalled.

[tarcieri]

I plan on having a PR up this weekend

[tarcieri]

Draft PR here: #28

Would appreciate early feedback on the API prior to a final release.

[dnaka91]

This looks awesome 🤩

Had a look at the PR changes and this is so clean, as you have access to the internal encoding APIs. Mine is much more hacky as I had to re-implement those parts.

Saw a few lines where allocations could maybe be avoided, but that's rather nitpicking and probably not an issue (also, would make the code less readable).

Will give it a try later today and see how it goes.

[tarcieri]

@dnaka91 curious what allocations you think could be avoided.

The main one that might at first look like it could be avoided is encoding SignedData. Naively it seems like you could just hash it as it impls Writer which is impl'd for Sha256 and Sha512, however while that works for DSA, ECDSA, and RSA, it doesn't work with Ed25519.

Even PROTOCOL.sshsig says the following:

The data is concatenated and passed to the SSH signing function.

[tarcieri]

Also, the encoding APIs will soon be public: #29

[dnaka91]

I should probably rephrase that. Not fully being avoided, but instead being delayed, so that in case of an error, it wouldn't allocate.

One was during sign, where the namespace.to_owned() could be done later, after the signing. But is that even possible to fail and instead rather fallible due to the API?

And the other one I thought of was during decoding, where the String::decode and Vec::decode could eventually be replaced with reference versions (so on &str and &[u8], if they even exist), and then use to_owned in the end, once all fields succeeded to decode.

So really just super subtle things and only in case of errors.

Probably not really worth changing 😔

[tarcieri]

This has now been implemented. Closing.