ssh-key: add partial support for U2F signatures verification

[a-dma]

Hi,

I saw the recent release of v0.5 that added support for SSH signatures, and noticed that U2F/sk signatures were still missing.

I did some preliminary work to implement verification of ed25519-sk signatures. This is almost in a mergeable state, there's a few outstanding items and before doing too much I thought I'd check in with you.

Here's a short recap:

• only verification is supported as signature generation would require interaction with hardware devices which is a lot more work;
• only sk-ed25519 is currently implemented, p256 is TBD;
• I made a few design choices about where to store the various components of the signature, I'm not super familiar with the project so if you have objections on how some of the things are implemented, I'm happy to change stuff around. Generally, feel free to change things yourself if you want. Mostly I tried to keep everything inside the Sshsig layer, but it could make sense to have a dedicated one for this format as those signature are a sort of wrapper around regular SSH signatures;
• one of the tests (encoding the signature from raw bytes) currently fails. I am not entirely sure what is the best abstraction to hook into. The Signature decoding mechanism expects length-prefixed data, but sk signatures have 5 trailing bytes that are just added at the end. They are part of the signed data. Is there a good way to read those? Alternatively they could be removed from the test data given that they're not included in the length.

As mentioned before, I can (time permitting) incorporate comments that you may have, or I'm perfectly fine if you'd like to take it from here and hit this into shape.

Bye
A.

[tarcieri]

This is definitely something we'd like to add. See also #4, where I had discussed with @obelisk about merging some similar code from the sshcerts crate (including signing support).

Just a general thought having looked through the code: this implementation is putting a lot of code in SshSig which should probably go in the lower-level Signature type, especially the modifications to the SignedData struct.

Putting the implementation in Signature rather than SshSig permits more use cases, like signing low-level SSH protocol messages (e.g. for SSH authentication).

Also it'd be good to avoid breaking changes to public APIs for now, like the one made to SshSig::signed_data.

[a-dma]

Hi,

thanks for the reply. I have made a few changes and moved most of the additions into the Signature layer. In my opinion it is slightly harder to figure out what's going on, but perhaps that's not something that the user should be concerned with. The public interface should be unchanged at this point.

I've also added tests for SshSig and all tests pass now.

[tarcieri]

It looks like there are redundant imports when the crate is built with --all-features

[tarcieri]

You need to consolidate these imports with the ones for the rsa crate.

[a-dma]

I noticed the lints but haven't had the time to fix them. I have pushed a new commit now.

All checks seem to pass now.

[tarcieri]

@a-dma can you rebase? I think #48 should "fix" the build failure (which seems to be a rustc OOM)

[a-dma]

@tarcieri done.

[tarcieri]

@a-dma looks like the build is failing. In one case, it's with the alloc feature enabled but nothing else

[a-dma]

Interesting, I couldn't see those failures before, did you allow more checks?

It looks like the issue is KeyData::is_sk_ecdsa_p256() being defined only when the ecdsa feature is enabled. I've split the check that uses it to also depend on that feature. I hope that's an acceptable solution.

[tarcieri]

Looks good now, thanks!