-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Added bufferevent proxy(socks5&https) support #1514
base: master
Are you sure you want to change the base?
Conversation
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.
Thank you for your contribution, it does looks interesting!
But the code has some coding style issues that should be resolved first, plus it reads from socket directly.
I thought about this couple of times, but never have time for this, my idea was to provide layer, like evrpc, to provide SOCKS support (or maybe it will be easy to extend evhttp instead).
P.S. sorry for the delay
I've tweaked the coding style and moved the code from the example code to the trunk(master). |
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.
First of all thank you for the effort! I would really love to see SOCKS5 support in libevent!
I've tweaked the coding style
There are still something left, please address all of them.
I also tried to change the raw-socket read/write to a bufferevent operation, but it was unsuccessful because of ssl request was always sent before the tcp request(proxy handshake).
I really can't deal with the sequence of bev to send proxy handshake.I want some help with the details.
Take a look at the comment for evhttp_connection_set_proxy
, does this help?
http.c
Outdated
if (!(evcon->proxy_address = mm_strdup(str_value))) | ||
break; | ||
// username and password, not support now | ||
if ((str_value = evhttp_uri_get_userinfo(uri))){ |
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.
Should return an error in this case then.
http.c
Outdated
timeout.tv_sec = 3; | ||
FD_ZERO(&w); | ||
FD_SET(fd, &w); | ||
if (select(fd + 1, NULL, &w, NULL, &timeout) <= 0) |
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.
Such code cannot be merged it should be done with a proper non-blocking API via callbacks not select while the data is there.
http.c
Outdated
}else if (2 == evcon->proxy_type){ | ||
if (socks5_handshake(fd, evcon->proxy_target_address,evcon->proxy_target_port)) | ||
break; | ||
}else |
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 are lots of style issues like this in the code
}else | |
} else |
http.c
Outdated
@@ -2873,7 +3119,15 @@ evhttp_connection_connect_(struct evhttp_connection *evcon) | |||
*/ | |||
evhttp_connection_cb_cleanup(evcon); | |||
return (0); | |||
} | |||
}else { | |||
if (evcon->connectcb != NULL){ |
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 are lots of style coding issues when spaces used instead of tabs:
if (evcon->connectcb != NULL){ | |
if (evcon->connectcb != NULL){ |
sample/https-client-proxy.c
Outdated
if (evhttp_connection_set_proxy(evcon,"https://192.168.88.209:8888") < 0) | ||
break; | ||
// if (evhttp_connection_set_proxy(evcon,"socks5://192.168.88.1:1920") < 0) | ||
// break; |
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.
This cannot be merged, consider adding getopt and support multiple SOCKS types.
http.c
Outdated
// port | ||
evcon->proxy_port = evhttp_uri_get_port(uri); | ||
if (evcon->proxy_port == -1) | ||
evcon->proxy_port = (strcasecmp(scheme, "https") == 0) ? 443 : 1080; |
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 are trailing whitespaces, please cleanup the patch.
sample/https-client-proxy.c
Outdated
break; | ||
if (evhttp_connection_set_proxy(evcon,"https://192.168.88.209:8888") < 0) | ||
break; | ||
// if (evhttp_connection_set_proxy(evcon,"socks5://192.168.88.1:1920") < 0) |
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.
Does it work now?
http.c
Outdated
} | ||
|
||
int | ||
evhttp_connection_set_proxy(struct evhttp_connection *evcon, const char* proxystr) |
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.
It is not correct to provide such API, since connection can be already in the connected state.
So consider something like evhttp_connection_with_proxy_base_bufferevent_new
/evhttp_connection_with_proxy_base_new
instead.
And I guess in your comment you were writing that you need to initiate socks5 request without TLS, then you can use plain bufferevent, but not raw operations on sockets.
Once SOCKS5 initialized you can wrap into bufferevent with TLS for https, this can be done with bufferevent_mbedtls_filter_new
/bufferevent_openssl_filter_new
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.
Okay, so I was thinking about avoiding too much change, making as few changes as possible to the libevent trunk code. It just as an example to give people who need to make temporary changes to implement the functionality of the proxy.
With your tips, I have the direction of change. I will try to submit it after modification according to your instructions, which may take some time.
@zakoland are you interested in providing socks support as a separate layer?
|
I'm so Sorry. I didn't notice your reply two weeks ago. I will promptly make the necessary modifications in accordance with your requirements. |
The latest code has been submitted, but I think proxy is not just a requirement for the HTTP subsystem, and buffervent connections can also requests web via proxy. I think that proxy requirements should be modified in the buffervent of the connection layer. Therefore, I added a callback function after establishing TCP to send proxy requests, and then proceeded with SSL and HTTP request sending. |
I found that cannot use proxies(socks5/https) when use evhttp mod.
It will reset existed connection when evhttp open first request. I think that proxy must use connection before http/https first request.But bufferevent can create connection via proxy, because it will not auto reset connection.
So I added support code and unit-test code for proxies,i hope to merge them into the master.
Thank you very mush!