[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request: Proxy overhaul Part 4: More proxy changes and
From: "Andreas Hartmetz" <ahartmetz () gmail ! com>
Date: 2011-09-29 20:13:31
Message-ID: 20110929201331.22062.35998 () vidsolbach ! de
[Download RAW message or body]
> On Sept. 25, 2011, 2:20 p.m., Andreas Hartmetz wrote:
> > I'd actually be interested to hear which testing you did.
> >
> > The "ResolveHostNamesBeforeProxyCheck" option seems strange. In which situations \
> > is this supposed to be set / not set?
>
> Dawit Alemayehu wrote:
> The "ResolveHostNamesBeforeProxyCheck" option is used to give the user the ability \
> to turn DNS lookups of request URLs on or off before checking them against the "No \
> Proxy For" list. This makes it possible for us to let people enter IP address \
> ranges, e.g. "192.168.0.1/24" in the "NoProxyFor" list while at the same time \
> protecting those people that do not want to have any form of name resolution to \
> happen at the client application level. The default behavior is what it currently \
> is, no lookup, since we do not support IP address ranges right now.
> As far as testing goes, I created a basic SOCKS server using ssh, ssh -D 9999 \
> 127.0.0.1, and a very basic PAC script file that contains the following:
> function FindProxyForURL( url, host )
> {
> var resolved_ip = dnsResolve(host);
>
> if (isInNet(resolved_ip, "127.0.0.1", "255.255.255.0"))
> return "DIRECT";
>
> return "SOCKS 127.0.0.1:9999; DIRECT";
> }
>
> I am also in the process of testing all these changes agains a real proxy sever. I \
> am going to test against bother privoxy and squid. To test this however, you also \
> need the next patch in the series, https://git.reviewboard.kde.org/r/102696/.
OK, I see what ResolveHostNamesBeforeProxyCheck does now. Thanks.
> On Sept. 25, 2011, 2:20 p.m., Andreas Hartmetz wrote:
> > kio/kio/kprotocolmanager.cpp, line 471
> > <http://git.reviewboard.kde.org/r/102691/diff/1/?file=37173#file37173line471>
> >
> > This looks more like
> > "if no proxy scheme is given, assume SOCKS"
>
> Dawit Alemayehu wrote:
> Ahh... Did you mean, "if no proxy could be found for the scheme of the given url, \
> assume SOCKS" ? Even then that comment belongs to the if statement above the one \
> where this comment currently resides. Perhaps I should move the comment down inside \
> the if statement.
I am now more confused than before.
What I (still) think the code is doing is: if there is a proxy URL given with no \
scheme, prepend "socks://" to the proxy URL.
- Andreas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102691/#review6796
-----------------------------------------------------------
On Sept. 25, 2011, 4:15 p.m., Dawit Alemayehu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102691/
> -----------------------------------------------------------
>
> (Updated Sept. 25, 2011, 4:15 p.m.)
>
>
> Review request for kdelibs.
>
>
> Description
> -------
>
> This patch is the 4th in the serious of patches designed to resolve bugs and \
> missing functionality in KDE's proxy manager. The changes made with this patch are \
> as follows:
> * Add code that resolves a request url's hostname before attempting to match
> it against the no proxy for list so long as the "ResolveHostNamesBeforeProxyCheck"
> option is set.
>
> * Allow "DIRECT" as a special keyword in the list of proxy server addresses
> returned in slaveProtocol(const QString& protocol, QStringList& proxy).
>
> * Change KProtocolManager::proxyFor to properly handle the changes in the new
> proxy management dialog (KDE 4.8) where the proxy server port, in the
> manual proxy configuration mode, will be saved separated from the address with
> a white space.
>
> * Move the code that accounts for SOCKS proxy from KProtocolManager::proxyFor
> to KProtocolManager::proxyForUrl where it belongs. The current implementation
> only works correctly under one circumstance while breaking the previous behavior
> of the function.
>
> * Fix KProtocoManager::proxiesForUrl so that it accounts for the proxy
> exception list.
>
> * Update API documentation to reflect the changes above.
>
>
> Diffs
> -----
>
> kio/kio/kprotocolmanager.h 11e43fe
> kio/kio/kprotocolmanager.cpp 50ebb6e
>
> Diff: http://git.reviewboard.kde.org/r/102691/diff/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Dawit Alemayehu
>
>
[Attachment #3 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/102691/">http://git.reviewboard.kde.org/r/102691/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On September 25th, 2011, 2:20 p.m., <b>Andreas \
Hartmetz</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">I'd actually be interested to hear which testing you did.
The "ResolveHostNamesBeforeProxyCheck" option seems strange. In which \
situations is this supposed to be set / not set?</pre> </blockquote>
<p>On September 25th, 2011, 4:15 p.m., <b>Dawit Alemayehu</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The \
"ResolveHostNamesBeforeProxyCheck" option is used to give the user the \
ability to turn DNS lookups of request URLs on or off before checking them against \
the "No Proxy For" list. This makes it possible for us to let people enter \
IP address ranges, e.g. "192.168.0.1/24" in the "NoProxyFor" list \
while at the same time protecting those people that do not want to have any form of \
name resolution to happen at the client application level. The default behavior is \
what it currently is, no lookup, since we do not support IP address ranges right now.
As far as testing goes, I created a basic SOCKS server using ssh, ssh -D 9999 \
127.0.0.1, and a very basic PAC script file that contains the following:
function FindProxyForURL( url, host )
{
var resolved_ip = dnsResolve(host);
if (isInNet(resolved_ip, "127.0.0.1", "255.255.255.0"))
return "DIRECT";
return "SOCKS 127.0.0.1:9999; DIRECT";
}
I am also in the process of testing all these changes agains a real proxy sever. I am \
going to test against bother privoxy and squid. To test this however, you also need \
the next patch in the series, https://git.reviewboard.kde.org/r/102696/.</pre> \
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">OK, I see what \
ResolveHostNamesBeforeProxyCheck does now. Thanks.</pre> <br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On September 25th, 2011, 2:20 p.m., <b>Andreas \
Hartmetz</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="http://git.reviewboard.kde.org/r/102691/diff/1/?file=37173#file37173line471" \
style="color: black; font-weight: bold; text-decoration: \
underline;">kio/kio/kprotocolmanager.cpp</a> <span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; \
">QStringList KProtocolManager::proxiesForUrl( const KUrl &url )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">469</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> \
<span class="c1">// Make sure the scheme of SOCKS proxy is always set to \
"socks://".</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This looks more like \
"if no proxy scheme is given, assume SOCKS"</pre> </blockquote>
<p>On September 25th, 2011, 4:15 p.m., <b>Dawit Alemayehu</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ahh... Did you mean, \
"if no proxy could be found for the scheme of the given url, assume SOCKS" \
? Even then that comment belongs to the if statement above the one where this comment \
currently resides. Perhaps I should move the comment down inside the if \
statement.</pre> </blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I am now \
more confused than before. What I (still) think the code is doing is: if there is a \
proxy URL given with no scheme, prepend "socks://" to the proxy URL.</pre> \
<br />
<p>- Andreas</p>
<br />
<p>On September 25th, 2011, 4:15 p.m., Dawit Alemayehu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated Sept. 25, 2011, 4:15 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">This patch is the 4th in the serious of patches designed to resolve bugs \
and missing functionality in KDE's proxy manager. The changes made with this \
patch are as follows:
* Add code that resolves a request url's hostname before attempting to match
it against the no proxy for list so long as the \
"ResolveHostNamesBeforeProxyCheck" option is set.
* Allow "DIRECT" as a special keyword in the list of proxy server \
addresses
returned in slaveProtocol(const QString& protocol, QStringList& proxy).
* Change KProtocolManager::proxyFor to properly handle the changes in the new
proxy management dialog (KDE 4.8) where the proxy server port, in the
manual proxy configuration mode, will be saved separated from the address with
a white space.
* Move the code that accounts for SOCKS proxy from KProtocolManager::proxyFor
to KProtocolManager::proxyForUrl where it belongs. The current implementation
only works correctly under one circumstance while breaking the previous \
behavior of the function.
* Fix KProtocoManager::proxiesForUrl so that it accounts for the proxy
exception list.
* Update API documentation to reflect the changes above.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kio/kio/kprotocolmanager.h <span style="color: grey">(11e43fe)</span></li>
<li>kio/kio/kprotocolmanager.cpp <span style="color: grey">(50ebb6e)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102691/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic