16103 - SOCKS V5 Implementation

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[b.benowitz]

Unable to load pages behind SOCKS Server. No meaningful messages displayed.

Comment 1

[b.benowitz]

Attachment: Error messages in apprunner window

Comment 2

[Gagan]

Socks support isn't in yet. Marking for later and assigning to rick.

Comment 3

[leger]

Bulk move of all Networking-Core (to be deleted component) bugs to new
Networking component.

Comment 4

[ruslan]

-> marketing

Comment 5

[Greg_Norris]

> -> marketing

I'm not in any great hurry to appear foolish, but what do you mean by that?

Comment 6

[Warren Harris]

Netscape doesn't intend to implement socks for this product, although we welcome 
a submission to mozilla.org.

Comment 7

[Alan S. Jones]

I hated to see Socks Proxy Server support pushed off.  I wanted to stick in some 
information incase anyone was looking later.....

Main Socks page
http://www.socks.nec.com/

If you want socks support in Mozilla and you are on Windows one can use this:
http://www.socks.nec.com/socks5.html

SOCKS v5 is an IETF standard (RFC 1928)

Comment 8

[justin]

I'll implement SOCKS support.
See my post in netscape.public.mozilla.netlib for details.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Just be sure to do it so that it works both with and without PSM,
that is so that both http and https URLs work with it.

Comment 10

[justin]

I have a working NSPR IO layer implementing SOCKS proxying.
Once I've cleaned it up, I'll submit it and start working on
the transports and protocols so that they will make use of it.
Since it's an NSPR IO layer, it should work fine with the SSL layer,
but it will require some minor changes to nsSocketTransport.
I can test SSL and SOCKS layers together before the protocol changes,
however, to make sure everything is interacting correctly.

Comment 11

[justin]

Attachment: SOCKS support for HTTP and FTP, with UI configuration

Comment 12

[justin]

The above patch should bring mozilla up to netscape 4.x's socks support,
except for SSL over SOCKS. It should work, but it's not for me. I'll continue
working on it, and adding SOCKS support to the other protocols.

Comment 13

[ruslan]

Ok. Justin Bradford <[e-mail]> has implemented SOCKS support. To get it 
in we'll need to move the tag on NSPR to get the latest changes for non-blocking 
connect and make a few API changes. Otherwise it's working great. Changes are 
not drastic, but still fair in size. Nominating for nsbeta2, chaning the target 
milestone and awaiting the decision from PDT.

Comment 14

[Phil Peterson]

[nsbeta2-] not required for seamonkey beta2 but you can always check in mozilla 
community contributions with approval from brendan or waterson. cc'ing Wan-Teh 
for comments about needing a new rev of NSPR

Comment 15

[Wan-Teh Chang]

The current implementation of SOCKS support does
blocking connect and hence does not need a new rev
of NSPR.

Comment 16

[justin]

Attachment: patch to make ssl work over socks -- this has some replacement patches/files for the previous patch

Comment 17

[ruslan]

Brendan? Can we check this in?

Comment 18

[justin]

Attachment: A new and improved version of the socks patch.

Comment 19

[justin]

Attachment: the one new file to add to the tree for socks

Comment 20

[justin]

Ruslan committed all (but one) of the new files, so the new patch is 
against that. The second attachment is actually a tar file with the
one remaining file: extensions/psm-glue/src/MANIFEST

The mac build files have not yet been updated, but I'll do that once
I get approval to commit this patch.

Comment 21

[Brendan Eich [:brendan]]

Justin, looks great -- a=[e-mail] if you attend to these nits:

- Can't tell due to diff -wu, but I hope all tabs have been expanded,
  and indentation looks uniform and even and stuff.

- Does CMT_ProxyStepUp need to modify its final argument, which is a
  copy created using nsCAutoString by nsPSMSocketInfo::ProxyStepUp?
  I couldn't tell from reading 
http://lxr.mozilla.org/mozilla/source/security/psm/lib/client/cmtssl.c#414

  Maybe the final paramter to CMT_ProxyStepUp should be const char*?
  I'm also wondering why mHostName is an nsString, instead of an
  nsCString, but that's not your doing.  Might be worth filing a bug.

  Should CMT_ProxyStepUp error return values propagate back via bad
  nsresults through securityInfo->ProxyStepUp() to its caller?

- Is it really necessary to hold onto the PSM service via gPSMService?
  I see only one use of that variable, in nsSSLIOLayerConnect (whose
  name follows the class naming convention, not the static PR_CALLBACK
  naming convention [such as there is]).  If you're using this service
  only at connect-time, why not use NS_WITH_SERVICE to make a local,
  auto-pointer-held reference?

  It's bad to hold onto services forever, especially in embedding cases
  where things may need to unload and return all memory to the app.

- else after return in nsFtpConnectionThread::CreateTransport -- a
  non-sequitur that's not repeated at the bottom of that method, where
  there's a nice final return that several cases "fall into" without
  unnecessary else's.

- Argh, nsIHTTPChannel.idl uses InterCaps, not interCaps, in attribute
  names -- what's up with that?  The xpidl compiler will capitalize an
  attribute name before prepending Get or Set in the C++ binding, and
  the JS binding wants interCaps (JS and Java style, and Mozilla XPIDL
  house style for that matter).

/be

Comment 22

[Brendan Eich [:brendan]]

I should have written a=[e-mail], check in bug please attend to these 
nits either first, or in a quick followup checkin.

/be

Comment 23

[justin]

1. I've checked tabs and indentation and everything looks fine.

2. That non-const argument is passed down into the PSM code and used in the 
message encoder. I just looked over the code and it would appear that making it 
const would not affect anything. However, I didn't write any of the involved 
code (I'm  simply calling an existing function) and I've never looked beyond 
CMT_ProxyStepUp prior to 10 minutes ago. A practically identical path is taken 
by the normal SSL connection call, which also has non-const arguments...

2b. I return from PSMSocketInfo::ProxyStepUp the return code from 
CMT_ProxyStepUp, so the error should already be propagated to the 
SocketTransport, which is its caller.

3. Usage/storage of PSM service by SSL layer: The PSM service basically 
represents a daemon, and I suspect multiple services imply multiple daemons. So, 
while, I agree that it shouldn't be a static sitting around there, I'm not sure 
what effect it would have on the behavior (possibly causing start-psm to 
load/unload frequently when accessing a site via SSL; possibly spawning many psm 
daemons; etc). I believe the case is now, when SSL is used, psm runs and doesn't 
quit until the browser does. That's not good, but I don't know how to fix 
it right now.

I should also point out that I haven't written or even touched most of the parts 
you're referring to here. My changes to the SSL stuff were very basic. The 
AddToSocket which does exactly the same thing as NewSocket (excepting the 
allocation of a socket) and enabling the existing SSL proxy mechanisms. I can't 
address #2 or #3 without studying the PSM module more, and I think they're 
unrelated to this patch (as they're existing issues). I'll file bugs for both 
address them later.

4. I've removed the unnecessary else from the FTP's CreateTransport

5. I've changed all of the attributes in nsIHTTPChannel.idl to interCaps style, 
including the one attribute I am adding.

Should I go ahead with the commit?

Comment 24

[ruslan]

Sorry - was on vacation. Is it not landed yet?

Comment 25

[justin]

I committed the code last night. Is anyone having trouble on non-Linux 
platforms?

Comment 26

[justin]

I've tested a Win32 nightly build with my SOCKS code. It appears to be working 
fine, except for SSL connections (which work fine on Linux). Can anyone else 
confirm this?

Comment 27

[ruslan]

I guess this is done, right Justin?

Comment 28

[b.benowitz]

As the originator of this problem, I had a chance to test the 7/6/2000 Nightly build on Linux and I
still find 2 problems:
1) When you originally set the SOCKS HOST name and port in preferences and then click OK, 
 the main window starts loading fine. But the tinderbox window continues to say Loading... for
 ever. Performance suffers as well. If you exit Mozilla and restart, everything is fine.

2) Secure Pages don't seem to load. However, I'm not sure that those fixes were committed, yet.

Comment 29

[justin]

I haven't experienced your first problem, but I'll try to replicate it.

As for the SSL problems, the code was committed, and it does work on Linux.
I'm not gotten it to work on win32, but I'm not sure if I have the PSM module
installed/configured. I'll look into this more.

Comment 30

[Greg_Norris]

It doesn't seem to work at all here.  After setting the socks host and port to 
the same values which IE uses successfully, mozilla is unable to load any 
site.  For example, if I try to load mozilla's homepage the taskbar will show 
"Resolving host www.mozilla.org", but it never gets any further.

The OS is NT4, SP6.

Comment 31

[justin]

What version of SOCKS does your firewall support?
Mozilla only supports version 5 right now.

Comment 32

[Greg_Norris]

That would explain it... I'm pretty sure that ours is only SOCKS4.  I'll try to
verify this, just to be sure.

Thanx!

Comment 33

[Steven Ihde]

I created a patch that adds Socks4 support to Justin's Socks5 layer.  It detects
the server protocol version by trying Socks4 first, then Socks5.  This test is
rather expensive if the server is Socks5 (extra TCP connection setup/close). 
This raises some issues... should a cache of Socks server versions be
maintained?  It would be relatively simple to cache only the most recent server,
but a proxy autoconfig script could return different Socks servers for different
destination addresses during the same session, so this might not be ideal.  But
it might be adequate.  A full-blown cache would be more work, but doable. 
Perhaps there is already some infrastructure for this sort of thing?

Or would it be better to expose this in the configuration UI?

Any suggestions?

Comment 34

[[e-mail]]

Might want to make this a separate bug.

There should be selector UI that supports this, but even more so, we might want
to just completely re-do the UI. The old UI doesn't work well for SOCKS in
general because there is a lack of clarity for order of precedence.

My feeling is that we should just EOL SOCKS V4. It's not specified in an RFC,
and there doesn't seem to be much in the way of handshaking or backwards
compatability.

A -quick- look at the SOCKS V5 RFC seems to show that the client initiates the
call for a version...

http://www.faqs.org/rfcs/rfc1928.html

 The VER field is set to X'05' for this version of the protocol.  The
   NMETHODS field contains the number of method identifier octets that
   appear in the METHODS field.

Comment 35

[[e-mail]]

I've re-read the RFC's and NEC's specs. My reading is that there is no effective
handshake for versions. My gut says we should add a "SOCKS V4" and SOCKS V5"
selector in manual prefs. If you don't, then your version failover (which isn't
a real handshake) might fill up a SOCKS server with error messages.
 

Comment 36

[[e-mail]]

VERIFYING BUG:
Used a packet trace...
http://www.packetgram.com/pktg/proxy/socksclientversion.html

General information on how when SOCKS is used for connections
http://www.packetgram.com/pktg/docs/nscp/ns6/ns6proxy.html#socks

clarified Summary for accuracy.
put a URL into URL field.
keyed for release note

Want any of the following? 

A fix for the prefs (say "SOCKS V5", not "SOCKS"), see bug 50679

create a new bug.

SSL and SOCKS (either combination)
SOCKS V4
SOCKS server version handshaking

Comment 37

[benc]

-relnote - other SOCKS bugs are tagged w/ relnote keyword.