42.52 KB, image/jpeg
36.77 KB, image/jpeg
10.93 KB, patch
|Details | Diff | Splinter Review|
10.84 KB, patch
|Details | Diff | Splinter Review|
I've implemented SOCKS 4 (see http://bugzilla.mozilla.org/show_bug.cgi?id=65583). The preferences dialog currently allows for SOCKS server name. It also needs a way to select SOCKS 4 or SOCKS 5. The value of the selection is network.proxy.socks_version, which should be "4" or "5". One way to implement the UI is to change the label of the edit control back to "SOCKS Host" (from the newly changed "SOCKS v5 Host") and add a radio button for the SOCKS version. Additionally, a suggestion was made to create a UI similar to the AIM client, which has one edit control for the proxy server name, and radio buttons for the protocol (something like: SOCKS 4, SOCKS 5, HTTP, and HTTPS.)
I am taking ownership and adding nsenterprise keyword.
Assignee: sgehani → smeredith
I could have sworn this was filed somewhere already, but maybe that's just the same issue for PAC, etc.
Steve, I suggest that the first time a connection is made to a particular SOCKS host, it should send a version identifier/method selection message assuming that it is SOCKS v5 (VER = X'05'). If it works, store that information in the prefs. If it doesn't work, try again with SOCKS v4. If that works, store *that* information in the prefs. Then in any future session, if your currently set version doesn't work, retry with the other version before failing. That way, you don't have to bother/rely on the user knowing which particular version a particular SOCKS host is, you don't have people's Mozilla configuration breaking when somebody upgrades/downgrades their SOCKS proxy, and you don't need these radio buttons at all. Which is just as well, since you don't have room to display them in the prefs panel anyway -- already the bottom of the Automatic proxy configuration URL field and button are being clipped in the Mac Classic theme, even without the extra row for SOCKS version. (sairuh, is there a bug on that already?) If you can't do that, please comment in this bug and we'll go to plan B.
We can't. The socks protocol versions are not backwards compatible. I don't think theres even an 'incorrect version' error result, although I may be wrong.
The SOCKS server will send a rejection reply, then drop your TCP connection. That's a bit of a technical problem because of current implementation of the SOCKS layer. We considered trying this approach, but decided that there might be a compelling reason for a user to select one or the other. However, I don't think we came up with an example of such a reason.
Status: NEW → ASSIGNED
I do insecure wingate socks5/4 checking on my irc network.... the design for my bot is very simple. On connect, the host is scanned for wingate (which i wont discuss) and socks ports. on port 1080, the binary packet "5 1 0" is sent to the host (assuming the connection is open.) If the host replies "5 0" then it is known to be socks 5! On replies "5 255" or "0 91 0 0 0 0 0 0" the host is closed and reopened for smart fallback to socks 4 scans. Once open again, the following binary packet is sent: "4 1 27 88 64 112 171 241" which will either reply "0 90 0 0 0 0 0 0" on sucess or "0 91 0 0 0 0 0 0" on failure. 4 1 27 (hi port) 88 (lowport, 27*256+88=7000) 188.8.131.52 Maybe this will help out on the decision for the browser... let it decide incase the user is unsure on what to pick. Just thought I would throw my 2 cents in!
There was a discussion about version scanning in another SOCKS bug. I think this is a horrible idea because some SOCKS servers log errors on failed connection attempts. Using a auto-handshake would potentially fill up a server with tons of spurious requests.
Another thing to think about: to fully support SOCKS 5, we need to be able to specify a user name and password to authenticate to the SOCKS 5 server. We currently only support unauthenticated connections. The hardest part about adding username and password support is the UI. So far, this enhancement hasn't been requested, but it is required by the SOCKS 5 spec.
NIM does this, so maybe we could poach some XUL from the commercial build...
What does 4.x do to determine version? In the dialog it just asks for socks host in 4.x and not ther version.
A comment on the attached screenshot that I made on IRC the other day to Bradley Baetz: Before checking in any more content to this preference panel please ensure that the panel content fits on Mac in BOTH skins. This is a non-optional requirement.
4.x only supports SOCKS 4. What about the help text in the dialog? Is it usefull, or can we sacrafice it?
What "help" text?
Ok, it appears that plan A is unworkable if we don't want to spam SOCKS 4 servers with error messages. Narf. So, plan B. Plan B has three parts. Firstly, get rid of the useless group box. (Since it encloses everything, what exactly is it grouping the items *from*?) Secondly, get rid of the help text. (Giving something like this help text only misleads people into thinking that it's a good idea for them to try to use it when they haven't been told to -- and if they need to use it, they'll know what to put in each field anyway.) And thirdly, put the SOCKS row in alphabetical order above the SSL row. That gives us a panel which looks like this: Proxies ::::::::::::::::::::::::::::::::::::::::::: ( ) _Direct connection to the Internet (*) _Manual proxy configuration _FTP: [ ] _Port: [ ] _Gopher: [ ] Port: [ ] _HTTP: [ ] P_ort: [ ] SO_CKS: [ ] Por_t: [ ] ( ) SOCKS v_4 (*) SOCKS v_5 _SSL: [ ] Po_rt: [ ] _No Proxy for: [ ] Example: .mozilla.org, .net.nz ( ) _Automatic proxy configuration _URL: [ ] ( _Reload ) ::::::::::::::::::::::::::::::::::::::::::::::::::: Note the alignment: all the protocol `:'s are aligned with the `:' in `URL:', and the SOCKS v4 radio button is aligned with the left of the SOCKS host field. While you're there, please change the no-proxy-for example as shown above: currently in Mac Classic it's clipped as `.yourcompany.com, .yourcompany.c...', which isn't very helpful. BTW Bradley, when are you going to get around to fixing that `_P_o_r_t:' business? :-)
bbaetz had thoughts about the order we should use the prefs you enter (if you have http and socks proxies, which is used first?). I argued for strict 4xp compatability, b/c these are migrated prefs. We need to solve that issue next, because it might be better to move SOCKS to the bottom, and put a note indicating what order of precedence is used.
mpt: when you file that bug about rearranging this dialog entirely? :) benc: One change at a time... Guys, this is a simple patch. Lets not turn this bug into a "redo the proxy pref pane" bug.
Well, the new stuff has to fit, so there's going to be at least some rearranging.
changed summary for searchability. bbaetz: by "one" change, I assume that you mean we should stick to discussing the layout of the panel. mtp: how about putting the order of elements as: http, ssl, sockks? If SOCKS is used last, then it should be last...
Summary: Prefs page for proxies needs to allow SOCKS 4 or v5 selection. → SOCKS: add version selector to prefs
umm mpt, we need the SOCKS v4 and SOCKS v5 options for PAC as well, can that be fixed? Or should we revisit it later when we actually support it? ;)
In the spirit of PAC, it makes the most sense for the PAC file to specify the version. Let's continue that discussion in bug 78176.
So I've made the changes to the window layout as discussed, except that I left the group box around the controls because all the other pref windows have group boxes, and when I tried it without, it looks inconsistent. It looks good in modern and classic on Windows and Linux. It looks good on classic Mac, but I haven't tried it on modern yet because mozilla crashes if I press the Apply Theme button. :-(
One last question: All other label say "Proxy" but SOCKS says "Host". Shouldn't it also say "Proxy?"
You've swapped the access keys on the port of socks and ssl (_r_ vs _t_). mpt was going to redesign this back in March, but I never saw the ASCII art :)
No, I swapped the order of SOCKS and SSL per mpt. The access keys remain attached to their original items.
When can we expect to see this in a build? The lack of Socks4 support means than no-one at a large, 3 letters-in-it's-name, company can use the program, since the firewall here is Socks v4.
kad: socks support is in the trunk build (see bug 65583). Theres just no UI for the version pref - you have to set it manually.
Well, there is a XUL bug 92358 blocking this. I could code a workaround into the .js, but I'd rather not. Then I'll just need some reviews, and I'll check it in. The actual SOCKS 4 support was checked in the other day: bug 65583. You can add user_pref("network.proxy.socks_version", 4); to your prefs.js to use it.
Keywords: patch, review
Created attachment 43678 [details] [diff] [review] Proposed patch for previously attached screenshot.
My only comment on the patch is that the <spring/> tag has been deprecated, and you should now use <spacer/> instead. Other than that, r/sr=hewitt
socks is different from other proxies (techinically, its really a tunnel). It should be last. And the port access keys should be in order, I guess. r=bbaetz with the first change, and with or without the second.
Checked in the fix.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
reassigning QA contact to myself
QA Contact: benc → trix
changing QA contact to email@example.com
QA Contact: trix → lrg
This preference is controllable and set properly by the UI currently. Changes in the pref value manually, are accurately displayed in the UI. Verified on MacOS, Win and Linux, marking as verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.