Opened 6 years ago

Closed 5 years ago

#8510 closed task (fixed)

Add useful Hidden Service related events to the Tor control port

Reported by: hellais Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs controller needs-proposal
Cc: arma, runa, asn, karsten Actual Points:
Parent ID: #8993 Points:
Reviewer: Sponsor:

Description

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?

Child Tickets

TicketStatusOwnerSummaryComponent
#9715closedupdate tor spec for hidden service descriptor asynchronous eventsCore Tor/Tor

Change History (41)

comment:1 Changed 6 years ago by 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
  • 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

comment:2 in reply to:  1 Changed 6 years ago by rransom

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.

  • Hidden service descriptor received

Duplicate of #3459.

Events:

  • Descriptor published to HSDir

Duplicate of #3459.

  • 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.

comment:3 Changed 6 years ago by nickm

Keywords: tor-hs controller needs-proposal added
Milestone: Tor: unspecified

Is there anything left to this ticket?

comment:4 Changed 6 years ago by arma

Parent ID: #8993

comment:5 Changed 6 years ago by dave2008

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

Last edited 6 years ago by dave2008 (previous) (diff)

comment:6 Changed 6 years ago by asn

Status: newneeds_review

comment:7 Changed 6 years ago by asn

Initial review. Haven't tested yet. I'd like to review again.

  • Nitpicking, but the error message Unknown directory is not a LongName as the spec indicates.
  • Hm, if (hs_dir_node) {. Is !hs_dir_node possible in that code? If yes, it should have an else clause to initialize hs_dir to a helpful string (it's currently printed as NULL in control_event_hs_descriptor_requested()). If no, then maybe it should be an assert?
  • Unittests would be awesome!

Other than that, code looks sane. Will need to test it to see it in action though.

comment:8 Changed 6 years ago by dave2008

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.

comment:9 Changed 6 years ago by dave2008

FYI, I have updated both the spec and the code.

One problem regarding tests, it seems like a non trivial task to write tests for control event, is mocking a dir_connection_t for connection_dir_client_reached_eof function the right thing to do here?

comment:10 Changed 6 years ago by nickm

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.

comment:11 Changed 6 years ago by dave2008

OK, thanks for your help :)

I have added the test case, I guess am I am ready for the second round of review.

comment:12 Changed 6 years ago by asn

Thanks for the code, will try to find some time to review this "soon".

Last edited 6 years ago by asn (previous) (diff)

comment:13 Changed 6 years ago by asn

Status: needs_reviewneeds_revision

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_msg global variable is a bit confusing -- especially if we expect to add more unit tests to test_hs.c. Can you at least document that the variable is a helper for a specific test? Same goes for the STR_HS_ADDR constants, which I would personally only define within the body of test_hs_desc_event().
  • Check out test_streq() instead of test_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!

comment:14 Changed 6 years ago by asn

BTW, I assume you have tested this while connecting to actual hidden services on the tor network and it works, right?

comment:15 Changed 6 years ago by 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.

comment:16 in reply to:  15 Changed 6 years ago by asn

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? :) )

comment:17 Changed 6 years ago by dave2008

I plan to write more HS code and tor code in general after I am done with current semester :)

I am cleaning up the code right now, do you prefer I squash all commits into one or keep the original tiny commits?

comment:18 in reply to:  17 Changed 6 years ago by asn

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.

comment:19 Changed 6 years ago by asn

You might also want to add a changes file that describes the feature that you added (example commits: 71e0ca02b57f7945d922a8708a2c97815a9350ad 907711d790a391c12a398d3cc402c37a68576381). Feel free to also add your name in there if you want.

comment:20 Changed 6 years ago by dave2008

OK, I have just updated my branch, free let me know if I can further improve it :)

comment:21 Changed 6 years ago by asn

Maybe you forgot to git add the test_hs.c file.

comment:22 Changed 6 years ago by dave2008

oops, added now. Better not doing this kind of work in a class ;p

comment:23 Changed 6 years ago by asn

Status: needs_revisionneeds_review

Looks good to me! Thanks for the code!

Nick?

comment:24 Changed 6 years ago by nickm

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.

comment:25 Changed 6 years ago by dave2008

Thanks for the feedback, I have updated my branch accordingly :)

comment:26 Changed 5 years ago by dave2008

Hi, I have rebased my branch to latest master. Anything I can further improve here?

comment:27 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

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.

comment:28 Changed 5 years ago by dave2008

Hi Nick,

Glad to hear you back :)

I will start cleaning my patch tomorrow, all the suggestions looks fine to me, except I am not exactly sure on how to get the tor_assert one done. I will look more into it tomorrow as well.

Thanks again for reviewing the patch!

comment:29 Changed 5 years ago by dave2008

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?

comment:30 Changed 5 years ago by dave2008

Anyway, I have updated my branch and fixed all the problems you pointed out. Please let me know if there is anything to improve :)

comment:31 Changed 5 years ago by asn

Sorry for not getting back to you in a while dave2008!

Code looks good to me.

Maybe a unit test of the if (!node) { case of node_describe_by_id() would make this even more robust?

comment:32 Changed 5 years ago by 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?

comment:33 in reply to:  32 Changed 5 years ago by asn

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.c indeed. And there doesn't seem to be a place with lots of router.c unittests either (test_dir.c contains a few).

I would suggest making your own test_router.c. We will need to unit test the rest of the funcs at router.c at some point anyway... If you think that's a poor idea, maybe you can put the test in test.c or test_hs.c but it doesn't look very suiting...

comment:34 Changed 5 years ago by dave2008

I actually prefer the test_router.c way :) I will work on it tonight.

comment:35 Changed 5 years ago by dave2008

@asn, @nickm, I have added the test case for node_describe_by_id and rebased my branch on latest master.

comment:36 Changed 5 years ago by asn

LGTM :)

comment:37 Changed 5 years ago by nickm

Last changes I hope:

  • Should we really be using numeric values 0, 1, 2 to indicate authorization types? We usually use human-readable strings in the rest of the controller protocol.

comment:38 Changed 5 years ago by dave2008

Done, string representation for auth_type are "NO_AUTH", "BASIC_AUTH" and "STEALTH_AUTH".

I have also updated my torspec branch.

comment:39 Changed 5 years ago by nickm

Merged, with a tweak to make sure that the control_event_hs_* functions are never called with NULL arguments.

comment:40 Changed 5 years ago by dave2008

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.

comment:41 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_revisionclosed

Looks okay; I'm merging, and tweaking the test to use strlcpy and memcpy as appropriate.

Note: See TracTickets for help on using tickets.