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

firefoxPackages.tor-browser: 8.5.6 -> 9.0.4 #77507

Closed
wants to merge 6 commits into from

Conversation

@oxij
Copy link
Contributor

@oxij oxij commented Jan 11, 2020

It builds. It works. Should be ready to merge.

git log

  • Revert "firefoxPackages.tor-browser*, tor-browser-bundle: remove"

    This reverts commit 1efaa03.

    Reverts #77452. /cc @flokli @alyssais

  • firefoxPackages.tor-browser: unpack sources much more efficiently

    We don't need to spend time tracking hardlinks in cp,
    find ... -exec ... is also very very very very sloooooooooooooooooooooooooooooow,
    find followed by xargs with batching is equivalent, but much faster.

  • firefoxPackages.tor-browser: mark older versions as insecure

  • firefoxPackages.tor-browser: 8.5.6 -> 9.0.4

  • firefoxPackages.tor-browser: fix build ("only" warnings)

  • firefoxPackages.tor-browser: add a reference to the upstream ticket

nix-instantiate environment

  • Host OS: Linux 4.9, SLNOS 19.09
  • Nix: nix-env (Nix) 2.3
  • Multi-user: yes
  • Sandbox: yes
  • NIXPKGS_CONFIG:
{
  checkMeta = true;
}

nix-env -qaP diffs

  • On x86_64-linux:
    • New (2):
      • firefoxPackages.tor-browser
      • tor-browser-bundle
  • On aarch64-linux: ditto
  • On x86_64-darwin:
    • New (1):
      • firefoxPackages.tor-browser

tor-browser-bundle now needs to be reworked for tor-browser with internalized tor-launcher. /cc @joachifm

Copy link
Member

@andir andir left a comment

I am in favor of having a privacy focused Firefox variant in Nixpkgs. There are issue that I can see with the current state of the packaging. I commented on those.

My biggest pain point are those patches we are applying and knowing which they are. We (as Nixpkgs people) are shipping some variant of a browser with random patches someone decided are required. The problem here (for me) is not that someone decided on those patches. The problem is not that we package a privacy oriented version of Firefox. The problem is that nobody else packages this specific variant and calls it tor-browser thus expectations from previous experiences might apply. As I noted in the diff the patches might be right and noble but we are relying on a few people that are maintaining it outside of Nixpkgs.

Whenever I looked at the SLONS/tor-browser repo I could not (easily) glimpse at what the patches you added are and which were coming from upstream tor-browser. Of course I can invoke git and run diffs and whatnot but that isn't really something we should have to do.
I am therefore asking: How many patches are these and can't we just have them in Nixpkgs and use the upstream TB sources? That would also increase the likelihood of someone working on the package as they do not have to go through an intermediate repo outside of Nixpkgs. Of course this burries the risk of upstream TB adding some "invasive" thing that you would want to remove from the package. I personally have trust in the TB developers to not screw their users over on purpose and if we only have to apply a few (slightly changing) patches to get rid of the bundling (and some Nix specifics) we can have a privacy focused Firefox variant but not call it Tor Browser.

As commented below: Can we just get rid of old versions as soon as a the support for a version ends? What is the gain of having old versions around if we do not want people to use them because "better" alternatives exist?


in rec {

tor-browser-7-5 = (tbcommon {

This comment has been minimized.

@andir

andir Jan 11, 2020
Member

Can we remove tor-browser-7-5 and tor-browser-8-5 if they aren't supported anymore? What is the argument to keep them?

This is a version of TorBrowser with bundle-related patches
reverted.
I.e. it's a variant of Firefox with less fingerprinting and

This comment has been minimized.

@andir

andir Jan 11, 2020
Member

So, it has bundle related patches reverted and has therefore less fingerprinting surface? I usually hear the argument that you should use the upstream tor browser bundle binary to reduce fingerprinting.

Are we able to point to those patches? I'd be interested to see them but going throught the commits manually suggests that they might not always be on-top of the tree but burried somwhere further down.

other UNIX program and doesn't expect you to run it from a
bundle.
It will use your default Firefox profile if you're not careful

This comment has been minimized.

@andir

andir Jan 11, 2020
Member

This sounds dangerous to me. You are already changing the profile location in one of the patches. Couldn't we give it a dedicated directory? I assume people use this browser because they want privacy that they can't get with their "normal" Firefox profile. I also don't think the longDescription is a place to bury the user guide to using this package.

even! Be careful!
It will clash with firefox binary if you install both. But it
should not be a problem because you should run browsers in

This comment has been minimized.

@andir

andir Jan 11, 2020
Member

This sounds like very opinionated talking. It might be generally accepted that having more isolation is better but I do not think this kind of statement should be here.

If you should not run it as the same user or on a VM why aren't we providing the tooling around that? I think that would be reasonable if that level of isolation is suggested. I am running Firefox in different namespaces as well but would never expect that from others unless there would be the required tooling for users to deal with it.

];

extraConfigureFlags = [
"--disable-tor-launcher"

This comment has been minimized.

@andir

andir Jan 11, 2020
Member

IMO this is the biggest issue I have with the way of packaging this. It is good to have a privacy focused variant of Firefox around but I doubt many people would expect this when they see tor browser because this has actually no connection to using tor. Arguably most people would expect a tor browser to use nothing but tor. This might not be your/our fault but just something I think we should be aware of.

Also: Shouldn't this be moved into the TB related default configure flags? Is this special about this version of TB?

@flokli
Copy link
Contributor

@flokli flokli commented Jan 11, 2020

@oxij I assume you're not necessarily using the tor functions, but really want something similar as icecat?

@oxij
Copy link
Contributor Author

@oxij oxij commented Jan 11, 2020

Copy link
Member

@vcunat vcunat left a comment

Won't build as-is (for me, commit c8d6ff7):

/build/tor-browser/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Logging.h:266:35: error: format not a string li
teral and no format arguments [-Werror=format-security]
  266 |         mozilla::detail::log_print(moz_real_module, _level,    \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  267 |                                    MOZ_LOG_EXPAND_ARGS _args); \
      |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~
@vcunat
Copy link
Member

@vcunat vcunat commented Jan 11, 2020

I assume we'll want something like:

--- a/pkgs/applications/networking/browsers/firefox/common.nix
+++ b/pkgs/applications/networking/browsers/firefox/common.nix
@@ -166,8 +166,8 @@ stdenv.mkDerivation (rec {
   ++ lib.optionals (!isTorBrowserLike) [
     "-I${nss.dev}/include/nss"
   ]
-  ++ lib.optional (pname == "firefox-esr" && lib.versionAtLeast ffversion "68"
-                                          && lib.versionOlder ffversion "69")
+  ++ lib.optional (lib.versionAtLeast ffversion "68"
+                  && lib.versionOlder ffversion "69")
     "-Wno-error=format-security");
 
   postPatch = lib.optionalString (lib.versionAtLeast ffversion "63.0" && !isTorBrowserLike) ''

EDIT: now it builds and starts for me, so I updated the PR.

@alyssais
Copy link
Member

@alyssais alyssais commented Jan 12, 2020

@flokli
Copy link
Contributor

@flokli flokli commented Feb 2, 2020

@oxij I'm inclinded closing this, especially with #79115.

There haven't been any comments on neither the code review, nor the fundamental points from @alyssais .

@oxij
Copy link
Contributor Author

@oxij oxij commented Feb 3, 2020

oxij and others added 6 commits Jan 11, 2020
@oxij oxij force-pushed the oxij:pkgs/tor-browser-904 branch from 0036c68 to 6d2e2b4 Feb 3, 2020
@oxij
Copy link
Contributor Author

@oxij oxij commented Feb 3, 2020

@flokli
Copy link
Contributor

@flokli flokli commented Feb 4, 2020

The code needed to support those versions is tiny compared to the space that would be needed to keep two separate almost full-system closures (firefox is the largest package on my system) when you need to install them, so using an older nixpkgs is not a good option, IMHO.

Disk space still is much cheaper compared to the the additional time maintainers have when having to think about and verify all the various permutations

I heard multiple complaints about how complicated things currently are, and really don't see disk consumption a valid argument, sorry.

@oxij
Copy link
Contributor Author

@oxij oxij commented Feb 5, 2020

@oxij
Copy link
Contributor Author

@oxij oxij commented Feb 5, 2020

Copy link
Contributor

@flokli flokli left a comment

@oxij as written in #79115, we can discuss the re-introduction of icecat once the situation is resolved upstream - I don't really see this coming any time soon.

As for this PR, some very valid points have been raised in #77507 (comment), and I'm seeing many valid questions from @andir not adressed here.

@oxij
Copy link
Contributor Author

@oxij oxij commented Feb 7, 2020

@joachifm
Copy link
Contributor

@joachifm joachifm commented Feb 8, 2020

The standards that this is being held to seem a little unfair to me. If the prevailing consensus is to remove the package regardless, I think that should be stated outright and this PR closed. Based on its technical merits alone, I'd integrate this.

@flokli
Copy link
Contributor

@flokli flokli commented Feb 8, 2020

@andir, who did quite some firefox maintenance, raised some very valid questions inside his review, yet none of these have been answered inline, or at all:

  • why versions based on unsupported packages have been re-introduced regardless
  • why the way some things are handled should offer less fingerprinting surface
  • the misleading tor-browser name

This behaviour is a very exhausting communication culture, and I'm also not really a fan of the violent language used in some places (like naming cleanup efforts as "insane", and simply ignoring questions).

As continuously explained in multiple places, including #79115, maintainability of the firefox expression is important to nixpkgs maintainers.
We'd like to be able to address firefox security updates promptly. This includes being able to test things, and maintaining high quality and dropping unsupported stuff (which is what #79115 does).

Adding things that are hard/impossible to test and maintain, not documented and hardly understandable, with contributors hard to reach are a huge maintenance burden and risk.

I'm also a fan of a privacy-first and software-freedom friendly browser, and I'm pretty certain we can add something like this to nixpkgs, if it comes up with a manageable patchset, but I don't think the approach suggested in this PR is the right way forward.

@flokli flokli closed this Feb 8, 2020
@grahamc
Copy link
Member

@grahamc grahamc commented Feb 8, 2020

Just to reiterate a few points, I think we're all on board with bringing tor-browser back to life. I know @flokli is. The requirements I think I see listed here seem pretty reasonable for something like tor-browser:

  • probably not disable the tor launcher? I'm not the expert here, but that feels shady to me
  • build from upstream's sources
  • not include old or out of date versions
  • if patches are needed, include them with fetchpatch, not by pointing to an alternative source -- ideally with a reference to an upstream ticket
@flokli
Copy link
Contributor

@flokli flokli commented Feb 8, 2020

I don't even think building tor-browser from source ourselves is a good idea in first place.

There's so many fingerprinting side-channels, and I assume upstream is much better aware of these - which is why I'd be extremely cautious with calling something tor-browser that isn't.
I have less problems with a "privacy-first and software-freedom friendly browser" (as something like Icecat + some patches could be), but that's not tor-browser, and shouldn't be called like this.

@oxij
Copy link
Contributor Author

@oxij oxij commented Feb 8, 2020

@ahiaao
Copy link
Contributor

@ahiaao ahiaao commented Feb 10, 2020

@grahamc

probably not disable the tor launcher? I'm not the expert here, but that feels shady to me

I would prefer removing the tor launcher. It makes it so I am somewhat pointlessly running two instances of the Tor daemon at all times. Additionally, the tor daemon started by tor-browser-bundle-bin is run under my user account instead of the tor account like the regular daemon. I personally would prefer that the browser could just use my regular Tor daemon that I always have running. Due to how lightweight the Tor daemon it is not a burden to always have it running the background. The one case when I can think that it is undesirable is if you use networks where Tor is prohibited. In that case having it only running when using it makes sense. If that is a use case that you want to pursue it would be cool if there were a way to have more than just the Tor browser opt in to the functionality.

@oxij
Copy link
Contributor Author

@oxij oxij commented Feb 10, 2020

Since #79115 was merged this is now officially dead. Congratulations.

If anybody still want this (and other useful but removed from Nixpkgs stuff) then you are welcome to SLNOS#1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.