Should SignRequest's pubkey be a Credential too?

[wiktor-k]

#33 added support for adding certificates (thanks @baloo 🙇 )

I wonder if SignRequest::pubkey should now be a Credential (which wraps keys and certs alike) since today it's just a KeyData.

Thoughts?

(I'll probably need to make a reproducer 🤔 )

[jcspencer]

Good pickup!

So it looks like openssh-agent.c internally parses key blob from the SSH_AGENTC_SIGN_REQUEST message into a sshkey struct, which can represent (by the looks of things) a raw public key or a certificate. You can see their parser logic here.

Long story short, I think OpenSSH allows certs here, so we should too 😄

I'll take a further dive into this, I think SSH_AGENTC_REMOVE_IDENTITY and SSH_AGENT_IDENTITIES_ANSWER might be able to return these too (following the same logic) - though I'd have to confirm.

[wiktor-k]

Thanks for the confirmation @jcspencer !

I plan to test all these messages locally, capture them using the proto-dumper example and then use as test cases but I wasn't sure if I'm missing something 😅

[wiktor-k]

Okay, I've spent a moment coding this up and it seems Credential is mostly for private keys and certificates and not for public keys and certificates (as all the messages mentioned previously need) so I guess I need to summon the... Super @baloo ! What do you think about it Arthur? Do we need a separate pair of Credential types but for public keys (KeyData + Certificate)? Without that the Sign gets only this:

[2024-06-19T10:58:34Z DEBUG ssh_agent_lib::agent] Request: SignRequest(SignRequest { pubkey: Other(OpaquePublicKey {
algorithm: Other(AlgorithmName {
id: "[email protected]" }),
key: OpaquePublicKeyBytes([238, 210, 185, 241, 197, 0, 187, 245, 99, 14, 143, 86, 205, 71, 120, 196, 144, 38, 61, 166, 10, 137, 105, 216, 171, 206, 37, 36, 137, 162, 86, 64]) }), data: [1, 0, 1], flags: 385 })

[baloo]

Yeah, we need to to add another one of the enum Certificate but for public keys.

[wiktor-k]

Ack! The private data parsing was merged as part of #33.

Thanks for the sanity check! 👍