Allows clients tolerate lines before version #75
Conversation
artemredkin
force-pushed
the
support_lines_before_version
branch
3 times, most recently
from
ee8bd24 to
a3293de
Compare
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
Cool, great start @artemredkin! I've got a few notes.
| throw NIOSSHError.unsupportedVersion(version) | ||
| } | ||
|
|
||
| private func validateVersion<S: StringProtocol>(_ version: S) throws -> Bool { |
There was a problem hiding this comment.
There's no real reason to make this generic over StringProtocol: we're only gonna operate on Substring so let's just accept that.
There was a problem hiding this comment.
we call this function with both String and Substring, we call it with string on line 66. Maybe alternative is to covert string to subtring there?
There was a problem hiding this comment.
Oh yeah, let's do that. String -> Substring conversion is free.
| private func validateVersion<S: StringProtocol>(_ version: S) throws -> Bool { | ||
| if version.hasPrefix("SSH-") { | ||
| let start = version.index(version.startIndex, offsetBy: 4) | ||
| let end = version.index(start, offsetBy: 3) |
There was a problem hiding this comment.
These indices need to be validated: if the string was "SSH-" then both would be ineligible to be subscripted and the slice would be invalid. We need to confirm that the total count is appropriate.
| } | ||
|
|
||
| private func validateVersion<S: StringProtocol>(_ version: S) throws -> Bool { | ||
| if version.hasPrefix("SSH-") { |
There was a problem hiding this comment.
Also all of this is being done on the grapheme clusters. We should consider doing all the work on the String UTF8View instead, which will give us much better performance as we don't need to worry about unicode here.
There was a problem hiding this comment.
great idea, done!
| return | ||
| } | ||
| } | ||
| throw NIOSSHError.protocolViolation(protocolName: "version exchange", violation: "version string not found") |
There was a problem hiding this comment.
I don't think we have a test for this error, do we?
There was a problem hiding this comment.
good catch, yes, added a test
artemredkin
force-pushed
the
support_lines_before_version
branch
2 times, most recently
from
ac54309 to
6eb89b7
Compare
| switch role { | ||
| case .client: | ||
| // split by \n | ||
| for line in banner.utf8.split(separator: 10) { |
There was a problem hiding this comment.
We can give this a useful name: let carriageReturn = UInt8(ascii: "\n").
There was a problem hiding this comment.
ah, looks nice, fixed
Motivation: RFC 4253, section 4.2 allows for server to send some lines before version in version exchange. Right now we expect that version start with `SSH-` only. We should support this. Modifications: - Modifies version verification to allow clients tolerate lines before version - Adds tests that verify that client accepts lines and server doesn't Result: Closes apple#74
artemredkin
force-pushed
the
support_lines_before_version
branch
from
6eb89b7 to
22a5477
Compare
2 participants
Motivation:
RFC 4253, section 4.2 allows for server to send some lines before
version in version exchange. Right now we expect that version start with
SSH-only. We should support this.Modifications:
version
Result:
Closes #74