Add bandit to pre-commit to detect common security issues

[hussein-awala]

closes: #34241

This PR adds bandit to pre-commit hooks and static checks, to detect common security issues.

I'm opening it as draft to get the report, then try to fix/ignore the issues in separate PRs.

[potiuk]

Those are tools that are installed in the image. You likely need to add bandit as dependency in the .pre-commit-config.yml as "dependencies" (see other examples).

[potiuk]

See the comment above - this pre-commit does not require Breeze CI image so it should be added before those that need it.

[potiuk]

See:

## ADD MOST PRE-COMMITS ABOVE THAT LINE

[hussein-awala]

The pre-commit fails with this error:

Executable `bandit` not found

I tried both CI image pre-commits and normal pre-commits, and it failed in both cases, knowing that I added bandit to _devel_only_static_checks list, do I need to open the PR from a branch in Airflow repo?

[potiuk]

No - you need to use additional_dependencies . The way how regular pre-commits work is that pre-commit knows the language python and knows how to create an environment for it automatically. This is what additional_dependencies is for. In case of python precommit, this is really a specification of virtualenv it should create (so you need to add bandid as requirement THERE not in airfllow dependencies.

It understands other languages - for example for node you can specify npm dependencies in additional_dependencies field.

Pre-commit is actually even smart enough to re-use virtualenvs it creates between different checks if they have the same requirements or even between different projects that use pre-commit - because the venvs for pre-commit are created in ~/.cache/pre-commit/ in your home directory.

You can see exactly what happens when you remove ~/.cache/pre-commit folder. The first thing pre-commit will do it will find all the distinct venvs and node env and other language envs and will create all of them. It will take few minutes but you will see what's happening and you will understand what's going on.

[potiuk]

So just to add - basically with pre-commit you have 20 or so pre-commit venvs that are automatically updated and maintained by pre-commit (distinct set of requirements). They have nothing to do with either airflow requirements nor breeze image requirements :). It's just using (well) the approach that for every tool (every check) you should have separate venv that you run it in. And pre-commit does all the heavy lifting to:

a) maintain them (upgrade when you change the requirements
b) share them between checks if several checks use the same set of requirements
c) make sure each check uses the right venv..

[potiuk]

And to add even more: system pre-commits are special - they will just run whatever you put there as a script. This is how we are adding the pre-commits that are using breeze CI image to run stuff - those scripts of ours will make sure the image is locally built and they will run docker run to run whatever they need to run as part of the check. There the environment managerment responsibility is passed from pre-commit to breeze essentially.

[potiuk]

(or we rather used to do it) -> I recalled that I converted all such "scripts" to python to make it easier. So those are now python ones with very small set of requirements ['rich>=12.4.4', 'inputimeout', 'pyyaml'] for examplte that run bootstrapping python script (./scripts/ci/pre_commit/pre_commit_mypy.py for example), but essentially what the script does is it runs some docker command:

  cmd_result = run_command(
        [
            "docker",
            "run",
            "-t",
....

So the bootstrapping script uses the small ['rich>=12.4.4', 'inputimeout', 'pyyaml'] venv - to launch docker command in airflow's CI image....

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

[potiuk]

I guess conflicts/rebase needed now :)

[Taragolis]

Just wondering, any reason why we can't use ruff implementation https://docs.astral.sh/ruff/rules/#flake8-bandit-s ?

[potiuk]

Just wondering, any reason why we can't use ruff implementation https://docs.astral.sh/ruff/rules/#flake8-bandit-s ?

Sounds like good idea :)

[hussein-awala]

Just wondering, any reason why we can't use ruff implementation https://docs.astral.sh/ruff/rules/#flake8-bandit-s ?

I checked it when I started working on this PR, and I found that the implementation is still in progress, and there are a few bugs (astral-sh/ruff#1646), it could be a good future optimization or an alternative solution when we decide what are the rules we want to check if they are all supported by ruff plugin.

[potiuk]

[potiuk]

looking good

[hussein-awala]

Yes, I just restarted some CI jobs that failed because of resources/network issues, I will mark it ready for review once all CI check are green.