Add support for certificates in requests and responses#101
Open
Add support for certificates in requests and responses#101
Conversation
Signed-off-by: Wiktor Kwapisiewicz <wiktor@metacode.biz> See: #98 Co-authored-by: Sasha F <cornflake.matchbox397@icantbelieveitsnotgmail.com>
Signed-off-by: Wiktor Kwapisiewicz <wiktor@metacode.biz> See: #98 Co-authored-by: Sasha F <cornflake.matchbox397@icantbelieveitsnotgmail.com>
Signed-off-by: Wiktor Kwapisiewicz <wiktor@metacode.biz>
Signed-off-by: Wiktor Kwapisiewicz <wiktor@metacode.biz>
wiktor-k
force-pushed
the
wiktor-k/fix-support-for-certs
branch
from
57390cb to
1f86f25
Compare
wiktor-k
commented
Mar 19, 2026
Comment on lines
+143
to
+159
| // FIXME: This needs to be rewritten using Certificate::decode_as when ssh-key 0.7.0 hits stable, see: https://github.com/wiktor-k/ssh-agent-lib/pull/85#issuecomment-3751946208 | ||
| let alg = String::decode(reader)?; | ||
|
|
||
| let remaining_len = reader.remaining_len(); | ||
| let mut buf = Vec::with_capacity(4 + alg.len() + remaining_len); | ||
| alg.encode(&mut buf)?; | ||
| let mut tail = vec![0u8; remaining_len]; | ||
| reader.read(&mut tail)?; | ||
| buf.extend_from_slice(&tail); | ||
|
|
||
| if Algorithm::new_certificate(&alg).is_ok() { | ||
| let cert = Certificate::decode(&mut &buf[..])?; | ||
| Ok(Self::Cert(Box::new(cert))) | ||
| } else { | ||
| let key = KeyData::decode(&mut &buf[..])?; | ||
| Ok(Self::Key(key)) | ||
| } |
Owner
Author
There was a problem hiding this comment.
@baloo I have a feeling like this should belong somewhere in ssh-key itself. The detection of whether we have a cert or not and branching looks super ugly to me. It took me a while to browse Algorithm::new_certificate code to check if it will return Err on non-certificates rather than returning Other or something like this.
What do you think about it? 🤔
Edit: the problem here is that even if we first decode Algorithm it doesn't seem to have any indication if it's a certificate algorithm or not...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a replacement PR obsoleting #98 (thank for the PoC @sfriedenberg-etsy 🙏 ).
As it says, it adds support for parsing certificates and renames several fields for consistency. This should solve a long standing issue reported by @overhacked. If anyone can test it in the wild I'd be really happy. For some messages we don't have serializations (e.g.
RemoveIdentitywith certs) but I don't think this should block the change.The implementation of
decode_aslooks ugly and should be replaced as soon as ssh-key 0.7 is released. In the meantime this should work using existing APIs.