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

WIP: firefox/wrapper.nix: make firefox wrapper configurable by the user #57554

Closed
wants to merge 2 commits into from

Conversation

xaverdh
Copy link
Contributor

@xaverdh xaverdh commented Mar 13, 2019

Motivation for this change

This is a proof of principle for allowing firefox and its variants to be customized and extended more easily.
Could be a first step towards solving #15959.

Very WIP at the moment.

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/firefox-extensions/1122/39

@worldofpeace
Copy link
Contributor

Nice, I too discovered that we could accomplish this with policies, good to see something in the works 👍

@hyperfekt
Copy link
Contributor

hyperfekt commented Mar 13, 2019

Thanks for taking this on!
I don't think we should expose extensions as a separate config option as long as they are imperative instead of declarative, given that they so thoroughly violate the expectations usually put upon modules and packages.
I barely know what I'm talking about, but maybe user namespaces with bind mounts are something we would want to look into to provide declarative extensions without having to hack around in Firefox itself.

@xaverdh
Copy link
Contributor Author

xaverdh commented Mar 13, 2019

I am not quite sure what you mean by extensions being "imperative" in this context.
The option as it is now, works as follows:
You can specify either a local xpi file or an url (e.g. https://addons.mozilla.org/firefox/downloads/file/1189923/noscript_security_suite-latest-fx.xpi) from which the xpi will get download. Then the policies engine will ensure that the extension is installed (on startup).
We could make it more declarative by using "Locked" perhaps, this will prevent the real state diverging from the one as specified in the config.
See the docs here

@xaverdh
Copy link
Contributor Author

xaverdh commented Mar 13, 2019

user namespaces with bind mounts are something we would want to look into to provide declarative extensions without having to hack around in Firefox itself.

This was actually my first approach, and I have an implementation using user namespaces and bind mounts in my own nixos config. I did not attempt to upstream that, because its quite "advanced" technology; nixpkgs is more than just linux. I just don't know if that works on all platforms we might want to support.

Edit: The logic in my nixos config is overly complicated, because the distribution subdirectory does not exist in the nixpkgs firefox builds. If we changed that, it could be as simple as:

  • enter new user and mount namespace
  • bind mount configuration into the installation directory

If this solution is preferred, I can also implement that.

@hyperfekt
Copy link
Contributor

hyperfekt commented Mar 13, 2019

Imperative means it will permanently install these to your home directory instead of being confined to the package, as a package option should be. Maybe I am alone in this concern but that doesn't seem very compatible with the Nix philosophy.
If you've already got the bindmount solution working, maybe we want to provide a wrapper with that specifically for Linux? Anyone not on Linux could still choose to permainstall extensions via the policy even if it wasn't exposed as a separate option, but having them do it themselves should provide more opportunity to realize the special effect that will have and doesn't force us to support that hacky-at-best mechanism for the forseeable future.

@xaverdh
Copy link
Contributor Author

xaverdh commented Mar 13, 2019

Ah ok, I see what you mean by imperative vs declarative now.
Ok, maybe we should Indeed do some work on the extensions option.
But this can be done with policies (there is also an Uninstall option).

@hyperfekt
Copy link
Contributor

But this can be done with policies (there is also an Uninstall option).

For that you would need to calculate the new policy at runtime from the policy of the most recently executed version. I don't even want to begin to imagine the complexity involved. That would be hard in a NixOS option, and it sounds like just asking for trouble in a package option.

@xaverdh
Copy link
Contributor Author

xaverdh commented Mar 13, 2019

Yes, that does sound rather complicated. Also there are still quite a few bugs in the current implementation of the related policies options.
I will remove the extensions option from this pull request for now. The remaining stuff is still useful on its own.
Maybe we could include a hint in the docs that extensions can be included in the extraPolicies option, mentioning the imperative nature of this approach.

@xaverdh
Copy link
Contributor Author

xaverdh commented Mar 14, 2019

Actually, do we have a place for documentation of individual package options?

@timokau
Copy link
Member

timokau commented Mar 14, 2019

Not really, but it would probably fit next to the vim section in the manual: https://nixos.org/nixpkgs/manual/#users-guide-to-vim-pluginsaddonsbundlesscripts-in-nixpkgs

@xaverdh
Copy link
Contributor Author

xaverdh commented Mar 15, 2019

Ok, I ended up adding the docs to package-notes.xml. But we really need a separate manual for packages / package options.
Or user docs in general...

The older
<link xlink:href="https://support.mozilla.org/en-US/kb/customizing-firefox-using-autoconfig">
AutoConfig </link> method, and the more recent
<link xlink:href="https://github.com/mozilla/policy-templates/blob/master/README.md#extensions">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hook to #extensions probably shouldn't be here, as the explanation is not extension-specific.

disables autoplaying videos, could be specified as follows (the syntax of
AutoConfig is a superset of user.js):
<programlisting>
firefox.override { extraPrefs = '' pref("media.autoplay.default",1); ''; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer this to be split up over multiple lines for readability:

firefox.override {
  extraPrefs = ''
    pref("media.autoplay.default",1);
  '';
}

</programlisting>
To disable upload of telemetry data, you could use:
<programlisting>
firefox.override { extraPolicies = { DisableTelemetry = true; }; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that the json options start with an uppercase letter, going against the conventional nix style. I don't really know what to do about it though.


<para>
The policies framework can be used to enforce the installation of
extensions as follows:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before showing how to install extensions, we should list the caveats:

  • installs into user dir, no automatic unintallation
  • no upgrades which will be a dealbreaker to most

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what do you mean by "no upgrades" exactly?
The addon should update as usual, unless you specify an exact version in the url and lock this, I guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that is true. First of all I would assume (but don't know for sure), that a link like https://addons.mozilla.org/firefox/downloads/file/1189923/noscript_security_suite-latest-fx.xpi links to a specific version (despite having latest in the filename, as there is also a specific fileid).

And even if that isn't the case, there is this: mozilla/policy-templates#207

Have you tried this with an addon that updated afterwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mozilla/policy-templates#158 explains what the "latest" part in the url does. I did not try with local extensions, I will do that.


<para>
The firefox (wrapper) package can be configured through several mechanisms.
The older
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the old/new wording fits here. It suggests that the "new" method is a replacement for the old one, which it is not.

pkgs/applications/networking/browsers/firefox/wrapper.nix Outdated Show resolved Hide resolved
@@ -111,14 +126,55 @@ let
cp -R --no-preserve=mode,ownership ${browser}/Applications/${browserName}.app $out/Applications
rm -f $out${browser.execdir or "/bin"}/${browserName}
'' + ''
if [ ! -x "${browser}${browser.execdir or "/bin"}/${browserName}" ]

# Link the runtime. The executable itself has to be copied,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you looked into patching firefox? While this solution works, it is a little hacky.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have, and I find the code to be surprisingly unreadable for a newcomer. I can spend some more time on that, of course.
If somebody else with more knowledge of the mozilla codebase wants to take a shot at that / point me to the right location in the sources, I am quite happy to do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the autoconfig part, I guess the readConfigFile and openAndEvaluateJSFile here are responsible: https://github.com/mozilla/gecko/blob/72f5488046d2d4cc8c0470f071db8f504629f95b/extensions/pref/autoconfig/src/nsReadConfig.cpp#L120

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but where is XREAppDist actually set?
here ?

That in turn references NS_GRE_DIR, which comes from aGREDir passed to nsXREDirProvider::Initialize.

here perhaps?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ld returned 1 exit status, there may be a more informative error message earlier in the log.

Maybe you ran out of ram or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A friend of mine lent me some more capable hardware, it compiles now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Is it working?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the policies engine working. AutoConfig appears to be more trouble, because of the lock file + obfuscation value + config file proper thing.
Also its cpp instead of javascript, so debugging is harder.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Yeah AutoConfig does seem a little unnecessary complicated.

This commit changes the way the firefox wrapper works to allow passing
system wide configuration to the firefox instance, in the form of an
enterprise policies file (policies.json) and/or an AutoConfig file.

Since firefox essentially hard codes the paths to these configuration
files, and due to the immutable nature of the nix store, we have to
copy/symlink the entire runtime.
Also since firefox resolves those paths relative to the "true" path of
the binary (i.e. following symlinks), the binary itself has to be copied.
This documents configuring firefox by passing options to the wrapper.
@nbp
Copy link
Member

nbp commented Apr 5, 2019

@xaverdh reading the patches, I wonder, is there any reasons to not copy everything over to the new directory?

Knowing that you can uncompress the tarball anywhere you want, I would expect the build result to be position independent of its top-level directory. Making a copy (maybe hard-link) could be a simpler way than selecting the few files which are mandatory.

@xaverdh
Copy link
Contributor Author

xaverdh commented Apr 5, 2019

The actual runtime is position independent (on linux at least). The wrapper is not (there is already a wrapper coming from the firefox-unwrapped derivation), so that path and the symlink have to be rewritten. I went for symlinks instead of hardlinks, because I did not want to make assumptions on the file system used, etc. Also how would using hard links simplify anything really?

Making a recursive copy would simplify the code somewhat I guess, at the cost of using more memory.

@xaverdh
Copy link
Contributor Author

xaverdh commented Apr 5, 2019

So maybe if we could change firefox-unwrapped to not wrap the binary (e.g. move that code to the actual wrapper), this could be simplified a lot.
I don't know how to do that in detail though.

@Qubasa
Copy link
Contributor

Qubasa commented Aug 28, 2019

@samueldr So based on the wrapper from @xaverdh I have added extension support and a lot of anti tracking / speed improvements for firefox. If this gets merged I can then open a new pull request.
If someone is interested the wrapper is here: https://github.com/Luis-Hebendanz/nix-configs/blob/master/overlays/firefox-with-config.nix
And an example usage is here: https://github.com/Luis-Hebendanz/nix-configs/blob/master/firefox.nix

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@ryantm ryantm marked this pull request as draft October 23, 2020 03:09
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 23, 2020
@xaverdh
Copy link
Contributor Author

xaverdh commented Dec 3, 2020

Closing since #91724 was merged

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

Successfully merging this pull request may close these issues.

None yet

10 participants