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] chromium.withPackages/chromium-extensions: init #98014

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ryneeverett
Copy link
Contributor

@ryneeverett 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)

@samuelgrf
Copy link
Member

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

@ryneeverett ryneeverett force-pushed the chromium-withpackages branch 2 times, most recently from 37918b6 to e5ce2b5 Compare September 15, 2020 14:59
@danielfullmer
Copy link
Contributor

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

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

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

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

@ryneeverett
Copy link
Contributor Author

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

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

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

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)

@danielfullmer
Copy link
Contributor

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

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.

@ryneeverett ryneeverett changed the title chromium.withPackages/chromium-extensions: init [WIP] chromium.withPackages/chromium-extensions: init Oct 11, 2020
@ryneeverett
Copy link
Contributor Author

ryneeverett commented Oct 11, 2020

Finally got around to testing a commit patching chromium. It does seem to successfully persist extension id's across extension rebuilds but the state does not persist and it appears to introduce some sort of extension database corruption (after rebasing and rebuilding I can no longer reproduce any error message so they would seem to have been unrelated).

For the record I did consider patching in the full id (rather than the current approach of using the name within chromium to generate a consistent id) and decided against it. It would basically require implementing the id_util.cc file in nix and I don't see a lot of value in that. Also the ConvertHexadecimalToIDAlphabet method doesn't seem trivial to implement in nix.

ryneeverett and others added 10 commits October 22, 2020 15:28
Resolve NixOS#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.
As pointed out by @samuelgrf ublock could be confusing and the author
is fairly consistent in preferring the ublock origin namespace.
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.
Extension id's are generated from a hash of the extension path. In
development this is fine as the extension is updated in place, but as a
nix package this would cause state to be lost on every rebuild when the
path changes.  Therefore we replace the path with the extension's name
for the purposes of generating an id.
@stale
Copy link

stale bot commented Jul 19, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants