Skip to content
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

support for proxy authentication #28

Open
tolot27 opened this issue Nov 9, 2018 · 6 comments
Open

support for proxy authentication #28

tolot27 opened this issue Nov 9, 2018 · 6 comments

Comments

@tolot27
Copy link

tolot27 commented Nov 9, 2018

Currently, proxy authentication is not supported because of lack of support in CPR (libcpr/cpr/issues/248).

@TheAssassin
Copy link
Member

@tolot27 do you know whether we can work around that in some way? Or would we have to patch CPR? What's your use case?

@tolot27
Copy link
Author

tolot27 commented Nov 9, 2018

I've already merged the PR libcpr/cpr/pull/253 from @paulohefagundes locally but the proxy settings are still not used:

https://github.com/AppImage/zsync2/blob/7857ff19daa36139bf3c5d8b7340ad475af5c33d/src/zsclient.cpp#L152.

In this line the response.status_code is 0 rather than the expected 206.

I don't know whether https://github.com/AppImage/zsync2/blob/7857ff19daa36139bf3c5d8b7340ad475af5c33d/src/legacy_http.c#L91 is executed because the line
https://github.com/whoshuu/cpr/blob/3d14e61ed247a90a07db26fb107beb3086a035d6/cpr/session.cpp#L383 is not entered.

At work, I'm behind a corporate proxy with basic authentication. Hence, I've run into this kind of issue with many software tools.

@TheAssassin
Copy link
Member

Thanks for your detailed response. In fact, the legacy HTTP stuff is not used any more. It should be removed, I guess. We use only CPR for making HTTP connections. If we have to change anything in how we use CPR, we can surely do that.

The PR you referenced is rather outdated, and might not necessarily work as intended any more.

@TheAssassin
Copy link
Member

Hm, CPR seems like it isn't maintained very actively any more. Perhaps it's time for us to switch?

@tolot27
Copy link
Author

tolot27 commented Nov 9, 2018

Switch to what?

Currently, I'm at home an cannot continue testing and development. If setup_curl_handle is not called anymore, I can simply try calling cpr::SetProxies(proxies); and cpr::SetProxyAuth(auth); somewhere else, e.g. after https://github.com/AppImage/zsync2/blob/7857ff19daa36139bf3c5d8b7340ad475af5c33d/src/zsclient.cpp#L264

Nevertheless, I thought CURL would read the http_proxy environment variable by itself. It does not seem to be the case.

In fact, the legacy HTTP stuff is not used any more.

Then zsclient.cpp could be cleaned up?

@TheAssassin
Copy link
Member

Switch to what?

That's the question. Back in 2017, CPR was actively maintained, and provided a much less annoying API than curl's easy C interface. I am not aware of alternatives, but to be fair, I haven't invested any time in searching yet.

In fact, the legacy HTTP stuff is not used any more.

Then zsclient.cpp could be cleaned up?

With legacy, I mean code in legacy/. This code was ported from the original zsync project to avoid having to reimplement the entire zsync algorithms.

I just checked the source code, and it seems that in contrary to my previous statement, some of the HTTP requests we make are still performed by legacy_http.c, only "session setup" stuff is done in zsclient.cpp using CPR.

We could only replace that code by rewriting it in a more C++-ish style. Right now, we don't have any plans to do so right now. It doesn't seem like too much work, though, the code only performs a set of range requests which we could do through CPR as well.

It would be great if you could check whether calling those methods on the session will at least make the requests called using CPR work as intended, then we can discuss whether to replace the range request code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants