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

python3Packages.matrix-nio: permit insecure Olm during check phase #336694

Conversation

AndrewKvalheim
Copy link
Contributor

Description of changes

Olm has a known vulnerability but is only an optional dependency of nio, so in theory nio should by default be unaffected. nio’s tests, however, cover its full suite of extra features, so Olm is still evaluated as a dependency of the check phase. Since the check phase doesn’t process user data or access the network this vulnerability isn’t relevant and can be ignored, allowing nio to evaluate and ultimately be run without Olm.

I’m not confident in this implementation. Is there a tidier way to do it? Should the permission be narrowed to specifically this vulnerability? I haven’t found any precedent for this and wonder if something like a checkPhasePermittedInsecurePackages would be warranted.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using git cherry-pick NixOS/#336651 && nixpkgs-review rev HEAD.
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Olm has a known vulnerability (NixOS#334638) but is only an optional
dependency of nio, so in theory nio should by default be unaffected.
nio’s tests, however, cover its full suite of extra features, so Olm is
still evaluated as a dependency of the check phase. Since the check
phase doesn’t process user data or access the network this vulnerability
isn’t relevant and can be ignored, allowing nio to evaluate and
ultimately be run without Olm.
@emilazy
Copy link
Member

emilazy commented Aug 23, 2024

Ah, sorry for dropping the ball on this. (I was thinking of another package!) I agree that we should solve this. We should also backport the fix to 24.05 before I merge #335189.

We could also just do one of these:

  • Re‐enumerate the optional dependencies again with the override for olm, since there aren’t that many.
  • Define e2eDeps = python-olm': [ atomicwrites … python-olm' … ] and use it appropriately for both values.
  • Skip all of this and don’t make Hydra build libolm again and just disable the libolm‐requiring tests since after all that’s not exactly a “first‐class” supported code path now.

I think I’d prefer any one of them, since they’re simpler than the construction here.

@emilazy
Copy link
Member

emilazy commented Aug 23, 2024

Can we handle #336052 here too since it’s the exact same issue?

@emilazy
Copy link
Member

emilazy commented Aug 23, 2024

Also I think instead of olm = lib.findFirst (p: p.pname == "olm") null pythonPackage.buildInputs it should be fine to just say pkgs.olm? I guess that would make overlaying python-olm with python-olm.override { olm = …; } weird, but if we’re worried about that case we should just add, say, passthru = { inherit olm; } to python-olm.

Anyway I lean toward just disabling the checks.

@AndrewKvalheim
Copy link
Contributor Author

Thanks, how’s that for a simpler implementation? Please also feel free to just edit yourself if that’s easier.

@SuperSandro2000
Copy link
Member

Can we please stop such hick and just ignore the effected tests?

@AndrewKvalheim
Copy link
Contributor Author

Yes.

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.

3 participants