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.

Sign up for GitHub

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

Allow retrieving peer SSH Protocol Version String #260

Merged
merged 2 commits into from
Mar 14, 2024
Merged

Allow retrieving peer SSH Protocol Version String #260

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ddef32c
Allow retrieving peer SSH Protocol Version String
amtelekom Mar 7, 2024
36bf210
Keep remote sshid as bytes
amtelekom Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions russh/src/client/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,7 @@ where
strict_kex: false,
alive_timeouts: 0,
received_data: false,
remote_sshid: String::from_utf8_lossy(sshid).into(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@Rondom Rondom Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
session_receiver,
session_sender,
Expand Down
5 changes: 5 additions & 0 deletions russh/src/client/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,4 +395,9 @@ impl Session {
0
}
}

/// Returns the SSH ID (Protocol Version + Software Version) the server sent when connecting
pub fn remote_sshid(&self) -> &str {
&self.common.remote_sshid
}
}
1 change: 1 addition & 0 deletions russh/src/server/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,7 @@ async fn read_ssh_id<R: AsyncRead + Unpin>(
strict_kex: false,
alive_timeouts: 0,
received_data: false,
remote_sshid: String::from_utf8_lossy(sshid).into(),
})
}

Expand Down
5 changes: 5 additions & 0 deletions russh/src/server/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1028,6 +1028,11 @@ impl Session {
}
}

/// Returns the SSH ID (Protocol Version + Software Version) the client sent when connecting
pub fn remote_sshid(&self) -> &str {
&self.common.remote_sshid
}

pub(crate) fn maybe_send_ext_info(&mut self) {
if let Some(ref mut enc) = self.common.encrypted {
// If client sent a ext-info-c message in the kex list, it supports RFC 8308 extension negotiation.
Expand Down
1 change: 1 addition & 0 deletions russh/src/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ pub(crate) struct Encrypted {

pub(crate) struct CommonSession<Config> {
pub auth_user: String,
pub remote_sshid: String,
pub config: Config,
pub encrypted: Option<Encrypted>,
pub auth_method: Option<auth::Method>,
Expand Down
Loading