Bug 41844&40982: Stop using the control port directly and draft a backend for the circuit display and bridge settings

Merge Info

Related Issues

• #41844 (closed)
• #40982 (closed)

Backporting

Timeline

• Immediate: patchset needed as soon as possible
• Next Minor Stable Release: patchset that needs to be verified in nightly before backport
• Eventually: patchset that needs to be verified in alpha before backport
• No Backport (preferred): patchset for the next major stable

(Optional) Justification

• Emergency security update: patchset fixes CVEs, 0-days, etc
• Censorship event: patchset enables censorship circumvention
• Critical bug-fix: patchset fixes a bug in core-functionality
• Consistency: patchset which would make development easier if it were in both the alpha and release branches; developer tools, build system changes, etc
• Sponsor required: patchset required for sponsor
• Other: please explain

Merging

• Merge to tor-browser - !fixups to tor-browser-specific commits, new features, security backports
• Merge to base-browser - !fixups to base-browser-specific commits, new features to be shared with mullvad-browser, and security backports
  • NOTE: if your changeset includes patches to both base-browser and tor-browser please clearly label in the change description which commits should be cherry-picked to base-browser after merging

Issue Tracking

• Link resolved issues with appropriate Release Prep issue for changelog generation

Review

Request Reviewer

• Request review from an applications developer depending on modified system:
  • NOTE: if the MR modifies multiple areas, please /cc all the relevant reviewers (since gitlab only allows 1 reviewer)
  • accessibility : henry
  • android : dan
  • build system : boklm
  • extensions : ma1
  • firefox internals (XUL/JS/XPCOM) : ma1
  • fonts : pierov
  • frontend (implementation) : henry
  • frontend (review) : donuts, richard
  • localization : henry, pierov
  • nightly builds : boklm
  • rebases/release-prep : dan_b, ma1, pierov, richard
  • security : ma1
  • signing : boklm, richard
  • updater : pierov
  • misc/other : pierov, richard

Change Description

Reason for the change

We (richard and me) thought that not using the Tor controller directly (i.e., by calling controller, create a new socket etc etc) is a good idea. And we wanted to do this to refactor/delete Torbutton, and for the future Arti integration.

There were 3-4 places in which we used it:

• Onion Authentication (easy to solve, I just added a wrapper to TorProtocolService - not a definitive solution, but this helps with changes that aren't part of this MR)
• Circuit display (difficult to solve)
• Bridge cards (similar to the circuit display, so I proposed to do a backend for both of them)

Tor events

We have already a class that listens on the events on the control port, so I decided to hook up more events, in particular the circuit events and the STREAM SUCCEEDED events.

Events for the circuit display

The circuit events are used to proactively collect the nodes used by a circuit (and to delete them when tor closes the circuit).

We use the STREAM SUCCEEDED events to associate SOCKS credentials to a circuit ID.

My design choice was not to store stream or credentials data at this stage. Instead, I decided to always notify observers.

The reason is that we already store data at a higher level, and I preferred avoiding a double logic to keep/delete data. Of course, this can be revisited, if you think it isn't a good idea.

Also, higher levels won't see data such as circuit ID or stream ID: we just pass SOCKS credentials and node fingerprints.

Events for the bridge cards

In addition to that, when a new circuit is built we store the fingerprint of the first node of the last opened circuit. Whenever it changes, we notify observers.

We ignore 1-node circuits, that are used to probe bridges, or get the consensus, or whatever else. Considering them not useful to Tor Browser users seems a safe assumption.

Notice: this doesn't work well, because tor doesn't actually have a current bridge/guard. It has a primary and a secondary. And in some quick tests I've done, the secondary was used for about 1/3 of the circuits (I tried to reach at least 20-30 circuits).

We might want to do an additional UX study for this, but I guess this is in any case a better solution than what we currently have.

The domain isolator

The domain isolator is where the actual backend for the circuit display is.

This makes a lot of sense, because it's already what allows users to have different circuits.

And it's already the glue between browsers (as MozBrowsers) and SOCKS credentials. It's the thing that generates the SOCKS credentials in the first place!

And when it does so, it also has access to the browser object (more or less, we need a logic to get the object from other data). So, we can do a real association between a MozBrowser object and the SOCKS credentials, without having to guess.

This association can happen immediately, even when the circuit data isn't available yet (or the circuit hasn't been decided or built). This is the difference between a null circuit and an empty array: null means that the browser hasn't done requests through the proxy, an empty array means that it started requests, but the data isn't available, yet.

The domain isolator can also get the chrome window, hence the circuit display. I decided not to create a registration mechanism, but to make the domain isolator call a getter and a function on the circuit display (the getter to check if it's enabled, the function to pass the data). I think this is actually simpler, even though less abstract. We can discuss about that.

Circuit display

At this point, I've removed all the references to the controller in the circuit display, and I changed it to query the domain isolator instead.

I've created a temporary animation just to show that we can do something to show that we're refreshing the data.

Probably we'll want to change it before merging, but I thought it could make sense to include it.

I don't know if it's all the data the circuit display will need (I don't think a first load is different from a new circuit).

Also, the circuit button is now shown also for local schemes (file://, about:, etc) when they do a request through the proxy. Removing this check from there seems a good thing to me.

Other notes

• The animation of course will need to be replaced/removed for now. Having the empty panel when we don't really have data is quite strange.
• Using the back button doesn't hide the circuit display button if it was shown.
• I haven't tested domain isolation disabled. It's something we don't actually want users to do, and this MR is already big enough.
• This branch includes the first two commits of !695 (merged), just to make my life easier. But at this point, it should not depend on merging that one before.
• I've made a built on tb-build-05 to be sure to avoid caches in my local build, and everything seems fine: https://tb-build-05.torproject.org/~pierov/testbuild/tor-browser-linux64-testbuild_ALL-41700.tar.xz
• I hope we don't get UX changes eventually, because AFK timings isn't great, and I'd like to get this in 13.0a1.

/cc @richard