-
Notifications
You must be signed in to change notification settings - Fork 47
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.
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
Allows clients tolerate lines before version #75
Conversation
ee8bd24
to
a3293de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, let's do that. String
-> Substring
conversion is free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, fixed
} | ||
|
||
private func validateVersion<S: StringProtocol>(_ version: S) throws -> Bool { | ||
if version.hasPrefix("SSH-") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great idea, done!
return | ||
} | ||
} | ||
throw NIOSSHError.protocolViolation(protocolName: "version exchange", violation: "version string not found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have a test for this error, do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, yes, added a test
ac54309
to
6eb89b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nit.
switch role { | ||
case .client: | ||
// split by \n | ||
for line in banner.utf8.split(separator: 10) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can give this a useful name: let carriageReturn = UInt8(ascii: "\n")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
6eb89b7
to
22a5477
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, LGTM.
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