Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tor-browser-bundle-bin: rewrite using fhs-user-env approach #103156

Closed
wants to merge 1 commit into from
Closed

tor-browser-bundle-bin: rewrite using fhs-user-env approach #103156

wants to merge 1 commit into from

Conversation

xaverdh
Copy link
Contributor

@xaverdh xaverdh commented Nov 8, 2020 •
edited

Motivation for this change

Fix #83096

This amounts to a complete change of how we package the tor browser bundle. Instead of keeping it in the store, and fixing things after the fact, it creates an fhs-user-env, copies the upstream runtime into the local data path and runs it inside that environment.

This has some implications:

  • Updating (after the initial setup) is done by the browser itself. This is an implementation details and could probably be changed with some effort, if that's desired. (Note to self: need to test if auto-update works)
  • We have to somehow deal with people transitioning from the old wrapper.. currently the approach of this pr is to nuke the old tor-browser data (except for the top level Downloads folder – if present) error out if an old profile is detected and delegate migration to the user. Suggestions on handling this more gracefully are appreciated here.
  • I tried to keep some level of API compat between the old wrapper and the new interface, but whether this is desired is probably debatable.
  • I started from scratch mostly (except for the prior art of @yurkobb here), which means that some things may not work (yet) as they used to. I count on your feedback to help fix this ;-)

Most importantly I would like to get some feedback from some of the current maintainers, if this approach seems sane. I also added myself as maintainer of this package, in case we agree that its the right way to proceed.

cc @offline @matejc @doublec @thoughtpolice @joachifm @hax404 @cap @KarlJoad

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@xaverdh xaverdh mentioned this pull request Nov 8, 2020
Closed
10 tasks
@yurkobb
Copy link
Contributor

yurkobb commented Nov 9, 2020

Hi and thanks for working on this!

I think maybe it would be safer to just issue an error and exit if there is a previous TBB in a users' $HOME rather than nuking it.

Although Tor Browser is very much about resetting to a clean state frequently, there are valid use cases for storing some long-term data in the profile, like bookmarks. Users with stored bookmarks might want to export them first.

@xaverdh
Copy link
Contributor Author

xaverdh commented Nov 9, 2020

Hi and thanks for working on this!

I think maybe it would be safer to just issue an error and exit if there is a previous TBB in a users' $HOME rather than nuking it.

^^ I implemented this approach, does seem safer.

One issue currently is that it crashes with something glib / gtk related when opening / saving a file.

@xaverdh
Copy link
Contributor Author

xaverdh commented Nov 9, 2020

Ok, the crashes were related to XDG_DATA_DIRS missing some stuff. Copied the relevant parts from the old wrapper and it works now.

@chkno
Copy link
Member

chkno commented Nov 12, 2020

This seems antithetical to reproducibility. Before this change, the binary that runs is the one specified in configuration & verified by hash. After this change, the binary that runs is whatever the auto-update server last sent, & will change from run to run.

This describes a tor-browser-bundle-bin-installer, not the tor-browser-bundle-bin. If folks find value in having this, great, but please don't replace tor-browser-bundle-bin with it.

@yurkobb
Copy link
Contributor

yurkobb commented Nov 12, 2020

I support naming the this package differently, but I recommend keeping tor-browser-bundle-bin marked broken, since although its binary is reproducible, its behavior and state is not at all (mission-critical addons randomly disabled / crashing).

In general few distributions package Tor Browser itself; some package torbrowser-launcher instead, which does a similar job as this package - initially downloading tbb.

I would name this package tor-browser-installer.

@xaverdh
Copy link
Contributor Author

xaverdh commented Nov 12, 2020

Well this could also be extended to not use the built in auto-update mechanism and instead do updates in the start script if applicable. But if #103570 fixes the issue I will close this anyway. Might still be useful to pick up in the future tough, when mozilla decides to further "lock down" the browser, and remove the remaining methods of sideloading extensions.

@yurkobb
Copy link
Contributor

yurkobb commented Nov 12, 2020

Hm, what if we had both options available to the users? Can we have this package as well, perhaps under tor-browser-bundle-bin-mutable name?

I imagine there might be some people who would like to have a mutable Tor Browser installation (i.e. as close to the original Tor Browser). One use case is being able to update immediately via Tor Browser itself.

@xaverdh
Copy link
Contributor Author

xaverdh commented Nov 13, 2020

Hm, what if we had both options available to the users? Can we have this package as well, perhaps under tor-browser-bundle-bin-mutable name?

I imagine there might be some people who would like to have a mutable Tor Browser installation (i.e. as close to the original Tor Browser). One use case is being able to update immediately via Tor Browser itself.

Agreed. With #103570 merged, I will close this pr and open a new one for a mutable variant of the tor browser bundle.
@yurkobb would you like to help maintain it, if it gets merged?

@xaverdh xaverdh closed this Nov 13, 2020
@xaverdh xaverdh mentioned this pull request Nov 13, 2020
Draft
10 tasks
@yurkobb
Copy link
Contributor

yurkobb commented Nov 13, 2020

Hm, what if we had both options available to the users? Can we have this package as well, perhaps under tor-browser-bundle-bin-mutable name?
I imagine there might be some people who would like to have a mutable Tor Browser installation (i.e. as close to the original Tor Browser). One use case is being able to update immediately via Tor Browser itself.

Agreed. With #103570 merged, I will close this pr and open a new one for a mutable variant of the tor browser bundle.
@yurkobb would you like to help maintain it, if it gets merged?

If I can be useful, yes, although my knowledge of the internals of firefox's profiles is near zero. But I can do simple updates I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tor Browser: add-ons disabled on start
3 participants
@xaverdh @yurkobb @chkno