Opened 6 years ago

Closed 6 years ago

#9859 closed enhancement (implemented)

Add flag thresholds to the networkstatus created by a bridge authority

Reported by: sysrqb Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge trivial
Cc: arma, karsten, atagar, isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

A Bridge Authority does not output its flag thresholds at any point, but this information is useful for debugging and general understanding. We can log these values, but then we will rely on direct access to the server, if this information is included in the network status, then it will be accessible to the people who need it. Credit goes to arma for the idea.

Child Tickets

Change History (29)

comment:1 Changed 6 years ago by arma

The first question is: does the bridge authority compute values for the various variables used in dirserv_get_flag_thresholds_line() ?

comment:2 Changed 6 years ago by sysrqb

I didn't see that a bridge auth took any code paths that called dirserv_compute_performance_thresholds(), so possibly not, which may actually be the cause of #1117, but I'll look into this further and confirm this is the case.

comment:3 Changed 6 years ago by sysrqb

Status: newneeds_review

Please see bug9859_rebased in my public repo. This branch should actually solve #1117, also. The problem is that the bridge auth never computes the flag thresholds, so all bridges are at least as good as zero, unless they aren't good at all.

I successfully ran this overnight on a test bridge auth with a single test bridge uploading to it, but it can use more testing. Is this a good candidate for Chutney?

comment:4 Changed 6 years ago by nickm

Status: needs_reviewneeds_revision

Reviewing be044f0a8ff1927422f0f2fd17635bea6a0c6546 ...

It looks plausible, except that we're trying to avoid the "x = tor_malloc(n); tor_snprintf(x, n, ...)" pattern in new code. Instead, use "tor_asprintf(&x, ...)".

Is this a good candidate for Chutney?

It could be, but you'd need to make your own torrc templates to do so. Linus or I can walk you through that if it isn't apparent how from the source and the readme.

comment:5 Changed 6 years ago by sysrqb

Status: needs_revisionneeds_review

Thanks for the comments! tor_asprintf is much better for this, I made that change. I also decided that the patch wasn't exactly what we want. It yielded the correct results, but it also changes the resulting output to the control port, which is less than ideal.

I pushed bug9859_rebased_2, which I think it a better patch.

Regarding Chutney, I hacked it a bit so it would do what I wanted, but overall it was a success.

stable-uptime=18391 stable-mtbf=19583 fast-speed=54000 guard-wfu=0.000% guard-tk=0 guard-bw-inc-exits=54000 guard-bw-exc-exits=54000 enough-mtbf=1 ignoring-advertised-bws=0
r test009br So4T1MobX5/KK/NHNZDAw7JDzWc hRdlnfQoOlWZKCITUZvamk9yunU 2013-10-11 10:31:45 127.0.0.1 5009 0
s Fast Guard Running Stable Valid
w Bandwidth=54
p reject 1-65535
r test010br UJkummsyfyPV+Tkv5xnoDBgTRG4 OhLHKRXMwoV1beeerm2bSTiMyYA 2013-10-11 10:41:45 127.0.0.1 5010 0
s Running Valid
w Bandwidth=3
p reject 1-65535
r test014br 0Si0953tzxo05XR+GK9JxRi9EfQ cAPmcu9q81ulTNRPUvqd239Qvsw 2013-10-11 10:33:45 127.0.0.1 5014 0
s Fast Guard Running Stable Valid
w Bandwidth=57
p reject 1-65535
r test015br 8akZGwVemVHPJv1Da95weuSsbD0 SIUENxUCVvG9AJc1zLyrPD2fW4Q 2013-10-11 10:35:45 127.0.0.1 5015 0
s Fast Guard Running Stable Valid
w Bandwidth=54
p reject 1-65535

comment:6 Changed 6 years ago by nickm

Cc: arma karsten added

Looks mergeable to me.

Roger, Karsten: Does anything rely on this data _not_ being at the start of the networkstatus-bridges file?

comment:7 Changed 6 years ago by karsten

Actually, yes, parts of metrics might rely on that. Will fix that in the next hours and let you know here when it's fixed. Thanks for the heads-up!

comment:8 Changed 6 years ago by karsten

I'm sorry, I'm running out of time before the weekend (7pm here, real life calling). I need to update metrics-db and test the changes a bit, or it might do unexpected things. I can do this on Monday. Please hold back this change until Tuesday or Wednesday. Will let you know here when metric-db is ready.

comment:9 Changed 6 years ago by arma

I had a quick look at bug9859_rebased_2 and it didn't look crazy.

We should encourage sysrqb to be sure that bridgedb can handle documents of this new form, but hopefully he's already thought of this. :)

comment:10 Changed 6 years ago by sysrqb

The bridgedb parser will happily ignore this, so that won't be an issue.

Karsten, would it be better if we moved the threshold line to the end? There's no rush to merge this immediately (it looks like Tonga is running -rc, so we probably won't be able to start collecting this info for a few months).

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

Cc: atagar added

Replying to sysrqb:

Karsten, would it be better if we moved the threshold line to the end?

No, at the start is fine.

There's no rush to merge this immediately (it looks like Tonga is running -rc, so we probably won't be able to start collecting this info for a few months).

Understood. Though I'd want to prepare metrics-db now, rather than being surprised by Tonga upgrading. But preparing metrics-db shouldn't be hard, I just need to sit down for a moment to think about any potential problems.

And here's one such problem: your new line should start with "flag-thresholds ", or we'll have a hard time parsing it once we decide to add more lines, or when we decide we want to remove the "stable-uptime" part. Can you add that keyword? Like:

-  tor_asprintf(&thresholds_and_status, "%s\n%s", thresholds, status);
+  tor_asprintf(&thresholds_and_status, "flag-thresholds %s\n%s", thresholds, status);

Oh, and just to be sure: there is nothing sensitive in that line, so that it can be copied as is into sanitized descriptors, right?

We should also ask atagar for his thoughts on the new line, because he'll have to extend stem to support this new line in sanitized bridge network statuses. atagar, do you see any problems with this new line, or do you have suggestions for making it easier to parse (in addition to my suggestion to prefix the line with a keyword)?

comment:12 Changed 6 years ago by atagar

This absolutely should have a prefix keyword, like everything else in consensus (otherwise this line will require special handling). Would you mind making the spec addition first? I'm not entirely clear from this backlog what precisely you're adding but it sounds akin to the 'params' keyword (but with string rather than numeric values). Parsers need a formal definition of what both the keys and values can include.

comment:13 Changed 6 years ago by karsten

atagar, sorry, I might not have given you enough context. This is an addition to bridge network statuses, not consensuses. Also note that this line is very similar to the flag-thresholds line in votes which has a spec definition.

comment:14 Changed 6 years ago by arma

Can we make it "exactly the same as", rather than just "very similar to", so the spec can be the same?

comment:15 Changed 6 years ago by karsten

I just wrote "very similar to", because I didn't check in detail before. Just checked that it's "exactly the same as" once the line starts with "flag-thresholds".

comment:16 Changed 6 years ago by atagar

From the parser stem has for it I'm guessing...

  • bridge network status documents are not part of the spec
  • they're only available via the metrics site
  • their format was...
published [timestamp]
[list of v2 router status entries]

... and it's now becoming...

published [timestamp]
flag-thresholds [parameters]
[list of v2 router status entries]

Is that right? Apologies if I'm way off base. :)

comment:17 Changed 6 years ago by arma

They were intended to be the same as v2 networkstatus documents.

But it's possible they aren't.

comment:18 Changed 6 years ago by atagar

Found the bit of the metrics site that describes them (see the bottom of this). V2 network status documents have quite a few more fields.

comment:19 Changed 6 years ago by karsten

Bridge network status documents are not part of dir-spec.txt. They are like v2 network statuses (or even v3 consensuses?) but without header and footer. I added the "published" part to the start of sanitized bridge network statuses to make them easier to handle, but that part is not contained in the non-sanitized statuses.

comment:20 Changed 6 years ago by atagar

They are like v2 network statuses (or even v3 consensuses?)

This actually seems like an important detail since it determines the type of router status entry. Presently stem treats them as v2.

I added the "published" part to the start of sanitized bridge network statuses to make them easier to handle, but that part is not contained in the non-sanitized statuses.

I treated 'published' as a mandatory field since the metrics page is the closest thing we have to a spec (and it says it's included). If this is a more generic descriptor type then it should probably be part of the dir-spec, otherwise my perception is that this is a metrics-specific type of descriptor.

comment:21 in reply to:  20 ; Changed 6 years ago by karsten

Status: needs_reviewneeds_revision

Replying to atagar:

They are like v2 network statuses (or even v3 consensuses?)

This actually seems like an important detail since it determines the type of router status entry. Presently stem treats them as v2.

What's the difference between v2 and v3 router status entries?

I added the "published" part to the start of sanitized bridge network statuses to make them easier to handle, but that part is not contained in the non-sanitized statuses.

I treated 'published' as a mandatory field since the metrics page is the closest thing we have to a spec (and it says it's included). If this is a more generic descriptor type then it should probably be part of the dir-spec, otherwise my perception is that this is a metrics-specific type of descriptor.

Fine to perceive this as metrics-specific type of descriptor.

But regardless of dir-spec or not, it seems that we're on the safe side by adding the "flag-thresholds" keyword to make the new line exactly the same as in v3 votes.

sysrqb, can you add that keyword? And do you agree that this line can be copied as is into sanitized bridge network statuses?

comment:22 Changed 6 years ago by atagar

What's the difference between v2 and v3 router status entries?

V3 adds the or-address, bandwidth, measured, flag to indicate if unmeasured, microdescriptor hashes, and (most important) the relay's microdescriptor exit policy. Maybe none of those are especially important for bridges.

comment:23 in reply to:  21 ; Changed 6 years ago by sysrqb

Replying to karsten:

But regardless of dir-spec or not, it seems that we're on the safe side by adding the "flag-thresholds" keyword to make the new line exactly the same as in v3 votes.

sysrqb, can you add that keyword?

Yeah, adding this sounds like a good plan. I admit I hadn't thought about this, but I'll add the prefix.

And do you agree that this line can be copied as is into sanitized bridge network statuses?

I'm hesitant to say this should be public, but I want arma's and nick's input on this. BridgeDB doesn't really take advantage of the status flags, except for supplying stable bridges, so it shouldn't affect bridge distribution now. I'm less certain this will be true in the future or if public availability could have negative consequences. otoh, this info will be good for metrics, so we should maintain an archive...so maybe they should remain unsanitized.

comment:24 in reply to:  22 Changed 6 years ago by sysrqb

Replying to atagar:

What's the difference between v2 and v3 router status entries?

V3 adds the or-address, bandwidth, measured, flag to indicate if unmeasured, microdescriptor hashes, and (most important) the relay's microdescriptor exit policy. Maybe none of those are especially important for bridges.

Yeah, none of those are included in this Bridge networkstatus. Right now, it is exactly the same as the output returned from a NEWCONSENSUS event or NS event. This patch will simply prepend an additional line with the flag threshold values for bridges (but only when this networkstatus is created by the Bridge Auth).

comment:25 in reply to:  23 ; Changed 6 years ago by karsten

Replying to sysrqb:

Replying to karsten:

And do you agree that this line can be copied as is into sanitized bridge network statuses?

I'm hesitant to say this should be public, but I want arma's and nick's input on this. BridgeDB doesn't really take advantage of the status flags, except for supplying stable bridges, so it shouldn't affect bridge distribution now. I'm less certain this will be true in the future or if public availability could have negative consequences. otoh, this info will be good for metrics, so we should maintain an archive...so maybe they should remain unsanitized.

There's also the rule of thumb that we shouldn't use data that we don't make public. If something is too sensitive to publish, we shouldn't collect it.

But I don't see a reason for redacting these lines. The only thing that we're trying to hide from the public is bridge IP addresses. I don't see how flag thresholds would make it any easier to find bridge IP addresses.

comment:26 in reply to:  25 Changed 6 years ago by sysrqb

Cc: isis added
Status: needs_revisionneeds_review

Replying to karsten:

There's also the rule of thumb that we shouldn't use data that we don't make public. If something is too sensitive to publish, we shouldn't collect it.

Ok, I agree. That's a good rule and we should follow it for this.

But I don't see a reason for redacting these lines. The only thing that we're trying to hide from the public is bridge IP addresses. I don't see how flag thresholds would make it any easier to find bridge IP addresses.

True, I only worried that, in the future, these threshold values will allow an operator to manipulate or game BridgeDB or one of the distributors, but I agree that preventing this via obscurity is not a way to solve this problem.

Pushed bug9859_5 based on master, which adds the keyword.

Any other thoughts?

Isis, sorry to bring you into this so late.

comment:27 Changed 6 years ago by nickm

The code looks okay to me. Are there any pending objections from above that weren't addressed?

comment:28 in reply to:  27 Changed 6 years ago by karsten

Replying to nickm:

The code looks okay to me. Are there any pending objections from above that weren't addressed?

I don't see any pending objections that aren't addresses by now. Fine to merge, I'd say.

comment:29 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Okay; merged into master. Thanks, all!

Note: See TracTickets for help on using tickets.