Add support for SOCKS parameters in Bridge and {Client,Server}TransportPlugin lines
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 socks5 address:port [username=X] [password=Y]
Activity
changed milestone to %Tor: 0.2.5.x-final
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 :)
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.
-
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?
-
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.
-
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 (moved), and I have a proposal in the making.
-
Trac:
Parent: #3591 (moved) to #4685 (moved) -
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.
- 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).
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.
Removing parent ticket relationship to #4685 (moved) in order to close it.
Trac:
Parent: #4685 (moved) to N/A-
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.
Trac:
Status: needs_review to needs_revision -
Trac:
Parent: N/A to #7211 (closed) -
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 (moved)-specific, which also makes sense. I'm preparing a branch that does this change. - 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).
-
Pushed new stuff to branch
bug3594inhttps://git.torproject.org/user/asn/tor.git.All your comments should be addressed, apart from the ones regarding
tor_escape_string(). Should I maketor_escape_string()purely #3594 (moved)-specific, and cut thesetandescape_chararguments?(Also, yes, I should replace
tor_char_is_in_set()withstrchr().) -
Replying to asn:
Pushed new stuff to branch
bug3594inhttps://git.torproject.org/user/asn/tor.git.All your comments should be addressed, apart from the ones regarding
tor_escape_string(). Should I maketor_escape_string()purely #3594 (moved)-specific, and cut thesetandescape_chararguments?Please, yeah. (Unless we can use any of the existing escaping implementations in Tor, which might be better.)
-
Small update: I added the commit that simplifies
tor_escape_string()in branchbug3594ofhttps://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. -
Hi. I rebased and organised branch
bug3594. It can now be found in branchbug3594_rebasedofhttps://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 ofbug3594on top of it.Apart from the rebase, the code of
bug3594andbug3594_rebasedis identical except from some improvements in the comments.Also, the fixup commits of
bug3594are now squashed into the commits ofbug3594_rebased. This might make reviewing a bit harder. If you need help you can check out the fixup commits ofbug3594individually and the code will be the same inbug3594_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.Trac:
Status: needs_revision to needs_review 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.
- string_is_key_value() looks okay, but comment should describe the format it tries to recognize
-
Thanks for the review!
I think I addressed all your comments in branch
bug3594_rebased_and_fixedinhttps://git.torproject.org/user/asn/tor.git.I'd also like to replace
tor_addr_port_lookup()inparse_bridge_line()with something liketor_addr_port_parse()but I'm not sure what's the reason the_lookup()function was originally there.Trac:
Status: needs_revision to needs_review -
After reading #7153 (moved) and looking at
faf4f6c6d1da54b0a6b0c9946112f2e448867a8f, I see that invalidate_transport_socks_arguments()I reject SOCKS arguments larger thanMAX_SOCKS5_AUTH_SIZE_TOTALeven if I don't know the SOCKS version that the pluggable transport proxy is going to use. Maybe this check shouldn't happen invalidate_transport_socks_arguments()so that we allow large SOCKS arguments if SOCKS4a is used. Trac:
Keywords: pt tor-bridge deleted, pt tor-bridge flashproxy added
Cc: N/A to dcf@torproject.orgmentioned in issue #4685 (moved)
mentioned in issue #5299 (closed)
mentioned in issue #7211 (closed)
moved to tpo/core/tor#3594 (closed)