[AIRFLOW-2809] Fix security issue regarding Flask SECRET_KEY #3651

XD-DENG · 2018-07-27T07:17:19Z

JIRA

My PR addresses the following Airflow JIRA issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-2809
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a JIRA issue.

Description

Here are some details about my PR, including screenshots of any UI changes:

Background

Currently there is a configuration item secret_key in the configuration .cfg file, with a default value "temporary_key".

Issue

Most admins would ignore it and just use the default value "temporary_key". However, this may be very dangerous. User may modify the cookie if they try the default SECRET_KEY while the admin didn't change it.

**In Flask documentation, it's suggested to have a SECRET_KEY which is as random as possible (http://flask.pocoo.org/docs/1.0/quickstart/ ). **

My Proposal

If Admin explicitly changed the SECRET_KEY in .cfg file, we use this SECRET_KEY given by Admin.

If the default SECRET_KEY is not changed in .cfg file, randomly generate SECRET_KEY. Meanwhile, print INFO to remind that a randomly generated SECRET_KEY is used.

This solution will not affect user experience at all.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

Passes git diff upstream/master -u -- "*.py" | flake8 --diff

Fokko · 2018-07-27T08:57:27Z

@XD-DENG Can you prepend the squash the commits and prepend the message with the Jira ticket [AIRFLOW-2809] ...

This looks like a good idea to me, maybe check what the other committers think about this.

cc @kaxil @bolkedebruin @jgao54

XD-DENG · 2018-07-27T09:34:06Z

Thanks @Fokko . Have squashed the commits and prepended the JIRA ticket label as suggested.

It's recommended by Falsk community to use random SECRET_KEY for security reason. However, in Airflow there is a default value for secret_key and most users will ignore to change it. This may cause security concern.

codecov-io · 2018-07-28T16:59:38Z

Codecov Report

Merging #3651 into master will increase coverage by <.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #3651      +/-   ##
==========================================
+ Coverage    77.5%   77.51%   +<.01%     
==========================================
  Files         205      205              
  Lines       15747    15751       +4     
==========================================
+ Hits        12205    12209       +4     
  Misses       3542     3542

Impacted Files	Coverage Δ
airflow/www/app.py	`99.01% <83.33%> (-0.99%)`	⬇️
airflow/models.py	`88.58% <0%> (+0.04%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 449a7fd...c3f231b. Read the comment docs.

kaxil · 2018-07-28T19:27:34Z

Thank you @XD-DENG , @Fokko This makes sense to me as well. LGTM

Fokko

LGTM

The same issue was fixed for /www previously in PR apache#3651 (JIRA ticket 2809)

It's recommended by Falsk community to use random SECRET_KEY for security reason. However, in Airflow there is a default value for secret_key and most users will ignore to change it. This may cause security concern. Closes apache#3651 from XD-DENG/patch-2

The same issue was fixed for /www previously in PR #3651 (JIRA ticket 2809)

…e#3729) The same issue was fixed for /www previously in PR apache#3651 (JIRA ticket 2809)

It's recommended by Falsk community to use random SECRET_KEY for security reason. However, in Airflow there is a default value for secret_key and most users will ignore to change it. This may cause security concern. Closes apache#3651 from XD-DENG/patch-2

…e#3729) The same issue was fixed for /www previously in PR apache#3651 (JIRA ticket 2809)

It's recommended by Falsk community to use random SECRET_KEY for security reason. However, in Airflow there is a default value for secret_key and most users will ignore to change it. This may cause security concern. Closes #3651 from XD-DENG/patch-2 (cherry picked from commit dfa7b26)

The same issue was fixed for /www previously in PR #3651 (JIRA ticket 2809) (cherry picked from commit fe6d00a)

…e#3729) The same issue was fixed for /www previously in PR apache#3651 (JIRA ticket 2809) (cherry picked from commit fe6d00a) (cherry picked from commit a8900fa) (cherry picked from commit 5b08ec2c3b5b0e67dcdd176a5b3ecbd6f0318a6e) (cherry picked from commit b3711ff)

It's recommended by Falsk community to use random SECRET_KEY for security reason. However, in Airflow there is a default value for secret_key and most users will ignore to change it. This may cause security concern. Closes apache#3651 from XD-DENG/patch-2 (cherry picked from commit dfa7b26) (cherry picked from commit 2f3b1c7)

…e#3729) The same issue was fixed for /www previously in PR apache#3651 (JIRA ticket 2809) (cherry picked from commit fe6d00a) (cherry picked from commit a8900fa)

- BugFix: Tasks with ``depends_on_past`` or ``task_concurrency`` are stuck (apache#12663) - Fix issue with empty Resources in executor_config (apache#12633) - Fix: Deprecated config ``force_log_out_after`` was not used (apache#12661) - Fix empty asctime field in JSON formatted logs (apache#10515) - [AIRFLOW-2809] Fix security issue regarding Flask SECRET_KEY (apache#3651) - [AIRFLOW-2884] Fix Flask SECRET_KEY security issue in www_rbac (apache#3729) - [AIRFLOW-2886] Generate random Flask SECRET_KEY in default config (apache#3738) - Add missing comma in setup.py (apache#12790) - Bugfix: Unable to import Airflow plugins on Python 3.8 (apache#12859) - Fix setup.py missing comma in ``setup_requires`` (apache#12880) - Don't emit first_task_scheduling_delay metric for only-once dags (apache#12835) - Update setup.py to get non-conflicting set of dependencies (apache#12636) - Rename ``[scheduler] max_threads`` to ``[scheduler] parsing_processes`` (apache#12605) - Add metric for scheduling delay between first run task & expected start time (apache#9544) - Add new-style 2.0 command names for Airflow 1.10.x (apache#12725) - Add Kubernetes cleanup-pods CLI command for Helm Chart (apache#11802) - Don't let webserver run with dangerous config (apache#12747) - Replace pkg_resources with importlib.metadata to avoid VersionConflict errors (apache#12694) - Clarified information about supported Databases

It's recommended by Falsk community to use random SECRET_KEY for security reason. However, in Airflow there is a default value for secret_key and most users will ignore to change it. This may cause security concern. Closes apache#3651 from XD-DENG/patch-2 (cherry picked from commit dfa7b26)

The same issue was fixed for /www previously in PR apache#3651 (JIRA ticket 2809) (cherry picked from commit fe6d00a)

The same issue was fixed for /www previously in PR apache/airflow#3651 (JIRA ticket 2809) (cherry picked from commit fe6d00a54f83468e296777d3b83b65a2ae7169ec) GitOrigin-RevId: a8900fa5f2b8963e9f57ba4ae5520a5d339aeaad

The same issue was fixed for /www previously in PR apache/airflow#3651 (JIRA ticket 2809) GitOrigin-RevId: fe6d00a54f83468e296777d3b83b65a2ae7169ec

XD-DENG force-pushed the patch-2 branch 3 times, most recently from 0f28717 to 48bff00 Compare July 27, 2018 09:33

XD-DENG force-pushed the patch-2 branch 3 times, most recently from 71f60cc to 511125a Compare July 27, 2018 10:01

[AIRFLOW-2809] Fix security issue regarding Flask SECRET_KEY

c3f231b

It's recommended by Falsk community to use random SECRET_KEY for security reason. However, in Airflow there is a default value for secret_key and most users will ignore to change it. This may cause security concern.

XD-DENG force-pushed the patch-2 branch from 511125a to c3f231b Compare July 28, 2018 16:22

Fokko approved these changes Jul 29, 2018

View reviewed changes

asfgit closed this in dfa7b26 Jul 29, 2018

XD-DENG deleted the patch-2 branch July 29, 2018 10:10

XD-DENG added a commit to XD-DENG/airflow that referenced this pull request Aug 10, 2018

[AIRFLOW-2884] Fix Flask SECRET_KEY security issue in www_rbac

97b2735

The same issue was fixed for /www previously in PR apache#3651 (JIRA ticket 2809)

XD-DENG mentioned this pull request Aug 10, 2018

[AIRFLOW-2884] Fix Flask SECRET_KEY security issue in www_rbac #3729

Merged

6 tasks

kaxil pushed a commit that referenced this pull request Aug 10, 2018

[AIRFLOW-2884] Fix Flask SECRET_KEY security issue in www_rbac (#3729)

fe6d00a

The same issue was fixed for /www previously in PR #3651 (JIRA ticket 2809)

XD-DENG mentioned this pull request Aug 10, 2018

[AIRFLOW-2886] Secure Flask SECRET_KEY #3738

Merged

6 tasks

feng-tao mentioned this pull request Aug 14, 2018

[AIRFLOW-2888] Remove shell=True and bash from task launch #3740

Merged

6 tasks

lxneng pushed a commit to lxneng/incubator-airflow that referenced this pull request Aug 20, 2018

[AIRFLOW-2884] Fix Flask SECRET_KEY security issue in www_rbac (apach…

8c0d4c3

…e#3729) The same issue was fixed for /www previously in PR apache#3651 (JIRA ticket 2809)

aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019

[AIRFLOW-2884] Fix Flask SECRET_KEY security issue in www_rbac (apach…

b5ffff6

…e#3729) The same issue was fixed for /www previously in PR apache#3651 (JIRA ticket 2809)

ashb pushed a commit that referenced this pull request Dec 1, 2020

[AIRFLOW-2884] Fix Flask SECRET_KEY security issue in www_rbac (#3729)

2d7d28a

The same issue was fixed for /www previously in PR #3651 (JIRA ticket 2809) (cherry picked from commit fe6d00a)

zachliu mentioned this pull request Dec 10, 2020

Add changes from 1.10.14 #12993

Merged

cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021

Fix Flask SECRET_KEY security issue in www_rbac (apache#3729)

ac1c222

The same issue was fixed for /www previously in PR apache#3651 (JIRA ticket 2809) (cherry picked from commit fe6d00a)

dependabot bot mentioned this pull request Mar 18, 2021

chore(deps): bump apache-airflow from 1.10.6 to 1.10.14 in /packages/pipeline_workflows Open-Speech-EkStep/audio-to-speech-pipeline#4

Merged

haroldwoo mentioned this pull request Jul 8, 2021

[DSRE-9] - Upgrade Airflow wtmo to 1.10.15 bridge release prior to 2.0 mozilla/telemetry-airflow#1335

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-2809] Fix security issue regarding Flask SECRET_KEY #3651

[AIRFLOW-2809] Fix security issue regarding Flask SECRET_KEY #3651

XD-DENG commented Jul 27, 2018 •

edited

Loading

Fokko commented Jul 27, 2018

XD-DENG commented Jul 27, 2018

codecov-io commented Jul 28, 2018 •

edited

Loading

kaxil commented Jul 28, 2018

Fokko left a comment

[AIRFLOW-2809] Fix security issue regarding Flask SECRET_KEY #3651

[AIRFLOW-2809] Fix security issue regarding Flask SECRET_KEY #3651

Conversation

XD-DENG commented Jul 27, 2018 • edited Loading

JIRA

Description

Background

Issue

My Proposal

Tests

Commits

Documentation

Code Quality

Fokko commented Jul 27, 2018

XD-DENG commented Jul 27, 2018

codecov-io commented Jul 28, 2018 • edited Loading

Codecov Report

kaxil commented Jul 28, 2018

Fokko left a comment

Choose a reason for hiding this comment

XD-DENG commented Jul 27, 2018 •

edited

Loading

codecov-io commented Jul 28, 2018 •

edited

Loading