Support identification string without carriage return #109
Conversation
|
Can one of the admins verify this patch? |
7 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
Sources/NIOSSH/SSHPacketParser.swift
Outdated
| if slice.starts(with: "SSH-".utf8) { | ||
| // Get all data upto the last line feed we found and filter out carriage returns. | ||
| slice = self.buffer.readableBytesView | ||
| let versionData = slice[slice.startIndex ..< lfIndex].filter { $0 != carriageReturn } |
There was a problem hiding this comment.
Why are we filtering out carriage returns?
There was a problem hiding this comment.
I thought it'd be cleaner to consistently have line feeds only. This was also influenced by the fact that the original implementation seems to only expect additional lines with line feeds only. See also: https://github.com/apple/swift-nio-ssh/blob/main/Tests/NIOSSHTests/SSHConnectionStateMachineTests.swift#L522
I see that all tests succeed as well if the carriage returns of the additional lines are not filtered out. The carriage return at the end should still be dropped of course, its corresponding line feed is not part of the version String either. Shall I update it to only drop the carriage return at the end (if exists)?
There was a problem hiding this comment.
I'm inclined to say that we should keep the carriage returns if they're present, and only drop the [CR]LF at the end.
There was a problem hiding this comment.
It's updated, let me know what you think. Also, I don't have much experience with NIO/ByteBuffer[View], so please verify my logic there.
orobio
force-pushed
the
support-identification-string-without-carriage-return
branch
from
3cdab98 to
b2d2405
Compare
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
One really minor tweak here but otherwise this looks great!
Sources/NIOSSH/SSHPacketParser.swift
Outdated
| self.buffer.moveReaderIndex(forwardBy: slice.startIndex.distance(to: lfIndex).advanced(by: 1)) | ||
| return version | ||
| } else { | ||
| slice = slice.dropFirst(slice.startIndex.distance(to: lfIndex.advanced(by: 1))) |
There was a problem hiding this comment.
We can probably clean this up a bit by doing:
| slice = slice.dropFirst(slice.startIndex.distance(to: lfIndex.advanced(by: 1))) | |
| slice = slice[slice.index(after: lfIndex)...] |
There was a problem hiding this comment.
That definitely looks much better! It's updated.
Connecting to an OpenSSH 7.4 server fails due to that it only uses line feeds and no carriage returns in the protocol version exchange. This change makes sure that the identification string is properly parsed when no carriage returns are present. Since other clients have no problem connecting to OpenSSH 7.4, not expecting a carriage return seems to be common. The following from RFC 4253 also indicates that this is no problem: "Implementers who wish to maintain compatibility with older, undocumented versions of this protocol may want to process the identification string without expecting the presence of the carriage return character..."
orobio
force-pushed
the
support-identification-string-without-carriage-return
branch
from
b2d2405 to
ff66eb8
Compare
|
@swift-server-bot test this please |
3 participants
Connecting to an OpenSSH 7.4 server fails due to that it only uses line
feeds and no carriage returns in the protocol version exchange.
This change makes sure that the identification string is properly
parsed when no carriage returns are present.
Since other clients have no problem connecting to OpenSSH 7.4, not
expecting a carriage return seems to be common. The following
from RFC 4253 also indicates that this is no problem:
"Implementers who wish to maintain compatibility with older,
undocumented versions of this protocol may want to process
the identification string without expecting the presence of
the carriage return character..."