firefoxPackages.tor-browser: 8.5.6 -> 9.0.4 #77507
Conversation
ofborg
bot
added
8.has: package (new)
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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?
|
@oxij I assume you're not necessarily using the tor functions, but really want something similar as icecat? |
|
the patches
SLNOS changes are laid strictly on top over upstream tor-browser. Your problem is that GitHub is sorting patches weirdly. Clone the repo with git and you'll see each branch having exactly three patches with reverts on top of https://git.torproject.org/tor-browser.git upstream. (Except for the new "hacky" branch having four.)
a few people that are maintaining it outside of Nixpkgs
Sure, but what's alternative? IceCat is both technically inferior to tor-browser, uses lots of patches that constantly break so that `make-icecat` can't be run over anything except the version it was specifically made for (which also takes hours, since it goes over the whole source to search and replace Firefox -> IceCat and similar), and is maintained by a single person. SLNOS/tor-browser (until now) had three trivial reverts over well-maintained upstream.
not a "Tor Browser"
That's a valid concern, but until now it was exactly "Tor Browser" without the "Bundle" part. With version 9 now I'm considering reverting more stuff. So now it might make sense to give it another name, maybe. But before version 9 it was exactly what you should have expected from having installed "Tor Browser" as a normal UNIX app.
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?
7-5 can run old plugins. The ideas is to remove 8-5 on the next update unless somebody reports an issue with 9-0.
So, it has bundle related patches reverted
Yes.
and has therefore less fingerprinting surface?
Strike out "therefore". Less fingerprinting surface compared to Firefox, ~same compared to upstream Tor Browser Bundle.
I usually hear the argument that you should use the upstream tor browser bundle binary to reduce fingerprinting.
Yes, that's a valid argument. Thought, I doubt the difference between the binary version and this can really be discerned by any web apps since this does not change anything in the UI and builds with the same configure flags. But if you want to be absolutely certain, then yes, you should use the original binaries (e.g. in the case somebody can distinguish the version of libpng used by the build or something like that, which, I'm sure, is not as impossible as it sounds).
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.
Set both upstream and the SLNOS repo with as remotes on a single git repository, run `git log --all --graph`.
This sounds dangerous to me. You are already changing the profile location in one of the patches.
The idea was to make the smallest possible change, and before version 9 it was pretty much Firefox as it should have been, so it made sense, kind of. But now it might make sense to rename it, maybe. There would be quite a problem with profile migration, thought.
This sounds like very opinionated talking. It might be generally accepted that having more isolation is better I do not think this kind of statement should be here.
Why not? I don't think it's false, and it's an advice, not a law, or something.
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.
I tried in #36274. In general, the problem that we can't do that without some OS support. Users can't setuid between non-root users in UNIX, kernel sandboxes and VMs need also need some OS setup.
Arguably most people would expect a `tor browser` to use nothing but `tor`.
As noted above, maybe. Before version 9 "Tor Browser" did not require tor daemon, thought, so it was a fair name, IMHO. I'm open to suggestions how to call this thing now.
Also: Shouldn't this be moved into the TB related default configure flags? Is this special about this version of TB?
I wanted this to be a noop for the older versions. Also, what if we decide we want the version with the launcher? So, for now, I decided to keep it outside of the `common.nix`.
@flokli
like icecat, but better
Yes, that's pretty much the idea. I imagine "Tor Browser Bundle" was designed to be "tor daemon + launcher + Firefox configured for it by default", but then it turned out that Firefox is not actually a very good browser for private browsing, which spurred the whole "Tor Browser" thing (which is an unfortunate name, since "Tor Browser" did not require tor daemon until version 9). But then they started integrating too much stuff into there (for the obvious reason that most their users run Windows and nothing is ever easy to do the right way on Windows), which spurred SLNOS/tor-browser, which tries to undo the damage.
|
There was a problem hiding this comment.
Won't build as-is (for me, commit c8d6ff7824):
/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); \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~
|
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. |
|
I think it's important that, if we're going to include this in Nixpkgs,
it
- Not be called Tor Browser
- Demonstrate that it has more than one or two users
- Not make life difficult for people who work with the Firefox package
(I don't, much, but it's important that the people who do are okay
with it)
|
ofborg
bot
added
the
2.status: merge conflict
|
What do you expect me to do, then?
This branch with the patch by @vcunat works perfectly (thanks, @vcunat, btw). I'm using it. It is still the latest version of Tor Browser. The thing is still "tor-browser without the bundle part", IMHO, since all those patches still do exactly the same thing as before, the new patch simply disables tor-launcher somewhat improperly (an upstream ticked was made, reference added with the new commit). If you want it to be named something else: feel free, the branch is open to edits, as it is, that suggestion is not actionable. Demonstrating other users is not actionable. Code review is not actionable either, I did review those patches before myself, they didn't change in this version, the new patch was made by me, I read all the related code, can I prove any of it? no, neither could you. Since we can't force anyone into anything (like confessing about using this, or into doing code review) I don't know what you want here.
As to the last point, obviously, I'm against removing anything, in particular #79115. 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.
|
This reverts commit 1efaa03.
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.
oxij
force-pushed
the
pkgs/tor-browser-904
branch
from
0036c68 to
6d2e2b4
Compare
|
Rebased onto master fixing a trivial conflict.
|
ofborg
bot
removed
the
2.status: merge conflict
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. |
|
verify all the various permutations
Let's be real here, nobody really verifies anything in Nixpkgs before it gets merged into master, unless it is a very small scope change to a package you yourself use. Problems are usually found by users after the changes go into master. Occasionally by automated tests run by Hydra.
Diffs between the outputs of `nix-env -qaP` do help, but if your change touches a large number of packages almost nobody ever rebuilds everything (I did when doing `doCheckByDefault` changes, which took months of CPU build time).
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
Firstly, it's not only disk, but also RAM, because all those shared libraries are _shared_, which is the whole point.
Secondly, it depends on your requirements. If you force this design decision you'll just force out anyone who can't have that much disk and RAM. E.g. most single-board ARM devices.
So, I'd say this is a tradeoff that needs to be discussed in general. I'm not aware of the official policy here. Before this fashionable zeal of removing useful stuff from Nixpkgs started I felt like a the general policy was the reverse one: merging derivations that could be merged. /cc @Ericson2314 on this point.
That is to say, I think changing this policy needs an RFC.
I heard multiple complaints about how complicated things currently are, and really don't see this a valid argument, sorry.
Links to discussions? I don't see what exactly is complicated there, all those variables do is influence a bunch of `configureFlags` and some lists of patches. The only exception is build system change of firefox version 58, which did add some complexity there, but the good thing is: _you just don't need to touch those parts_ and everything will work out. In fact, if people really do complain, we could make a more general version of `common.nix` that would use different drivers for versions before 58 and after to hide that part, but I don't really see the point, in my experience with that derivation, I had a problem with that part once and after I learned _not to touch those parts_ those problems simply evaporated.
complexity and #79115
In fact, I think your #79115 actually _adds_ complexity: you can't really simplify `common.nix`, because you can't remove "enough" (and what you do remove will get reintroduced back the next time icecat updates anyway), but you add complexity with all the aliases and stuff.
|
|
Updated the OP.
|
There was a problem hiding this 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.
|
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.
Please explicitly specify which points and what you expect me to do about them. From my point of view I answered all the complaints that can be realistically resolved.
|
|
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. |
|
@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:
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. 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. |
|
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:
|
|
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 |
|
* probably not disable the tor launcher? I'm not the expert here, but that feels shady to me
1) Can you explain your reasoning? Tor launcher is just a thing that launches tor daemon when you launch tor-browser, I don't see how disabling it can be "shady", it is even a official configure flag (albeit currently a broken one).
2) Your are free to try to built it with that option enabled. Neither me nor the original SLNOS maintainer were able to, after lots and lots of effort. Also note that it would also make tor-browser derivation try to run its own tor daemon, which is counterproductive even if we could, since we have a superior NixOS tor module that is at the very least user-isolated from the browser (and also declaratively configured and all that NixOS stuff). Ideally, we should patch tor-launcher so that it would control NixOS's tor daemon (which we could, in principle), but that's another patch we don't have and, unless someone good with tor-browser sources steps in, would not have in a foreseeable future. Also, after this discussion I feel it would just become another patch of contention ("More fingerprinting opportunities!") even if we would mysteriously get it somehow.
* 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
Alright, sure, that is actionable, I can do that. The first item would be very inconvenient for many users since upstream sources are blocked in many countries, but if you want that, I can do it, sure.
So, can you make a binding promise that you would merge this if I
- remove old versions,
- change the source to `fetchgit` from upstream and apply SLNOS patches with `fetchpatch` instead, and
- rename this into "not-tor-browser" (or whatever else of your choice, not on your list, but a recurring complaint above; which does not make sense to me, see "read the previous statement" below, again, but whatever)
?
If yes, then please explicitly say "I promise to merge this if those complaints are addressed" and reopen this, because as it is, I feel like I'm just being bullied by being asked do the impossible here, which is not very motivating for spending effort on this PR.
@flokli
- why versions based on unsupported packages have been re-introduced regardless
As I said above, because removing them is easy. For firefox derivations I prefer to keep at least one version back, because, unfortunately, things frequently break there: usually plugins stop working properly (yes, even after firefox 52, unfortunately, new plugin system also breaks) and you have to either disable them until they update, or wait on the previous version, which, IMHO, should be a user choice.
- why the way some things are handled should offer less fingerprinting surface
As I said above, compared to Firefox, not vanilla TBB. Why? Simply because it is tor-browser source + four patches that make it work with Nix properly. Just take a look at the tor-browser patches on top of Firefox (https://git.torproject.org/tor-browser.git) yourself.
I don't even think building tor-browser from source ourselves is a good idea in first place.
... because ... fingerprinting ... (again)
Again, those who want the arguably more vanilla experience are free to use the official binary package `patchelf`ed for Nix. Which, by the way, also introduces its own fingerprinting opportunities, since our `tor-browser-bundle-bin` derivation (`pkgs/applications/networking/browsers/tor-browser-bundle-bin/default.nix`) is far from being a trivial wrapper, since, of course, we _CAN'T HAVE THE ORIGINAL TOR BROWSER BUNDLING_ with Nix.
`tor-browser-bundle-bin` does lots wrapping so that the bundle could live outside of /nix/store, SLNOS patchset simply reverts the bundle patches that introduce the problem in the first place (and, in the latest version, also disables tor-launcher properly, since the upstream is broken). That's _the only thing SLNOS patchset does_.
READ THE PREVIOUS STATEMENT, TWICE, PLEASE! Seriously, I'm tired reiterating this and people ignoring this point and implying SLNOS patches do something mysterious or shady. Stop spreading FUD (Fear Uncertainty Doubt) and just took at the source already, please! Which is why I'm against renaming it, but whatever, please.
(Also, I'm sure Izuna from the English translation of "No Game No Life" will approve of the previous paragraph. That absolutely was not intentional.)
Anyway. If you want the same fingerprinting the official thing has you will have to use Tails.
- the misleading tor-browser name
Like I said above, it is not. But whatever, sure, I can change it, suggestions other that "not-tor-browser" are welcome.
This behaviour is a very exhausting communication culture,
I agree wholeheartedly. But I like then people write me fan mail about conversations like this a couple years later noting I was completely right in retrospect. Thus, I try to view any change through a prism of "will it be a good thing 5 years later or not", and #79115 is clearly not, which is why I will complain until most of that change is reverted, if it is ever merged, and will maintain a revert of it regardless of anyone's wishes.
Meanwhile, I think that making of non-actionable requirements should be made a Code Of Conduct violations or whatever.
and I'm also not really a fan of the violent language used in some places (like naming cleanup efforts as "insane",
Ha-ha. https://www.dictionary.com/browse/insane in that context clearly defines it as
> insane =def utterly senseless
Which sure is very "violent".
Also, I explicitly said I liked the cleanup part, but I do find the rest of that PR to be utterly senseless because you are either applying changes that will need to be immediately reverted in case either of icecat or tor-browser are to be reintroduced to Nixpkgs.
Also, I think that breaking other people's toys (that is, removing derivations you clearly don't even use yourself) is much more "violent".
and simply ignoring questions.)
Please point explicitly to anything I ignored, I don't think I ignored anything, neither in this message, nor in my messages above.
As continuously explained in multiple places, including #79115, maintainability of the firefox expression is important to nixpkgs maintainers.
Sure.
We'd like to be able to address firefox security updates promptly.
Sure. Since those don't require any changes to the build process, I don't really see how removal of anything from `common.nix`, nor how the removal of mostly unrelated derivations from `packages.nix` helps.
This includes being able to test things,
Sure. But by removing derivations you will be able to test _less_, not more. This makes absolutely no sense to me.
and maintaining high quality
This does not mean anything unless you define "high quality" via RFC or something. I think a derivation supporting more build options is higher quality than the reverse.
and dropping unsupported stuff (which is what #79115 does).
Which also does not make any sense. You don't even need to build that stuff. If you look at git-log you'll notice that those older derivations are getting broken from time to time and people that care about them fix them. Which is completely fine, IMHO. And adds near-zero maintenance cost to those editing the main firefox expression otherwise.
But if you goal is to improve maintainability, and people really do complain about the current state of `common.nix` somewhere (for which, I remind you, you did not corroborate any links to evidence), all you needed to do is to _copy_ `common.nix` and simplify it so that it only works for the latest version of firefox or whatever. Which, let's be honest, would not simplify a lot (hence my "utterly senseless" appraisal of such changes). But it easy to do, and it would not break anything for anyone else.
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.
Like I noted above, I disagree, since the stated method does not actually bring you closer to any of your stated goals, but if you want a marginally simpler `common.nix` -- just copy the expression and never touch the old one again. Problem solved.
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.
Did you actually read the source of SLNOS/tor-browser? I think, you did not. Did you ever run it? I doubt it. Because it exactly what you describe you want and you just removed it.
|
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. |
It builds. It works. Should be ready to merge.
git logRevert "firefoxPackages.tor-browser*, tor-browser-bundle: remove"
This reverts commit 1efaa03.
Reverts firefoxPackages.tor-browser*, tor-browser-bundle: remove #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,findfollowed byxargswith 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-instantiateenvironmentnix-env -qaPdiffstor-browser-bundlenow needs to be reworked for tor-browser with internalized tor-launcher. /cc @joachifm