Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#3594 closed enhancement (implemented)

Add support for SOCKS parameters in Bridge and {Client,Server}TransportPlugin lines

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: pt tor-bridge flashproxy
Cc: dcf@… Actual Points:
Parent ID: #7211 Points:
Reviewer: Sponsor:

Description

Add support for the optional key,value fields in Bridge lines:
bridge method address:port [[keyid=]id-fingerprint] [k=v] [k=v] [k=v]
and in TransportPlugin lines like:
ClientTransportPlugin <method> socks5 <address:port> [username=X] [password=Y]

Child Tickets

Change History (34)

comment:1 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-final

If this isn't implemented by now, I don't see it getting done as a small feature, or by the end of the month. Tentatively assigning this to 0.2.4

comment:2 Changed 6 years ago by asn

Grr! I'm not sure if the "passing arguments to transports through SOCKS"
idea can work.

Using the shared secret option of obfs2 as an example:
"""
Bridge obfs2 192.0.2.4:5555 shared_secret=test
ClientTransportPlugin obfs2 exec ./obfsproxy --managed
"""

In this case, tor will pass the "shared_secret=test" argument to
obfsproxy in its first SOCKS connection to it. This is way after the
initialization of obfs2 (and its crypto and its shared-secret).
The same problem occurs for any argument that must be passed to a
transport at initialization time.

Furthermore, server proxies don't use the SOCKS protocol, so there is
no way to pass arguments to managed server proxies.

Unfortunately, every solution I can think of, is stupidly dirty. All
of those solutions involve passing the arguments to the proxy using an
environment variable, and they all become very messy when I start
thinking of their implementation or usability.

I'm considering leaving this to be discussed during the dev. meeting,
if I don't get any better thoughts. Of course, any ideas are welcome :)

comment:3 Changed 6 years ago by nickm

I believe that some other mechanism is needed for server plugin configuration. But for client configuration, I don't see what's wrong with your example. You wouldn't want to give shared secrets to the client plugin at startup time, since it needs to know which shared secrets are associated with which connection: different connections could need different shared secrets.

comment:4 in reply to:  3 ; Changed 6 years ago by asn

Replying to nickm:

I believe that some other mechanism is needed for server plugin configuration. But for client configuration, I don't see what's wrong with your example. You wouldn't want to give shared secrets to the client plugin at startup time, since it needs to know which shared secrets are associated with which connection: different connections could need different shared secrets.

Hm, I'm not sure if this would work in the case of external proxies. The user would have to specify on startup time, which shared secret each SOCKS destination should use :/

Also, the current obfs2 code sets one shared-secret per obfs2 listener. Of course, we can change this.

My biggest issue is that we won't be able to set transport options in transport-startup time. So for example, if a transport needs a database of some kind to bootstrap, we won't be able to provide it (specific example, morpher will need the packet size probability distribution of the target protocol). Many such transports might be able to handle the database filename being passed on connection time (morpher probably will), but there might be transports that will need the filename on startup (for precomputation, or something).

As you can see, I don't have a good solution. Maybe we can add support both for options in transport-startup (using env. vars.), and per-connection (using SOCKS arguments for clients, and the new Extended ORPort for servers? or the new extended ORPort for both?)

Thoughts?

comment:5 in reply to:  4 ; Changed 6 years ago by nickm

Replying to asn:

Replying to nickm:

I believe that some other mechanism is needed for server plugin configuration. But for client configuration, I don't see what's wrong with your example. You wouldn't want to give shared secrets to the client plugin at startup time, since it needs to know which shared secrets are associated with which connection: different connections could need different shared secrets.

Hm, I'm not sure if this would work in the case of external proxies. The user would have to specify on startup time, which shared secret each SOCKS destination should use :/

Unless, say, Tor passed them as part of the socks handshake, right?

Also, the current obfs2 code sets one shared-secret per obfs2 listener. Of course, we can change this.

Right; that won't work as soon as you have two bridges with two different secrets.

My biggest issue is that we won't be able to set transport options in transport-startup time. So for example, if a transport needs a database of some kind to bootstrap, we won't be able to provide it (specific example, morpher will need the packet size probability distribution of the target protocol). Many such transports might be able to handle the database filename being passed on connection time (morpher probably will), but there might be transports that will need the filename on startup (for precomputation, or something).

Some options are per-connection; some are per-transport. These are different things; they cannot help but be thus.

As you can see, I don't have a good solution. Maybe we can add support both for options in transport-startup (using env. vars.), and per-connection (using SOCKS arguments for clients, and the new Extended ORPort for servers? or the new extended ORPort for both?)

Seems sane. Extended ORPort doesn't seem to make much sense for this, unless you radically change what it means.

comment:6 in reply to:  5 Changed 6 years ago by asn

Replying to nickm:

Replying to asn:

Replying to nickm:

I believe that some other mechanism is needed for server plugin configuration. But for client configuration, I don't see what's wrong with your example. You wouldn't want to give shared secrets to the client plugin at startup time, since it needs to know which shared secrets are associated with which connection: different connections could need different shared secrets.

Hm, I'm not sure if this would work in the case of external proxies. The user would have to specify on startup time, which shared secret each SOCKS destination should use :/

Unless, say, Tor passed them as part of the socks handshake, right?

Right!

Also, the current obfs2 code sets one shared-secret per obfs2 listener. Of course, we can change this.

Right; that won't work as soon as you have two bridges with two different secrets.

Yep, I was assuming that one should open two different listeners in that case. Guess I should make a ticket about this.

My biggest issue is that we won't be able to set transport options in transport-startup time. So for example, if a transport needs a database of some kind to bootstrap, we won't be able to provide it (specific example, morpher will need the packet size probability distribution of the target protocol). Many such transports might be able to handle the database filename being passed on connection time (morpher probably will), but there might be transports that will need the filename on startup (for precomputation, or something).

Some options are per-connection; some are per-transport. These are different things; they cannot help but be thus.

As you can see, I don't have a good solution. Maybe we can add support both for options in transport-startup (using env. vars.), and per-connection (using SOCKS arguments for clients, and the new Extended ORPort for servers? or the new extended ORPort for both?)

Seems sane. Extended ORPort doesn't seem to make much sense for this, unless you radically change what it means.

I meant the "new Extended ORPort", which is the port that will allow tor to pass rate-limiting information to transport proxies, etc. It was discussed in #3587, and I have a proposal in the making.

comment:7 Changed 6 years ago by asn

Parent ID: #3591#4685

comment:8 Changed 6 years ago by asn

Status: newneeds_review

Please see bug3594 in https://git.gitorious.org/mytor/mytor.git.

comment:9 Changed 6 years ago by nickm

Reviewing...

  • I think the names for tor_escape_string and string_is_key_value should really have something to say they're only for this particular use (socks argument parsing/encoding).
    • It makes me sad that we are proliferating ways to escape and unescape stuff.
    • Can string_is_key_value ever be given escaped srings?
    • Is it really invalid to have a "K=" to indicate that K is set to the empty string?
    • Will the escape function ever get called with escape_char != '
      ' ?
  • tor_char_is_in_set could be replaced with strchr


Other than that, looks mergeable in 0.2.4.x.

comment:10 Changed 6 years ago by rransom

https://en.wikipedia.org/wiki/Bencode is probably a better encoding for places like SOCKS 5 username and/or password fields where NULs are allowed.

comment:11 Changed 6 years ago by karsten

Parent ID: #4685

Removing parent ticket relationship to #4685 in order to close it.

comment:12 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

More stuff:

  • tor_escape_string() wants a different name.
  • Looks like tor_escape_string() can't handle \0s. Did we care about that? It's not documented really.
  • The comment "/ test for tor_escape_string() */" looks misplaced.
  • What's with all the strcpys in test_util_string_is_key_value? Can't string_is_key_value take them directly?
  • Why should Socks5ProxyUsername be mutually exclusive with Socks5Proxy and Socks5ProxyPassword? (In options_validate.) That seems wrong. Those options would be more r less meaningless without each other, I think.
  • re get_socks_arg_by_bridge_addrport: I don't like proliferating functions that look up an object and return a subfield of the object. I would rather have one set of functions that returns a bridge_info_t, and have the caller extract the subfield or call another function to do so. That way, if we want to look up bridges M ways and get N of their fields after the lookup, we only have M+N functions rather than potentially M*N.
  • Please don't use strcpy ever.
  • Please don't move around functions except in commits that do nothing besides moving functions.
  • That "null_pass" business is goofy; you can just say pass = "", right?

No changes file.

Doesn't pass make check-spaces.

comment:13 Changed 5 years ago by asn

Keywords: pt added

comment:14 Changed 5 years ago by nickm

Keywords: tor-bridge added

comment:15 Changed 5 years ago by nickm

Component: Tor BridgeTor

comment:16 Changed 5 years ago by nickm

Parent ID: #7211

comment:17 in reply to:  9 Changed 5 years ago by asn

Replying to nickm:

Reviewing...

  • I think the names for tor_escape_string and string_is_key_value should really have something to say they're only for this particular use (socks argument parsing/encoding).
    • It makes me sad that we are proliferating ways to escape and unescape stuff.

I agree. That's the main reason I made tor_escape_string() that generic. So that we could reuse it in existing code, or at least use it in the future the next time we need to escape something.

Seems like you prefer tor_escape_string() to be more #3594-specific, which also makes sense. I'm preparing a branch that does this change.

comment:18 Changed 5 years ago by asn

Pushed new stuff to branch bug3594 in https://git.torproject.org/user/asn/tor.git.

All your comments should be addressed, apart from the ones regarding tor_escape_string(). Should I make tor_escape_string() purely #3594-specific, and cut the set and escape_char arguments?

(Also, yes, I should replace tor_char_is_in_set() with strchr().)

comment:19 in reply to:  18 ; Changed 5 years ago by nickm

Replying to asn:

Pushed new stuff to branch bug3594 in https://git.torproject.org/user/asn/tor.git.

All your comments should be addressed, apart from the ones regarding tor_escape_string(). Should I make tor_escape_string() purely #3594-specific, and cut the set and escape_char arguments?

Please, yeah. (Unless we can use any of the existing escaping implementations in Tor, which might be better.)

comment:20 in reply to:  19 Changed 5 years ago by asn

Small update:
I added the commit that simplifies tor_escape_string() in branch bug3594 of https://git.torproject.org/user/asn/tor.git.

While I've tested it and it works OK, I must say that I've coded this branch through a wide time period and I don't feel too comfortable about its code. I don't really enjoy commits like a6eed8a402533cd51a0b66e7a1c65d205828e883.

It will also need to be rebased.

I'm leaving this in needs_revision. I'm flooded these days so if anyone would like to take the branch from here, it would be nice. Otherwise, I'll look into it again sometime during December.

comment:21 Changed 5 years ago by asn

Status: needs_revisionneeds_review

Hi. I rebased and organised branch bug3594. It can now be found in branch bug3594_rebased of https://git.torproject.org/user/asn/tor.git.

Due to the code migration to entrynodes.[ch] the rebase generated loads of conflicts, and I ended up making a new branch and applying the changes of bug3594 on top of it.

Apart from the rebase, the code of bug3594 and bug3594_rebased is identical except from some improvements in the comments.

Also, the fixup commits of bug3594 are now squashed into the commits of bug3594_rebased. This might make reviewing a bit harder. If you need help you can check out the fixup commits of bug3594 individually and the code will be the same in bug3594_rebased.

If you really can't review the new branch and you want the exact commit format of bug3594, tell me so and I will cook something up.

comment:22 Changed 5 years ago by andrea

In 89b21e929613e70be70e0a991bd65fb55b10a66b:

  • string_is_key_value() looks okay, but comment should describe the format it tries to recognize
    • NickM thinks it should use escaped()
  • Comment for tor_escape_str_for_socks_arg() mentions a 'set' param which doesn't actually exist.
  • The tor_escape_str_for_socsocksks_arg() is broken if you pass it the empty string (returns NULL). Does this matter?
  • It is far from immediately obvious on reading that tor_escape_str_for_socsocksks_arg() NUL-terminates its output; change to tor_malloc() and explicitly put a '\0' in.
  • The comment for tor_escape_str_for_socsocksks_arg() should mention that it allocates a new string.

comment:23 Changed 5 years ago by andrea

In 3fb2583d18c1528a5f2dcbf5eaeabf3dc75f9e83:

  • In parse_bridge_line(), we check for a key=value pair but don't use string_is_key_value()
  • parse_bridge_line() should have a unit test
  • ...and a comment documenting the format

comment:24 Changed 5 years ago by andrea

0630e66f91a5d63e0456c8a595bc011f8ebe0f56 looks fine to me

comment:25 Changed 5 years ago by andrea

In c7a41aa3f1a9297deb7136065da736fcb55d9e99:

  • SOCKS4_STANDARD_BUFFER_SIZE wants parentheses
  • Where does the 8 come from in connection_proxy_connect()? This should be a #define

comment:26 Changed 5 years ago by andrea

In d02b1126068e9bff347bfe5edcdf3197bbb51a99:

  • There ought to be a comment to the effect that the username/password length validation happens elsewhere and the assert really is a can't- happen.

comment:27 Changed 5 years ago by andrea

Status: needs_reviewneeds_revision

Can't merge as-is but looks not too much work to fix; I'll set this to needs_revision but leave it on 0.2.4.

comment:28 Changed 5 years ago by asn

Status: needs_revisionneeds_review

Thanks for the review!

I think I addressed all your comments in branch bug3594_rebased_and_fixed in https://git.torproject.org/user/asn/tor.git.

I'd also like to replace tor_addr_port_lookup() in parse_bridge_line() with something like tor_addr_port_parse() but I'm not sure what's the reason the _lookup() function was originally there.

comment:29 Changed 5 years ago by nickm

Type: defectenhancement

comment:30 Changed 5 years ago by nickm

This is looking pretty good to me. Andrea?

comment:31 Changed 5 years ago by asn

After reading #7153 and looking at faf4f6c6d1da54b0a6b0c9946112f2e448867a8f, I see that in validate_transport_socks_arguments() I reject SOCKS arguments larger than MAX_SOCKS5_AUTH_SIZE_TOTAL even if I don't know the SOCKS version that the pluggable transport proxy is going to use. Maybe this check shouldn't happen in validate_transport_socks_arguments() so that we allow large SOCKS arguments if SOCKS4a is used.

comment:32 in reply to:  30 Changed 5 years ago by andrea

Replying to nickm:

This is looking pretty good to me. Andrea?

I think I'm okay with this now.

comment:33 Changed 5 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final
Resolution: implemented
Status: needs_reviewclosed

Merging into 0.2.5 after asking George. Thanks!

comment:34 Changed 5 years ago by dcf

Cc: dcf@… added
Keywords: flashproxy added
Note: See TracTickets for help on using tickets.