Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#7952 closed enhancement (implemented)

Control port method to get the exit policy

Reported by: atagar Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay easy small-feature 025-triaged controller
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Presently controllers can GETCONF the ExitPolicy, but not get the actual exit policy we're running with. It would be nice if it was exposed via the control port.

Here's what I ended up writing in stem to get around this...

https://gitweb.torproject.org/stem.git/commitdiff/716f8a6

19:23 < armadev> atagar: re f0ae1eaee, where were you seeing exit policies with redundant lines? 
                 tor tries to get rid of redundant lines too. i wonder if it doesn't do it well 
                 enough, or doesn't do it pervasively enough?
19:24 < armadev> atagar: ...perhaps tor doesn't export its exit policy to the control port at all
19:43 < atagar> armadev: I don't think that it does, so I wrote a method for getting it...
19:43 < atagar> https://gitweb.torproject.org/stem.git/commitdiff/716f8a693e9b814da5e2c9df551dbe6768f4f324
19:43 < atagar> It would be nicer to fetch via the control port though. :)
19:43 < armadev> for these cases, it would be great if you opened a tor ticket to say what you want 
                 exported
19:44 < atagar> ok, will do
19:44 < armadev> i guess there should be a balance between how much work tor does, and how much 
                 work the controller does
19:44 < armadev> but in general, if the argument can go "each controller would have to implement 
                 this thing itself, which is best done in tor", then it should go into tor

Child Tickets

Change History (26)

comment:1 Changed 6 years ago by arma

In particular, Tor relays already compress (throw away redundancy) the exit policy, to put it in the descriptor. So it would be smart for us to offer on the control port whatever we are putting into our descriptor.

comment:2 Changed 6 years ago by arma

Keywords: tor-relay easy added
Milestone: Tor: 0.2.4.x-final

comment:3 Changed 6 years ago by arma

Here's the code from router_dump_router_to_string() that generates the string:

    int i;
    for (i = 0; i < smartlist_len(router->exit_policy); ++i) {
      addr_policy_t *tmpe = smartlist_get(router->exit_policy, i);
      if (tor_addr_family(&tmpe->addr) == AF_INET6)
        continue; /* Don't include IPv6 parts of address policy */
      result = policy_write_item(s+written, maxlen-written, tmpe, 1);
      if (result < 0) {
        log_warn(LD_BUG,"descriptor policy_write_item ran out of room!");
        return -1;
      }
      tor_assert(result == (int)strlen(s+written));
      written += result;
      if (written+2 > maxlen) {
        log_warn(LD_BUG,"descriptor policy_write_item ran out of room (2)!");
        return -1;
      }
      s[written++] = '\n';
    }

It would be smart to break that into its own function, so it can be called from that function and also from control.c.

comment:4 Changed 6 years ago by nickm

Keywords: small-feature added

Anybody able to do this for the small-feature deadline?

comment:5 Changed 6 years ago by nickm

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

comment:6 Changed 6 years ago by cypherpunks

Status: newneeds_information

Do we want GETCONF ExitPolicy to return an entire OR exit policy? Currently it returns only the value of ExitPolicy parameter in torrc. This seems to be consistent with the purpose of GETCONF command. Shouldn't the entire exit policy be retrievable via some other control port command?

Suppose one has refactored the above refenced exit policy dumping code into standalone function. Where in control.c should this function be called?

comment:7 in reply to:  6 Changed 6 years ago by nickm

Status: needs_informationnew

Replying to cypherpunks:

Do we want GETCONF ExitPolicy to return an entire OR exit policy? Currently it returns only the value of ExitPolicy parameter in torrc. This seems to be consistent with the purpose of GETCONF command. Shouldn't the entire exit policy be retrievable via some other control port command?

I would suggest a new instance of the GETINFO command; that's what is used for cases where we need to dump something that isn't a configuration value.

Suppose one has refactored the above refenced exit policy dumping code into standalone function. Where in control.c should this function be called?

GETINFO is table-driven, according to the table in getinfo_items. It also might help to read handle_control_getinfo and/or getinfo_helper_misc.

comment:8 Changed 6 years ago by cypherpunks

Status: newneeds_review

Is there a specific reason why POLICY_BUF_LEN value was 52? I had to bump it up since the new function was unable to represent an entire exit policy in 52 bytes in some cases.

comment:9 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

Thanks! To answer your question, POLICY_BUF_LEN was meant to hold a single IPv4 policy item, not the entire exit policy.

Here are some more comments:

  • router_get_my_routerinfo() is allowed to return NULL, so dereferencing its result without checking it is a bad idea. Similarly, I'd be more comfortable with a check that router->exit_policy is non-NULL, even though that strictly speaking shouldn't be needed.
  • Instead of accumulating these entries in a buffer, it's probably better to put them all in a smartlist, and then use smartlist_join_strings() to concatenate them. (That way we wouldn't need to worry about pointer math, running out of space, or the much-deprecated strcat().)
  • The preferred pattern to iterate over a smartlist is SMARTLIST_FOREACH_BEGIN/SMARTLIST_FOREACH_END.
  • I like the idea of breaking router_dump_exit_policy_to_string() into a new function.
  • Calling smartlist_add_asprintf(chunks, "%s\n", exit_policy); introduces a memory leak, since exit_policy is now never freed.
  • Perhaps there should be a variant GETINFO that *does* return IPv6 policy items? If so, the sensible way to implement it would probably be to add an IPv4/IPv6 flag as an argument to router_dump_exit_policy_to_string().
  • router_dump_exit_policy_to_string() definitely needs a unit test. (Let me know if it's not clear how to write one.)

comment:10 Changed 6 years ago by cypherpunks

Status: needs_revisionneeds_review

Can you provide a review of current practices regarding unit testing? It seems that Tor codebase does not rely on external frameworks/libraries for unit testing and has its own testing infrastructure. 

To implement unit test, do I need to create several instances of routerinfo_t structs, process them with router_dump_exit_policy_to_string() and use tor_assert() to see if I am getting the correct output? Should this code be placed in test/test_dir.c? 

Regarding IPv6 exit policies, do we need to implement GETINFO exit-policy/ipv6? What if torrc define both IPv4 and IPv6 exit policy entries? Maybe we need to implement GETINFO exit-policy/ipv4 to get IPv4-only entries as well. In that case, GETINFO exit-policy should return all (both IPv4 and IPv6) entries.

comment:11 in reply to:  10 Changed 6 years ago by nickm

Replying to cypherpunks:

Can you provide a review of current practices regarding unit testing? It seems that Tor codebase does not rely on external frameworks/libraries for unit testing and has its own testing infrastructure. 

To implement unit test, do I need to create several instances of routerinfo_t structs, process them with router_dump_exit_policy_to_string() and use tor_assert() to see if I am getting the correct output? Should this code be placed in test/test_dir.c? 

That sounds about right. But instead of using tor_assert, use tt_assert. The test framework we use is "tinytest", in src/ext/tinytest* ; the tinytest_demo.h file should explain how it works and give some idea of how to work with it.

Creating a new test/test_policies.c might also be a good idea. Also, there's no need to generate a full routerinfo_t here; filling in the policies structure should be enough.

Let me know if

Regarding IPv6 exit policies, do we need to implement GETINFO exit-policy/ipv6? What if torrc define both IPv4 and IPv6 exit policy entries? Maybe we need to implement GETINFO exit-policy/ipv4 to get IPv4-only entries as well. In that case, GETINFO exit-policy should return all (both IPv4 and IPv6) entries.

Yes, this all sounds quite reasonable to me.

Changed 6 years ago by cypherpunks

comment:12 Changed 6 years ago by cypherpunks

Please look at unit test I wrote. I am not exactly sure about the extent of testing the function. What cases of exit policy should be used for testing?

I see that routerinfo_t has a field

  /** What streams will this OR permit to exit on IPv6?
   * NULL for 'reject *:*' */
  struct short_policy_t *ipv6_exit_policy;

What is the purpose of this one? Can't exit_policy smartlist hold both IPv4 and IPv6 entries?

comment:13 Changed 6 years ago by nickm

The current testing looks okay to me.

short_policy_t is for policy summaries objects --they aren't the real policy, but cover only a port range.

I'll be happy to merge this once it can dump the elements for IPv6, IPv4, or both onto the controller.

comment:14 Changed 6 years ago by nickm

I've also started collecting this in a branch "bug7952" in my public repository at https://git.torproject.org/nickm/tor.git .

Also for reference, we find it far less confusing to take pseudonymous patches than anonymous ones. Is there a nickname you'd be willing to use for these?

comment:15 in reply to:  14 Changed 6 years ago by cypherpunks

Replying to nickm:

I've also started collecting this in a branch "bug7952" in my public repository at https://git.torproject.org/nickm/tor.git .

Also for reference, we find it far less confusing to take pseudonymous patches than anonymous ones. Is there a nickname you'd be willing to use for these?

Please credit the code I contributed to "rl1987".

comment:16 Changed 5 years ago by andrea

Keywords: 025-triaged added

comment:17 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Okay. I squashed those commits, then made some more cleanups in branch "bug9752_squashed". Then I squashed again in branch "bug9752_final". Now I'm merging that. Thanks! Please let me know if I messed anything up.

comment:18 Changed 5 years ago by nickm

Added documentation to control-spec as 42b373af2ed3fb006c01ab584d36e89228588b28

comment:19 Changed 5 years ago by rl1987

Keywords: controller added

Looks good to me.

Note: See TracTickets for help on using tickets.