From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.9+) Gecko/20020328 BuildID: 20020328 i have entries for http,ssl,ftp and socks5 proxy. If i open a webpage(same for ssl,ftp) the http proxy resolves the hostname on the proxy machine. If i use Chatzilla the hostname was resolved on the client machine. In a enviroment where the client has no access to an external dns server, the hostname must be resolved on the proxy machine. The socks5 protocol supports this topology. Socks4 needs a dns resolver on the client side. I have choosen socks5 in the proxy preferences but he works like a socks4 proxy(the dns part). Reproducible: Always Steps to Reproduce: 1. choose a socks5 proxy under proxy preferences 2. use chatzilla with an internet irc server 3. chatzilla/mozilla tries to resolve the hostnames on the client side Actual Results: i cannot connect to the irc servers, because of the client machine has no access to an external dns server. Expected Results: mozilla should pass the hostname to the socks5 proxy server and then the socks5 proxy should resolve the hostname. from the socks5 faq from the socks5-v1.0r11.tar.gz (www.socks.nec.com) package. What is SOCKS5 different from SOCKS4? 3. SOCKS4 clients require full support of DNS while SOCKS5 clients can rely on SOCKS5 server to perform the DNS lookup. How does SOCKS interact with DNS? In a SOCKS4 environment, SOCKS clients are required to be able to resolve IP address of remote hosts no matter whether they are local hosts or internet hosts. Therefore DNS must be configured in such a way that SOCKS clients' resolver is able to do so. Special arrangement needs to be made when more than one DNS servers are being used (such as dual DNS environment). In a SOCKS5 environment, the above requirement is no longer necessary. SOCKS clients can passing the un-resolvable host names to SOCKS servers and the servers will try to resolve those names. As a result, so long as one of the resolvers used by either SOCKS clients or SOCKS servers is able to resolve a given host, SOCKS will work OK.

NEW I sort of backwards confirmed this by setting my browser to use SOCKS 5, ran a packet trace on SOCKS 5 server, and entered a non-existent hostname on my browser. No connection occurred. (Did this backwards because while trying to do a straight up trace of a SOCKS 5 connection, found that SOCKS library might go deaf if you activate/deactivate SOCKS in a single browser session).

Temporarily "futuring" all PAC&SOCKS bugs to clear new-networking queue. I will review later. (I promise) If you object, and can make a case for a mozilla 1.0 fix, please reset milestone to "--" or email me.

*** Bug 161154 has been marked as a duplicate of this bug. ***

I think this can be done pretty easily, but I need to discuss my approach with nspr and netwerk people first. Also, how do we want to handle configuration? An app-wide switch (resolve locally or resolve remotely) or on a per-SOCKS server basis? I believe the later will be quite annoying, but I haven't looked at it much, yet. Also, there is an extension to SOCKS 4 (called SOCKS 4a) which allows one to do the same thing. We could open another bug for it, but since the vast majority of the work for the feature is common to both 4 and 5, I think we should make this bug about both.

-> justin@maxwell.ucsf.edu

Things have a tendency to not always go forward logically, so anything that is distinct should have it's own bug. Please file a bug on SOCKS4a, and put any links or spec information you might have in the bug.

This patch implements the hostname connection method for SOCKS 4 and 5 and is configurable via a new checkbox in the proxy preferences. I've tested everything on a couple different SOCKS servers, and it's all working fine.

This patch should be ready to go, so can anyone review it?

justin: i haven't fully reviewed the patch yet, but one nit: please declare a const unsigned long value for the flag value, (possibly) like so: interface nsIProxyInfo : nsISupports { ... const unsigned long TRANSPARENT_PROXY_RESOLVES_HOST = 1 << 0; };

I tried this patch, and it works for me. (had to apply a couple of chunks by hand, though, because the tree has changed some after this patch) I tried with a "socks server" on localhost (ssh2 client) and now I could connect an internal network (from the Internet, behind the firewall). Thanks! Now if I only could configure mozilla to use a SOCKS server based on an address match... (inclusion, not exclusion like now)

This new patch should apply cleanly now, and it uses a named constant for the flag (as requested in comment #9).

updating target

Comment on attachment 106288 [details] [diff] [review] updated patch patch looks good overall; just some nits... >Index: netwerk/base/src/nsSocketTransportService.cpp >+ // ...but only if the address we were sent is good... >+ if (PR_IsNetAddrType(addr, PR_IpAddrAny)) return; nit: insert newline before |return| statement. >Index: netwerk/socket/base/nsSOCKSIOLayer.cpp >+nsSOCKSSocketInfo::GetDestinationHost(char * *aDestinationHost) >+{ >+ if (!aDestinationHost) return NS_ERROR_NULL_POINTER; use NS_ENSURE_ARG_POINTER(aDestinationHost) instead. ... >+ return (*aDestinationHost == nsnull) ? NS_ERROR_OUT_OF_MEMORY : NS_OK; >+ } >+ else else after return is meaningless. >+nsSOCKSSocketInfo::SetDestinationHost(const char * aDestinationHost) ... >+ return (mDestinationHost == nsnull) ? NS_ERROR_OUT_OF_MEMORY : NS_OK; >+ } >+ else again, else after return is meaningless. >+nsSOCKSSocketInfo::GetDestinationPort(PRInt32 *aDestinationPort) >+{ how about adding NS_ENSURE_ARG_POINTER(aDestinationPort) here as well? or maybe remove it from above. >+ // get destination port >+ PRInt32 destPort = PR_ntohs(PR_NetAddrInetPort(addr)); >+ >+ if (PR_IsNetAddrType(addr, PR_IpAddrAny)) { >+ char * destHost; >+ info->GetDestinationHost(&destHost); use nsXPIDLCString instead of raw char pointer. nsXPIDLCString destHost; info->GetDestinationHost(getter_Copies(destHost)); that way you don't have to worry about freeing the string later. >+ unsigned int host_len = strlen(destHost); if you use nsXPIDLCString, then you don't have to compute the length of the string again. you can just use destHost.Length(). >+ char * destHost; >+ info->GetDestinationHost(&destHost); same nsXPIDLCString business here. >+ unsigned int host_len = strlen(destHost); destHost.Length() >+ if (PR_IsNetAddrType(addr, PR_IpAddrV4Mapped)) { >+ // store the ip >+ memcpy(request+4, (char*)(&addr->ipv6.ip.pr_s6_addr[12]), 4); i wish NSPR exposed a function or macro for extracting the IPv4 address out of a IPv4-mapped IPv6 address. >Index: netwerk/util/nsNetUtil.h thanks for fixing up these files, but they aren't part of the build. they'll most likely be cvs removed someday soon ;) can you attach a screenshot of the pref UI changes? we'll need to get someone from the UI team to approve the changes.

A new patch addressing Darin's review comments. I did not do anything regarding the IPv6 to IPv4 conversion, though.

Comment on attachment 107268 [details] [diff] [review] patch v3 it seems like it would be nice if there were an "advanced" SOCKS prefs dialog. can you find someone on the UE team to approve these XUL changes? r=darin

Darin, by UE do you mean someone from "Browser/Composer Front End QA", such as Sarah Liberman? Also, what do you think about proceeding with super-review and then just committing the back-end parts until the interface bit is settled?

justin: i sent mail to jaggernaut@netscape.com for UI review. let's see what he has to say. it sounds like a great idea to separate this patch into two parts. the backend is ready, and we could get it in quickly.

I'm okay with this UI change.

Comment on attachment 107268 [details] [diff] [review] patch v3 Since the UI bit was ok'ed, I'll just get superreview on the current patch, rather than breaking it into backend and UI pieces. Ok?

justin: ok

Justin, might it be more efficient to target this at a specific superreviewer? It seems nobody besides Darin has looked at the patch yet ;-(

Comment on attachment 107268 [details] [diff] [review] patch v3 Looks ok. I do have one concern though. Why is a separate checkbox needed in prefs? Can't this always be enabled for SOCKS5 and disabled for SOCKS4, or is that a bad idea for some reason?

Ping. Is this fix going forward, and if not, why? I would imagine there is more than 2 people in the world needing this. The latest patch no longer applies against the current tree (whole files have been renamed/removed). Is there anything that for example I could do to help speed this up?

Sami: it shouldn't take that much work to merge this with the trunk, but someone has to own this. i definitely don't have the time myself. justin?

brian: on for SOCKS5, off for SOCKS4 is probably an ok default (but would change the default behavior). However, there are almost certainly exceptions. People will want it to be configurable. Its ok with me if the feature is not accessible from the GUI, but since I don't use SOCKS, I'm not really the person to ask. As for the patch in general, I'll get a new version posted soon. I've been busy with other things lately. Once I post a new patch, will it need to be re-reviewed? Or can I just land the updated version?

a) to the best of my knowledge, all SOCKS servers that came out within the last 2 years support both SOCKS4 and SOCKS5. Not certain if SOCKS4 support is even necessary in Mozilla. b) According to SOCKS5 spec, DNS lookup can be done in local or remote. a select box that chooses either local DNS resolution or remote DNS resolution, in addition to a menu that one can use to select which domain to perform local/remote lookup would be extremely helpful. (given that most large corporation have seperate internal/external DNS) hope this makes it into the main branch soon.

justin: depends on how much the patch changes ;-)

I don't know if this needs to be a pref. The hackish mapping of just socket connectivity in SOCKS4 w/o supporting DNS as part of the application->network connection process was considered a huge weakness of SOCKS4. If you are using SOCKS 5, you want DNS delegated to the proxy server. I think I saw one person asking for proxy client to do DNS resolution and send only IP requests, but that person did not make a good technical case (+ there might be ways this behavior would really confuse the HTTP proxy). My suggestion is to make DNS delegation work for SOCKS5 with a hidden pref, but not make a UI change. If people start complaining that they don't like editing user.js, then get a new bug and start considering it. We have several other more pressing proxy pref UI improvements that should be done first (like bug 72444 and bug 50380). The whole UI is pretty overloaded, I don't know how much new functionality we can bolt on before it becomes completely senseless.

Has this bug been fixed? It is still assigned and no words since 9 months ago.

This is a joke. It has remained assigned for probably over a year with no update

assign it to you and fiy it yourself !

Was anything ever resolved for this? I find using the socks server built into ssh makes for a nice VPN alternative, but its painfull to use without remote dns resolution

The bug was resolved with Mozilla 1.1 and reoccured with 1.5 :(

(In reply to comment #32) > assign it to you and fiy it yourself ! I'm not allowed to assign it to myself. I was working on a fix for this last night, which was only half successful. I'll integrate my changes with Justin's to create a more current update. Out of interest, why was Justin's patch not accepted into trunk?

This patch updates Justin's patch to work with Mozilla 1.4.1, the version which I'm currently using. I'll try to update it to work with 1.7 when that's released, but don't have the bandwidth to use CVS :\ At the moment, it doesn't seem to automatically recognise changes made to the setting without a restart or some form of timeout. If anybody knows what causes this, let me know.

This is the patch updated for 1.8 alpha1. I'd really like to get this reviewed and into trunk soon. There is one thing in it that I'm not happy about; in nsSocketTransport2.cpp, mNetAddr is initialised as an AF_INET; this is to overcome problems when it's INET6. When initialised to inet6, NSPR adds the layer to send IPv4 over v6, which fails if the DNS resolution of the host in nsSOCKSIOLayer.cpp is for IPv4. The type of the connection in nsSOCKSIOLayer.cpp is dependent upon how it was initialised in nsSocketTransport2.cpp, but that doesn't know about what it will actually be connecting to. In 1.4.1, this was not a problem. (ie: the current implementation would likely fail connecting to an IPv6 SOCKS server.) If anybody knows this code better than me and wants to look at how it should be done, please do.

Comment on attachment 149807 [details] [diff] [review] Patch v5 (moz 1.8a1) i'm not really reading this code, but it looks like you're caching a pref w/o setting up a way to be informed when the pref is changed. this is bad form, and since i'm the one likely to have to clean it up later, i'd rather flag it now. > LOGERROR(("IPv6 not supported in SOCK 4.")); someone should fix that to 'is not' and 'SOCKS' ...

What timeless said. Also, what if someone does try to go to http://0.0.0.0/ ?

ok, This patch is in response to the feedback posted here. Major Changes: - Does handle 0.0.0.0 correctly. This involved updating the nsISocketProvider interface, and all of its dependencies, including code in security and directory. - Fixes 'SOCK 4' - Updates help to include documentation about the updated UI - Removed stale SOCKS4 code from netwerk/socket/base that is no longer in the build Major issues: - gtk2 build is broken, not sure if this patch is responsible for it. I'll have to test an identical build without patch and see. I can't see how this patch could break it, though. - That AF_INET kludge. Help appreciated. - Caching of preferences. This new setting appears to be handled the same as all other SOCKS settings. If the pref is changed, and you browse to a *different* webserver, it picks up the new configuration; browsing to the *same* webserver uses the old configuration. I can't see a good way around this; having nsSocketTransport depend on the contents of the proxyInfo variable seems like a bad idea - it might get freed or destroyed, and if nsSocketTransport references it, might need to free or destroy it when it's destroyed...

(In reply to comment #40) > - gtk2 build is broken, not sure if this patch is responsible for it. I'll > have to test an identical build without patch and see. I can't see how this > patch could break it, though. Confirmed that this patch didn't barf gtk2. It barfs all by itself on my setup.

timeless: this is already observing the preference. an observer exists for network.proxy.*, and the newly added code is in the observer. some comments on the patch... firstly, thanks for updating the help + nsXPIDLCString destHost; + destHost = info->DestinationHost(); this should be something like: const nsCString& destHost = info->DestinationHost(); to avoid the copying. in addition, nsXPIDLCString is really only meant to be used together with |out string| parameters in XPCOM interfaces. hm... shouldn't you, around nsSocketTransport2.cpp line 958, also reset proxyFlags? >If the pref is changed, and you browse to a >*different* webserver, it picks up the new configuration; browsing to the >*same* webserver uses the old configuration. really? why is that? doesn't a new proxyinfo get created each time a new channel is created? http-keepalive might be an issue here, but not really for this pref, since you don't need to re-lookup the http name if you already have a connection can you, in the future, use the -p flag when creating patches, so it's easy to see which function the changes are in?

(In reply to comment #40) > - Caching of preferences. This new setting appears to be handled the same as > all other SOCKS settings. If the pref is changed, and you browse to a > *different* webserver, it picks up the new configuration; browsing to the > *same* webserver uses the old configuration. see bug 233811, esp. bug 233811 comment 7

Comment on attachment 149980 [details] [diff] [review] Patch v6 [politically correct bloat] i don't suppose i could get you to consider using cvs diff (in the future). it helps when patches get old or files change and you need to do merges.

> firstly, thanks for updating the help Updated again for your aesthetic convenience :) > + nsXPIDLCString destHost; > + destHost = info->DestinationHost(); > this should be something like: > const nsCString& destHost = info->DestinationHost(); Done. > hm... shouldn't you, around nsSocketTransport2.cpp line 958, also reset > proxyFlags? Done, although this means that the proxyFlags must be set individually for each layer in the stack, so it also has to be initialised only for a socks/socks4 layer. >> If the pref is changed, and you browse to a >> *different* webserver, it picks up the new configuration; browsing to the >> *same* webserver uses the old configuration. > really? why is that? I was hoping you'd know :) > can you, in the future, use the -p flag when creating patches, so it's > easy to see which function the changes are in? Done. > i don't suppose i could get you to consider using cvs diff (in the > future). it helps when patches get old or files change and you need to > do merges. I know; the problem is I don't have a huge amount of bandwidth to play with. I've done a cvs diff this time, while I'm in CA soaking up the sun and network capacity, but I won't be able to do things like this forever...

Comment on attachment 152073 [details] [diff] [review] the guilt-free panacea If enabled, you no longer need DNS + access to your browser. how about: "Your browser no longer needs DNS access" hmm, 3 versions of this help file? *sigh* ah, this patch does already not apply anymore. unfortunate timing. I'll review this tomorrow...

Comment on attachment 152073 [details] [diff] [review] the guilt-free panacea hrm, it looks like this patch does not address PAC-configured SOCKS proxies. it probably should. see the new function BuildProxyList. + , mProxyTransparentResolvesHost(PR_FALSE) hmm, this is a bit of a bad name in my opinion. how about mProxyResolvesHost? or maybe mTransparentProxyResolvesHost I prefer mProxyResolvesHost, and the flag may be better called PROXY_RESOLVES_HOST, too... more tomorrow. note to self: Index: netwerk/socket/base/nsSOCKSIOLayer.cpp

this is attachment 152073 [details] [diff] [review] updated to trunk so that it compiles... I'm not sure how: + strncpy((char*) request+5, destHost, host_len); ever compiled; I added a .get() to destHost.

Comment on attachment 152073 [details] [diff] [review] the guilt-free panacea + if (info->Flags() && nsISocketProvider::PROXY_RESOLVES_HOST) { surely &, not &&?

/home/chb/clean-mozilla/mozilla/directory/xpcom/base/src/nsLDAPSecurityGlue.cpp:215: error: no matching function for call to `nsDerivedSafe<nsISSLSocketProvider>:: AddToSocket(int, char*&, int&, int, int, PRFileDesc*&, nsGetterAddRefs<nsISupports>)'

(In reply to comment #47) > I prefer mProxyResolvesHost, and the flag may be better called > PROXY_RESOLVES_HOST, too... Hmmm. PROXY_RESOLVES_HOST would match HTTP Proxies as well. So the current name probably is better.

Comment on attachment 152073 [details] [diff] [review] the guilt-free panacea + void Init(PRInt32 version, const char *proxyHost, PRInt32 proxyPort, + const char *destinationHost, PRInt32 destinationPort, PRInt32 flags); hm... why do you need tge destinationPort in here? Doesn't the one suffice that you get in the PRNetAddr, later on? ..which you do seem to be using: + PRInt32 destPort = PR_ntohs(PR_NetAddrInetPort(addr)); + destPort = info->DestinationPort(); is there ever a case where info->DestinationPort() is not equal to PR_ntohs(PR_NetAddrInetPort(addr))? + return NS_ERROR_FAILURE; // Hostname was too big for buffer. well, reasonable error codes are not really a strength of this file... + strncpy((char*) request+5, destHost, host_len); why cast to char*? in fact, why not just use memcpy, since you don't need any fancy string handling? + // add the destination port to the request no need for trailing whitespace :) + if (info->Flags() && nsISocketProvider::PROXY_RESOLVES_HOST) { hmm, second time where an && should be & + strncpy((char*) request+request_len, destHost, host_len); since you want a terminating zero here, and already checked the string length, why not just use "strcpy"? or strncpy with host_len+1. or memcpy with host_len + 1 :) (then you don't need the explicit set of the terminating null) + // store the ip + memcpy(request+4, (char*)(&addr->inet.ip), 4); why are you using memcpy here, and explicit assignments in the SOCKS 5 case? + // store the port + request[2] = (unsigned char)(destPort >> 8); + request[3] = (unsigned char)destPort; I'd prefer if this was moved back after the set of [0] and [1]... + <row> + <spacer/> + <checkbox id="networkProxySOCKSRemoteDNS" + label="&socksRemoteDNS.label;" + accesskey="&socksRemoteDNS.accesskey;" + prefstring="network.proxy.socks_remote_dns"/> + </row> hmm. with this change, this pref panel does not fit into the pref window :( (at least on my system) that's not good. unfortunately I can't think of a way to rearrange things in this panel... hm... so, r=me with the above changes; but please attach a new patch. darin should sr the patch. (socks4 protocol descriptions seem to be at: http://www.smartftp.com/Products/SmartFTP/RFC/socks4.protocol and 4a: http://www.smartftp.com/Products/SmartFTP/RFC/socks4a.protocol)

> how about: "Your browser no longer needs DNS access" Done. > hmm, 3 versions of this help file? *sigh* Hey, there was four... > hrm, it looks like this patch does not address PAC-configured > SOCKS proxies. it probably should. see the new function BuildProxyList. I've looked through that code before and still can't figure it out. I'll have another look and look at what's in .pac files...but this patch would involve extending that format - isn't it some kind of standard? > I'm not sure how: > + strncpy((char*) request+5, destHost, host_len); > ever compiled; I added a .get() to destHost. It didn't - I must have submitted a slightly older than current patch *blushes.* There was one LOG line that also needed a .get(), that's in this patch. > + if (info->Flags() && nsISocketProvider::PROXY_RESOLVES_HOST) { > surely &, not &&? Done - that was stupid *blushes again*. > /home/chb/clean-mozilla/mozilla/directory/xpcom/base/src/nsLDAPSecurityGlue.cpp:215: > error: no > matching function for call to `nsDerivedSafe<nsISSLSocketProvider>:: > AddToSocket(int, char*&, int&, int, int, PRFileDesc*&, > nsGetterAddRefs<nsISupports>)' Done - this was in earlier versions of this patch for a few revs, have no idea how it got dropped. > hm... why do you need tge destinationPort in here? Doesn't the one suffice that > you get in the PRNetAddr, later on? Done. Although, in previous versions of this patch, the PRNetAddr port would always be zero if this option was enabled, this behaviour has been changed. > + return NS_ERROR_FAILURE; // Hostname was too big for buffer. > well, reasonable error codes are not really a strength of this file... Done - I changed this to NS_ERROR_INVALID_ARG, which is closer, but not really perfect. Out of memory? There doesn't seem to be a good error for this. And there's still a *lot* of other NS_ERROR_FAILURE conditions in this function. > + strncpy((char*) request+5, destHost, host_len); > why cast to char*? in fact, why not just use memcpy, since you don't need any > fancy string handling? Done - I think the (char *) was there to avoid a warning from strncpy. > + // add the destination port to the request > no need for trailing whitespace :) Done. > + if (info->Flags() && nsISocketProvider::PROXY_RESOLVES_HOST) { > hmm, second time where an && should be & Done. > + strncpy((char*) request+request_len, destHost, host_len); > since you want a terminating zero here, and already checked the string length, > why not just use "strcpy"? or strncpy with host_len+1. or memcpy with host_len > + 1 :) (then you don't need the explicit set of the terminating null) Done - using strncpy with host_len + 1. I'm just too nervy to completely trust things like this, especially strcpy. memcpy might be ok. > + memcpy(request+4, (char*)(&addr->inet.ip), 4); > why are you using memcpy here, and explicit assignments in the SOCKS 5 case? Done - changed SOCKS4 to explicit assignments. In answer, that's just the way the code has always been, I didn't change anything, just adopted what was there. I'm in two minds here though, because memcpy could give better performance than byte-by-byte assignments, especially on IPv6. Maybe memcpy should be used instead... > + // store the port > + request[2] = (unsigned char)(destPort >> 8); > + request[3] = (unsigned char)destPort; > I'd prefer if this was moved back after the set of [0] and [1]... Done - it was like this because destPort may come from different places depending on the if, but now ports are handled the same in either case. > hmm. with this change, this pref panel does not fit into the pref window :( (at > least on my system) Hrm - I haven't changed this code since Justin was working on it, and it fits on both my dev machines (Linux+Mac.) What would make it fit on some and not others...?

>> hmm, 3 versions of this help file? *sigh* > Hey, there was four... And now there are two. So in the last two days, my patch broke due to its removal and reformatting of another file. So here is an update to fix help. Note: check out the spaces/tabs formatting in nav_help.xhtml. It's a classic.

(In reply to comment #53) > > hrm, it looks like this patch does not address PAC-configured > > SOCKS proxies. it probably should. see the new function BuildProxyList. > > I've looked through that code before and still can't figure it out. > I'll have another look and look at what's in .pac files...but this > patch would involve extending that format - isn't it some kind of > standard? Confirmed. I found this spec file: http://wp.netscape.com/eng/mozilla/2.0/relnotes/demo/proxy-live.html I don't see how this patch can be represented at all within this format, and there are several parts that make me wonder whether this patch is compatible with PAC-as-we-know-it. With this patch, there should *never* be an isInNet() in a PAC file, because that requires DNS resolution, and this patch is to eliminate exactly that. isResolvable() could be used on good PAC scripts to remove the requirement to resolve. But having done that, there needs to be a format to represent a SOCKS (with server-side nameresolution) and SOCKS (with client-side nameresolution.) Currently, the only return values I can find are either "DIRECT" or "PROXY host:port" (or lists thereof.) This patch would require an extension to this return value. I do not believe that it is worth waiting to land this patch based on a significant modification to the PAC file format. This should be handled in a seperate bug, and see if there is a demand for this. On sites that are currently using PAC, I imagine most will have DNS access (given the nature of the scripts and how they are used.) This is the *only* way our local proxy.pac is used from where I'm writing this: http://www.cs.rmit.edu.au/proxy.pac . So could somebody please mark the previous patch for review please?

Comment on attachment 154330 [details] [diff] [review] negative astrology hmm, PAC in mozilla already supports SOCKS and SOCKS5 (or was it SOCKS4?) instead of PROXY in pac files. isInNet / isResolvable are a problem of course, but we can ignore that, I think... I was just thinking that these SOCKS entries from PAC should respect this pref as well. can you not request reviews yourself?

(In reply to comment #56) > (From update of attachment 154330 [details] [diff] [review]) > hmm, PAC in mozilla already supports SOCKS and SOCKS5 (or was it SOCKS4?) > instead of PROXY in pac files. isInNet / isResolvable are a problem of course, > but we can ignore that, I think... I was just thinking that these SOCKS entries > from PAC should respect this pref as well. If we do ignore it, we'll still need to extend this format somehow, ie: return "SOCKS(servernameres) sockshost.com:1080" (I don't mean this is how it *should* be done, but that if PACs can do it, it has to be specified somehow...suggestions welcome.) > can you not request reviews yourself? Wasn't aware that I could - I just found how though, in the 'Edit' link (this is my first Moz patch, I'm a bugzilla n00b.)

Comment on attachment 154330 [details] [diff] [review] negative astrology >(I don't mean this is how it *should* be done, but that if PACs can do it, it >has to be specified somehow...suggestions welcome.) well, could PAC not just respect this preference? modules/libpref/src/init/all.js +pref("network.proxy.socks_remote_dns", false); would true be a better default value? netwerk/base/public/nsNetUtil.h + PRUint16 flags, wrong indentation netwerk/base/src/nsSocketTransport2.cpp + if ((mProxyTransparentResolvesHost == PR_TRUE) && make that if (mProxyTransparentResolvesHost && r=biesi

> well, could PAC not just respect this preference? Technically, of course. But again, I don't think this is a good approach - having a "Proxy Auto-Config" that requires manual configuration to work correctly seems a little suboptimal. The point I was trying to make before is that if this pref is to be included in PAC, then we need to think about the "right" format for that that is compatible with other environments. What I have done in this latest patch is created "flags" in PAC responses, one of which is defined for this patch, so instead of return "socks5 192.168.0.1:1080"; you can now have return "socks5 192.168.0.1:1080[clientdns]"; These flags are comma-delimited, and should handle whitespace around them without problems. Again, extending the PAC format is not something to be done lightly, but the idea of PAC is to eliminate manual config, so if we want a configurable proxy setting having a way to express it in PAC is probably better than not. > modules/libpref/src/init/all.js > +pref("network.proxy.socks_remote_dns", false); > would true be a better default value? Yes, it would. See comment #29. I can't see why any SOCKS5 environment should ever have this disabled, and I'd be surprised if there are SOCKS4 environments old enough in production to want it disabled, either. The false default was historic to avoid a behaviour change. But if you'll let me have true as a default, that's great ;) - I've updated this patch accordingly. > netwerk/base/public/nsNetUtil.h > + PRUint16 flags, > wrong indentation Fixed. > netwerk/base/src/nsSocketTransport2.cpp > + if ((mProxyTransparentResolvesHost == PR_TRUE) && > make that if (mProxyTransparentResolvesHost && Fixed.

Comment on attachment 154949 [details] [diff] [review] embrace and extend return "socks5 192.168.0.1:1080[clientdns]"; hrm, up to now PAC was working cross-browser in MSIE, NS4, Mozilla... I want benc and darin to agree to this change. I have not looked at the code for this yet. + // If it's a SOCKS proxy, default to doing name resolution on the + // server side. this, though, sounds good. + if ((type == kProxyType_SOCKS || type == kProxyType_SOCKS4)) might as well remove one pair of parentheses. >But if you'll let me have true as a default, that's great ;) sure :)

many companies deploy a single PAC file for all browsers. that means that any extensions to PAC need to be done in a backwards compatible way. adding a flags parameter as you have done breaks the parsing done by older builds. i suppose servers could do UA sniffing to serve the right PAC file :-/ i would recommend separating the issue of PAC specifying client-dns into a separate bug since the issue of how to specify the flag from PAC is going to be more contentious then everything else in this patch.

(In reply to comment #61) > many companies deploy a single PAC file for all browsers. that means that any > extensions to PAC need to be done in a backwards compatible way. adding a flags > parameter as you have done breaks the parsing done by older builds. Hrm, I tried to make sure the flags stuff wouldn't break the code as it existed, so I was hopeful it wouldn't break older Mozilla builds (can't speak for other browsers.) > > i would recommend separating the issue of PAC specifying client-dns into a > separate bug since the issue of how to specify the flag from PAC is going to be > more contentious then everything else in this patch. Agreed. So what should the 'interim' method for handling PAC be? Should a way (let's say, server-side name resolution) *always* be used when a PAC is specified? This has the advantage of consistency when using PACs, although it's not configurable. Or should the pref apply to PACs? This may mean inconsistent client behavior when using PACs. Also, how could the UI be arranged so that the checkbox is applicable to manual and auto proxy config but not direct?

> Hrm, I tried to make sure the flags stuff wouldn't break the code as it existed, > so I was hopeful it wouldn't break older Mozilla builds (can't speak for other > browsers.) I don't doubt that you made it work with older versions of Mozilla... my main concern is not with recent versions of Mozilla, but rather with other browsers. I should have said "older browsers" instead of "older builds" ;-) > Should a way (let's say, server-side name resolution) *always* be used when a > PAC is specified? This has the advantage of consistency when using PACs, > although it's not configurable. what does IE do? > Or should the pref apply to PACs? This may mean inconsistent client behavior > when using PACs. Also, how could the UI be arranged so that the checkbox is > applicable to manual and auto proxy config but not direct? ugh.. i'm not a UI expert and the proxy pref panel is already getting very cluttered. might be good to enlist Ben Goodger's help (or somebody else's who knows about UI design).

hm... does this pref require UI? if we change the default to "true", things should "just work" for SOCKS users... note that there is another bug which wants to add something to this panel - WPAD. this panel needs a redesign...

(In reply to comment #63) > > Should a way (let's say, server-side name resolution) *always* be > > used when a PAC is specified? This has the advantage of > > consistency when using PACs, although it's not configurable. > > what does IE do? IE for Windows only supports SOCKS4 and requires client-side DNS lookups. IE for Mac supports SOCKS5 and does server-side DNS lookups. I don't think either support configuration for this option. So I'll take that as a vote for "pick the best and go with it" and in this case, server side name resolution is the best. > hm... does this pref require UI? if we change the default to > "true", things should "just work" for SOCKS users... ok, that's the $1m question. When I wrote my solution for this bug, there was no pref, it was SOCKS5 only and it changed everything to use server-side name resolution, no questions asked. Justin's, on the other hand, had a pref, supported SOCKS4a and was generally a better patch, so I worked on his instead. My answer: SOCKS5 should "just work" with server-side name resolution, unless the server is horribly broken. I doubt there are any SOCKS5 servers *that* broken in use. SOCKS4 does not support server-side name resolution except as an extension, SOCKS4a. Any plain, SOCKS4 server requires client-side name resolution. Many SOCKS4 servers do support 4a (mine does, MS does) but there are many "very minimal" SOCKS4 implementations out there because the protocol was so simple, and these don't. So it doesn't *need* the UI unless we plan on having some support for small, SOCKS4 only servers. If we want to support these things, then there needs to be a way to change the pref, and surely a UI would be the better way... IMO, the Proxy panel needs to be less cluttered - maybe have the 3 "direct, manual, auto" options, and a button for "manual" that pops up the rest. This should work well with WPAD and this patch.

Full PAC handling has been removed, as has the double-brackets. PACs now all use server-side name resolution.

Comment on attachment 155243 [details] [diff] [review] strategic retreat looks good

Comment on attachment 155243 [details] [diff] [review] strategic retreat >Index: modules/libpref/src/init/all.js >+pref("network.proxy.socks_remote_dns", true); so, this changes our default behavior right? is that really a good idea? will we confuse users by enabling this pref by default? what do IE and Opera do? >Index: netwerk/base/public/nsIProtocolProxyService.idl >- nsIProxyInfo newProxyInfo(in ACString aType, in AUTF8String aHost, in long aPort); >+ nsIProxyInfo newProxyInfo(in ACString aType, in AUTF8String aHost, >+ in long aPort, in unsigned short aFlags); when changing the signature of an interface method, it is imperative that you change the UUID of the interface. >Index: netwerk/base/public/nsIProxyInfo.idl >+ [noscript, notxpcom] PRUint16 Flags(); the same applies to this interface. i think flags should be a PRUint32 since that is anyways the size of a WORD on most platforms so making it be a 16-bit integer isn't that useful. >Index: netwerk/base/src/nsProtocolProxyService.cpp >+ // If it's a SOCKS proxy, do name resolution on the server side. >+ if (type == kProxyType_SOCKS || type == kProxyType_SOCKS4) >+ flags |= nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST; shouldn't this be conditional on the pref? or are you saying that PAC will always do host resolution on the server? why enable for SOCKS4? shouldn't this be for SOCKS5 only? > if (mSOCKSProxyVersion == 4) > type = kProxyType_SOCKS4; > else > type = kProxyType_SOCKS; > port = mSOCKSProxyPort; >+ if (mSOCKSProxyRemoteDNS) >+ proxyFlags |= nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST; same question? shouldn't this be for SOCKS5 only? >Index: netwerk/base/src/nsSocketTransport2.cpp >+ if (!mProxyHost.IsEmpty() && mProxyTransparentResolvesHost) { >+ // Name resolution is done on the server side. Just pretend >+ // client resolution is complete, this will get picked up later. >+ // since we don't need to do DNS now, we bypass the resolving >+ // step by initializing mNetAddr to an empty address, but we >+ // must keep the port. The SOCKS IO layer will use the hostname >+ // we send it when it's created, rather than the empty address >+ // we send with the connect call. >+ mState = STATE_RESOLVING; >+ PR_SetNetAddr(PR_IpAddrAny, PR_AF_INET, SocketPort(), &mNetAddr); >+ rv = PostEvent(MSG_DNS_LOOKUP_COMPLETE, NS_OK, nsnull); >+ return rv; nit: "return PostEvent(...);" also, i seem to recall that our FTP code needs to know the IP address of the host it is talking to in order to know if the host is IPv4 or IPv6. if it is IPv6 then it needs to issue an EPSV command instead of a PASV command to initiate a passive data connection. how will this affect FTP to an IPv6 node over SOCKS? >Index: netwerk/base/src/nsSocketTransport2.h > PRBool mProxyTransparent; >+ PRBool mProxyTransparentResolvesHost; nit: these should probably use PRPackedBool instead of PRBool to reduce the size of the class a bitty bit ;-) >Index: netwerk/socket/base/nsISocketProvider.idl > void newSocket(in long aFamily, > in string aHost, > in long aPort, > in string aProxyHost, > in long aProxyPort, >+ in long aFlags, this interface also needs a new UUID. >Index: netwerk/socket/base/nsSOCKSIOLayer.cpp >+ // make sure the hostname will fit in the buffer we're using, >+ // taking into account the 7 other bytes we use. >+ if (host_len > sizeof(request) - 7) { >+ // Hostname was too big for buffer. >+ LOGERROR(("Hostname was too long to fit in the request buffer.")); >+ return NS_ERROR_INVALID_ARG; >+ } ouch.. so, is there nothing we can do to work around this problem? can the size of the SOCKS message be variable length? should we just malloc a buffer? >+ // make sure the hostname will fit in the buffer we're using, >+ // taking into account the other bytes we use. >+ if (host_len > sizeof(request) - request_len - 1) { >+ // Hostname was too big for buffer. >+ LOGERROR(("Hostname was too long to fit in the request buffer.")); >+ return NS_ERROR_INVALID_ARG; >+ } same issue here. >+ // the hostname is then added at the end of the request with NULL. >+ strncpy((char*) request+request_len, destHost.get(), host_len + 1); use PL_strncpyz instead to ensure null termination? strncpy does not promise to null terminate; however, since you have checked host_len ahead of time, this is not really an issue. so, feel free to ignore.

(In reply to comment #68) I will prepare a new patch to address the issues you've raised, but first, some quick responses to the 'policy' issues. > (From update of attachment 155243 [details] [diff] [review]) > >Index: modules/libpref/src/init/all.js > > >+pref("network.proxy.socks_remote_dns", true); > > so, this changes our default behavior right? is that really > a good idea? will we confuse users by enabling this pref by > default? what do IE and Opera do? This was discussed in comments #29, and #58-#60. Yes, it would be a behaviour change, but I don't think it would really be confusing. This is the default on some browsers, such as IE for Mac. It's a moot point on IE for Windows, which doesn't support SOCKS5. I cannot picture a sane scenario where changing this behavior would cause any issues on SOCKS5, or on a SOCKS4 server supporting 4a. The only issues arise with SOCKS4-only servers. Again, if you want the default to be disabled again, that's ok with me, but I'd prefer it to be enabled. > shouldn't this be conditional on the pref? or are you saying that > PAC will always do host resolution on the server? why enable for > SOCKS4? shouldn't this be for SOCKS5 only? See comment #62. If you want me to change this behaviour I'm happy to do that, but I'm really scared about the idea of a single .pac file generating different results depending on manual configuration. This seems like an administrative nightmare - and then there's the issue of UI design. > same question? shouldn't this be for SOCKS5 only? No - SOCKS4a supports this feature, and this patch supports SOCKS4a. Most SOCKS4 servers today do too. Things like MSProxy2.0 support 4a (loudly.) Most SOCKS4 servers should have this option enabled, it should only be disabled where the server doesn't support 4a. > also, i seem to recall that our FTP code needs to know the IP address > of the host it is talking to in order to know if the host is IPv4 or > IPv6. if it is IPv6 then it needs to issue an EPSV command instead of > a PASV command to initiate a passive data connection. how will this > affect FTP to an IPv6 node over SOCKS? I'll look into it, but my simple answer is I'd be really suprised if this patch modified the current behaviour here in any way. Out of interest, can't EPSV be used on IPv4...? > >+ // make sure the hostname will fit in the buffer we're using, > >+ // taking into account the 7 other bytes we use. > >+ if (host_len > sizeof(request) - 7) { > >+ // Hostname was too big for buffer. > >+ LOGERROR(("Hostname was too long to fit in the request buffer.")); > >+ return NS_ERROR_INVALID_ARG; > >+ } > > ouch.. so, is there nothing we can do to work around this problem? can the > size of the SOCKS message be variable length? should we just malloc a buffer? If you think it's important, of course we can. The main issue here was minimising the number of network sends. This is TCP, and there's no reason why it needs to be copied at all.

Comment on attachment 155243 [details] [diff] [review] strategic retreat >>Index: modules/libpref/src/init/all.js >>+pref("network.proxy.socks_remote_dns", true); >so, this changes our default behavior right? is that >really a good idea? will we confuse users by enabling >this pref by default? what do IE and Opera do? This patch retains the default of true. I will change this if it's absolutely required. I do consider it a good idea - it will fix DNS problems with SOCKS, subject only to problems of incomplete SOCKS implementations. It will only confuse if the SOCKS server is broken. And for the record: looking to things like IE for leadership on points like this is futile. They're not leading anymore, we are. >when changing the signature of an interface method, it >is imperative that you change the UUID of the interface. Fixed. >>Index: netwerk/base/public/nsIProxyInfo.idl >>+ [noscript, notxpcom] PRUint16 Flags(); >the same applies to this interface. i think flags >should be a PRUint32 since that is anyways the size >of a WORD on most platforms so making it be a 16-bit >integer isn't that useful. Fixed. >>Index: netwerk/base/src/nsProtocolProxyService.cpp >>+ // If it's a SOCKS proxy, do name resolution on the server side. >>+ if (type == kProxyType_SOCKS || type == kProxyType_SOCKS4) >>+ flags |= nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST; >shouldn't this be conditional on the pref? or are you >saying that PAC will always do host resolution on the >server? why enable for SOCKS4? shouldn't this be for >SOCKS5 only? As mentioned before, I don't think subjecting PACs to the pref is wise, but will do so if required. This code is PAC-only. And on reflection, you're right, enabling it for SOCKS4 is probably a mistake, so now it's always on for PAC with SOCKS5 and always off for PAC with SOCKS4. >> if (mSOCKSProxyVersion == 4) >> type = kProxyType_SOCKS4; >> else >> type = kProxyType_SOCKS; >> port = mSOCKSProxyPort; >>+ if (mSOCKSProxyRemoteDNS) >>+ proxyFlags |= nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST; >same question? shouldn't this be for SOCKS5 only? No - if the user wants to use SOCKS4a, they chose it, we support it. (aside: look at PuTTY for how this can work really well [Thanks Justin!]) >>Index: netwerk/base/src/nsSocketTransport2.cpp >>+ if (!mProxyHost.IsEmpty() && mProxyTransparentResolvesHost) { >>+ // Name resolution is done on the server side. Just pretend >>+ // client resolution is complete, this will get picked up later. >>+ // since we don't need to do DNS now, we bypass the resolving >>+ // step by initializing mNetAddr to an empty address, but we >>+ // must keep the port. The SOCKS IO layer will use the hostname >>+ // we send it when it's created, rather than the empty address >>+ // we send with the connect call. >>+ mState = STATE_RESOLVING; >>+ PR_SetNetAddr(PR_IpAddrAny, PR_AF_INET, SocketPort(), &mNetAddr); >>+ rv = PostEvent(MSG_DNS_LOOKUP_COMPLETE, NS_OK, nsnull); >>+ return rv; >nit: "return PostEvent(...);" Fixed. >>Index: netwerk/base/src/nsSocketTransport2.h >> PRBool mProxyTransparent; >>+ PRBool mProxyTransparentResolvesHost; >nit: these should probably use PRPackedBool instead of >PRBool to reduce the size of the class a bitty bit ;-) Fixed >>Index: netwerk/socket/base/nsISocketProvider.idl >> void newSocket(in long aFamily, >> in string aHost, >> in long aPort, >> in string aProxyHost, >> in long aProxyPort, >>+ in long aFlags, >this interface also needs a new UUID. Fixed >>Index: netwerk/socket/base/nsSOCKSIOLayer.cpp >>+ // make sure the hostname will fit in the buffer we're using, >>+ // taking into account the 7 other bytes we use. >>+ if (host_len > sizeof(request) - 7) { >>+ // Hostname was too big for buffer. >>+ LOGERROR(("Hostname was too long to fit in the request buffer.")); >>+ return NS_ERROR_INVALID_ARG; >>+ } >ouch.. so, is there nothing we can do to work around >this problem? can the size of the SOCKS message be >variable length? should we just malloc a buffer? My last statement here was premature. SOCKS5 uses a single byte to record the hostname length. It will not support a hostname of greater than 255 chars, we have to live with that. I used multiple sends to take it up to 255 chars without needing to copy or buffer, though. >>+ // make sure the hostname will fit in the buffer we're using, >>+ // taking into account the other bytes we use. >>+ if (host_len > sizeof(request) - request_len - 1) { >>+ // Hostname was too big for buffer. >>+ LOGERROR(("Hostname was too long to fit in the request buffer.")); >>+ return NS_ERROR_INVALID_ARG; >>+ } SOCKS4a uses NULL-terminated strings, so now with multiple sends, the sky's the limit. >>+ // the hostname is then added at the end of the request with NULL. >>+ strncpy((char*) request+request_len, destHost.get(), host_len + 1); >use PL_strncpyz instead to ensure null termination? >strncpy does not promise to null terminate; however, >since you have checked host_len ahead of time, this >is not really an issue. so, feel free to ignore. This code has been axed as part of the above.

> This patch retains the default of true. I will change ... > implementations. It will only confuse if the SOCKS > server is broken. OK, makes sense. Thanks for clarifying that. > And for the record: looking to things like IE for > leadership on points like this is futile. They're > not leading anymore, we are. Uhm... last I checked, our market share is minuscule next to that of IE. They have a SOCKS implementation, so it is prudent to consider their implementation when deciding on our default behavior. NOTE: It doesn't matter that IE is old... it is the market leader!!

Comment on attachment 155465 [details] [diff] [review] spot the deliberate mistake >it is imperative >that you change the UUID of the interface. I should not have missed that :( >> also, i seem to recall that our FTP code needs to know the IP address > I'd be really suprised if this patch modified the current behaviour here well, this patch certainly means that the FTP code does not know the IP Address of the host it is talking to... http://www.cse.ohio-state.edu/cgi-bin/rfc/rfc2428.html#sec-3 sounds like it can be used both for IPv6 and IPv4. - PRUint16 destPort = PR_htons(PR_NetAddrInetPort(addr)); + PRInt32 destPort = PR_ntohs(PR_NetAddrInetPort(addr)); hm? why this change?

(In reply to comment #72) <snip> > - PRUint16 destPort = PR_htons(PR_NetAddrInetPort(addr)); > + PRInt32 destPort = PR_ntohs(PR_NetAddrInetPort(addr)); > hm? why this change? Short answer: no idea. This code has been around since before I've been working on it, and I hadn't picked it up. I agree, it looks (at least superficially) as a bad idea, so I'll look into changing it. > NOTE: It doesn't matter that IE is old... it is the > market leader!! lol :) It may be prudent to consider what IE does when deciding defaults, but it can quickly come to a point where this holds us back in 2001 where they are. Imagine if SOCKS6 were released tomorrow: would we not support it? Or not support any of the new features because IE doesn't? Would we default to using SOCKS4? SOCKS5 provides us with new stuff like a well designed and implemented method for name resolution, as well as authentication (will look at this after this patch.) IMO, it's silly not to take advantage of these things because IE is still SOCKS4-only. In terms of PAC, I agree with you, because it's important that one PAC file works with all browsers. In terms of manual defaults, going with sane/sensible defaults seems preferable to going with IE's defaults. In most cases, IE's will be sane and sensible, but not always: server-side name resolution would be a bad idea if you're implementing SOCKS4, when it might not be there. It is sane and sensible on SOCKS5, where server support is a core requirement. IE recognised this when they implemented SOCKS5 on Mac.

Is it possible to move the PAC implementation discussion to a new bug, or is it inseperable? If no, my answer to the protocol extension is "no. please, no." Darin already extended the SOCKS directive once by adding "SOCKS4 and SOCKS5". We never tested other browsers to understand what they did w/ the SOCKS directive, and we never tested to see if they handled PAC files w/ SOCKS4 and SOCKS5 correctly. As for the manual proxy behavior, the short version is: SOCKS4 did not support Proxy-side DNS, SOCKS5 does, and should. Network administrators deploy SOCKS4 or SOCKS5 proxies, they understand this limitation, so we just need to implement this and document our protocol support clearly.

(In reply to comment #74) > Is it possible to move the PAC implementation discussion to a new bug, or is it > inseperable? If no, my answer to the protocol extension is "no. please, no." This has been seperated in the latest version of the patch for the reasons you describe. I haven't opened a new bug for it, because at the moment, it's a hypothetical bug - it only exists once this is committed.

So, if I were want to risk my life by asking about taking this for Fx 1.0RC1, what _technical_ work would need to be done? SOCKS5 is growing in use, and I'm starting to hear grumbling from a lot of deployment-corners about this.

Comment on attachment 155465 [details] [diff] [review] spot the deliberate mistake >Index: directory/xpcom/base/src/nsLDAPSecurityGlue.cpp > rv = tlsSocketProvider->AddToSocket(PR_AF_INET, > sessionClosure->hostname, defport, >+ nsnull, 0, 0, socketInfo.soinfo_prfd, > getter_AddRefs(securityInfo)); do me a favor and fix the indentation here so the params line up vertically. thx! >Index: extensions/help/resources/locale/en-US/cs_nav_prefs_advanced.xhtml >Index: extensions/help/resources/locale/en-US/nav_help.xhtml Do these need some sort of review or approval from the help system module owner? (I don't know who that is or even if there is such a person.) >Index: netwerk/base/public/nsIProxyInfo.idl >+ const unsigned short TRANSPARENT_PROXY_RESOLVES_HOST = 1 << 0; I think this flag deserves a comment explaining what it does. Though the name of the flag is pretty self-explanatory, a comment is always a good idea ;-) >Index: netwerk/base/src/nsProtocolProxyService.cpp >+ , mSOCKSProxyRemoteDNS(PR_FALSE) In prefs, this defaults to true. I think we should make this setting consistent with the value in all.js. > if (mSOCKSProxyVersion == 4) > type = kProxyType_SOCKS4; > else > type = kProxyType_SOCKS; > port = mSOCKSProxyPort; >+ if (mSOCKSProxyRemoteDNS) >+ proxyFlags |= nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST; So, I'm confused. It seems to me that we want SOCKS v5 to default to remote DNS, but we want SOCKS v4 to default to local DNS. Is that not true? If so, then doesn't this code cause problems for SOCKS v4? The default for remote DNS is true, so wouldn't that cause existing SOCKS v4 users to have to flip the pref to false in order to continue browsing the web? Perhaps we need to default remote DNS to false unless and until the user enables SOCKS v5 in the pref panel. In other words, we'd suggest remote DNS in the pref panel when SOCKS v5 is selected, but otherwise we'd default to false. Existing installations would then be unaffected by this patch. >Index: netwerk/base/src/nsSocketTransport2.cpp >+ if (mProxyTransparentResolvesHost && >+ ((strcmp(mTypes[i], "socks") == 0) || >+ (strcmp(mTypes[i], "socks4") == 0))) >+ proxyFlags |= nsISocketProvider::PROXY_RESOLVES_HOST; If mProxyTransparentResolvesHost is true, then perhaps there is no reason to check for "socks" or "socks4". You already did that check earlier when you assigned mProxyTransparentResolveHost, right? Also, I would call the parameter |sockFlags| or just |flags| instead of |proxyFlags| since nsISocketProvider does not refer to them as proxy specific flags. >Index: netwerk/socket/base/nsISocketProvider.idl >+ const long PROXY_RESOLVES_HOST = 1 << 0; please document this flag. >Index: netwerk/socket/base/nsSOCKSIOLayer.cpp >+ void Init(PRInt32 version, const char *proxyHost, PRInt32 proxyPort, >+ const char *destinationHost, PRUint32 flags); nit: make parameter list line up under the opening paranthesis. thx! >+ LOGDEBUG(("we are sending the server the hostname rather than IP\n")); technically, we might be sending an IP, right? perhaps this should say "rather than resolving it first" or something like that. >Index: xpfe/components/prefwindow/resources/content/pref-proxies.xul >+ <row> >+ <spacer/> >+ <checkbox id="networkProxySOCKSRemoteDNS" >+ label="&socksRemoteDNS.label;" >+ accesskey="&socksRemoteDNS.accesskey;" >+ prefstring="network.proxy.socks_remote_dns"/> >+ </row> This needs to be reviewed / approved by the Mozilla Seamonkey UI owner, which is probably Neil Rashbrook (last I heard). Also, what about Firefox pref UI? This patch doesn't include changes to the pref UI for Firefox. If this is to be considered for Firefox 1.0, then it seems that UI for it would need to be created. See: http://lxr.mozilla.org/mozilla/find?string=pref-connection.xul

cc'ing RJK and Neil for review of help system changes (Neil cc'd also for Seamonkey pref UI changes). thanks!

Regarding both help and pref UI changes, Stefan Borggraefe is trying to simplify the proxy panel so that he can add UI for WPAD support, which is bug 257758. It would probably be a good idea if we rolled this UI change into that patch.

Comment on attachment 155465 [details] [diff] [review] spot the deliberate mistake I had meant to minus this yesterday.

>+ const unsigned short TRANSPARENT_PROXY_RESOLVES_HOST = 1 << 0; Ignoring for a moment that Darin will tell you, if you pause for breath, that there is no such thing as a transparent proxy, SOCKS5 isn't transparent at all, is it?

> >+ const unsigned short TRANSPARENT_PROXY_RESOLVES_HOST = 1 << 0; > > Ignoring for a moment that Darin will tell you, if you pause for breath, that > there is no such thing as a transparent proxy, SOCKS5 isn't transparent at all, > is it? It is not a transparent proxy per the definition in RFC 2616 (section 1.3), but it is a "transparent proxy" as far as Necko is concerned. From Necko's point of view, a proxy is transparent if it does not require modifications to higher level protocols. SOCKS is such a layer in the network stack. E.g., the HTTP protocol need not be aware of the SOCKS protocol. The reality is that "transparent proxy" is a horribly overloaded term :-/ Our APIs speak of proxies, and proxy transparency in the sense that I have described, so this patch is consistent at least with other Necko APIs.

(In reply to comment #79) > Regarding both help and pref UI changes, Stefan Borggraefe is trying to simplify > the proxy panel so that he can add UI for WPAD support, which is bug 257758. It > would probably be a good idea if we rolled this UI change into that patch. I agree this would be a good idea. I could adopt the UI changes for Seamonkey from attachment 155465 [details] [diff] [review] and the Help file changes and integrate them into my patch for bug 257758 if you want. This would probably avoid some CVS conflicts. Also I wanted to ask Neil for review anyway, so this part of your patch will end up in the review queue of the Seamonkey front-end module owner (and Neil is a Help peer, too). Note however that bug 257758 does not deal with Firefox. So this is a different story.

ok, I've fixed most of the simple coding issues that Darin points out, but I'm a little confused as to where to proceed to from here. If the UI stuff should be removed from here and integrated into another patch, that's fine by me, but there are a few things to think about. This patch updates the Firefox UI (untested - somebody who builds this please have a look for me). What should happen to that? Also, unless the default issues are sorted, having this patch committed without a UI would be likely fatal. It would be nice to have a different default for SOCKS4 and SOCKS5. But doing so involves more issues: should changing from v4 to v5 (or vice-versa) modify the value automatically? Won't this be confusing for the user also? Or should there be two stored prefs, one for server-side DNS lookups in SOCKS4 and one for SOCKS5? Should Thunderbird have its UI updated too? SOCKS is probably more intensively used in the mail world than the web world. But as above, where should this happen? I'm really concerned that this patch will get bogged down trying to make it consensus-perfect; different people have different ideas, see different issues, and waiting for complete agreement may delay this patch for years. The function of this code had been reviewed and superreviewed previously, and the changes being made reflect different design ideas since then. Although perfection is a laudable aim, this is still a fix for a bug, and we do need to think which option is preferable: have something committed that can be improved (and the bug fixed in 1.8), or leave the bug around for one or two more releases aiming for a "better" fix.

Malcolm, I hear your concerns. I think we should try to separate the UI work from the backend work as much as possible precisely because each app may wish to do things slightly differently. How about this: If the pref is unassigned, assume remote dns for SOCKS v5 but not v4. If the pref is assigned, then use the value for both SOCKS v5 and v4. If you make the backend behave like this, then the backend is flexible enough, and we'd get a suitable default behavior. In parallel, or once the backend changes are done, we can go and engage the UI owners for the various products: Neil Rashbrook for SeaMonkey, Ben Goodger for Firefox, Scott Macgregor for Thunderbird, Daniel Glazman for Nvu, etc. Does this sound reasonable to you?

My pref suggestions requires of course that the pref is not given a default value in all.js.

(In reply to comment #86) >My pref suggestions requires of course that the pref is not given a default >value in all.js. The pref window won't leave the pref undefined, so we'd need to check on panel load to see if the pref was undefined and if so set it to whatever the default for the given SOCKS type is.

(In reply to comment #87) > The pref window won't leave the pref undefined, so we'd need to check on panel > load to see if the pref was undefined and if so set it to whatever the default > for the given SOCKS type is. Would that really work in practice? If the user has never been into prefs before, it will always default to the same value (if SOCKS5 is default, for instance, this pref will always default to true.) If the user changes from SOCKS5 to SOCKS4, this pref would remain true? And having been into prefs, a value would be saved, so the default would never occur again? Or are you suggesting one of those silly 3-state checkboxes (in effect)...so it also remembers a value of 'undefined'...and won't we need some way to remember this value after the user accepts the prefs dialog?

here's how i see it: once someone loads up the pref panel, it is fine to fix the value to true or false. why? well, if they then change the SOCKS proxy to v4, they can likewise change the remote DNS option at that time. i'm not sure there is much reason from a UI point of view to ever need to restore the unset mode. we'd only need to support that mode for compatibility / migration purposes really. does this make sense?

(In reply to comment #89) >here's how i see it: once someone loads up the pref panel, it is fine to fix >the value to true or false. OK, as long as we check the panel when it loads loads to see if it was undefined, so that it will get fixed with the correct setting.

It might be late in the game, but I'd still like to see separate bugs that explain what we are doing for PAC and the UI. I'm really confused as to the current state of the implementation.

doesn't look like this is going to ba baked enough for 1.0. renominate if a well tested patch appears.

Is anyone still working on fixing this? I tried setting SOCKS5_FAKEALLHOSTS=1 in the environment (per http://www.socks.permeo.com/TechnicalResources/DevelopDocuments/SOCKSifyingClientAp0CE5/RunsockManPage.asp) But that does not appear to be implemented either. This Mozilla "patch kit": http://yallara.cs.rmit.edu.au/~malsmith/products/mozkit/ Offers two interesting features: 1. Allow setting of an option in the PAC to indicate client or server side DNS resolution with SOCKS5 i.e. socks5 192.168.0.1:1080[serverdns] 2. A preference file setting: pref("network.proxy.socks_remote_dns", true); It does not deal with the GUI issue. However, simply implementing these back-end features would be a big help.

(In reply to comment #93) > Is anyone still working on fixing this? I've recently accepted a job with Microsoft and won't be able to work on this patch. > > I tried setting SOCKS5_FAKEALLHOSTS=1 in the environment > (per > http://www.socks.permeo.com/TechnicalResources/DevelopDocuments/SOCKSifyingClientAp0CE5/RunsockManPage.asp) > But that does not appear to be implemented either. That is only relevant if you're running Mozilla under their SOCKS library, using 'runsocks' on *nix, and (possibly) SOCKSCap on Win32. > > This Mozilla "patch kit": > > http://yallara.cs.rmit.edu.au/~malsmith/products/mozkit/ > > Offers two interesting features: > > 1. Allow setting of an option in the PAC to indicate client or server side DNS > resolution with SOCKS5 > > i.e. socks5 192.168.0.1:1080[serverdns] > > 2. A preference file setting: > > pref("network.proxy.socks_remote_dns", true); > > > It does not deal with the GUI issue. However, simply implementing these back-end > features would be a big help. That patch is an old version of what's attached to this bug; it's the same patch :) - although the UI should be implemented on all versions of this patch back to time immemorial (check out the Proxy section in Prefs.) - M

Unless someone else volunteers, I'll work on completing the backend changes for this patch. Please volunteer, I'm swamped! ;-)

*** Bug 215386 has been marked as a duplicate of this bug. ***

I went ahead and landed the backend changes from the latest patch (attachment 158939 [details] [diff] [review]) with the remote dns pref disabled by default. I think it is better to get the backend changes in place, so that people can test them out instead of holding up that part of this patch for the UI changes. I settled on disabling the remote dns pref by default because 1) that would change the behavior of existing setups, and 2) I found a pretty serious bug where failed DNS lookups result in no error being reported to the user. Both of those problems need to be resolved in a clean way before I'll agree to enabled the remote dns pref by default. This bug can stay open I suppose for the remaining work. Again, I don't expect to have any time to work on what's left. I just didn't want the backend work to bit-rot.

pushing out... i don't have cycles to work on pref ui for this. if anyone is interested in working on this bug, please let me know.

This patch fixes a number of SOCKS error issues. One nasty non-error condition is fixed by that '!mProxyTransparent' thing, which is a bug regardless of the backend patch that was committed. The rest are hacks to ensure that errors properly relate to either the proxy or the remote host. I don't like the messiness here. It'd be nice to just send PR_SetError an NS_ERROR_*, and collect this in nsSocketTransport2.cpp, but that doesn't appear to be how it's designed to work. On the other hand, this 'layered' model isn't designed to care about where a connection failure occurred, and with SOCKS errors, this matters.

Updated to be a bit more current, and remove the accesskey which now clashes with the "Set all" button. No character in this UI had an available accesskey, so I left it off. As I understand it, this pref panel is in for a big redesign anyway, so this patch should only be considered temporary for those wishing to use this feature.

This is seperated because it's non-critical, and the irredemably obscure help indentation seems to change from one madness to another every five minutes, so the UI patch should apply cleanly for a while even if this won't.

*** Bug 275807 has been marked as a duplicate of this bug. ***

the current patch for bug 257758 adds UI for this pref to seamonkey, adding dependency

*** Bug 279520 has been marked as a duplicate of this bug. ***

bug 257758 added the UI for this. anything left to do here?

Did that UI bug also add UI for Firefox? I'm using the latest nightly (01/23/2004) and I don't see an option to specify remote DNS lookups except in about:config.

ah, yeah. it didn't.

(In reply to comment #107) > ah, yeah. it didn't. Can we make this blocking-aviary1.1? As long as the functionality is in the Core and Mozilla Suite, it ought to be in firefox too. It's important to corporates who have firewalls without internal DNS servers and a few other special purposes that would set Firefox apart. I don't want this to get passed over for 1.1 when it's just a matter of adding a checkbox in the prefs.

Asking for blocking-aviary1.1, as Daniel Guido could have asked.

Darin, Does Malcolm's new error condition patch resolve the fault you detected?

> Does Malcolm's new error condition patch resolve the fault you detected? I don't know. I haven't had any free cycles to get back to this bug. Help wanted!

*** Bug 284371 has been marked as a duplicate of this bug. ***

Since I really need this feature (only way to make DNS-lookup on my net is via SOCKS5 or 4a): Is there currently a build/nightly that actually adds DNS-support for Socks? Or will I have to build my own firefox with the patches attached to this bug?

oh yes, nightly trunk builds of firefox support it. there is no pref UI though, afaik.

OK, thanks. Right now it doesn't work for me, but I'm not really sure if that's actually firefox' mistake. Did anybody get it working?

Nevermind, it was my mistake. DNS over Socks 5 works just fine. Thanks. :)

*** Bug 295148 has been marked as a duplicate of this bug. ***

It appears socks_remote_dns in about:config has been removed entirely as of 1.0.5. It would be nice if this actually worked again...

This is AFAIK only fixed on the trunk and not in the old Firefox 1.0.X branch

(In reply to comment #99) > Created an attachment (id=169178) [edit] > SOCKS error conditions fixes FWIW, this patch sounds like it fixes bug 241479

Comment on attachment 169178 [details] [diff] [review] SOCKS error conditions fixes @@ -1354,17 +1355,17 @@ nsSocketTransport::OnSocketEvent(PRUint3 - if ((status == NS_ERROR_UNKNOWN_HOST) && !mProxyHost.IsEmpty()) + if ((status == NS_ERROR_UNKNOWN_HOST) && !mProxyHost.IsEmpty() && !mProxyTransparent) Why this change? When is this event posted with an error status, when SOCKS is used? Why not store the OS error code in an nsresult variable and only cast once? some of the error codes in SOCKSIOLayer are really kinda wrong... imo you shouldn't map all kinds of stuff to connection refused.

Anything up with this? See also related bug #306819. This is a serious securit y bug (privacy leak), since it means the client leaks DNS requests when the "adv ertised" functionality is to route all traffic through the proxy. Example: you are a Chinese user behind the Great Firewall. You set up a SOCKS5 proxy through an ssh tunnel (not a big problem all by itself) to someplace outsi de the firewall, and start browsing Wikipedia through it. Your web traffic is a ll encrypted but the hostnames that you view all get leaked to the Chinese gover nment. Pretty soon, troops beat down your door and haul you to prison. Another example: you are at home browsing some evil organization's site through a proxy. When you visit www.evilorganization.com, the HTTP hits come from the proxy but the DNS hits come from your home. They use the DNS log to track you down and beat you up. I don't see why there needs to be a user preference. The browser (when using SO CKS5) should always let the proxy handle DNS, and never do it on the client. Wi th SOCKS4, the proxy configuration screen should clearly warn the user about thi s privacy leak.

I've got a PAC file that works fine for sending traffic to a SOCKS proxy by comparing URL patterns. For example: else if (shExpMatch(url, "http://site1.domain.com*")) return "SOCKS localhost:6001"; On Mac OS X, I configure an automatic proxy configuration file and everything sent to site1.domain.com is sent to the proxy, while everything else isn't. Safari uses this setting perfectly. This doesn't work under Firefox. I've seen in Bugzilla that there may be work on getting Firefox to use the System Preferences setting, but I was able to get it to work in Firefox with a few changes just the way it is. Here are the changes I needed: 1. Configure PAC file in Firefox preferences. 2. Open about:config and set network.proxy.socks_remote_dns to true. I need this because the server side of my SSH tunnel uses a permitopen=site1.domain.com key to restrict access for each user. Firefox needs to send the domain before it's resolved. 3. Change the PAC file to specify SOCKS5 instead of just SOCKS. Otherwise it appears that the domain is still resolved locally as sending domains is only part of SOCKS v5 protocol and I guess the default is NOT to use SOCKS v5 if "SOCKS" is used. I've also seen in Bugzilla for Firefox that there's been a large discussion on this topic, but it's hard to tell where it was left. The problem is that Mac OS X and Safari don't work if I specify "SOCKS5" for the return value in the PAC file. This leaves me with the need to have 2 different PAC files based on the browser used. This is of course far from ideal. Maybe there is some approach that preserves browser compatibility, but I haven't found it. I would like to see Firefox come up with a solution that preserves the functionality I need without breaking compatibility.

Could you not test the user agent in the PAC file, and use SOCKS or SOCKS5 depending on what you found? Though if Safari can get away with that behaviour too, maybe we should do the same in FF3...

From what I read, I am very limited in the functions I can call from a PAC file. The listing of functions in the PAC spec does not include anything I can use to detect the user agent (browser). They seemed pretty explicit that you can't do everything JavaScript can do. If you know otherwise, I would be more than happy to alter my PAC file so I can have one version. Again, it's very hard to tell from this bug thread what things have been decided (at least to me), but it sure would be nice if the PAC file defaulted to SOCKS v5 when using just "SOCKS" as the return value. I know you have many compatiblity problems to tackle with anything like this.

Hi, actually this functionality (servers-side DNS resolving for SOCKS5) is something that would also help a lot if you connect to servers that have IPv6 addresses in DNS. Right now, when Firefox is set to use a SOCKS proxy, and the target server has IPv4 and IPv6, it will do *nothing* - it seems to decide internally "I want to do IPv6, I cannot do that over SOCKS, so I don't do anything". It doesn't fall back to IPv4 either. This can be worked around by turning off IPv6 in the about:config - but turning off IPv6 is a workaround I really dislike. As my laptop travels quite a lot between IPv6-enabled networks and networks where I need to use SOCKS (and connect to servers that have IPv6), turning on and off IPv6 all the time is a real nuisance. Example server: http://www.space.net/

Since Comment 122, this bug has started to drift. From what I can tell, this stuff is working in core right? (I'll worry about UI later, it looks like the UI never took the flag). If so, can we resolve/fixed?

Yes, but I'd suggest providing the bug # for the UI first, so everyone can move their votes.

I would file a new bug, but I don't even know what the expected behavior is, so I don't know if we need a UI bug. If it is: Not used for SOCKS4, settable for only SOCKS5, and not used in PAC for SOCKS, then I don't think we need a UI pref.

With network.proxy.socks_remote_dns set to true (about:config in Firefox, UI in SeaMonkey) names are resolved on the remote (SOCKS5) host. Haven't tried with SOCKS4. Mind you, due to DNS prefetch (bug 453403) you'll still see some DNS traffic locally (see bug 488162), but in principle this works (internal names that aren't externally visible resolve just fine for me). I think we can mark this bug fixed. File a new bug on adding pref UI to Firefox or enabling remote DNS by default when SOCKS5 is used. If the mixed IPv4/IPv6 from comment 126 is still an issue, please file a follow-up bug on that as well.

Mac OSX 10.5.8 & Firefox 3.5.8 when with network.proxy.socks_remote_dns set to true: only "Manual proxy configuration" works . It not works with "Automatic proxy configuration URL" and "Use system's proxy configuration". I use "tcpdump -X -s0 -i ppp0 port domain" to test. when with "Automatic proxy configuration URL", the dns lookup is via nornal network, not via socks5. Here you can find my pac file: http://nehcfrus.googlecode.com/svn/trunk/network/surfchen.pac FYI: The system's proxy configuration is pac-based too.

(In reply to comment #131) > when with network.proxy.socks_remote_dns set to true: > > only "Manual proxy configuration" works . It not works with "Automatic proxy > configuration URL" and "Use system's proxy configuration". There is no guarantee the PAC file returns a SOCKS proxy server.

(In reply to comment #132) > (In reply to comment #131) > > when with network.proxy.socks_remote_dns set to true: > > > > only "Manual proxy configuration" works . It not works with "Automatic proxy > > configuration URL" and "Use system's proxy configuration". > > There is no guarantee the PAC file returns a SOCKS proxy server. Yes. But the PAC file is written by myself and the URL I used to test is the URL that will let PAC return SOCKS server. I event tested with this PAC configuration: function FindProxyForURL(url, host) { return "SOCKS 127.0.0.1:9060"; }

I confirm this. I realized this just yesterday that DNS quries are not being sent over proxy connection in case of proxy.pac file. Here is my proxy.pac function FindProxyForURL(url, host) { if(isInNet(host, "192.168.*.*", "255.255.0.0")) { return "DIRECT"; } else if (host == "localhost") { return "DIRECT"; } else { return "PROXY localhost:9000"; } }

(In reply to comment #134) > return "PROXY localhost:9000"; > } > } HTTP(S) proxies don't support remote DNS lookups; only SOCKS 4a and 5.

(In reply to comment #135) > (In reply to comment #134) > > return "PROXY localhost:9000"; > > } > > } > > HTTP(S) proxies don't support remote DNS lookups; only SOCKS 4a and 5. Well I expected it to work because DNS queries over HTTP proxy works if I do it manually.

So why is this bug still open?

(In reply to Michael Kaply (mkaply) from comment #138) > So why is this bug still open? The last official word was Darin in comment #97. I think this boils down to: 1. The UI changes for Firefox never happened (although SeaMonkey has UI.) 2. The error handling for SOCKS is (was?) busted. If a connection attempt is made via SOCKS, there was no way to disambiguate connection errors to the proxy vs. connection errors to the final host. I don't remember where the code settled on this - Darin was concerned about empty pages being returned to the user without meaningful errors, but I wanted this to go further and provide meaningful errors. It's possible that this has all changed in the last 7 years. 3. PAC handling is just odd. I don't know what happened here either, but a PAC can filter by name or IP, which doesn't work when the client has no idea what the IP is going to be. (How does this work for HTTP proxies?) 4. The default issue. It makes sense (to me at least) that this be enabled by default for SOCKS5 since the protocol requires support for it. Even the most common remaining SOCKS4 servers like MS Proxy server/ISA server/TMG server support 4a. There is obviously a compatibility risk with changing any default though, and the default before this change happened was client-side name resolution, since that was all that existed. That said, it would be logical to me to close this bug and open any new, more specific bugs for completeness of this feature.

5. There's also no way to control this setting via a PAC file, which would be strongly desirable. Without this, a PAC file will do different things depending on a client-specific setting, and if a site requires a particular setting they need to push down custom config, which defeats the purpose of having a PAC file. I investigated how to make this work, but it would need some changes to make this happen in a backwards compatible (and cross browser compatible) way.

Darin has long since been off the project. Hopefully some networking person will pick this up.

Has this been resolved for FF 11 yet?

More than ten years and counting... amazing :-) Please, vote for this bug to be fixed.

Comment #131 reproductable here too :-(.

(In reply to Gert Doering from comment #126) - I can confirm that the bug for proxy servers with both IPv4 and IPv6 still exists in Firefox 15.0.1 No dns queries are made.

Still exists in 18.0 and 19.0a2.

Ping...

still reproducible with Firefox 24.0 on Ubuntu 13.04 (x86_64).

I see a "Remote DNS" option for Socks 5 in firefox preferences (Firefox 30.0, Ubuntu) Does this mean this issue is fixed?

Works when I setup a Manual configuration, but does not work via PAC file. Firefox 35.0.1 on MacOS X 10.9.5

I can confirm that on FF 46.0.1 thi sis still a bug. I checked the "Remote DNS" checkbox and set SOCKS5 and the DNS requests are still resolve locally, rather than through the proxy (inspected with Wireshark). This is now 14 years old. There is another bug open for SOCKS5 to allow password auth which is also 10+ years old. Rather than adding minor changes, invisible to most users, why not fix such problems? Currently FF is not SOCKS5 compliant ...

We already added a pref UI (bug 665319).

I've tested on: - Firefox 47.0.1, 48.0.2 on Windows 10 x64 - Firefox 51.0a1 on OS X El Capitan Both works well (inspected w/ Wireshark), the DNS records are not resolved locally if the "Remote DNS" option is checked. The patch has landed ([1], comment 152). I think this works and should be closed. [1] https://github.com/mozilla/gecko-dev/commit/d19eff04b7231e6d4c7f0a95d03862bf1f708c40

OK folks: at this point we're unable to reproduce this bug locally. If you are still experiencing this issue please let us know. And if you can, also send us an HTTP log (it looks to me like our DNS code may need some logging annotations added to it to be fully useful for tracking this down, but even the existing logging would be a lot better than nothing): https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging#Turning_off_DNS_query_logging Thanks for looking into this Gary! Let's keep this bug open for a while to see if anyone replies.

Ignore the #Turning_off_DNS_query_logging part of the URI in the last comment (don't disable DNS logging!).

Will Thunderbird get a pref UI too?

It's almost a year after comment 154, I think it's time to close this.