Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should SignRequest's pubkey be a Credential too? #83

Open
wiktor-k opened this issue Jun 12, 2024 · 5 comments · May be fixed by #85
Open

Should SignRequest's pubkey be a Credential too? #83

wiktor-k opened this issue Jun 12, 2024 · 5 comments · May be fixed by #85

Comments

@wiktor-k
Copy link
Owner

#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
Copy link
Collaborator

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
Copy link
Owner Author

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
Copy link
Owner Author

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
Copy link
Collaborator

baloo commented Jun 20, 2024

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

@wiktor-k
Copy link
Owner Author

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

Thanks for the sanity check! 👍

@wiktor-k wiktor-k linked a pull request Jul 8, 2024 that will close this issue
wiktor-k added a commit to wiktor-k/SSH that referenced this issue Jul 9, 2024
This additional function is needed for SSH Agent Protocol where, based on
the algorithm, we need to parse the `Certificate` or the `KeyData`.

Without `decode_as` the `decode` function will greedily consume additional
string from the reader.

See: wiktor-k/ssh-agent-lib#83
Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
tarcieri pushed a commit to RustCrypto/SSH that referenced this issue Jul 10, 2024
…#233)

* Expose `KeyData::decode_as`
* Add `Certificate::decode_as`

This additional function is needed for SSH Agent Protocol where, based on
the algorithm, we need to parse the `Certificate` or the `KeyData`.

Without `decode_as` the `decode` function will greedily consume additional
string from the reader.

See: wiktor-k/ssh-agent-lib#83

Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants