Opened 6 years ago

Last modified 23 months ago

#9729 needs_revision enhancement

Make bridges publish additional ORPort addresses in their descriptor

Reported by: sqrt2 Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.5.1-alpha
Severity: Normal Keywords: ORPort, censorship, pt, multiple, addresses, andrea-review, tor-bridge
Cc: isis@…, ln5, daube Actual Points:
Parent ID: Points: 2
Reviewer: Sponsor: SponsorS-can

Description

I run Tor 0.2.4.17-rc as a bridge configured with multiple IPv4 addresses and one ORPort line for each, like so:

ORPort <n1>:443
ORPort <n2>:443
...
ORPort <nN>:443

When starting tor, the log will show that only <n1> is self-tested and a bridge descriptor is published:

[notice] Now checking whether ORPort <n1>:443 is reachable... (this may take up to 20 minutes -- look for log messages indicating success)
[notice] Self-testing indicates your ORPort is reachable from the outside. Excellent. Publishing server descriptor.

Tor should publish bridge descriptors for all the available addresses and the bridge authorities should probably put them into pools of different confidentiality levels.

For clarity, the documentation should explain Tor's bandwidth allocation as configured with Relay(Bandwidth|Burst)Rate (per address or total).

Child Tickets

Attachments (35)

0001-Make-bridges-publish-additional-addresses-in-their-d.patch (54.3 KB) - added by sqrt2 5 years ago.
proposed patch (without unused variable)
0002-First-review-round.patch (29.7 KB) - added by sqrt2 5 years ago.
Fixes from first review round
0003-Second-review-round.patch (21.8 KB) - added by sqrt2 5 years ago.
0001-Small-fixes-to-address.-ch.patch (3.6 KB) - added by daube 5 years ago.
first of three commits by daube
0002-add-tests-for-address-functions-changed-by-9729.patch (7.1 KB) - added by daube 5 years ago.
second of three patches by daube
0003-remove-depricated-test-of-get_interface_address.patch (1.8 KB) - added by daube 5 years ago.
third of three patches by daube
0004-fixes-to-address.-ch-and-test_addr.c.patch (3.1 KB) - added by daube 5 years ago.
fixes to bring daube's tests in line with 2nd review patch from sqrt2
0004-fixes-to-address.-ch-and-test_addr.c.2.patch (3.1 KB) - added by daube 5 years ago.
fixes to bring daube's tests in line with 2nd review patch from sqrt2
0008-fix-compiler-warnings-in-address-tests.patch (1.3 KB) - added by daube 5 years ago.
0009-update-comments-in-test_addr.c.patch (1.5 KB) - added by daube 5 years ago.
0010-test_addr.c-fix-compiler-warnings-simplify.patch (2.4 KB) - added by daube 5 years ago.
0011-small-fixes-to-address.c.patch (1.4 KB) - added by daube 5 years ago.
0012-add-tor_addr_clone-and-tests-for-it.patch (3.0 KB) - added by daube 5 years ago.
0013-s-assert-tor_assert-in-config.c.patch (700 bytes) - added by daube 5 years ago.
0014-fix-compiler-warnings-in-tor_addr_clone.patch (822 bytes) - added by daube 5 years ago.
0015-fix-compiler-C90-warnings-in-or-config.-ch.patch (4.1 KB) - added by daube 5 years ago.
0016-remove-extraneous-semicolon.patch (795 bytes) - added by daube 5 years ago.
0017-fix-warnings-in-nodelist.-ch.patch (1.8 KB) - added by daube 5 years ago.
0018-fix-small-issue-in-my-compiler-warning-popping.patch (3.8 KB) - added by daube 5 years ago.
0019-fix-warnings-in-or-router.-ch-routerparse.-ch.patch (3.3 KB) - added by daube 5 years ago.
0001-test_addr-add-mocking-fix-constness.patch (5.8 KB) - added by daube 4 years ago.
0002-fix-comment-in-mocked_get_iface_addr_raw.patch (885 bytes) - added by daube 4 years ago.
0003-address.c-make-copy-eq-fns-for-tor_addr_port_t.patch (2.9 KB) - added by daube 4 years ago.
0004-dirserv.c-break-up-complex-conditionals.patch (6.1 KB) - added by daube 4 years ago.
0005-nodelist.c-tidy-up-nodelist-module.patch (4.8 KB) - added by daube 4 years ago.
0006-s-tor_addr_compare-tor_addr_eq-where-appropriate.patch (8.3 KB) - added by daube 4 years ago.
0007-test_addr-add-more-tests-and-mock-more-things.patch (7.1 KB) - added by daube 4 years ago.
0008-get_interface_addr6-Test-mocking-goto-exit.patch (5.7 KB) - added by daube 4 years ago.
0009-or-nodelist-Make-a-few-local-functions-static.patch (1.7 KB) - added by daube 4 years ago.
0010-test_addr-Use-mocking-in-get_stable_addr6-test.patch (3.9 KB) - added by daube 4 years ago.
0011-test_addr.c-add-test-for-tor_addr_port_eq.patch (3.8 KB) - added by daube 4 years ago.
0012-test_addr-More-tests-of-test_addr_port_eq.patch (1.6 KB) - added by daube 4 years ago.
0013-Fix-all-checkSpaces-issues.patch (9.4 KB) - added by daube 4 years ago.
0014-address.c-const-ify-smartlist-loops.patch (2.1 KB) - added by daube 4 years ago.
0001-Allow-tor_addr_port_eq-NULL-NULL-to-be-true.patch (1.4 KB) - added by daube 4 years ago.
Address a comment of Andrea's

Download all attachments as: .zip

Change History (103)

comment:1 Changed 6 years ago by sqrt2

sysrqb has confirmed that there is only a descriptor for <n1> known to BridgeDB, and there are no additional "a" lines in the descriptor.

The other addresses don't appear in the log even at debug log level.

comment:2 Changed 6 years ago by sqrt2

Version: Tor: 0.2.4.16-rcTor: 0.2.4.17-rc

comment:3 Changed 6 years ago by sqrt2

Summary: Make bridges publish a bridge descriptor per ORPort addressMake bridges publish additional ORPort addresses in their descriptor

comment:4 in reply to:  1 Changed 6 years ago by isis

Replying to sqrt2:

sysrqb has confirmed that there is only a descriptor for <n1> known to BridgeDB, and there are no additional "a" lines in the descriptor.

There is only supposed to be one "a" line in a microdescriptor.

The other addresses don't appear in the log even at debug log level.


Out of curiousity, the extra <n2>, <n3, etc. addresses you're adding: are they IPv4 addresses?

Because there was this strange thing that I just noticed yesterday where the "or-address" lines in the @type-bridge-server-descriptors are only IPv6 addresses, never IPv4. The same thing goes for the "a" lines; they are always IPv6 addresses for bridge descriptors.

bridgedb@ponticum:/srv/bridges.torproject.org$ cat from-authority/networkstatus-bridges | grep '^a ' | sort | uniq | wc -l
115
bridgedb@ponticum:/srv/bridges.torproject.org$ cat from-authority/bridge-descriptors | grep 'or-address' | sort | uniq | wc -l
122

The extra ones in those 122 addresses could just be newer bridges, because two of my test bridges are among them. Also doing

bridgedb@ponticum:~$ cat /srv/bridges.torproject.org/from-authority/networkstatus-bridges | grep -Pzo 'a .*\na .*'


comes up with two descriptors which have two "a" lines, but on closer inspection my grep was matching an "a" on the end of the nickname string for two different bridges, not the beginning of the line.

So there are no @type bridge-networkstatus nor @type bridge-server-descriptor descriptors with multiple "a" or "or-address" lines. When either of these lines do occur, they are always IPv6 addresses.

comment:5 Changed 6 years ago by sqrt2

Yes, these are all IPv4 addresses (though I could also add reachable IPv6 addresses if that helps with testing).

The fact that in reality there are only IPv6 addresses appears to be a due to missing code that would make anything else happen; quoting struct routerinfo_t from or.h:

  /** A router's IPv6 address, if it has one. */
  /* XXXXX187 Actually these should probably be part of a list of addresses,
   * not just a special case.  Use abstractions to access these; don't do it
   * directly. */
  tor_addr_t ipv6_addr;
  uint16_t ipv6_orport;

comment:6 Changed 6 years ago by isis

Cc: isis@… added
Status: newneeds_information
Version: Tor: 0.2.4.17-rcTor: 0.2.5.1-alpha

From L1448-1552 in torspec.git/dir-spec.txt:

     "a" SP address ":" port NL

        [Any number]

        The "or-address" element as specified in 2.1.

would seem to imply that we can have as many as we want (at least it won't affect descriptor parsing), and that they can be IPv4 or IPv6 addresses.

though in tor.git/src/or/router.c in router_rebuild_descriptor(), the routerinfo_t *ri only has one extra orport, which is always used for IPv6:

  /* For now, at most one IPv6 or-address is being advertised. */
  {
    const port_cfg_t *ipv6_orport = NULL;
    SMARTLIST_FOREACH_BEGIN(get_configured_ports(), const port_cfg_t *, p) {
      if (p->type == CONN_TYPE_OR_LISTENER &&
          ! p->no_advertise &&
          ! p->bind_ipv4_only &&
          tor_addr_family(&p->addr) == AF_INET6) {
        if (! tor_addr_is_internal(&p->addr, 0)) {
          ipv6_orport = p;
          break;
[...]
    if (ipv6_orport) {
      tor_addr_copy(&ri->ipv6_addr, &ipv6_orport->addr);
      ri->ipv6_orport = ipv6_orport->port;
    }


So this behaviour is not a bug, though for bridges it is certainly desirable to have more than one address per bridge.

If you make bridges send multiple descriptors to the BridgeAuthority, there could be some problems:

  1. The same bridge (in this case, by "bridge" I mean "box") would have multiple entries in BridgeDB's hash rings, meaning an increased likelihood of being handed out. Similarly, in the "social bridge disributor" design for BridgeDB, because this one box has more descriptors in the database, it's likelihood of being handed out is correspondingly higher.

This is probably fine as long as each of these descriptors is a different IP address, and there is not an easy way, given one of these IPs/descriptors, to discover the other ones (i.e. via a reverse DNS query and then a regular DNS query to get the other addresses for that host).

  1. You might have to do a bit of juggling to keep track of which descriptor was updated and resent to the BridgeAuthority when, though my guess is that in practice routers with multiple IP routes configured are not going to be shifting IP addresses too much.
  1. You probably should not allow multiple descriptors to contain the same address. I.e. something like:
    Bridge
      |- desc1
      |    |- ORPort 1.1.1.1:443
      |    |- ORPort 12.12.12.12:4443
      |    |_ ServerTransportListenAddr obfs3 1.1.1.1:3333
      |
      |_ desc2
           |- ORPort 2.2.2.2:2222
           |_ ORPort 12.12.12.12:6666
    

would be bad for the following reasons:

  1. Double the chance of handing out 12.12.12.12, so double the chance it gets blocked.
  2. Since these separate descriptors would essentially be separate bridges, for the social bridge distribution scheme (the last of the redesign is being hammered out now), if desc1 got blocked (i.e. the 1.1.1.1 IP address was blackholed), then desc2 would also be blocked via the 12.12.12.12 address. In this case, the clients of 2.2.2.2 would stop earning coins for the desc2 bridge, since it's been blocked. It might be possible to further revise the scheme to deal with cases like this, but you can see how it would start to get hairy.

My preference would be to have one descriptor per IP address (that way PTs can run on that same address under alternate ports). Perhaps nickm and others have different concerns, however.

comment:7 Changed 6 years ago by sqrt2

Contrary to what I wrote initially when I said Tor should publish "descriptors for each address", I think that the OR should not be trusted with deciding how exactly its addresses are going to be published, as not to make it possible to corrupt BridgeDB in the sense that statistics would show many bridges, but many of them are easily blocked as a group due to "treacherous" descriptors. (An odd type of attack, admittedly.) I agree that it is probably best if BridgeDB then treats each IP address as a separate bridge when it comes to handing out bridge addresses.

Therefore, staying within the scope of this ticket, I think that Tor should publish just one descriptor with additional "a" lines containing both alternative IPv4 and IPv6 addresses. (I suppose for the BridgeDB side it would then make sense to open a separate ticket against the BridgeDB component?)

comment:8 Changed 6 years ago by isis

For BridgeDB, there is not a specific ticket, but the problem of how to treat multiple addresses assigned to one "bridge" (and how to define exactly what one "bridge" is) is dealt with in multiple tickets as this is currently an ongoing issue. See #8614, #9652, and #9626.

Also, there was proposal 186-multiple-orports, the status of which is "partially implemented" due to, it seems, the ADDRLIST which you previously mentioned not being implemented yet. Since the proposal was previously accepted, I'd recommend reading it and searching around for the tickets related to it, and talk to whoever worked on it to determine the best way forward.

comment:9 Changed 6 years ago by nickm

I think that it's fine to publish as many "or-address" lines per descriptor as we want. What would be harder is getting Tor to handle addresses beyond the first IPv4 address and the first IPv6 address.

The partial state of 186 is because it was much easier to implement the case with exactly one of each address type than it was to get clients to use additional addresses of each type. It also seemed hard to get authorities to do the right thing with testing large numbers of addresses, in the timeframe we had.

comment:10 Changed 5 years ago by sqrt2

I've been looking through the code, and (considering that I can use my alternate bridge addresses as Bridge and that works flawlessly) my impression is that as an OR, reachability self-testing is the main area that currently depends on addr/or_port being simple uints.

It is also my impression, considering that IPv6 self-testing is currently not being done at all, that this ticket could be addressed with another hack. We coul add another field for additional IPv4 addresses/ports to routerinfo_t, and modify router_dump_router_to_string() and router_parse_entry_from_string() appropriately. In other words, we would be ignoring reachability testing for the additional ORPorts completely.

(One could do the same for additional IPv6 addresses, but because blocking entire netranges is much more dangerous for IPv4 than it is for IPv6, additional IPv4 bridge addresses are probably a lot more useful to the Tor network than is the case for IPv6 addresses.)

Am I missing something or would that work, and should we do it?

comment:11 Changed 5 years ago by nickm

I think that if it's bridges-only, and we have some mechanism to make sure that the test for each happens at some point, it's not a huge issue where the test occurs in the first revision; whether it's a self-test or a test performed by the bridge authority or by bridgedb, or something else entirely.

So long as testing is happening _somewhere_, adding an additional-addresses field to routerinfo wouldn't be a horrible thing.

That said, we shouldn't give users bridge addresses without something testing them.

comment:12 Changed 5 years ago by sqrt2

Reachability self-checking for multiple addresses requires that we can EXTEND a circuit to a specific IP address (and not just a specific OR) and that we can detect to which address a CREATE cell was sent.

Directory authorities, however, just do a TLS handshake with a specific address/port, and it shouldn't be too hard to adapt dirserv_single_reachability_test() to do this for every address in the descriptor.

I'll try to write a patch implementing these features in the coming days.

comment:13 Changed 5 years ago by nickm

ok. best to make sure this gets rate-limited, so that a dirserver doesn't go crazy if it sees a node with 100 ports. This should also be bridges-only for now.

Eventaully, we will want the self-test to happen, so that nodes don't accidentally make themselves get rejected if they have one non-working address out of some large number.

comment:14 Changed 5 years ago by ln5

Cc: ln5 added

comment:15 Changed 5 years ago by sqrt2

Status: needs_informationneeds_revision

OK, here it goes. What this is basically supposed to do is

  • Make address detection handle interfaces that have multiple addresses of the same address family better. get_interface_address{,6} returns a list, get_stable_interface_address still returns a single address that only changes when it must.
  • If we are a bridge, add additional listener addresses to our descriptor. (This is routerinfo_t.more_or_listeners.)
  • Make additional or-address lines in descriptors parseable.
  • Give directory authorities the ability to test the reachability of additional addresses in the descriptor. (Limited to 7, for now.)

I've tested this some, but I'd appreciate it of course if someone else gave it a spin as well.

comment:16 Changed 5 years ago by sqrt2

I have updated the attached patch to a version that I believe to be working properly. I've also written an explanation of the bigger changes in this patch to hopefully make understanding what it does easier:

First, if we consider tor to possibly have multiple OR listener addresses, when detecting addresses on an interface, we must be able to return multiple addresses. Therefore, we modify get_interface_address6() to return a smartlist_t. Now, during configuration, we must take into account that not any routable IP address on an interface is in fact a good address to use. For this, we add find_good_addr_from_list(), that will pick one of the addresses returned by get_interface_address6(), preferring addresses that the user has explicitly configured for our purpose. (resolve_my_address() has gained a parameter "listener_type" that specifies this purpose: OR, directory, etc.)

We must now take care of the fact that get_interface_address6() is also used by client_check_address_changed() to find if a client needs to rekey because its address has changed. Because depending on the operating system to always return interface addresses in the same order seems like asking for trouble, we need a mechanism to return an interface address that only changes if we can't use the previous one anymore (the interface isn't configured with this address anymore). This mechanism is get_stable_interface_address6().

We also modify test_addr.c to make it compile with the new get_interface_address6().

The address found by resolve_my_address() ends up as our main IPv4 address. To maintain compatibility with existing IPv6 code, we also need to find a main IPv6 address to put into routerinfo_t.ipv6_addr. This code is now in router_get_main_ipv6_listener_address(). Together, these can be queried in router_get_main_listener_address_by_af().

Sometimes, we need to find our OR listener port and don't have a routerinfo_t to look it up. Previously, we would just iterate through open connections in router_get_active_listener_port_by_type_af(). However in the presence of multiple listeners per address family, we need to also specify the exact listener address to do this in a stable manner. As the old function only gets called in situations where we also know the listener address, this function is now router_get_active_listener_port_by_addr_type_af() and associated functions have been changed accordingly.

Finally, we add a smartlist_t *more_or_listeners to routerinfo_t and, if we are a bridge, populate it with all the additional addresses in router_rebuild_descriptor(). We modify router_dump_router_to_string() and router_parse_entry_from_string() to include these additional addresses. We also adapt various other functions concerned with querying a router for addresses or ports.

We want the bridge authorities to test the reachability of these additional addresses. For this purpose, we modify dirserv_single_reachability_test() to test at most 7 additional addresses (in order not to overload the bridge authorities).

Because we're checking multiple addresses for reachability now, we need to keep track of reachability for each address separately. For this, we add a struct addr_reachability_t that contains a tor_addr_port_t and the time we could reach this address/port last. The last_reachble and last_reachble6 fields of node_t are replaced by a smartlist_t *last_reachable. To manage this new field, we add node_set_last_reachability() and node_get_af_last_reachability(), and node_af_reachable_since() with helper functions addr_replied(), all_listeners_replied(). Finally, we modify various functions in dirserv.c to make use of this new API.

Last edited 5 years ago by sqrt2 (previous) (diff)

Changed 5 years ago by sqrt2

proposed patch (without unused variable)

comment:17 Changed 5 years ago by ln5

Lacking the brains for a high-level review, I'm going for a partial
look-at-details review for now, in the hope that it might help shake
out some issues.

Also, note that most of router*.[ch] and test/test_addr.c have _not_
been reviewed yet.

  • are we leaking memory in get_interface_address6() in "smart way" when return_addrs is empty after the loop (returning NULL)?
  • the construct with r = return_addrs in get_interface_address6() is a bit odd IMO; the test !r will never be true f.ex.
  • are we leaking memory (the list in r) in get_interface_address()?
  • slight code duplication in dirserv_orconn_tls_done() when calling node_set_last_reachability()
  • is the check for the ipv6 address in dirserv_single_reachability_test() backwards? i.e. should check for tor_addr_compare() == 0
  • did router_get_active_listener_port_by_addr_type_af() get the tor_addr_compare() test backwards too?
  • the call to tor_free() in router_get_advertised_or_port_by_af() (at router.c:1508) is never reached and we're leaking that memory
  • needs a ChangeLog entry (i.e. a file in changes/)
  • documentation for last_discovered_ipv*_address refers to a function that does not exist (change to get_stable_interface_address6?)

Formatting and style.

  • remove trailing whitespace
  • no need to break a line like "port_cfg_t * router_get_main_ipv6_listener_address(const smartlist_t *ports);"
  • i'd add curly braces to constructs like

else

/* We got here from options_validate(), and we're just checking if we're

  • globally reachable and can be a directory authority. This function will
  • be called again when ports have been configured and we can pick the right
  • address then. */

log_fn(notice_severity, LD_CONFIG,

"We have at least one usable address, that's enough right now.");

  • indentation is wrong in a couple places (like address.c:1479)
  • long lines here and there (see 'Wide' when doing "make check-spaces")
  • no need for '\' to continue a line (like dirserv.c:2966)
  • i'd break lines like this in two:

if (chan) command_setup_channel(chan);

comment:18 Changed 5 years ago by sqrt2

Thanks for taking a look :) I'm going to attach a patch with changes as follows:

are we leaking memory in get_interface_address6() in "smart way" when return_addrs is empty after the loop (returning NULL)?

Yes. Fixed that.

the construct with r = return_addrs in get_interface_address6() is a bit odd IMO; the test !r will never be true f.ex.

The test !r will be true whenever we got there through goto err. (I've added a comment to make this clearer.)

are we leaking memory (the list in r) in get_interface_address()?

Fixed.

slight code duplication in dirserv_orconn_tls_done() when calling node_set_last_reachability()

Fixed.

is the check for the ipv6 address in dirserv_single_reachability_test() backwards? i.e. should check for tor_addr_compare() == 0

Fixed.

did router_get_active_listener_port_by_addr_type_af() get the tor_addr_compare() test backwards too?

Damnit, fixed.

the call to tor_free() in router_get_advertised_or_port_by_af() (at router.c:1508) is never reached and we're leaking that memory

Fixed.

needs a ChangeLog entry (i.e. a file in changes/)

I will create one when I know that I haven't missed something major or done something otherwise stupid.

documentation for last_discovered_ipv*_address refers to a function that does not exist (change to get_stable_interface_address6?)

Fixed.

remove trailing whitespace

Fixed.

no need to break a line like "port_cfg_t * router_get_main_ipv6_listener_address(const smartlist_t *ports);"

I was under the impression that there is an 80-character by line limit, also in header files.

i'd add curly braces to constructs like [...]

OK.

long lines here and there (see 'Wide' when doing "make check-spaces")

Fixed.

indentation is wrong in a couple places (like address.c:1479)

What do you mean? It looks fine to me.

no need for '\' to continue a line (like dirserv.c:2966)

I wish I had an ISO keyboard with a large enter key :( (The backspace is right above Enter on mine.)

i'd break lines like this in two: [...]

That's not strictly my code and appears like this also in circuitbuild.c, so I'll leave it alone.

Changed 5 years ago by sqrt2

Fixes from first review round

comment:19 Changed 5 years ago by nickm

So sorry for the long delay. I'll try to finish review by end of year.

In the meantime, the most useful thing to add here would be unit tests for all the new code, to whatever extent is possible. (The new mocking code in 0.2.5 should make it possible to test stuff that might not have been very testable before.

Can you describe in detail how you've tested this ? (Apologies if you've already said; just link if so.)

QUick notes:

  • In get_interface_address6(), we used to never return internal addresses, but it looks like that code got removed?
  • In get_interface_address6(), are there still any leaks of addr? For example, in the case where we return NULL instead of goto err?
  • In get_stable_interface_address6(), do we leak 'list' ?
  • All of the functions that allocate a new thing for their return value should document "returns a newly allocated copy of". Example: get_first_address_by_af. Or perhaps it should take a const smartlist_t * and return a reference to the appropriate element.
  • get_interface_address -- is it still necessary?
  • Why is find_good_addr_from_list IPv4-only? IT seems that having it take a tor_addr_t would make it more robust.


I will try to find more stuff later. I need to remember that just because a patch is long doesn't mean that I can't usefully review part of it. :/

comment:20 Changed 5 years ago by sqrt2

Also sorry for the delay, unfortunately I didn't get to do any coding at 30c3 :(

Replying to nickm:

In the meantime, the most useful thing to add here would be unit tests for all the new code, to whatever extent is possible. (The new mocking code in 0.2.5 should make it possible to test stuff that might not have been very testable before.

I assume you're talking about the MOCK* macros from test/testsupport.h. I will take a look at this.

Can you describe in detail how you've tested this ? (Apologies if you've already said; just link if so.)

I haven't done any rigorous testing. What I've done is (making use of a number of globally routable addresses I have) built a small network with a bridge directory server and a bridge with AlternateBridgeAuthority, adding addresses to the the local interface and configuring some of them/all of them/none of them as ORPorts, then observing that the bridge detects its proper addresses and that the directory authority learns of the bridge and its addresses.

So far, I've tested this with IPv4 only.

  • In get_interface_address6(), we used to never return internal addresses, but it looks like that code got removed?

In resolve_my_address(), we want to allow explicitly configured internal addresses (except if we are a DirAuth, step three). get_interface_address6() now also returns internal addresses so find_good_addr_from_list(allow_internal=1) can match these to any explicitly configured addresses.

  • In get_interface_address6(), are there still any leaks of addr? For example, in the case where we return NULL instead of goto err?

Yes, fixed.

  • In get_stable_interface_address6(), do we leak 'list' ?

Fixed.

  • All of the functions that allocate a new thing for their return value should document "returns a newly allocated copy of". Example: get_first_address_by_af. Or perhaps it should take a const smartlist_t * and return a reference to the appropriate element.

Fixed.

  • get_interface_address -- is it still necessary?

I don't think so. I've removed it now.

  • Why is find_good_addr_from_list IPv4-only? IT seems that having it take a tor_addr_t would make it more robust.

Fixed.

I have also added some code to handle the case where we guess a hostname, but the resolved address is not actually one of the configured ORPorts.

Changed 5 years ago by sqrt2

comment:21 Changed 5 years ago by daube

Cc: daube added

comment:22 Changed 5 years ago by daube

Hi all,

I've finally got time to add some tests. I've so far written some for all the bits that changed in common/address.c.

Additionally, some small "bugfixes" have been added to common/address.c, mostly using smartlist_free to free all memory allocated in smartlist_new(), not just the pointer to the struct.

Let me know if i've screwed anything up.

daube

Changed 5 years ago by daube

first of three commits by daube

Changed 5 years ago by daube

second of three patches by daube

Changed 5 years ago by daube

third of three patches by daube

comment:23 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-final
Status: needs_revisionneeds_review

comment:24 Changed 5 years ago by ln5

Trying to make a branch.

@sqrt2: 0001-Make-bridges-publish-additional-addresses-in-their-d.patch doesn't apply to any of tor-0.2.4.17-rc (or -16 or -18) or master@{2013-11-17}. What is the baseline?

@sqrt2: You could try out Chutney which runs a local tor network on localhost. No need for publicly reachable addresses. https://gitweb.torproject.org/chutney.git

comment:25 Changed 5 years ago by daube

@ln5:
All of my patches are applied on top of @sqrt2's patches, if that helps.

Will take a look at chutney for tests.

daube

Changed 5 years ago by daube

fixes to bring daube's tests in line with 2nd review patch from sqrt2

Changed 5 years ago by daube

fixes to bring daube's tests in line with 2nd review patch from sqrt2

comment:26 Changed 5 years ago by daube

Ignore that second identically named patch, they're the same. I accidentally clicked submit twice. How does one remove a patch?

comment:27 Changed 5 years ago by ln5

@daube would you be able to publish a git branch containing all the different patches somewhere?

comment:29 Changed 5 years ago by nickm

Here's a preliminary code review.

In address.c:

  • The code in get_stable_interface_address6 seems to assume that 'family' is AF_INET or AF_INET6. Maybe it should check that early on.
  • It would probably be good idea to have a function to malloc and copy a single address; that pattern happens a lot.
  • get_stable_interface_address6 looks like it could benefit from the "goto end" pattern of having a single point of cleanup.

In config.c:

  • We use tor_assert, not assert().
  • XXX come back and re-review resolve_my_address. It's too complicated. :(

In dirserv.c:

  • Wow, that conditional in dirserv_set_router_is_running is pretty complicated. Can we turn it into a function?
  • Local variables named "l" are begging for confusion with numberes named "1".

In nodelist.c:

  • In node_set_last_reachability, what is the point of setting ar->addrport.port and ar->addrport.addr immediately after checking whether those fields are equal to the values you're about to set them to? And why are we copying ap->addr into *addr?
  • The other new functions here need documentation.
  • addr_replied should have a name that makes clear that it's a predicate. Perhaps "addr_has_replied"? Similarly with all_listenres_have_Replied. Also, its arguments should be const.
  • all_listeners_have_replied looks a little expensive. We should make sure it isn't in a critical path.
  • Some of these functions should probably be static. Or STATIC, if they are getting tested directly.

test_addr.c:

  • Maybe some of these tests should use the test mocking functionality, so that they don't depend so heavily on the actual IP addresses of the running host?

Throughout:

  • Probably most of the loops should be declaring the type as "const tor_addr_t *", not "tor_addr_t *".
  • Try building this branch after running configure with "--enable-gcc-warnings". That will turn on a bunch of warnings that aren't on by default, but which we use to write cleaner code.
  • Most tor_addr_compare usage could be more cleanly written as tor_addr_eq.
  • Try to write if-else statements with braces on both the "if" and "else" clauses. Leaving braces out makes the code a little harder to review, since I need to make sure that the indentation is right.
  • This could still use more testing. Taking a look at the results of running the test coverage tools (see doc/HACKING for more info), I see that only about 1/3 of the new or modified lines have any tests reaching them.

comment:30 Changed 5 years ago by andrea

Keywords: 025-triaged added

comment:31 Changed 5 years ago by nickm

Keywords: nickm-review-0255 added

comment:32 Changed 5 years ago by nickm

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

Okay, wrt the unfixed issues from early March, the size of the diff, and the amount of untested code[*], I don't believe we can get this in for 0.2.5.

[*] messing around with coverage, I find that ~120 of the new or modified lines here have test coverage, and ~400 don't. That's not so safe.

comment:33 Changed 5 years ago by nickm

Keywords: nickm-review-0255 removed

Changed 5 years ago by daube

Changed 5 years ago by daube

Changed 5 years ago by daube

Changed 5 years ago by daube

Changed 5 years ago by daube

comment:34 Changed 5 years ago by daube

G'day all,

I'm getting close to knocking off all the issues listed in Nick's comment 29. But, before I do any more, I'd like to make sure that:

A), I'm on the right track with the use of the MOCK macro and friends,
and, B), That these changes are mergeable into master. At the minute, it looks as though the merge would conflict rather dramatically. Is there anything I can do to fix that? Should I branch off the current master, and try to re-apply my (and Benedikt's?) commits, or is there some more intelligent way?

The current HEAD of my changes is here:
https://github.com/kdmurray91/kdm-tor/tree/tik9729tests

Thanks, and excuse my novice-ness,
daube

comment:35 in reply to:  29 Changed 5 years ago by daube

See inline updates below. I'll keep editing this comment as I finish things off.

daube

Replying to nickm:

Here's a preliminary code review.

In address.c:

  • The code in get_stable_interface_address6 seems to assume that 'family' is AF_INET or AF_INET6. Maybe it should check that early on.

Fixed

  • It would probably be good idea to have a function to malloc and copy a single address; that pattern happens a lot.

Implemented, it's called tor_addr_clone:
tor_addr_t *tor_addr_clone(const tor_addr_t *src);

  • get_stable_interface_address6 looks like it could benefit from the "goto end" pattern of having a single point of cleanup.

In config.c:

  • We use tor_assert, not assert().

Fixed

  • XXX come back and re-review resolve_my_address. It's too complicated. :(

In dirserv.c:

  • Wow, that conditional in dirserv_set_router_is_running is pretty complicated. Can we turn it into a function?

Refactored out. We now check if a router is contactable in a static function. There's also a new static helper around node_af_reachable_since that takes care of IPv4 & IPv6 called node_reachable_since.

  • Local variables named "l" are begging for confusion with numberes named "1".

Couldn't find this (sorry). Unless you're referring to variables named ri or rl?

In nodelist.c:

  • In node_set_last_reachability, what is the point of setting ar->addrport.port and ar->addrport.addr immediately after checking whether those fields are equal to the values you're about to set them to? And why are we copying ap->addr into *addr?

Clean, tidied and a memory leak fixed (I think).

  • The other new functions here need documentation.

I've done my best

  • addr_replied should have a name that makes clear that it's a predicate. Perhaps "addr_has_replied"? Similarly with all_listenres_have_Replied. Also, its arguments should be const.

Fixed, they now have 'has/have' in their names. And params are const X.

  • all_listeners_have_replied looks a little expensive. We should make sure it isn't in a critical path.

I'll let someone more familiar with the codebase as a whole speak to this.

  • Some of these functions should probably be static. Or STATIC, if they are getting tested directly.

Namely? Does making all_listeners_have_replied and addr_has_replied static make sence. Can't see any issues with this myself. I won't commit this yet though.

test_addr.c:

  • Maybe some of these tests should use the test mocking functionality, so that they don't depend so heavily on the actual IP addresses of the running host?

I've tried to mock out get_interface_addresses_raw, which seems to be the root function in getting "real" addresses. Does this approach make sense, and have I done so correctly?

Throughout:

  • Probably most of the loops should be declaring the type as "const tor_addr_t *", not "tor_addr_t *".

Will work on this.

  • Try building this branch after running configure with "--enable-gcc-warnings". That will turn on a bunch of warnings that aren't on by default, but which we use to write cleaner code.

It's clean now!

  • Most tor_addr_compare usage could be more cleanly written as tor_addr_eq.

Done

  • Try to write if-else statements with braces on both the "if" and "else" clauses. Leaving braces out makes the code a little harder to review, since I need to make sure that the indentation is right.

Will work on this too.

  • This could still use more testing. Taking a look at the results of running the test coverage tools (see doc/HACKING for more info), I see that only about 1/3 of the new or modified lines have any tests reaching them.

This I will work on. I have gcov working now.

comment:36 Changed 5 years ago by nickm

Keywords: 026-triaged-1 added; 025-triaged removed
Status: needs_revisionneeds_review

comment:37 Changed 4 years ago by nickm

Status: needs_reviewneeds_revision

ping; any progress here?

comment:38 Changed 4 years ago by daube

Sorry Nick, I've been neglecting this for too long.

I actually fixed most of the outstanding items from my last post, the only thing left to do is more testing of non-trivial routing code that I don't fully understand. I'm keen to write these tests but I may need specific guidance in some cases. I'm going to rebase this branch onto current master, and then make a single patch file with the whole set of changes. Is this how you'd like the patch?

regards,
daube

comment:39 Changed 4 years ago by nickm

That sounds fine, though generally I prefer that stuff that's already been reviewed and stuff that hasn't been reviewed yet go into separate patches if possible.

comment:40 Changed 4 years ago by daube

Hi Nick,

Ive attempted the rebase, and there are some conflicts that I'm not sure I can resolve with 100% certainty of not screwing something up. Thus, I've attached the patches that are new, and will let someone more familiar with the history of changes to master do the rebase.

Thanks, daube.

Changed 4 years ago by daube

comment:41 Changed 4 years ago by nickm

Status: needs_revisionneeds_review

comment:42 Changed 4 years ago by nickm

Hi! I went to review this and I'm afraid I'm a little confused by the right order to take all of these patches in. Could you possibly put a branch on github or some other hosting site for me to look at?

comment:43 Changed 4 years ago by daube

Hi Nick,

Sorry, the patches I've made are a bit of a mess. I'm not used to working with a patch-based contribution system.

There's a branch up here:
https://github.com/kdmurray91/kdm-tor/tree/tik9729tests

As always, github will generate an all-in-one patch against master:
https://github.com/kdmurray91/kdm-tor/compare/tik9729tests.patch

or between the last patch you've reviewed to my latest work:
https://github.com/kdmurray91/kdm-tor/compare/20718a4a0b9dd572e70d36661be5bee3d06559f0...tik9729tests.patch

comment:44 Changed 4 years ago by nickm

Keywords: andrea-review added

comment:45 Changed 4 years ago by andrea

Milestone: Tor: 0.2.6.x-finalTor: 0.2.7.x-final
Status: needs_reviewneeds_revision

--- begin code review ---

fca9c63ecefa7aef5fa6c0f20b694b8f614d31a5:

  • "Return when we can't find an address" in comments before get_interface_address6() is missing either NULL or the empty list, and should specify. Looks like the actual behavior is to return NULL.
  • It looks like we're removing a tor_addr_is_internal() check from get_interface_address6(); is this correct?
  • If smartlist_len(return_addrs) == 0 at the end of the get_interface_addresses_raw() path in get_interface_address6(), the empty list will be leaked when the function returns NULL.
  • Okay, this is fixed in 836461227083938acf86f4f52170f0816c6266d6
  • Why is this function even called get_interface_address6() when it looks like it can return AF_INET or AF_INET6 addresses and now does so in the plural?
  • The error exit path in the non-get_interface_addresses_raw() case of get_interface_address6() can leak return_addrs. (looks like e8ea5abb681052cc8b90453cbb14b8a78efc11dd fixed this)
  • The comment for get_first_address_by_af() should document that the caller must free the returned tor_addr_t, if any.
  • Fixed by no longer allocating a new tor_addr_t in 58ebb9b6d3908ee7b35fd5203252b0e2b421cbf8
  • Does this notion of firstness make any sense? Shouldn't we be regarding addresses as logically an unordered set? Hmm, is this used anywhere except for as a helper for get_stable_interface_address6()? If not, I should be less worried about its exact semantics but it should be declared static.
  • get_stable_interface_address6() consists of three consecutive sections, each of which branch on AF_INET vs. AF_INET6 and then contain identical code except for using last_discovered_ipv4_address or last_discovered_ipv6_address. Using a tor_addr_t initialized to point to one or the other of those static variables at the beginning of the function would be less redundant.
  • The new function get_first_address_by_af() is not declared static, but it seems to only be used in address.c and isn't added to address.h.
  • Okay, removing the tor_addr_is_internal() check from get_interface_address6() makes more sense in light of find_good_addr_from_list().
  • Why does find_good_addr_from_list() use uint32_t * rather than tor_addr_t *? This looks like it won't be IPv6-friendly at all.
  • Fixed in 58ebb9b6d3908ee7b35fd5203252b0e2b421cbf8
  • These changes to resolve_my_address() look okay, modulo above-noted IPv6 issues; has this function been changed since this patch was written to not assume IPv4, by any chance?
  • We're deleting a bunch of code from client_check_address_changed() that avoids redundancy using the technique I recommended above for get_stable_interface_address6(), and replacing it with a call to get_stable_interface_address6(). Can we please avoid clone-and-hack anti-refactoring?
  • I think the logic is correct in the many introductions of node_af_reachable_since() in dirserv.c.
  • Is skipping all reachability testing (in dirserv_single_reachability_test()) for bridges with large numbers of addresses the right behavior? What do bridge authorities do with this data? Would testing a random sample of addresses be useful?
  • addr_replied() and all_listeners_replied() in nodelist.c should have explanatory comments and it looks like they should be static too.
  • I'm not sure I like this adding a list of *additional* listeners to routerinfo_t business; it breaks symmetry among the set of addresses we're listening on. What's the motivation for singling out one address as primary?
  • Similar comments on the existence of router_get_main_listener_addr_by_af(); is the motivation for designating one address as primary avoiding having to change a mess of old code that makes that assumption perhaps?
  • Maybe we should consider renaming router_pick_published_address() to match router_get_main_ipv6_listener_address() if we're going down this path.
  • So in the descriptor generated in router_dump_router_to_string(), the only distinction being preserved between main and additional or-address lines is that the 'main' orport comes first?
  • The ticket is full of assumptions that this is *for bridges*, but the same code (called via router_parse_list_from_string()) is used on dirauths to parse uploaded descriptors. I don't see any checks that uploaded descriptors to dirauths *lack* extra or-address lines, so we're expanding the set of things that are valid descriptors to upload. I don't think that causes any serious problems *right now* since I didn't see any client- side changes here for picking an address to use, but I'd really rather changes like that happen consciously. What's the correct behavior for the dirauths here? What will dirauths not yet running this patch do if they happen across a router descriptor with extra or-address lines?

836461227083938acf86f4f52170f0816c6266d6:

  • Fixed the memory leak in get_interface_address6(); good.
  • A bunch of needful style fixups.
  • Fixed a comparison bug in router_get_active_listener_port_by_addr_type_af() that I didn't notice but ln5 did.

e8ea5abb681052cc8b90453cbb14b8a78efc11dd:

  • Avoids a null pointer deref I missed.
  • Fixes memory leaks in get_stable_interface_address6() and get_interface_address().
  • Adds get_first_address_by_af() to address.h, but is it ever called from anywhere else? Perhaps it should be static instead.

29c149db6c9fcd496992eb5a8c23fa943a518654:

  • Adds some unit tests. Good.

58ebb9b6d3908ee7b35fd5203252b0e2b421cbf8:

  • Fixes some memory leaks and constness
  • Changes get_first_address_by_af() so the caller doesn't need to to free anything.
  • Changes around some of how we copy tor_addr_t in get_stable_interace_address6(); I like this better since it's clear where the last_discoverd_ipv4_address/last_discovered_ipv6_address are supposed to get allocated/freed, but it still seems to have more duplicated code than necessary; cf. my suggestions in comments on fca9c63ecefa7aef5fa6c0f20b694b8f614d31a5.
  • Removes the get_interface_address() function.
  • Good, all those weird uint32_ts in find_good_addr_from_list() are addressed here.
  • resolve_my_address() looks better; not carrying IPv4 dependencies any more and I hadn't thought of the awareness of user's config aspect this improves.

06d76fe4025d38db9ddec2f134a371324b09d001:

  • This is just a merge of 58ebb9b6d3908ee7b35fd5203252b0e2b421cbf8. Looks fine to me.

4698c309a1bb635ec1eb8eca5b6a25f24389126f:

  • Removes stale test, looks fine.

5a1b50dcae0837ff7fb9afc245c74bf9f371b35d:

  • This fixes callers of get_first_address_by_af() to handle the new behavior introduced in 58ebb9b6d3908ee7b35fd5203252b0e2b421cbf8. Looks fine to me.

fc6c446841d4081ed3a7406738b09830e372b72d:

  • Compiler appeasement which only touches test suite; looks fine to me.

e29b61f63e2cfd0d623bb2e1f02a2b5cb385257f:

  • Comment improvements in test suite; looks fine to me.

1ccfb795f0e5e79925de4b374221b47e12d68320:

  • More test suite tweaks; looks fine to me.

c18593e772e43056590a72c0b0275f8300a3b0d0:

  • Style fixups in address.c and add an address family sanity check to get_stable_interface_address6(); looks okay to me modulo what 75d7b0d53e6d5cbc1f1b98f7b7d341828b8d2eee covers.

b1faa03570ccfe33a629ccd9485736ed5c8e3f68:

  • Implements a new utility function to clone a tor_addr_t; looks okay to me.

485f91efaccd5f358f86dd00cba9ffc96a335961:

  • Looks fine to me.

75d7b0d53e6d5cbc1f1b98f7b7d341828b8d2eee:

  • Compiler warning appeasement on b1faa03570ccfe33a629ccd9485736ed5c8e3f68; looks okay to me.

c18429f19d4920d6583965626a7ebe1deb3296bb:

  • Compiler appeasement; looks fine.

b7c07e78c0de1bdc7db316e133b2c771ad916486:

  • Looks fine to me.

73b0af833222a3431c971c26b2d0bcd10ac82eab:

  • Compiler appeasement and use of tor_addr_clone(); looks fine to me.

d8d610ae507eebcb505f801ac1056c53a959370f:

  • Fixes some initializations in find_good_addr_from_list()

20718a4a0b9dd572e70d36661be5bee3d06559f0:

  • Compiler appeasement; looks fine, but why does router_get_main_ipv6_listener_address() have an unused param?

3d8a714920c45bd6f03d0d6b8efb0dd47091d039:

  • This looks okay.

e2ac66e16bf5e524101e3f75829760b2a38c1927:

  • Minor comment fix in test suite; looks fine.

66c75a40ca6ba4e5b6cc65e835039fe8afd7d6d8:

  • This introduces copy and equality-testing functions for tor_addr_port_t; they look fine, except that tor_addr_port_eq(NULL, NULL) should perhaps return 1.

f4b35098ed3783d029fef2241456f661b1ff0a19:

  • This looks fine to me

329c30c098066e1ad3167732c4702f102260287b:

  • This looks fine but it'd sure be nice if the commit description mentioned what bugs it was fixing.

68b473b9f2b2795a18b14f57a089f596e98e630f:

  • This looks okay

a4fdffc975719f9b35ffe2e23fca89696f471f1c:

  • This looks okay.

e42b225eacd6fb6650cad5c5940e30102bf1a201:

  • This looks fine to me.

b292fc563937ffa7f3e8e9084c3306deb69f60fd:

  • This looks okay.

6530497d8474fb50fe79e54c1556e8275ebabbc0:

  • This looks okay to me.

75c821e60b619d9dbfd104eed36fee54f5f98b1a:

  • This looks okay to me.

dd586150424e32bd7641552b93812ddfe470c5ca:

  • This looks okay to me.

95ff579c205594ca07b5ae41448b3bc35fc4047f:

  • Formatting fixes; this looks okay.

10092ddeb9df6e89ca7cae1aaf1744ee32792738:

  • This looks fine to me.

Summary:

This is a complicated branch and I don't like the idea of merging it this
late in the release cycle, so I'm deferring it to 0.2.7-alpha. I'm marking
it needs_revision for the lack of consideration of effect on the dirauths
and the broken symmetry of designating one orport as primary and the others
as additional, but an acceptable revision might consist of a persuasive
argument that it does the right thing and an added comment to that effect.

--- end code review ---

comment:46 Changed 4 years ago by daube

Thanks for your speedy review Andrea!

Attached is a patch that addresses your comment on 66c75a40ca6ba4e5b6cc65e835039fe8afd7d6d8. It makes tor_addr_port_eq(NULL, NULL) == 1 by testing if the two tor_addr_t * arguments point to the same location (i.e., if (a == b)). I hope this is sufficient.

Changed 4 years ago by daube

Address a comment of Andrea's

comment:47 Changed 4 years ago by nickm

Keywords: 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:48 Changed 4 years ago by daube

Hi Nick and Andrea,

Just thought I'd make clear that as I'm not the original author of this patch series, I'm not able to answer the remainder of Andrea's comments/requests. I'm happy to implement suggestions, but don't have the depth of knowledge to comment on, e.g., Andrea's concerns about behaviour of DirAuths WRT multiple or-addresses per OR.

Thanks,
daube

comment:49 Changed 4 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

comment:50 Changed 4 years ago by nickm

Keywords: 028-triaged added

comment:51 Changed 4 years ago by nickm

Points: small/medium

comment:52 Changed 3 years ago by nickm

Keywords: pre028-patch added

comment:53 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

These seem like features, or like other stuff unlikely to be possible this month. Bumping them to 0.2.9

comment:54 Changed 3 years ago by nickm

Sponsor: SponsorS-can

Tagging these bridge- and PT- items as S-can.

comment:55 Changed 3 years ago by nickm

Keywords: tor-bridge added

comment:56 Changed 3 years ago by nickm

Points: small/medium2

small/medium => 2.

comment:57 Changed 3 years ago by nickm

Keywords: nickm-deferred-20160905 added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

Deferring many tickets that are in needs_revision with no progress noted for a while, where I think they could safely wait till 0.3.0 or later.

Please feel free to move these back to 0.2.9 if you finish the revisions soon.

comment:58 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:59 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:60 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:61 Changed 2 years ago by nickm

Keywords: 027-triaged-in added

comment:62 Changed 2 years ago by nickm

Keywords: 027-triaged-in removed

comment:63 Changed 2 years ago by nickm

Keywords: 027-triaged-1-out removed

comment:64 Changed 2 years ago by nickm

Keywords: 026-triaged-1 removed

comment:65 Changed 2 years ago by nickm

Keywords: 028-triaged removed

comment:66 Changed 2 years ago by nickm

Keywords: nickm-deferred-20160905 removed

comment:67 Changed 2 years ago by nickm

Keywords: pre028-patch removed

comment:68 Changed 23 months ago by nickm

Keywords: censorship pt added; bridge removed
Severity: Normal
Note: See TracTickets for help on using tickets.