Allow retrieving peer SSH Protocol Version String #260
Conversation
russh/src/client/mod.rs
Outdated
| @@ -697,6 +697,7 @@ where | |||
| strict_kex: false, | |||
| alive_timeouts: 0, | |||
| received_data: false, | |||
| remote_sshid: String::from_utf8_lossy(sshid).into(), | |||
There was a problem hiding this comment.
I am wondering whether it is a good idea to use String as a type here. I guess there is nothing that says that the line has to be valid UTF-8. In practice I would assume the all non-broken implementations would only use ASCII which is a subset of UTF-8. The from_utf8_lossy would account for non-UTF-8, but a user of an API maybe does not want a lossy conversion to UTF-8, but what exactly came over the wire.
What do you think?
There was a problem hiding this comment.
RFC4253 (SSH Transport) Section 4.2 defines
Both the 'protoversion' and 'softwareversion' strings MUST consist of
printable US-ASCII characters, with the exception of whitespace
characters and the minus sign (-).
So any valid SSH-ID is also valid UTF-8; of course the use of utf8_lossy would hide invalid implementations, but I don't think that's necessary here.
On the other hand, String makes it easier to continue working with the SSHID instead of pushing the task of conversion on the user - although I'd be open to changing it to a Vec<u8> if someone feels really strongly about this.
There was a problem hiding this comment.
RFC4253 (SSH Transport) Section 4.2 defines
Both the 'protoversion' and 'softwareversion' strings MUST consist of
printable US-ASCII characters, with the exception of whitespace
characters and the minus sign (-).
I agree that it may be fine. I do not feel strongly about this.
There was a problem hiding this comment.
I agree with @Rondom - russh as a protocol library shouldn't make destructive decisions about data even if it's so rare and insignificant as invalid-UTF server ID - let's leave conversion up to the user
|
I've changed it to return the raw |
Allow retrieving peer SSH Protocol Version String