Add support for certificates in requests and responses#101
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
| // 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)) | ||
| } |
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...
There was a problem hiding this comment.
Isn't this whole bit of logic covered in the implementation of Decode.decode() for KeyData in the upcoming ssh-key 0.7.0? See https://github.com/RustCrypto/SSH/blob/9abf981c48d975d0d25d1696b61be6fb7f8c3076/ssh-key/src/public/key_data.rs#L290-L299
There was a problem hiding this comment.
Indeed! So the entire PublicCredential thing will disappear when ssh-key 0.7.0 is released. The point of this PR is to have something that works until 0.7 happens.
Do you think it makes sense or should we wait? 🤔 (I was leaning on "wait" months ago but now thought an intermediate release with this may be beneficial too)
|
I'd encourage mainlining this first, as it looks like ssh-key's 0.7.0 has been bumping rc's for a while:
|
3 participants
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.