Add messages with certs

[wiktor-k]

This is very much WIP but I've captured some messages which include certs and fortunately they make the roundtrip tests fail so it's easy to see when this is fixed.

Fixes: #83 (when implemented)

[wiktor-k]

The actual change depends on a new version of SSH crates which would include this PR: RustCrypto/SSH#233

The integration tests need to be extended with signing with the certificate (id_rsa needs to be first manually removed as ssh-keygen "helpfully" signs with a local key if the remote one is not available for any reason).

[jcspencer]

Hi @wiktor-k, this looks good!

From an API perspective, I'm wondering whether the different Credential and CertKeyData types might be a bit confusing?

I'm not sure of the best way to tackle it (whether it's just a naming thing, or making a compound type).

Having them separate does have benefits from a type system perspective (e.g., ease of use), but potentially now that there is two "wrapper" structures for public/private key/certificate data, we could think of a different way to name them?

Happy to hear your thoughts!

[wiktor-k]

Hi @jcspencer, I agree that the naming could be... well... improved 😅

Just for the record for this PR I was mainly aiming at making it pass the tests (as in: parse the messages that are captured, and this works now!) so I guess some refactor will be surely needed.

But now that you've asked the question I always thought these two provided two separate types since Credential is for private keys (also ones embedded in certs) and CertKeyData (naming suggestions very much welcome! 🙏 ) is for public keys (also ones embedded in certs). One of them is used in AddIdentity commands and the other one in RequestIdentities, which, of course, return only public keys.

What do you think about it? Do note, as I said, that I didn't spend too much time on cleaning up this thing so you may have a better overview on what's going on.

(one way or another this PR is stuck until our deps release new version of their crates - fortunately my PR there was swiftly merged!)

[jcspencer]

Great work on this one @wiktor-k!

I'm thinking potentially something like PublicCredential (for the current CertKeyData) and PrivateCredential (for the current Credential)? Just makes it a bit clearer to consumers what's going on I think!

That reminds me, I got a change merged into ssh-encoding a while back to (en/de)code boolean values in SSH format; will have to wait for a new release there so we can implement it over here!

[wiktor-k]

Great work on this one @wiktor-k!

I'm thinking potentially something like PublicCredential (for the current CertKeyData) and PrivateCredential (for the current Credential)? Just makes it a bit clearer to consumers what's going on I think!

Great idea! I'll implement the change as soon as possible.

That reminds me, I got a change merged into ssh-encoding a while back to (en/de)code boolean values in SSH format; will have to wait for a new release there so we can implement it over here!

I've seen your change. I've already pestered the maintainer for releasing a new version: RustCrypto/SSH#233 (comment) so... hopefully they'll remember 😅

Thanks for your input, valuable as always! 🙇

[wiktor-k]

Hmm... we need to rethink the approach here given that it may take a while for the ssh-key to be updated.

I think I'll split this PR into two: one which introduces new API structure but purposefully does not work, and then, after ssh-key is updated, we un-ignore the tests that would be broken.

[wiktor-k]

Okay, I've made the renames and, for the time being, made the PublicCredential parse only KeyDatas until RustCrypto is advanced to stable and we solve issues mentioned by #86.

But this PR should already be good for merging since the API here won't change even when we add the ability to additionally parse certificates.

Phew 😅

[jcspencer]

lgtm!

[jcspencer]

@wiktor-k @baloo have the upstream changes landed that will allow us to add the missing cases here / merge this and cut a new release?

[wiktor-k]

From my cursory look at the latest version: https://crates.io/crates/ssh-key/0.7.0-pre.1/dependencies they still don't have ed25519 support :/

Maybe @baloo can clarify the situation since they're closer to the RustCrypto ecosystem :)

[baloo]

We can't release support for ed25519 yet because we're still on a git version of https://github.com/dalek-cryptography/curve25519-dalek.

This will require dalek-cryptography/curve25519-dalek#676 to merge and then make a pre-release of it.

[wiktor-k]

This will require dalek-cryptography/curve25519-dalek#676 to merge and then make a pre-release of it.

I think dalek-cryptography/curve25519-dalek#676 requires first releasing a non-pre versions of Rust Crypto crates 🤔 I'm wondering if it's possible to see if there are any blockers from releasing stable versions (I'm using sha2 pre version myself due to important changes there).

[baloo]

I don't know if they need non-pre versions. I don't know what the exact relationship with rustcrypto is.

[wiktor-k]

Thanks for all your input @baloo. I'll try to push this forward in the other repo and will see what comes out of it 😅