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.apache-airflow: 2.5.0 -> 2.5.1 #214565

Closed
wants to merge 2 commits into from

Conversation

risicle
Copy link
Contributor

@risicle risicle commented Feb 4, 2023

Description of changes

https://nvd.nist.gov/vuln/detail/CVE-2023-22884

This is an attempted security-bump of a currently-broken package - it can't sensibly be merged yet. That said, this can be tested by temporarily reverting commits 46a2dfc and 6d10040.

This should be properly fixed once flask-appbuilder adds support for flask-sqlalchemy >= 3 (see dpgaspar/Flask-AppBuilder#1940 for some possible movement), but in the meantime I'm going to turn my attention to the stable branch...

Note the minor changes I made to the update-providers.py script here. More details on this in the commit comment.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

…better

while you'd think `allowAliases = false` would do this job for us,
some still seem to slip through. deduplicate packages with the
same outpath before checking for multiple results.

avoid use of `assert` for raising failures as python considers
these statements omittable.
@LeSuisse
Copy link
Member

I just stumbled upon this PR while trying to update Apache Airflow to 2.5.2 since it was bumped to 2.5.1 in 067b77f

For reference my attempt for 2.5.1 -> 2.5.2: LeSuisse@9d210dc

@mweinelt
Copy link
Member

mweinelt commented May 9, 2023

https://www.openwall.com/lists/oss-security/2023/05/08/2

IMO: No maintainer feedback for months, let's mark it insecure and drop it a few months down the road.

@gbpdt
Copy link
Contributor

gbpdt commented May 9, 2023

Didn't see this before, happy to take a look.

@graham33
Copy link
Contributor

graham33 commented May 9, 2023

It looks like @risicle has already done some work on this. It's blocked by the fact that flask-appbuilder doesn't work with flask-sqlalchemy >= 3, which has already been upgraded in nixpkgs. There is a draft PR but it hasn't been progressed since last year. I've filed dpgaspar/Flask-AppBuilder#2038 to try to move it along.

One option would be to add separate versions of these dependencies within apache-airflow, similar to what we do for home-assistant. Since airflow is often used as a standalone server process this might be to too terrible.

@mweinelt
Copy link
Member

mweinelt commented May 9, 2023

One option would be to add separate versions of these dependencies within apache-airflow, similar to what we do for home-assistant. Since airflow is often used as a standalone server process this might be to too terrible.

Where do we do what exactly for Home Assistant?

Home Assistant is in a different boat, since it uses buildPythonApplication and is outside the python package set. That is something I've recommended to the airflow maintainers on multiple occasions over the years. That would allow more individualized dependency management.

@graham33
Copy link
Contributor

Where do we do what exactly for Home Assistant?

Home Assistant is in a different boat, since it uses buildPythonApplication and is outside the python package set.

I was referring to this:

pkgs/top-level/all-packages.nix
1417:  apache-airflow = with python3.pkgs; toPythonApplication apache-airflow;

Airflow is both an application and a python package, and it's feasible to use it as a standalone app without necessarily having the module importable in a larger package set (e.g. people can use shell command tasks, and run a separate python interpreter from them if they want).

So my suggestion is to mark apache-airflow as broken in pythonPackages, add 'local' dependencies for flask-appbuilder et al. (if feasible without a huge amount of work), so that pkgs.apache-airflow can be made to work. Then hopefully the dependency issue resolves over time and we can remove the hacks and re-enable the python package.

Let me know if that makes sense. It's obviously far from ideal!

@mweinelt
Copy link
Member

mweinelt commented May 11, 2023

IMO the airflow package should be moved out of the python package set. All packages that require it as a library can then live inside its own python package set instead.

The idea for home-assistant was this:

diff --git a/pkgs/servers/home-assistant/default.nix b/pkgs/servers/home-assistant/default.nix
index c31862f59d1a..13fc0d523855 100644
--- a/pkgs/servers/home-assistant/default.nix
+++ b/pkgs/servers/home-assistant/default.nix
@@ -300,6 +300,7 @@ let
       # internal python packages only consumed by home-assistant itself
       home-assistant-frontend = self.callPackage ./frontend.nix { };
       home-assistant-intents = self.callPackage ./intents.nix { };
+      homeassistant = super.toPythonPackage home-assistant { };
     })
   ];

@graham33
Copy link
Contributor

Hopefully superseded by #231944

@wegank
Copy link
Member

wegank commented Sep 1, 2023

Superseded by #235081.

@wegank wegank closed this Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants