Opened 7 months ago

Closed 5 months ago

#22207 closed enhancement (implemented)

Add bridge authority fingerprint to sanitized bridge network statuses

Reported by: karsten Owned by: metrics-team
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: intro, SponsorM, review-group-20
Cc: isis, iwakeh Actual Points: .5
Parent ID: Points: .5
Reviewer: isis Sponsor:

Description

When I looked through existing code that uses methods from DescriptorFile other than DescriptorFile#getDescriptors() for #22141, I spotted this code in metrics-web:

          if (descriptorFile.getFileName().contains(
              "4A0CCD2DDC7995083D73F5D667100C8A5831F16D")) {
            authority = "Tonga";
          } else if (descriptorFile.getFileName().contains(
              "1D8F3A91C37C5D1C4C19B1AD1D0CFBE8BF72D8E1")) {
            authority = "Bifroest";
          }

Not beautiful, I know, but that's not the point. The point is that we need to parse the bridge authority fingerprint from the file name, because it's not contained in the descriptor itself. That seems bad, and relatively easy to fix.

Right now, the header of a sanitized bridge network status looks like this:

@type bridge-network-status 1.1
published 2017-05-09 07:57:32
flag-thresholds stable-uptime=1075980 stable-mtbf=2384868 fast-speed=21000 guard-wfu=98.000% guard-tk=691200 guard-bw-inc-exits=318000 guard-bw-exc-exits=318000 enough-mtbf=1 ignoring-advertised-bws=0

We could easily include a "fingerprint" line there, like this:

@type bridge-network-status 1.2
published 2017-05-09 07:57:32
flag-thresholds stable-uptime=1075980 stable-mtbf=2384868 fast-speed=21000 guard-wfu=98.000% guard-tk=691200 guard-bw-inc-exits=318000 guard-bw-exc-exits=318000 enough-mtbf=1 ignoring-advertised-bws=0
fingerprint 1D8F3A91C37C5D1C4C19B1AD1D0CFBE8BF72D8E1

Ideally, we'd modify the bridge authority code to include this line, too, and then go through archived bridge network statuses and retroactively put it in.

This is somewhat related to #12951 when we added a "published" line to bridge network statuses in a similar way.

I'm copying isis to hear what they think about putting in a "fingerprint" line.

Child Tickets

Change History (20)

comment:1 in reply to:  description Changed 7 months ago by isis

Keywords: intro added

Replying to karsten:

I'm copying isis to hear what they think about putting in a "fingerprint" line.


Sounds good to me! Generally, I'm against sticking extra things in descriptors, but this is small and it's definitely better than doing hacky things with filenaming.

Tagging this as 'intro' since it should be a relatively easy patch for a newcomer! :) See the changes to src/or/networkstatus.c for #12951 in commit 1431bc22f6d3287a7aac73567b27a40ed963cb6c (as Karsten mentioned) for adding a new line to the networkstatus file.

comment:2 Changed 7 months ago by karsten

Component: Metrics/CollecTorCore Tor/Tor

Sounds good!

This ticket was originally supposed to track changes to CollecTor's bridge descriptor sanitizer to include the bridge authority fingerprint in network statuses produced by current and older tor versions. But let's tackle the Core/Tor Tor side of things first, and we'll implement the same thing in CollecTor after that. Changing component to Core Tor/Tor.

comment:3 Changed 6 months ago by karsten

Status: newneeds_review

Did you mean commit 374b531dba966780f2e59163ca80b1b5a0b8f14c above for the #12951 change?

Please find my branch task-22207 with a patch. It's been a while since I wrote my last tor patch (maybe that means I'm the newcomer you were looking for!), so please bear with me if I missed any recent coding convention changes.

If a feature freeze of some sort makes it impossible to merge this patch right now, can you tell me whether this patch will go in after the freeze? Basically, can I count on bridge authorities to follow this spec in the future, that is, produce a "fingerprint" line following the current "flag-thresholds"? If so, I'll start putting them in in CollecTor, too.

Thanks!

comment:4 Changed 6 months ago by isis

Reviewer: isis

comment:5 in reply to:  3 Changed 6 months ago by isis

Actual Points: .5
Keywords: SponsorM added
Milestone: Tor: 0.3.1.x-final
Points: .5
Status: needs_reviewmerge_ready

Replying to karsten:

Did you mean commit 374b531dba966780f2e59163ca80b1b5a0b8f14c above for the #12951 change?


Yes, oops.

Please find my branch task-22207 with a patch. It's been a while since I wrote my last tor patch (maybe that means I'm the newcomer you were looking for!), so please bear with me if I missed any recent coding convention changes.


LGTM

If a feature freeze of some sort makes it impossible to merge this patch right now, can you tell me whether this patch will go in after the freeze? Basically, can I count on bridge authorities to follow this spec in the future, that is, produce a "fingerprint" line following the current "flag-thresholds"? If so, I'll start putting them in in CollecTor, too.


I think this would go into 0.3.2.x? It could maybe go in earlier though, since it's small and doesn't effect much. Nick, do you have opinions?

(FWIW, BridgeDB is fine without changing anything.)

comment:6 Changed 6 months ago by nickm

Since this only affects dump_bridge_status_to_file(), it can't do too much harm. So I'd take it in 0.3.1 if need be.

Is the data format specified here? If so, we need a spec patch too. If not, we should open a ticket about making sure we have a specified format.

comment:7 in reply to:  6 Changed 6 months ago by isis

Replying to nickm:

Since this only affects dump_bridge_status_to_file(), it can't do too much harm. So I'd take it in 0.3.1 if need be.

Is the data format specified here? If so, we need a spec patch too. If not, we should open a ticket about making sure we have a specified format.


I think the only places this is documented is in BridgeDB's docs and a bit in Stem's docs, but BridgeDB skips everything in parsing the networkstatus until the first r line, so Stem has never really needed to learn to read the prelude stuff. (We can and maybe should open a ticket for getting some of that documentation into torspec and keeping it more up-to-date.)

comment:8 Changed 6 months ago by atagar

Hi Isis. Actually the bridge sanitation specification is located at...

https://collector.torproject.org/#bridge-descriptors

comment:9 in reply to:  8 Changed 6 months ago by isis

Replying to atagar:

Hi Isis. Actually the bridge sanitation specification is located at...

https://collector.torproject.org/#bridge-descriptors


Oh right, good call, https://collector.torproject.org/#type-bridge-network-status will probably need to be updated to say that there's also a fingerprint field.

comment:10 Changed 6 months ago by karsten

Cc: iwakeh added

https://collector.torproject.org/#type-bridge-network-status is indeed a rather informal specification of sanitized bridge network statuses. We'll update that specification as soon as CollecTor includes the new fingerprint line, either from original bridge network statuses or from filenames of statuses produced until Bifroest is updated. We'll call that version of bridge network statuses @type bridge-network-status 1.2. In the next steps, we'll need to update metrics-lib and Stem to recognize the new version and line.

But do we need a formal specification document for original bridge network statuses produced by Bifroest and consumed by BridgeDB and CollecTor's bridge descriptor sanitizer? That document would probably live next to dir-spec.txt in torspec.git. We didn't have such a document until now, but I can see the value of having it. It might be a fine collaboration task for the network and metrics teams, maybe including a grammar that we can use to generate parsers! (That all would be a new ticket.)

comment:11 in reply to:  10 Changed 6 months ago by iwakeh

Replying to karsten:

...

But do we need a formal specification document for original bridge network statuses produced by Bifroest and consumed by BridgeDB and CollecTor's bridge descriptor sanitizer? That document would probably live next to dir-spec.txt in torspec.git. We didn't have such a document until now, but I can see the value of having it. It might be a fine collaboration task for the network and metrics teams, maybe including a grammar that we can use to generate parsers! (That all would be a new ticket.)

Yes, that would be a great opportunity for collaboration and finding out if this formal way of protocol definition (incl. code generation for various languages) could be useful in general.

comment:12 Changed 6 months ago by atagar

A formal specification would be much appreciated. I've had a harder time understanding how to parse sanitized bridge descriptors than things covered by the dir-spec.

comment:13 Changed 5 months ago by nickm

Keywords: review-group-20 added

Creating review-group-20

comment:14 Changed 5 months ago by nickm

Ping? Can somebody write up the spec patch here so I can merge the implementation?

comment:15 Changed 5 months ago by karsten

nickm: Turns out there is no specification of original bridge network status documents, just an informal specification of sanitized bridge descriptors on the CollecTor page. There is a little part in the BridgeDB specification that says what BridgeDB expects to find in a bridge network status document, but that's probably not the best place for specifying these documents. And we briefly speculated above about writing a new specification document for bridge descriptors based on a grammar, but we can still do that after adding "fingerprint" lines to bridge network statuses.

So, what do you think about including a new subsection in dir-spec.txt for bridge network status documents? They are technically not v3 documents, but we could mention that at the beginning of the subsection. I'm thinking of appending a new subsection to "3. Directory authority operation and formats", so "3.12. Writing bridge network statuses". I can write a patch for that if that makes sense.

comment:16 in reply to:  14 Changed 5 months ago by isis

Replying to nickm:

Ping? Can somebody write up the spec patch here so I can merge the implementation?


I patched CollecTor's website in the bug22207 branch of my collector fork.

comment:17 in reply to:  15 Changed 5 months ago by isis

Replying to karsten:

nickm: Turns out there is no specification of original bridge network status documents, just an informal specification of sanitized bridge descriptors on the CollecTor page. There is a little part in the BridgeDB specification that says what BridgeDB expects to find in a bridge network status document, but that's probably not the best place for specifying these documents. And we briefly speculated above about writing a new specification document for bridge descriptors based on a grammar, but we can still do that after adding "fingerprint" lines to bridge network statuses.

So, what do you think about including a new subsection in dir-spec.txt for bridge network status documents? They are technically not v3 documents, but we could mention that at the beginning of the subsection. I'm thinking of appending a new subsection to "3. Directory authority operation and formats", so "3.12. Writing bridge network statuses". I can write a patch for that if that makes sense.


It sounded from this ticket that people wanted better documentation for both unsanitised and sanitised bridge descriptors, so I made tickets for both (#22826 and #22827, respectively).

(I'm also happy to do the unsanitised one, and I have no opinions where it should live so dir-spec.txt is fine by me.)

Last edited 5 months ago by isis (previous) (diff)

comment:18 Changed 5 months ago by nickm

Merged to 0.3.1 and forward! Please close this ticket unless there's more to do on it?

comment:19 Changed 5 months ago by nickm

I fixed a wide line with 9919638e98000b14982810db28e3f37fd1106bc9 so that 'make check-spaces would pass again.

comment:20 Changed 5 months ago by karsten

Resolution: implemented
Status: merge_readyclosed

Thanks, nickm, for merging (and correcting) the patch!

And thanks, isis, for the CollecTor patch. We're currently moving the CollecTor page to a page on Tor Metrics (#21551). I'll apply your patch there once #21551 is merged, which I hope will be today or tomorrow.

Other than that, I think we can continue the documentation parts on #22826 and #22827 and close this ticket.

In fact, I'll just go ahead and do that last part. If I overlooked something, please somebody re-open. Thanks, all!

Note: See TracTickets for help on using tickets.