Expose danger_connect_async_without_providing_domain_for_certificate_verification_and_server_name_indication #34
Conversation
There was a problem hiding this comment.
- Can you please rebase the changes so that this commit also includes the change from the previous one?
- Can the copy-pasting be avoided? It seems like we end up having functions which are almost equal, probably we could have rewritten it in a bit different way, so that the common code is enclosed in a separate function?
|
Hi @qtbeee , thanks for your pull request. Sorry for the late response. cc @agalakhov |
|
Hello, I've made the changes you've requested. |
There was a problem hiding this comment.
Hi, there are several things which may be written a bit shorter, also I would like to suggest to avoid the copy-paste from client_async_tls(). The only difference between safe and danger connect here would be passing or not passing the domain name to the wrap_stream() function.
The client_async_tls() should still ensure, that the domain name is supplied to the wrap_stream() function though.
| } | ||
|
|
||
| // Helper function for reducing duplicate code | ||
| fn get_wrapped_stream<S>(socket: S, domain: String, mode: Mode, dangerously: bool) |
There was a problem hiding this comment.
This function, together with its additional parameter dangerously is actually not required. You could write it a bit simpler, by changing the signature of wrap_stream() a bit. The wrap_stream() function accepts a domain parameter, for the purpose of allowing it to perform a danger connection, you can just change the type of the domain from String to Option<String>, in this case both danger_wrap_stream() and get_wrapped_stream() are not necessary anymore, i.e. the same thing can be rewritten shorten and simplier ;)
| Ok(m) => m, | ||
| Err(e) => return Box::new(future::err(e.into())), | ||
| }; | ||
| let domain = match request.url.host_str() { |
There was a problem hiding this comment.
There is a function which already does the same, it's called domain() and it has been used before, what was the reason to not using it?
| Ok(domain) => domain, | ||
| Err(err) => return Box::new(future::err(err)), | ||
| // Make sure we check domain and mode first. URL must be valid. | ||
| let mode = match url_mode(&request.url) { |
There was a problem hiding this comment.
This duplicates the body of client_async_tls().
| .and_then(move |socket| client_async_tls(request, socket))) | ||
| } | ||
| if dangerously { | ||
| Box::new(tcp_connect((domain.as_str(), port), handle).map_err(|e| e.into()) |
There was a problem hiding this comment.
This duplicates the body of client_async_tls().
|
This pull request is not valid anymore since Moreover there is another pull request #41 with the re-implementation, it seems to be more up-to-date. |
Fixes snapview#34. Signed-off-by: Alexey Galakhov <[email protected]>
With the absence of Happy Eyeballs in Rust, I needed a way to disable certificate checking when trying to connect with an IP address instead of a URL.