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

chromium.withPackages/chromium-extensions: init #98014

Open
wants to merge 9 commits into
base: master
from

Conversation

@ryneeverett
Copy link
Contributor

ryneeverett commented Sep 14, 2020

Motivation for this change

Resolve #76863.

This supports the syntax:

(chromium.withPackages(ps: [
  ps.<extension-package>
]))

Upon investigation into the many api's chromium has had for loading
extensions over the years, it appears that the only remaining way to
load extensions from a local path is by flag which indicates that adding
them to the wrapper like this is the only sane option. Chromium's
existing plugins.nix already does most of the lifting towards creating
the wrapper.

This also adds a builder -- buildChromiumExtension -- which basically just
requires that the current working directory be the plugin in the
installPhase.

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.
Things to do (help wanted)
  • Run chromium test. (I don't have the horsepower locally or the ofborg bits.)
  • Documentation. (where should this go?)
  • Persistent extension state. (see comments)
  • Decentraleyes loads welcome page every time? (see comments)
ryneeverett added 3 commits Sep 14, 2020
Resolve #76863.

This supports the syntax:

    (chromium.withPackages(ps: [
      ps.<extension-package>
    ]))

Upon investigation into the many api's chromium has had for loading
extensions over the years, it appears that the only remaining way to
load extensions from a local path is by flag which indicates that adding
them to the wrapper like this is the only sane option. Chromium's
existing plugins.nix already does most of the lifting towards creating
the wrapper.

This also adds a builder -- buildChromiumExtension -- which basically just
requires that the current working directory be the plugin in the
installPhase.
@samuelgrf
Copy link
Contributor

samuelgrf commented Sep 15, 2020

Can you rename uBlock to uBlock origin, so users don't get the two mixed up.

@ryneeverett ryneeverett force-pushed the ryneeverett:chromium-withpackages branch from 4fb6539 to 37918b6 Sep 15, 2020
As pointed out by @samuelgrf ublock could be confusing and the author
is fairly consistent in preferring the ublock origin namespace.
@ryneeverett ryneeverett force-pushed the ryneeverett:chromium-withpackages branch from 37918b6 to e5ce2b5 Sep 15, 2020
@danielfullmer
Copy link
Contributor

danielfullmer commented Sep 15, 2020

Something like this is also especially useful for ungoogled-chromium, which doesn't use the chrome web store.

However, there is at least one problem with using unpacked extensions like this: Each chromium extension has a unique "extension id" like kabgoaeodlokjhpcbkopamcaelaoibpo. Normally, this extension ID is based on the public key used to sign the .crx file. For unpacked extensions, it is instead based on the sha256 of the path of the extension. See
https://chromium.googlesource.com/chromium/src/+/refs/tags/86.0.4240.41/chrome/browser/extensions/unpacked_installer.cc#225
https://chromium.googlesource.com/chromium/src/+/refs/tags/86.0.4240.41/components/crx_file/id_util.cc#60

What this means for us is that if there is any change to extension derivation hash in nixpkgs, we'd end up with a different path under /nix/store, which would mean a different extension ID. If these extension IDs change, we'd lose any state associated with them.

It might be possible to patch chromium to generate consistent extension IDs (perhaps by stripping the /nix/store/<hash>- prefix off the path and depending only on the derivation name.)

@ryneeverett
Copy link
Contributor Author

ryneeverett commented Sep 16, 2020

It might be possible to patch chromium to generate consistent extension IDs (perhaps by stripping the /nix/store/- prefix off the path and depending only on the derivation name.)

Alternately, would it be an option to symlink extensions into the profile and then pass the consistent profile paths? It looks like it would work as symlinks aren't resolved until after the id is generated, but I'm not very familiar with nix policy on what can go in the profile or how to symlink stuff into the profile.

@danielfullmer
Copy link
Contributor

danielfullmer commented Sep 16, 2020

Did you have something in mind about what the consistent profile paths could be? I don't think it should be in either the nixos system profile (shouldn't be nixos-specific) or the user profile (since then it depends on the username).

(Sorry about inadvertently closing this)

@danielfullmer danielfullmer reopened this Sep 16, 2020
@ryneeverett
Copy link
Contributor Author

ryneeverett commented Sep 16, 2020

I'd propose installing it into the same profile chromium is installed into, regardless of whether it's system or user.

@danielfullmer
Copy link
Contributor

danielfullmer commented Sep 16, 2020

@ryneeverett Sure, but what path do you propose to pass to --load-extension?

@ryneeverett
Copy link
Contributor Author

ryneeverett commented Sep 16, 2020

For example:
system installation -> /nix/var/nix/profiles/system/sw/lib/chromium-extensions/ublock-origin
user installation -> /nix/var/nix/profiles/per-user/ryne/profile/lib/chromium-extensions/ublock-origin

@danielfullmer
Copy link
Contributor

danielfullmer commented Sep 16, 2020

I'd still like to be able to build (for example) chromium.withPackages (ps: [ ps.ublock-origin ]), and then immediately execute it via ./result/bin/chromium. This built path is not part of any profile (user or system), and if we used --load-extension paths referring to /nix/var/nix/profiles, it would end up impurely using whatever we currently had in our profile.

Also, those paths depend on things that might change, such as the username or the system profile name (see, for example, the --profile-name option of nixos-rebuild). The path could also change if a user moves their chromium package from their system profile to their user profile (or vice-versa)

(edit: Thanks for the PR by the way. This is definitely a feature I'm interested in as well.)

@ryneeverett
Copy link
Contributor Author

ryneeverett commented Sep 16, 2020

I'd still like to be able to build (for example) chromium.withPackages (ps: [ ps.ublock-origin ]), and then immediately execute it via ./result/bin/chromium.

I don't necessarily agree with your other points but this one I have no answer to. If a package can exist without a profile then the profile is useless for linking dependencies in this way.

@danielfullmer
Copy link
Contributor

danielfullmer commented Sep 16, 2020

Just in case it's interesting to you, I've started experimenting with this using the bypass-paywalls-chrome extension here: ce3d14d
I also added a commit (b1ddc86) which automatically removes the key or update_url entry from the manifest.json if it exists. The key manifest entry is included bypass-paywalls-chrome, and since we aren't signing these extensions, we should remove the key if it exists. I expect other chromium extensions might have that issue as well.

I've also found that the decentraleyes extension loads into its "2.0 Says Hello" page every time I start chromium. Normally this should only happen the first time, right? Is this a side-effect of loading it as an unpacked extension?

@ryneeverett
Copy link
Contributor Author

ryneeverett commented Sep 16, 2020

I'm thinking the simplest patching approach would be to add an environment variable to the wrapper with a serialized mapping of {nix-store-path -> extension-name} and then patch in to GenerateIdForPath:

python/pseudocode:

nixmap = json.loads(os.getenv('NIX_CHROMIUM_EXTENSIONS`, ''))
path = nixmap.get(path, path)
ryneeverett and others added 4 commits Sep 16, 2020
This is an artifact of an earlier revision in which I was going to put a
nix-support folder in each extension rather than have a separate
chromium-extensions derivation. It was a lot simpler but didn't work because
chromium ignores multiple --load-extension flags.
@ryneeverett ryneeverett force-pushed the ryneeverett:chromium-withpackages branch from aef2921 to 2f06aee Sep 16, 2020
@danielfullmer
Copy link
Contributor

danielfullmer commented Sep 17, 2020

Passing a mapping via an environment variable makes a lot of sense. Another option would be a mapping of nix-store-path -> extension-id. It would allow us to manually set the extension id to any desired value via nix. If desired, it could still be based on the sha256 of the extension name. Another option would be to set it to the same id as is used by existing extensions in the chrome web store. Hopefully we could then use the same state as the existing extensions. There may be other conflicting issues associated with using the same extension id. (maybe security?)

@ryneeverett
Copy link
Contributor Author

ryneeverett commented Sep 17, 2020

Another option would be a mapping of nix-store-path -> extension-id. It would allow us to manually set the extension id to any desired value via nix. If desired, it could still be based on the sha256 of the extension name.

Yes, perhaps it would be more robust to control the id in nix rather than in patched chromium. However I think it would be a slightly more involved patch because GenerateIdForPath appears to be used at least one other relevant place in the code base to load unpacked extensions and it should still perform it's function if a non-store path is passed to it.

Another option would be to set it to the same id as is used by existing extensions in the chrome web store. Hopefully we could then use the same state as the existing extensions. There may be other conflicting issues associated with using the same extension id. (maybe security?)

I'm not sure sharing state with webstore-installed extensions by default is desirable. I also don't think it would work out of the box anyway since chromium seems to isolate the storage of local and webstore extension state. I don't foresee any conflicts or security issues though. It also might facilitate sharing state with webstore installations via browser sign-in, but I'm doubtful. Unpacked extensions are designed for development use so I wouldn't be surprised if features like that aren't supported.

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.

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