BridgeDB doesn't allow bridges to change their distribution mechanism

If a bridge configures BridgeDistribution https and then changes its mind and configures BridgeDistribution any, BridgeDB won't allow that and keeps thinking that the bridge wants to be distributed over HTTPS. The offending code is here.

This surprised me and I would consider it unexpected behaviour. Bridges should be able to change their distribution mechanism any time.

Trac:
Parent Ticket: #31280 (moved)

added actualpoints::1.15 component::circumvention/bridgedb owner::phw parent::31280 points::0.5 priority::medium resolution::fixed reviewer::cohosh s30-o23a1 severity::normal sponsor::30-can status::closed type::defect labels

Replying to phw:

This surprised me and I would consider it unexpected behaviour. Bridges should be able to change their distribution mechanism any time.

Agreed.

I pushed a patch to my defect/33631 branch.

This was more work than I expected because in addition to inserting a bridge into a new hashring, the patch required new methods that make it possible to remove a bridge from its previous hashring.

Trac:
Status: assigned to needs_review

Trac:
Reviewer: N/A to cohosh

Hey! Looks good, I like the general structure of the changes. Some feedback:

• Since we're ignoring the returned distributor from the SQL query here, it makes more sense to modify the query to return only what we need:

cur.execute("SELECT id FROM Bridges WHERE hex_key = ?", (h,))

• The logic for removing bridges by finding their index here seems a bit unwieldy to me. I noticed another, similar function of the code here that does the following:

        index = 0
        logging.debug("Inserting %s into hashring..." % bridge)
        for old_bridge in self.bridges[:]:
            if bridge.fingerprint == old_bridge.fingerprint:
                self.bridges[index] = bridge
                break
            index += 1

Is there a better way to handle this? Or a way to be more consistent? Perhaps we could refactor this logic into its own function that takes a bridge and returns its index in self.bridges. Would making self.bridges a map help?

• Do the tests also check that bridges were removed from all subrings? Can we do that easily?

Trac:
Status: needs_review to needs_revision

Replying to cohosh:

• Since we're ignoring the returned distributor from the SQL query here, it makes more sense to modify the query to return only what we need:

cur.execute("SELECT id FROM Bridges WHERE hex_key = ?", (h,))

Good catch! I fixed it here.

• The logic for removing bridges by finding their index here seems a bit unwieldy to me. I noticed another, similar function of the code here that does the following: {{{

    index = 0
    logging.debug("Inserting %s into hashring..." % bridge)
    for old_bridge in self.bridges[:]:
        if bridge.fingerprint == old_bridge.fingerprint:
            self.bridges[index] = bridge
            break
        index += 1

}}}

Is there a better way to handle this? Or a way to be more consistent? Perhaps we could refactor this logic into its own function that takes a bridge and returns its index in self.bridges. Would making self.bridges a map help?

That's a good observation, also fixed.

• Do the tests also check that bridges were removed from all subrings? Can we do that easily?

Actually, no. Fixed here.

Trac:
Status: needs_revision to needs_review

Oh, and while I was at it, I also removed the class FixedBridgeSplitter, which appears to be unused.

Nice, these changes look great!

Trac:
Status: needs_review to merge_ready

Merged in f015596. I also upgraded our Twisted dependency to 20.3.0 because of two recent CVEs:

• https://nvd.nist.gov/vuln/detail/CVE-2020-10108
• https://nvd.nist.gov/vuln/detail/CVE-2020-10109

Trac:
Status: merge_ready to closed
Actualpoints: N/A to 1.15
Resolution: N/A to fixed

closed

changed time estimate to 4h

added 9h 12m of time spent

mentioned in issue #31280 (moved)

moved to tpo/anti-censorship/bridgedb#33631 (closed)