-
Notifications
You must be signed in to change notification settings - Fork 117
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
Bugfix HTTPS SNI and IP Address #139
Conversation
Motivation: Solving the SNI Bug Modifications: Added an internal extension on String for checking if the hostname is an IP Address -- see the private extension on SNI. Additionally using the IPv4Address and IPv6Address Function from Network above 10.14 as protecting with #availabe. Adding the test for HTTPS and IP in as hostname Result: We get results with an IP as Hostname
Can one of the admins verify this patch? |
2 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
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.
Seems like a generally good patch! I've left a few notes in the diff if you don't mind.
anyone else want to review ? to fix that bug? |
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'm basically happy with this patch, though it'd be nice if you could fix up the noise in the diff I highlighted.
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.
Still fine with this.
@artemredkin or @weissi want to take a look here as well? |
|
||
#else | ||
internal extension String { | ||
var isIPAddress: 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.
@Lukasa should we add smth like this to NIO? This is unrelated to this PR but we could make a 'good-first-issue'
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.
Yeah, I wouldn't object, though we'd be extending a type we don't own so we'd have to prefix the method name, which is kinda gross.
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.
And actually don't we have this already in the form of SocketAddress(ipAddress:port:)
?
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.
@Lukasa ish, need to give a port too. But maybe that's not too bad?
extension String {
var isIPAddress: Bool {
return SocketAddress(ipAddress: self, port: 0) != nil
}
}
?
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 suspect it's fine, and the only reason not to use it here is that you do have to heap-allocate for that code.
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.
love it, thank you!
@swift-server-bot test this please |
@swift-server-bot test this please |
@swift-server-bot test this please |
Use the UNIX implementation across all platforms to avoid unnecessary complexity introduced by platform specific implementations
Motivation:
Solving the SNI Bug
Modifications:
Added an internal extension on String for checking if the hostname is an IP Address -- see the private extension on SNI. Additionally using the IPv4Address and IPv6Address Function from Network above 10.14 as protecting with #availabe.
Adding the test for HTTPS and IP in as hostname
Result:
We get results with an IP as Hostname