Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#2711 closed defect (fixed)

status/clients-seen should answer even when uptime < 24 hours

Reported by: arma Owned by:
Priority: Medium Milestone: Deliverable-Mar2011
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Right now Tor bridges only disclose clients-seen in their extra-info descriptors if the bridge has been up 24 hours, for privacy reasons. Fine.

But we also use the same algorithm for answering questions from the controller. The result is that many bridge users who are actually serving clients are told that they have no users. This is the wrong feedback to give them.

Child Tickets

Change History (7)

comment:1 Changed 9 years ago by arma

geoip_get_bridge_stats_controller() in src/or/geoip.c is the function to look at. I think we want to modify geoip_bridge_stats_write() so it takes various arguments about what to do, and doesn't just rewrite globals.

Options include:

A) Simply ignore the "if (now < start_of_bridge_stats_interval + WRITE_STATS_INTERVAL)" line for controller answers. Simple to do, not really harmful, since the data is going only to the relay operator anyway.

B) Sum up the bridge users seen, but don't break them down by country, to keep some sort of privacy. I think that would substantially decrease the value of the feedback.

C) List the countries seen, but not how many users have been seen (not even padded up to a multiple of 8). This approach would still make the feedback reasonably useful. I'm not clear on what privacy benefits it actually provides though.

D) Combine B and C -- so e.g. answer with "Up to 24 users, coming from cn, sa, and ar".

I think I prefer A, and I think Nick prefers C. I look forward to hearing his thoughts.

comment:2 Changed 9 years ago by nickm

I prefer C or D, but I wouldn't object terribly to A. I would object a little, though: it's possible for controllers to do stupid stuff with it, and unlike other stupid stuff that controllers can do, it's stupid stuff that affects somebody other than the person running the controller. In practice, though, the scope of harm is small and the scenario is a bit contrived.

As for refactoring, I agree that we ought to simplify the dataflow here. There is AFAICT no reason that we should have to round-trip to disk to get this info. But that's potentially separate; design is needed.

comment:3 Changed 9 years ago by nickm

Status: newneeds_review

I've done up a quick patch that simplifies the code and does A. It also simplifies the code a lot by removing the parse logic and splitting the generate logic into two nice separate asprintf calls. It's less efficient, since we now walk the whole table every time the controller asks us for information, but so long as the controller doesn't do that every second, we should be fine.

See branch "feature2711" in my public repository.

comment:4 Changed 9 years ago by karsten

Replying to nickm:

See branch "feature2711" in my public repository.

The idea to implement option A seems reasonable. I found (and fixed?) two issues with your patch though:

  • In geoip_bridge_stats_write(), we always need to return start_of_bridge_stats_interval + WRITE_STATS_INTERVAL and not now + WRITE_STATS_INTERVAL. If we don't, we break the logic when to call this function the next time. I changed the return after formatting the bridge stats string into a goto.
  • In load_bridge_stats(), we need to parse the loaded bridge stats string to find out if it is well-formatted and still recent enough. This was done as part of parse_bridge_stats_controller() which is now gone. I put the relevant parts back as validate_bridge_stats() and changed load_bridge_stats() to call that function.

Please see branch feature2711 in my public repository.

comment:5 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks good. Merging for feature2711 onto mine, removing a double-newline, finishing a comment, squashing, and removing the result onto maint-0.2.2. Thanks!

comment:6 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:7 Changed 7 years ago by nickm

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