Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#4561 closed project (implemented)

Extend Tor to no longer assume that a relay or bridge has exactly one address

Reported by: karsten Owned by: ln5
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: SponsorG20120331 tor-client
Cc: nickm, ln5 Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


We should extend Tor to no longer assume that a relay or bridge has exactly one address. This probably means implementing proposal 186, but maybe it means more than that. We should make sure that all places in Tor use a list of addresses per relay or bridge. This gets tricky when we want to connect to a relay or bridge; do we try all addresses at once? or just one? or do we fall back to the next if the first fails?

The code changes are part of the March 31, 2012 deliverable for sponsor G. Merging code into master is sufficient here, no (alpha) releases or bundles involved.

Child Tickets

Change History (21)

comment:1 Changed 8 years ago by karsten

Owner: karsten deleted
Status: newassigned

We don't have a person to lead this deliverable yet.

comment:2 Changed 7 years ago by karsten

Here are some notes from Nick, Roger, Linus, and I discussing the topic yesterday:

  • For Tor 0.2.4.x, Nick and Roger agreed that every relay or bridge will have exactly one IPv4 and zero or one IPv6 address. From this follows that a bridge descriptor can have at most two "or-address" lines and the bridge network status can have at most two "a" lines. One of these lines may contain an IPv6 address and ports, the other will, if present, contain the IPv4 in the "router" line and list all ports not in the "r" line.
  • In order to finish this ticket in a way that we can honestly say we completed it, we're going to change all places accessing a uint32_t containing an IPv4 address (e.g., ri->addr) to call a function (like get_an_addr(ri)). This similarly applies to rs and possibly other places where IP addresses are referenced in the Tor code.

comment:3 Changed 7 years ago by karsten

Owner: set to ln5

Assigning this ticket to Linus after talking to him.

comment:4 Changed 7 years ago by ln5

Status: assignedaccepted

Branch bug4561 in my public repo contains suggested changes for
routerinfo_t. Please comment on the general idea of creating access
functions for the uint32_t ipv4 address and using them (except in
router.c where we access the uint32_t directly).

comment:5 Changed 7 years ago by ln5

Status: acceptedneeds_review

comment:6 Changed 7 years ago by arma

I talked to ln5 more on irc. Looks like a reasonable direction to go. The patch so far looks pretty clean, which is a good sign.

We have some coding style questions that I'm hoping ln5 will summarize and that we should get nickm's opinion on.

comment:7 Changed 7 years ago by ln5

Is it this style OK?

THING_get_PROP(const *THING_t thing, DST_t *dst); /* I think I'd prefer s/_get_1/. */
THING_set_PROP(THING_t *thing, const SRC_t *src);

It conflicts with the common

copy_THING(THING_t *dst, const THING_t *src);

comment:8 Changed 7 years ago by nickm

We aren't totally consistent with argument order. Generally, here's what I do:

Pretend this is an OO language, where there are functions in classes. When you would have a function named foo in class bar, name the C funtion "bar_foo" and have the first argument be a bar_t* or a const bar_t * as appropriate.

So the style that you describe above seems fine.

comment:9 Changed 7 years ago by ln5

Good, thanks.

Opinions on THING_get_PROP() and THING_set_PROP() vs.

comment:10 Changed 7 years ago by ln5

Branch bug4561_2 contains changes to routerinfo_t (now squashed) and routerstatus_t.
Please review the routerstatus_t patch (3b4d3ae5).

comment:11 Changed 7 years ago by ln5

Man, I suck at git. :-(

See branch bug4561_4.

comment:12 Changed 7 years ago by nickm

This seems straightforward and correct. It could use a changes file.

comment:13 Changed 7 years ago by ln5

I will leave the uint32_t source_addr of struct networkstatus_v2_t as
it is since it's accessed exactly once in the whole of tor. This is
when it's being set in networkstatus_v2_parse_from_string().

comment:14 Changed 7 years ago by ln5

Access to the last data structures containing IPv4 addresses stored in
a uint32_t have been converted to use wrapper functions, reflecting
the fact that it's an IPv4 address in there.

Please review and merge.

comment:15 Changed 7 years ago by ln5

It's still in branch bug4561_4 in my public repo.

comment:16 Changed 7 years ago by karsten

Branch bug4561_4 looks plausible, at least to me. The only (non-)issue I found is that "make check-spaces" is unhappy. ;)

What's the plan to merge this patch? Does it go into 0.2.3.x or 0.2.4.x? In any case we'll have to close this ticket, because the work for the deliverable is done. If we have to wait for 0.2.4.x, we'll have to create a new ticket for merging the patch once 0.2.4.x is there. I can do that, just let me know.

comment:17 Changed 7 years ago by ln5

Thanks. Fixed (my) long lines.

I'm pretty sure it goes into 024.

comment:18 in reply to:  17 Changed 7 years ago by karsten

Resolution: implemented
Status: needs_reviewclosed

Replying to ln5:

I'm pretty sure it goes into 024.

I created #5532 for merging your bug4561_4 branch once we have an 0.2.4.x branch.

The deliverable was "code changes to support IPv4 or IPv6 network addressing." When I created this ticket I said "merge code into master," but I meant that as opposed to making releases or bundles. We didn't explicitly promise to merge something into a master branch. Closing this sponsor-driven project ticket now to show that we're done with what we promised.

comment:19 Changed 7 years ago by karsten

Keywords: SponsorG20120331 added
Milestone: Sponsor G: March 31, 2012

Switching from using milestones to keywords for sponsor deliverables. See #6365 for details.

comment:20 Changed 7 years ago by nickm

Keywords: tor-client added

comment:21 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.