Opened 6 years ago

Closed 4 years ago

#8405 closed enhancement (implemented)

Provide a control port command to query the circuit used for SOCKS u+p

Reported by: mikeperry Owned by: mikeperry
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client, mike-0.2.5, TorBrowserTeam201410
Cc: arthuredelstein@…, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Once we start isolating streams by domain in the browser, it will be useful to have a way to ask Tor what circuit it is currently using for a given SOCKS username+password, so we can provide a tooltip or other indication directly in the browser UI.

Child Tickets

Change History (35)

comment:1 Changed 6 years ago by rransom

Status: newneeds_revision

See my branch isolation-debug-v4 ( https://git.torproject.org/rransom/tor.git isolation-debug-v4 ).

comment:2 Changed 6 years ago by rransom

(Some of the reasons that I didn't get that branch merged at the time were (a) its output format was ugly, and I didn't want it carved into stone in control-spec.txt for all time; (b) its output complied with the spec for extended events (see §4.1), but the Python crap can't handle a QuotedString containing spaces; (c) I never got around to dumping isolation values for circuits, only streams; (d) the details of the stream-isolation code should be unspecified and subject to change.)

(Also, that branch predates smartlist_add_asprintf.)

comment:3 Changed 6 years ago by mikeperry

Owner: set to mikeperry
Status: needs_revisionassigned

Thanks for this, Robert. I'll see if I can clean it up for 0.2.5.x.

comment:4 Changed 6 years ago by mikeperry

Keywords: mike-0.2.5 added

comment:5 Changed 5 years ago by mikeperry

Another way to do this might be to provide a query to ask Tor which circuit was used for a given SOCKS source port. There would be no ambiguity for such a query (unlike querying for SOCKS u+p, which may have ended up spanning several circuits if there were weird failures or odd ports).

comment:6 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

comment:7 Changed 5 years ago by arthuredelstein

Cc: arthuredelstein@… added

comment:8 Changed 5 years ago by arthuredelstein

Status: assignedneeds_review

Here's my attempted implementation. If we send SETEVENTS CIRC via the ControlPort from TorBrowser, we should get u+p updates whenever a new circuit is created.

comment:9 Changed 5 years ago by nickm

Does this actually work? It will tell you values for those fields (but not the other isolation fields) if a circuit is first created with isolation set... but I don't think it will tell you when an existing circuit gets an isolation field set.

Perhaps it makes more sense to attach ISO_* fields to STREAM events, and then to watch both STREAM events and ATTACHSTREAM events? Would that work for you? If so, I think rransom's approach above is better in that case.

comment:10 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

Another way to do this might be to provide a query to ask Tor which circuit was used for a given SOCKS source port.

Well, you can get this info now by watching STREAM and ATTACHSTREAM events, I believe. (Would you even need to see isolation info for that?)

comment:11 in reply to:  9 ; Changed 5 years ago by arthuredelstein

Status: needs_revisionneeds_review

Replying to nickm:

Does this actually work? It will tell you values for those fields (but not the other isolation fields) if a circuit is first created with isolation set... but I don't think it will tell you when an existing circuit gets an isolation field set.

Perhaps it makes more sense to attach ISO_* fields to STREAM events, and then to watch both STREAM events and ATTACHSTREAM events? Would that work for you? If so, I think rransom's approach above is better in that case.

OK, I've posted a patch based on rransom's approach. Does this seem like the right thing now? Thanks for your help.

comment:12 in reply to:  11 ; Changed 5 years ago by rransom

Status: needs_reviewneeds_revision

Replying to arthuredelstein:

OK, I've posted a patch based on rransom's approach. Does this seem like the right thing now? Thanks for your help.

You must not output the SOCKS4 auth string without escaping it. Either use esc_for_log_len (and add it if it hasn't already been added to Tor somewhere) like I did or use base16_encode.

Remember that some people will think that a hex-encoded string is encrypted. At the very least, be aware that hexifying strings makes it harder for a human to read the control-port output.

Consider dynamically allocating the hex-encoding buffers for SOCKS5 auth strings, or at least not allocating a full kilobyte on the stack -- you're about to smartlist_add_asprintf the contents anyway, so 512 bytes of buffer should be enough.

Remember to update control-spec.txt to document at least what is actually being used by other applications.

comment:13 Changed 5 years ago by nickm

Does this seem like the right thing now?

Before I review this one, I'd like to hear from the TBB team about whether this general approach meets their needs. I don't think I've seen an answer there. I'm happy with an events-based approach, but the whole point of this ticket is to improve TBB usability, so I'd like to know whether we're actually going to do that before we merge.

comment:14 in reply to:  12 Changed 5 years ago by arthuredelstein

Replying to rransom:

Thanks for looking this over!

You must not output the SOCKS4 auth string without escaping it.

Stupid mistake. Fixed.

Either use esc_for_log_len (and add it if it hasn't already been added to Tor somewhere) like I did or use base16_encode.

At the very least, be aware that hexifying strings makes it harder for a human to read the control-port output.

Remember that some people will think that a hex-encoded string is encrypted.

Yes, I originally wanted to use esc_for_log, but it doesn't currently escape all possible dangerous characters. Examples include \= and \space. A client with a good parser that correctly recognizes a quoted string likely won't have any problem, but I didn't want to inadvertently break any existing naive parsers. So what do you think is the best option? (1) Use esc_for_log as is and assume good client parsers, (2) make esc_for_log safer, or (3) use base16_encode?

Consider dynamically allocating the hex-encoding buffers for SOCKS5 auth strings, or at least not allocating a full kilobyte on the stack -- you're about to smartlist_add_asprintf the contents anyway, so 512 bytes of buffer should be eno

Fixed.

Remember to update control-spec.txt to document at least what is actually being used by other applications.

Will do, once we settle on a final patch.

I've now posted a new version with the fixes mentioned.

comment:15 in reply to:  13 Changed 5 years ago by arthuredelstein

Replying to nickm:

Does this seem like the right thing now?

Before I review this one, I'd like to hear from the TBB team about whether this general approach meets their needs. I don't think I've seen an answer there. I'm happy with an events-based approach, but the whole point of this ticket is to improve TBB usability, so I'd like to know whether we're actually going to do that before we merge.

To summarize: I'm working on #5752 (isolate streams by URL bar domain) and descendants. I have implemented a patch for #3455, where the browser is using a unique SOCKS username and password (u+p) for each URL bar domain, and tor accordingly sets up a separate circuit for each unique u+p. The next step (#8641) is to implement a UI, attached to the browser's URL bar, that shows the progress of a circuit as it's being built, and the exit IP of that circuit. So we need to be able to query/monitor tor for the circuit ID corresponding to the u+p.

At this point, I don't expect to need any isolation flags apart from SOCKS u+p. So we could remove the other isolation flag stuff from the patch. But, given the work Robert was doing, I imagined you might want all isolation flags covered, for the sake of completeness. I'd be glad to hear your opinion either way.

In the meantime I can start working with the TBB team on a design and prototype for the #8641 UI, which may help solidify exactly what we need in the patch for this ticket. Any suggestions are much appreciated!

comment:16 Changed 5 years ago by arthuredelstein

I added a new version of the patch to correct a char array length bug. (Also accidentally added an irrelevant patch.)

comment:17 in reply to:  13 Changed 5 years ago by arthuredelstein

Status: needs_revisionneeds_information

nickm: I've now posted a patch for #8641 that provides a Tor circuit display UI and requires no patching of Tor. The #8641 patch works by assuming that (given my patch from #3455) the first STREAM connection for each new circuit is to the URL bar domain. So I monitor CIRC and STREAM events and display a mapping from domain to circuit nodes.

But, of course, we only receive the first STREAM event after the circuit is already built. It would be nicer to show the circuit as it is extended node by node. For this purpose, we would need to add USERNAME and PASSWORD parameters to the CIRC events (at least to the original CIRC LANUCHED event. Would you be open to such a patch?

comment:18 Changed 5 years ago by mikeperry

Keywords: MikePerry201408R added

comment:19 Changed 5 years ago by nickm

(status update: I'm still really wanting to hear from the TB team about which proposed interface might work for them.)

comment:20 Changed 5 years ago by arthuredelstein

Keywords: TorBrowserTeam201408 added

comment:21 Changed 5 years ago by mcs

Cc: mcs added

comment:22 Changed 5 years ago by arthuredelstein

If this is any help, my latest version of the path is posted above. (Please ignore older versions.) This patch prints SOCKS_USERNAME and SOCKS_PASSWORD parameters in circuit status events. If this patch or a similar one lands, I will update my patch for #8641 to make use of these flags.

comment:23 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201410 added; MikePerry201408R TorBrowserTeam201408 removed

At a glance, I think this is useful. I am a little worried that we may end up needing more info, because the STREAM NEW events don't have a circuit id yet, and #8641 also has the assumption that the first STREAM event on a new circuit is the top-level domain (which is not correct).

So, in short: yes, this is what we want the control port to do with SOCKS u+p. But we may also want it to do more, too.

Most likely though we won't finalize this until October, due to the Firefox 31 rebase work.

comment:24 Changed 5 years ago by mikeperry

Parent ID: #5752

We took this patch for the Tor in TBB 4.5-alpha-1. Will let you know how it works out with the rest of our username+password isolation.

comment:25 Changed 5 years ago by nickm

Any updates here, Mike?

comment:26 Changed 4 years ago by nickm

Any updates here?

comment:27 Changed 4 years ago by nickm

Status: needs_informationneeds_review

comment:28 Changed 4 years ago by nickm

16:52 < arthuredelstein> nickm: The #8405 patch is currently being applied to 
                         the tor included in TB 4.5-alpha builds: 
https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/gitian/descriptors/linux/gitian-tor.yml#n79
16:53 < nickm> arthuredelstein: is it working well?  Is it useful?
16:55 < arthuredelstein> I would say it is working well and is useful. We are 
                         using it for the Tor circuit display, which shows the 
                         Tor circuit for each URL bar domain.
16:55 < nickm> arthuredelstein: ok.  May I quote you on the bugtracker? :)

comment:29 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged to master; thanks!

Note: See TracTickets for help on using tickets.