dMZX Forums: SOCKS and/or HTTP proxy support -> Archived Requests -> Tracker

MegaZeux's network layer does not support SOCKS or HTTP proxies. As a result, features such as the updater (and soon the archive viewer, MZXNet) will not work without a direct internet connection.

This shouldn't be too hard to implement, but I'll have to look into it.

--ajs.

Updating status to: Awaiting Feedback
Issue fixed in: GIT
Updating Introduced In Version to: GIT

The biggest difficulty in this was figuring out the pointers used to pass around the host struct. After that was set, this became fairly trivial.

Anyways, SOCKS4 support is added and should be up in my branch momentarily. ajs, if you could have a look at this, I'd appreciate it. I changed a few updater fundamentals to work with the proxy.

Updating Introduced In Version to: 2.83

Looks generally good to me. Few things I'd like you to fix before I merge it..

  // For vhost resolution
  if (h->proxied) // cover SOCKS transparency
  {
    snprintf(line, LINE_BUF_LEN, "Host: %s", h->endpoint);
  }
  else
  {
    snprintf(line, LINE_BUF_LEN, "Host: %s", h->name);
  }

Don't call snprintf() on both halves of the conditional; rather, introduce a const char *host_name = h->name; then do if(h->proxied)\nhost_name = h->endpoint; It's cleaner and generates better code.

char socksreq[512];

Don't do this. You've got some constant data at the beginning which is fixed in size (and much shorter than 512 bytes) and you've got some variable length stuff for the username and host. Just use multiple __send() commands, then you don't need to copy anything into an intermediary buffer.

I'd also like it if you put the SOCKS protocol data into a structure (pack it if necessary) and then cast that when passing it to __send(). I also think we shouldn't be using the "invalid IP" hack SOCKS 4a provides; we do other DNS lookups with host functions, why can't we also look up the target? Can we get away without a username (just sending \0)? Wikipedia's description is vague.

__send(h, socksreq, 10 + strlen(username) + strlen(target_host));

Check error codes from __send please. The __recv may block.

Finally, is there any way you can think of to better integrate the SOCKS handshake? I'd prefer it if you didn't need to open-code the SOCKS handshake in users of host_connect(); it would be better if this detail was hidden from the user (of course, adding error codes for SOCKS failures is certainly a fine thing to do).

--ajs.

I'll check into that stuff as soon as I get another chance to look at MZX. I'm also going to attempt to shoehorn in SOCKS5 support at some point hopefully in the next week.

Thoughts on where to put the logic for SOCKS checking? Inside the connection library routine, transparent to the utilizing functions?

I'd put it inside host_connect, as I think you're suggesting. You might want to rename/refactor what's currently called "host_connect", but it should be an easy change. I'm not asking for incredibly good integration here, just a bit more than what has been done. In an ideal world, updater.c wouldn't need to be changed at all, but I think realistically it will just need the new error handling adding. It certainly shouldn't be calling additional networking functions.

SOCKS5 sounds good -- this should again be quite a straightforward improvement.

--ajs.

Yes, that is what I was suggesting; I'm not very coherent when I'm sleep-deprived

Anyways, as for the username field I think since I'm rolling in SOCKS5 support (and thus have to add a username field to the configuration anyways) I'm going to push it down to be user-configurable. My big concern is for SOCKS servers that backtrack an IDENTd query to the requesting system and expect the IDENT and the USERID part of the frame to match. I'll probably send "ANON" as the default username instead of "megazeux" just for sake of privacy, but most everything I'm seeing indicates the need for something in that userid field.

identd is pretty obsolete, I'd be surprised if any SOCKS proxy worth connecting to is stringent about it. I noticed that SOCKS5 removed the requirement for this junk.

There's simply no way to know what the ident is for a given user, other than to query localhost. I think this complexity outstrips what we're trying to do here, which is a very minimal bolt-on.

--ajs.

ajs, I have most of those changed implemented but I ran into a fairly sizable roadblock; I'm trying to figure out a good way to access the config file struct from within host.c. Thoughts?

Implemented in my fork, waiting on ajs' approval and merge.

Okay another review. You know about the multiple send() thing and my feelings on it: basically I want hard evidence it fundamentally doesn't work or I want it to be done that way.

This tree has some hacks to gdm2s3m which I assume were accidentally checked in. I won't be pulling those. The changelog also has some stuff strangely deleted from it.

The mods to host_layer_init() et al look good, and I'm glad you've had the courage to change a major interface like this to get the job done. It's exactly the right way to do it.

+  return _raw_host_connect(h, hostname, port);
+}
+
+static bool _raw_host_connect(struct host *h, const char *hostname, int port)

Use of a static function before it's defined? Does this even compile?

   size_t line_len;
+  const char *host_name = h->name;

Can you keep decls in reverse christmas-tree style at the top of functions? It makes the code easier to read.

+  if (h->proxied)
+  {
+    host_name = h->endpoint;
+  }

There's lots of places you've used a pair of braces with just a single statement inside them. In general in the MZX source, and probably without exception in the network layer, we only use braces if a) they're necessary or b) they improve readability. If I'm not nesting an if() I'll generally just remove the braces, it means I can fit more code on my screen at once. Can you consider making this cleanup?

+// SOCKS Proxy support
+enum proxy_status proxy_connect(struct host *h, const char *target_host, int target_port)

This function should be static and moved above its first caller, or locally prototyped before use if moving it isn't possible.

+  ptr = handshake;
+  handshake[2] = target_port / 255;
+  handshake[3] = target_port % 255;
+  ptr += 13;
+  strcpy(ptr, target_host);
+  ptr += strlen(target_host);
+  *(ptr++) = 0;
+  if (!__send(h, handshake, 14 + strlen(target_host)))

This function needs fixing, as discussed. For now it's bad because it plays tricks with handshake (what's ptr for anyway?) and isn't properly bounds checking the array. All the proxy connect functions are similarly afflicted.

case '\7':

case 7: no?

That's it. Thanks.

--ajs.

The vast majority of these have been fixed. I've done a lot of research re; the multiple sends and the core issue revolves around the fact that SOCKS is designed to be an overlay protocol which emulates the Network layer of the OSI model.

To that end, it's designed such that each handshake phase is exactly one data frame. __send() breaks data into multiple frames, reproducibly, and the data is not rejoined for the protocol on the other side. Unfortunately, this frame data is not flexible, and as such, is required by protocol to be in a single (in this case, ethernet) frame.


My commit is located at http://github.com/mz...b4937345a9605c7 , cleaned up as you suggested.

There were some further changes required for GCC. The static functions needed moved, I made that function static (it could be), the octal (including some INVALID octal like \8) was removed. Here's a patch which does this work:

http://mzx.devzero.c...-socks-gcc.diff

I've also cleaned up some other stuff -- the host_layer_init() function wasn't being called on non-WIN32 platforms, so I was getting a NULL pointer de-ref in host_connect under Linux. I've also moved the static global from the header (don't declare globals in headers!!). On the bright side, what you've implemented so far seems to work fine with SSH's built-in SOCKS5 proxy feature, under Linux. So the code is mostly sound now.

BTW I don't agree with your explanation for the send() woes. TCP is a streaming protocol: there are no frames! SOCKS is an application level protocol, it can't see Ethernet frames. I think you're just getting confused by what the protocol analyzer is showing you, which isn't what the application will see.

Can you tell me what SOCKS proxy you're using to test this and I'll try to repro it. Then I'll try to diagnose the issue myself.

--ajs.

My send() splitting suggestion works fine with SSH's built-in SOCKS5 proxy under Linux. This blows away the frame theory. It might be a WinSock issue, or a bug in the proxy server you're using (which we should of course try to work around if sane).

--ajs.

Updated at your suggestion (including the diff)

http://github.com/mz...3f62fdbc92ecaa5

Updating status to: Confirmed

Pulled and pushed out in Git 04706cc.

Won't close this until IPv6 support is added and the sa_data hacks are removed. The feature is perfectly functional for IPv4 on at least two platforms now. Great work!

--ajs.

Updating status to: Flagged For Future Version

This won't make 2.83b and there's been little demand.

--ajs.

Moving to: MegaZeux Feature Requests

Updating status to: Fixed
Updating version to: None
Issue fixed in: 2.92f

Finished implementing SOCKS5 IPv6 and username/password support in GIT 9c8c2b8c. I also fixed some other things I encountered while doing this like the SOCKS4 functions ignoring error codes and the gross misuse of the ai_addr field. IPv6 support still needs some more work to be generally usable, though (namely using AI_V4MAPPED and AI_ADDRCONFIG in the getaddrinfo hint flags and adding a config file option to tweak this).

Moving to: Archived Requests