Add useful Hidden Service related events to the Tor control port
Today we discussed with arma, asn and runa about some possible events to allow us to register via the Tor control port.
These are the events that should be exposed:
-
Hidden service descriptor requested
-
Hidden service descriptor received
-
Introduce1 cell sent
-
Introduce2 cell received
-
Rendezevous1 cell sent
-
Rendezevous2 cell received
Useful data from a running hidden service are:
Events:
-
Descriptor published to HSDir
-
Updated number of introduction points
Useful information to be exposed:
-
Number of IP currently connected to
-
Addresses of the IP currently connected to
Are there some other events that would be useful to have?
Activity
changed milestone to %Tor: unspecified
It turns out that most of these events, are already implemented in tor 0.2.3-11-alpha (see: https://gitweb.torproject.org/torspec.git/blob/HEAD:/control-spec.txt#l1353).
The ones that are missing are the following:
-
Hidden service descriptor requested
-
Hidden service descriptor received
Events:
-
Descriptor published to HSDir
-
Updated number of introduction points
Useful information to be exposed:
-
Number of IP currently connected to
-
Addresses of the IP currently connected to
-
Replying to hellais:
It turns out that most of these events, are already implemented in tor 0.2.3-11-alpha (see: https://gitweb.torproject.org/torspec.git/blob/HEAD:/control-spec.txt#l1353).
The ones that are missing are the following:
- Hidden service descriptor requested
Duplicate of #3459 (moved).
- Hidden service descriptor received
Duplicate of #3459 (moved).
Events:
- Descriptor published to HSDir
Duplicate of #3459 (moved).
- Updated number of introduction points
This can be detected from the circuit list, but probably should have a separate event.
Useful information to be exposed:
- Number of IP currently connected to
If “IP” means “introduction point”, this is already available in the circuit list.
- Addresses of the IP currently connected to
If “IP” means “introduction point”, this is already available in the circuit list.
Trac:
Parent: N/A to #8993 (moved)
Hi all,
I have implemented the asynchronous events for hidden service descriptor: https://github.com/houqp/tor/commits/hs_control
Please let me know anything wrong ;p
PS: update for spec is added to a child ticket: https://trac.torproject.org/projects/tor/ticket/9715
Trac:
Username: dave2008
Initial review. Haven't tested yet. I'd like to review again.
-
Nitpicking, but the error message Unknown directory is not a
LongNameas the spec indicates. -
Hm,
if (hs_dir_node) {. Is!hs_dir_nodepossible in that code? If yes, it should have anelseclause to initializehs_dirto a helpful string (it's currently printed asNULLincontrol_event_hs_descriptor_requested()). If no, then maybe it should be anassert? -
Unittests would be awesome!
Other than that, code looks sane. Will need to test it to see it in action though.
-
-
Hi asn,
Thanks for your review!
I am still not sure how to handle the unknown directory, should I change the spec to indicate it might return "Unknown directory"? Or should I return something else here? If so, generally what should I return for an unknown LongName?
Yep, I think calling assert on hs_dir_node makes more sense. I need to look more into it though ;p
As for tests, no problem, I will look into that as well.
Trac:
Username: dave2008 
Generally, I would try doing controller tests by mocking the funtions that write to the controller connection (e.g, "send_control_event_string()"), and by invoking the controller code rather than the directory code. connection_dir_client_reached_eof is very badly factored; trying to test that one without breaking it down into smaller functions first seems like an iffy choice to me.
-
Ah, nice! We are getting closer!
Some comments on the unit test:
- Can you add some brief docs to specify what the test is supposed to do?
- The
received_msgglobal variable is a bit confusing -- especially if we expect to add more unit tests totest_hs.c. Can you at least document that the variable is a helper for a specific test? Same goes for theSTR_HS_ADDRconstants, which I would personally only define within the body oftest_hs_desc_event(). - Check out
test_streq()instead oftest_assert(!strncmp(...)). - Also, you merged master on top of your branch. Ideally, your commits should be on top and you should rebase your branch to use master as its base.
If you don't have time to do the above tasks, just tell us and we can do them before merging.
Thanks!
Trac:
Status: needs_review to needs_revision -
Replying to dave2008:
OK, I will clean this up this weekend hopefully :)
Yes, I am actually using these events for my hidden service profiling tool right now.
Ah, nice! OK, I will wait for the updated branch!
(Also, are you planning on writing more HS code? :) )
-
Replying to dave2008:
I plan to write more HS code and tor code in general after I am done with current semester :)
Ah nice. Keep in touch.
I am cleaning up the code right now, do you prefer I squash all commits into one or keep the original tiny commits?
I would just do two commits. One with the code changes and a second with the unit test.
-
control_event_hs_descriptor_receive_end worries me a bit. It takes a fmt argument as a char*, and bypasses the GCC printf argument checking. Instead, why not give it an extra const char * argument to receive either "FAILED" or "RECEIVED"?
In test_hs.c, I'd suggest adding TT_FORK to the flags field, so that the tests are run in a subprocess.
-
Okay, another round of reviews. (Sorry about the delays):
- We should prefer tor_malloc to malloc(); we should prefer tor_malloc_zero() to malloc()+bzero(). We should use tor_free(), not free().
- This won't build with all our gcc warnings enabled. (It uses C99 constructs by declaring variables in the middle of blocks.)
- node_describe(rs) is a better choice than routerstatus_describe(node->rs) for describing nodes.
- The tor_assert(hs_dir_node) thing makes me worry about race conditions where the node goes away after the circuit is launched. Perhaps we can send events with node_describe(hs_dir_node) when hs_dir_node is non-null, and with the base 16 encoding if the ID when node_get_by_id() returns NULL? That would be a fine thing to wrap into a "node_describe_by_id()" function.
- Most of these new functions lack documentation. See doc/HACKING for a documentation style.
Anybody on the cc list want to offer to help dave2008 out with these changes? We've left him hanging for a while, they're fairly simple changes, and it would be cool to pick up some of our slack.
Trac:
Status: needs_review to needs_revision -
Hi Nick,
Sorry for my delay as well, got distracted in the last few weeks :(
One quick question regarding your forth point.
According to your suggestion, I plan to add a "node_describe_by_id()" function that:
- returns the LONGNAME of a node if node_get_by_id() returns non-null pointer.
- returns the base 16 encoding of identity_digest if node_get_by_id() returns null.
Then replace node_describe() call with node_describe_by_id().
Am I going in the right direction?
Trac:
Username: dave2008 -
Replying to dave2008:
Agree. I am wondering where which file I should put the testcase in.
Since node_describe_by_id is a generic router specific function, it obviously does not belong to test_hs.c. Is there any test module that is specifically for router related functions?
Hm, there is no
test_router.cindeed. And there doesn't seem to be a place with lots of router.c unittests either (test_dir.ccontains a few).I would suggest making your own
test_router.c. We will need to unit test the rest of the funcs atrouter.cat some point anyway... If you think that's a poor idea, maybe you can put the test intest.cortest_hs.cbut it doesn't look very suiting... -
As reported by @asn and @atagar reported, the longname reported in the control event is not consistent with control spec. I have added some patches to fix it: https://github.com/houqp/tor/tree/hs_control_fix
I noticed that all the functions in control.c either uses node_get_verbose_nickname or implement their own version of it to get the LongName. If the concept of LongName is specific to the control submodule. Maybe it's better to have one generic get_longname function and use it instead of duplicating the code again and again.
Trac:
Username: dave2008 mentioned in issue #9715 (moved)
moved to tpo/core/tor#8510 (closed)