Introduce configuration to disable md5 used for security #31171

longslvr · 2023-05-09T21:22:58Z

Add a configuration flag to disable the use of md5 for security in Python >= 3.9. If Python < 3.9 is being used, this flag will not provide any effect on the existing behaviour. This is mainly for backward compatibility suggested in: #25625

With this configuration combined with the newly introduced flag to change the caching hash method (introduced in: #30675), airflow will be able to run in the environment restricted by FIPS 140-2 requirements.

Background: I have been running in FIPS enabled environment for my company, in order to do that we would have to fork out Airflow and introduce all the changes so that Airflow is fully functional. That added extra complexity when upgrading Airflow to the latest version. Also, a few people show interest in running Airflow in FIPS environment so I want to introduce the change in Airflow so we can all take advantage of that.

Testing done:

run pre-commit
running unit tests with Breeze for the following Python versions: python 3.9, python 3.7
Starting up airflow with Breeze and running the preloaded example dags in both Python 3.9 and Python 3.7

Thanks @vchiapaikeo for the work on the hashlib wrapper and the configurable caching

boring-cyborg · 2023-05-09T21:23:05Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

kaxil · 2023-05-09T23:13:35Z

airflow/config_templates/config.yml

+security:
+  description: ~
+  options:
+    disable_md5_for_security:


for_security in the config seems redundant

uranusjr · 2023-05-10T06:52:21Z

What are the downsides if we always enable useforsecurity?

longslvr · 2023-05-10T14:32:14Z

The configuration is a more conservative approach to let the users opt-in instead of opt-out as this functionality is not commonly used.
This flag doesn't exist in Python prior to 3.9 so doing this way would safeguard the usage.

airflow/providers/google/cloud/hooks/bigquery.py

potiuk

Nice - in this form it is backwards compatible and great to have a separate configuration for it.

airflow/cli/commands/webserver_command.py

airflow/providers/google/cloud/hooks/bigquery.py

airflow/kubernetes/pod_generator.py

ashb

Do we need a config option for this? If it's possible to detect if the option will be accepted and just set it automatically? How is a user ID Airflow meant to know how we use md5 in the code to reasonably set this config option

If it only affects 3.9 let's just always set it there

ashb

-1 to config option.

How is a user of Airflow meant to know how we use md5 in the code in order to reasonably set this config option.

This is a property of the code, not of the installation/deployment.

longslvr · 2023-05-25T17:55:04Z

Do we need a config option for this? If it's possible to detect if the option will be accepted and just set it automatically?

If it only affects 3.9 let's just always set it there

I was going for the opt-in approach for these changes. Since the change is only required for the FIPS compliance, the configuration is to allow people to opt-in when running in FIPS environments instead of opt-out

ashb · 2023-05-25T17:56:55Z

Does it only work in FIPS environments, or is just ignored outside of one?

longslvr · 2023-05-25T18:09:52Z

Does it only work in FIPS environments, or is just ignored outside of one?

It works outside of FIPS environements. I will make this as default and remove the configuration.

@ashb it is done now

ashb

LGTM. @potiuk You happy with the config-less approach here?

…to minimum airflow 2.7.0

boring-cyborg · 2023-05-26T13:06:40Z

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

Co-authored-by: Long Nguyen <[email protected]> (cherry picked from commit 22e44ab)

umamaheswar52 · 2024-03-15T15:28:42Z

@longslvr - one quick confirmation needed pls. Suppose that if I install Apache Airflow 2.6.1 (or <= 2.7.3) successfully on FIPS enabled Ubuntu OS, shall I ensure myself Airflow (and its supporting libraries) are FIPS enabled?

longslvr · 2024-03-19T16:07:11Z

@longslvr - one quick confirmation needed pls. Suppose that if I install Apache Airflow 2.6.1 (or <= 2.7.3) successfully on FIPS enabled Ubuntu OS, shall I ensure myself Airflow (and its supporting libraries) are FIPS enabled?

For Airflow to function correctly in a FIPs-enabled environment, MD5 usage for security must be disabled. You can install Airflow but it might not be able to run. Prior to 2.7.3 you would have to fork Airflow and make your change to disable it yourself.
I would suggest updating Airflow to >= 2.7.3 and use the configuration to turn of MD5 for security

longslvr requested review from dstandish, jedcunningham, kaxil, XD-DENG and ashb as code owners May 9, 2023 21:22

kaxil reviewed May 9, 2023

View reviewed changes

longslvr force-pushed the introduce_configuration_for_md5_to_run_in_fips_environment branch from 440b724 to 9d2d023 Compare May 15, 2023 18:57

longslvr requested a review from kaxil May 15, 2023 20:48

eladkal reviewed May 16, 2023

View reviewed changes

airflow/providers/google/cloud/hooks/bigquery.py Outdated Show resolved Hide resolved

longslvr force-pushed the introduce_configuration_for_md5_to_run_in_fips_environment branch from 9d2d023 to d3ffc8a Compare May 17, 2023 03:02

potiuk approved these changes May 17, 2023

View reviewed changes

potiuk reviewed May 17, 2023

View reviewed changes

airflow/cli/commands/webserver_command.py Outdated Show resolved Hide resolved

potiuk reviewed May 17, 2023

View reviewed changes

airflow/providers/google/cloud/hooks/bigquery.py Show resolved Hide resolved

potiuk reviewed May 17, 2023

View reviewed changes

airflow/kubernetes/pod_generator.py Outdated Show resolved Hide resolved

longslvr force-pushed the introduce_configuration_for_md5_to_run_in_fips_environment branch 2 times, most recently from 3c4024b to 6128774 Compare May 19, 2023 16:43

longslvr force-pushed the introduce_configuration_for_md5_to_run_in_fips_environment branch from 6128774 to 1073dc8 Compare May 25, 2023 13:54

ashb reviewed May 25, 2023

View reviewed changes

ashb requested changes May 25, 2023

View reviewed changes

longslvr requested a review from ashb May 25, 2023 20:13

ashb approved these changes May 25, 2023

View reviewed changes

Long Nguyen added 6 commits May 25, 2023 20:41

Introduce configuration to disable md5 used for security

bf3dfc7

Allow backward compatibility for Airflow 2.4

0f29693

Update to clean up try import and add comment to remove when upgrade …

898f2e6

…to minimum airflow 2.7.0

Address testing failures

4b8183d

Disable md5 for security by default

bab8d2e

Fix unit test after changing the path to md5

0bb0f35

longslvr force-pushed the introduce_configuration_for_md5_to_run_in_fips_environment branch from c91dae6 to 0bb0f35 Compare May 26, 2023 00:42

eladkal approved these changes May 26, 2023

View reviewed changes

eladkal requested a review from potiuk May 26, 2023 08:58

ashb merged commit 22e44ab into apache:main May 26, 2023

longslvr deleted the introduce_configuration_for_md5_to_run_in_fips_environment branch May 26, 2023 14:51

eladkal added this to the Airflow 2.6.2 milestone May 26, 2023

eladkal added the type:improvement Changelog: Improvements label May 26, 2023

eladkal pushed a commit that referenced this pull request Jun 8, 2023

FIPS environments: Mark uses of md5 as "not-used-for-security" (#31171)

a880168

Co-authored-by: Long Nguyen <[email protected]> (cherry picked from commit 22e44ab)

eladkal modified the milestones: Airflow 2.6.2, Airflow 2.7.0 Jun 9, 2023

eladkal mentioned this pull request Jun 20, 2023

Status of testing Providers that were prepared on June 20, 2023 #32030

Closed

86 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce configuration to disable md5 used for security #31171

Introduce configuration to disable md5 used for security #31171

longslvr commented May 9, 2023 •

edited

Loading

boring-cyborg bot commented May 9, 2023

kaxil May 9, 2023

uranusjr commented May 10, 2023

longslvr commented May 10, 2023

potiuk left a comment •

edited

Loading

ashb left a comment •

edited

Loading

ashb left a comment •

edited

Loading

longslvr commented May 25, 2023

ashb commented May 25, 2023

longslvr commented May 25, 2023 •

edited

Loading

ashb left a comment

boring-cyborg bot commented May 26, 2023

umamaheswar52 commented Mar 15, 2024

longslvr commented Mar 19, 2024

Introduce configuration to disable md5 used for security #31171

Introduce configuration to disable md5 used for security #31171

Conversation

longslvr commented May 9, 2023 • edited Loading

boring-cyborg bot commented May 9, 2023

kaxil May 9, 2023

Choose a reason for hiding this comment

uranusjr commented May 10, 2023

longslvr commented May 10, 2023

potiuk left a comment • edited Loading

Choose a reason for hiding this comment

ashb left a comment • edited Loading

Choose a reason for hiding this comment

ashb left a comment • edited Loading

Choose a reason for hiding this comment

longslvr commented May 25, 2023

ashb commented May 25, 2023

longslvr commented May 25, 2023 • edited Loading

ashb left a comment

Choose a reason for hiding this comment

boring-cyborg bot commented May 26, 2023

umamaheswar52 commented Mar 15, 2024

longslvr commented Mar 19, 2024

longslvr commented May 9, 2023 •

edited

Loading

potiuk left a comment •

edited

Loading

ashb left a comment •

edited

Loading

ashb left a comment •

edited

Loading

longslvr commented May 25, 2023 •

edited

Loading