Opened 5 years ago

Closed 5 years ago

#12951 closed defect (implemented)

BridgeAuth should add a `published` line to bridge networkstatus documents

Reported by: isis Owned by:
Priority: Low Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: bridgedb-parsers, stem, bridgeauth
Cc: isis, karsten Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


While hacking on getting BridgeDB to use Stem's descriptor parsers, I noticed that @type bridge-network-status 1.0 documents do not have a published line, like other networkstatus documents. I proposed that a new @type bridge-network-status 1.1 descriptor type is created, where Bridge Authorities add a published line to the head of the document.

Related: #12254

Child Tickets

Change History (9)

comment:1 Changed 5 years ago by isis

Status: newneeds_review

If we decide that this is worth fixing...

I have a patch, it's a single commit in my bug12951 branch. Code review and advice for the preferred way to go about writing tests for these changes would be greatly appreciated.

comment:2 Changed 5 years ago by isis

As per Yawning's code review, I've removed the tor_free(published); line, due Yawning teaching me that statically-sized stack-based arrays don't need to be freed like dynamically-sized malloc'ed ones.

The new, rebased changes are in my bug12951_r1 branch.

comment:3 Changed 5 years ago by sysrqb

Cc: karsten added

This looks ok to me at first glace, but I'm personally more in favor of moving towards #12254 (likely based on a v3 ns-vote). Making the the networkstatus blob, which tor also returns to controllers, acceptable for the needs of BridgeDB by prepending various other values to it seems less than ideal. I think this patch can work in the short term, though.

Also, the "@type bridge-network-status 1.1" is metrics specific, so that's a Karsten-thing :) [CCing him so he knows we're talking about stuff he may be interested in]

comment:4 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-final

whoa! Missed this one because it didn't have a milestone.

My first reaction is that the code looks okay, and so long as we don't have any tools that will choke on this, we should merge it.

comment:5 Changed 5 years ago by nickm

(What tools look at this? Will they choke on it?)

comment:6 in reply to:  5 Changed 5 years ago by isis

Replying to nickm:

(What tools look at this? Will they choke on it?)

Stem looks for it, and raises an error if the "published" line isn't found. Which means that BridgeDB (in the development branch which uses Stem) fakes a "published" line. (This would remove the need for BridgeDB to do that, in the future.)

I think that the metrics-db code should be okay, because it appears to have a catch for when it finds lines which do not begin with "r", "w", "p", "a", or "s". Otherwise, in order to not trigger that error, it could easily be taught about the new bridge-networkstatus "published" lines where it handles the other networkstatus descriptor headers.

Karsten, any thoughts on the above?

comment:7 Changed 5 years ago by nickm

Sounds okay to me. If everything that looks at these documents is fine with a "published" line, we should just add it in. (Anybody else know anything that looks at them?)

comment:8 Changed 5 years ago by karsten

metrics-db now knows about the "published" line and won't break if Tonga puts it in. The other metrics* tools all use sanitized bridge statuses which already contain a "published" line and hence are not affected by this change. I'd say feel free to merge and deploy on Tonga. Sorry for not acting earlier.

comment:9 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Okay. Merged to master. Thanks!

Note: See TracTickets for help on using tickets.