[AIRFLOW-6352] security - ui - add login timeout #6912

tooptoop4 · 2019-12-26T22:17:46Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-6352] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-6352
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-6352], code changes always need a Jira issue.
- In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

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

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 (not including Jira issue reference)
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.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

potiuk · 2019-12-27T08:01:33Z

Same here @tooptoop4 -> I love the small fixes you provide but static checks are failing for them (and could be prevented easily by pre-commit).

tooptoop4 · 2019-12-27T09:27:35Z

travis unrelated?

potiuk · 2019-12-27T10:10:24Z

This change introduces a new behaviour (logout after 60 minutes). While it is good for security reasons (obviously) the UI of Airflow has a little bit different use patterns/characteristics than typical user-facing apps.

It's mostly internal use, with very small number of users, it's already behind a VPN and I guess often witht some kind of client certificates being verified by web seervers. I can imagine in those cases prolonged session persistency might be important feature for users using Apache Airflow. In many cases UI of Airflow can be used in a fashion similar to "operational dashboard" rather than the typical case of "login/do something/logout".

Since we have no auto-refresh yet, using Airflow as dashboard with 60 minutes logout session would not be super user-friendly.

I'd love to hear what others think about it, but I believe at the very least UPDATING.md should mention that new behaviour if we agree this is a good thing to introduce 60 minutes (or another period) timeout.

codecov-io · 2019-12-27T10:26:03Z

Codecov Report

Merging #6912 into master will decrease coverage by 0.01%.
The diff coverage is 60%.

@@            Coverage Diff            @@
##           master   #6912      +/-   ##
=========================================
- Coverage   84.72%   84.7%   -0.02%     
=========================================
  Files         679     679              
  Lines       38505   38528      +23     
=========================================
+ Hits        32623   32635      +12     
- Misses       5882    5893      +11

Impacted Files	Coverage Δ
airflow/www/app.py	`94.57% <60%> (-2.91%)`	⬇️
airflow/contrib/hooks/cassandra_hook.py	`0% <0%> (-100%)`	⬇️
airflow/models/__init__.py	`91.3% <0%> (-8.7%)`	⬇️
airflow/contrib/hooks/spark_submit_hook.py	`82% <0%> (-0.5%)`	⬇️
airflow/jobs/backfill_job.py	`91.59% <0%> (-0.29%)`	⬇️
airflow/www/views.py	`76.08% <0%> (ø)`	⬆️
airflow/executors/base_executor.py	`96.05% <0%> (ø)`	⬆️
airflow/models/taskinstance.py	`94.91% <0%> (+0.43%)`	⬆️

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 6fffa5b...b86c7ac. Read the comment docs.

RosterIn · 2019-12-27T11:40:36Z

I think it can become handy.
@potiuk Security isn't always about the "outside" world. It's also for inner one. Companies may use more than one instance of airflow. For example I have airflow install unique for HR data - this is extremely sensitive. I would welcome such feature for this instance but I wouldn't welcome it for my other airflow instances.

So I think this shouldn't be enforced for everyone.
This can be optional feature that users can set by themselves from airflow.cfg
I also recommend not to hard code 60 min. Let users decide what they want. It can be in the same setting:

force_log_out_after = 60 if set this will timeout the session and also use the number to determine the time

potiuk · 2019-12-27T11:59:20Z

Agree @RosterIn with security. Internal security should not be neglected.

It's just that security is never an on/off switch and "let's apply all the possible security practices" is not always best choice.

There are often multiple layers of security in different places so this logout might not be needed (for example when you have individual client certificates individually issued to your users and verified in proxy standing in front of Airflow.). This is often case in corporate environments (such as Airflow often is installed at) but almost unheard of in public-facing websites.

There is always a delicate balance "convenience vs. security" and sometimes enforcing some "best practices" for security with some inconveniences built in gives the opposite result. People tend to bypass security inconveniences by introducing even more insecure workarounds. For example in this case, I can very easily imagine a data engineer wanting a dashboard installing "auto-refresh" browser plugin to refresh the airflow dashboard every 20 minutes. Been there, done that. Such plugins are often vectors of attack on their own.

So yeah I agree with force_log_out_after conf value. I think having a separate conf entry for that is much better choice and gives freedom to admins to set their policy rules as they find best for their users.

potiuk

So to sum-up @tooptoop4 -> this is a good idea but we should make it configurable :).

tooptoop4 · 2019-12-27T17:51:23Z

@potiuk make sense, I've now guarded with config

potiuk

Great!

(cherry picked from commit 0721d62)

mik-laj · 2019-12-30T21:36:32Z

airflow/config_templates/default_airflow.cfg

@@ -370,10 +370,13 @@ default_wrap = False
 # on webserver startup
 update_fab_perms = True

+# Minutes of non-activity before logged out from UI
+# 0 means never get forcibly logged out


Can you add some documentation about this option? If there is no information about this feature in the documentation, very few people will be able to use it.

@mik-laj - > where do you think the documentation should be added ? Maybe you can provide some pointers? Most of the config options are documented in comments of the default_airlfow.cfg. Any other specific place you think this documentation should be added (and where all the previous options are documented?)

Related discussion: #6952 (comment)

I think, we can add short description in 'security.rst' file or a new file in howto directory.

(cherry picked from commit 0721d62)

tooptoop4 · 2020-03-06T16:26:53Z

@mik-laj any idea how this works? these lines are missing from app.py in 1.10.9
import datetime
import flask
import flask_login

NBardelot · 2020-03-12T16:40:29Z

@tooptoop4 sadly it doesn't work as-is (see https://issues.apache.org/jira/browse/AIRFLOW-6865).

dephusluke · 2020-03-16T16:52:36Z

I'm trying also to understand the origin of this bug. The imports appear in the file so how could they be missed?

I see also there is discussion about it in the commit 0721d62

@tooptoop4 have you figured it out?

renaldrozario · 2020-10-14T13:03:06Z

This issue is still open with version 1.10.12.. can someone advise when this will be fixed?

mik-laj · 2020-10-14T20:09:21Z

@renaldrozario We are now focusing on the development of Airflow 2.0 and we are limiting the new features that are released in Airflow 1.10. Airflow 2.0 will be released at the end of the year.

mik-laj · 2020-10-14T20:09:37Z

Is this information helpful to you?

tooptoop4 added 2 commits December 26, 2019 22:16

AIRFLOW-6352

bc0513d

isort

e139d63

potiuk requested changes Dec 27, 2019

View reviewed changes

guard auto logout with config

667d575

tooptoop4 requested a review from potiuk December 27, 2019 17:50

travis refire

b86c7ac

turbaszek approved these changes Dec 28, 2019

View reviewed changes

potiuk approved these changes Dec 28, 2019

View reviewed changes

potiuk merged commit 0721d62 into apache:master Dec 28, 2019

potiuk pushed a commit that referenced this pull request Dec 30, 2019

[AIRFLOW-6352] security - ui - add login timeout (#6912)

d12d400

(cherry picked from commit 0721d62)

mik-laj reviewed Dec 30, 2019

View reviewed changes

potiuk pushed a commit that referenced this pull request Jan 21, 2020

[AIRFLOW-6352] security - ui - add login timeout (#6912)

9f84763

(cherry picked from commit 0721d62)

galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020

[AIRFLOW-6352] security - ui - add login timeout (apache#6912)

da8f3ff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-6352] security - ui - add login timeout #6912

[AIRFLOW-6352] security - ui - add login timeout #6912

tooptoop4 commented Dec 26, 2019 •

edited

Loading

potiuk commented Dec 27, 2019

tooptoop4 commented Dec 27, 2019

potiuk commented Dec 27, 2019 •

edited

Loading

codecov-io commented Dec 27, 2019 •

edited

Loading

RosterIn commented Dec 27, 2019

potiuk commented Dec 27, 2019 •

edited

Loading

potiuk left a comment

tooptoop4 commented Dec 27, 2019

potiuk left a comment

mik-laj Dec 30, 2019

potiuk Dec 30, 2019

mik-laj Dec 30, 2019

tooptoop4 commented Mar 6, 2020

NBardelot commented Mar 12, 2020

dephusluke commented Mar 16, 2020

renaldrozario commented Oct 14, 2020

mik-laj commented Oct 14, 2020

mik-laj commented Oct 14, 2020

[AIRFLOW-6352] security - ui - add login timeout #6912

[AIRFLOW-6352] security - ui - add login timeout #6912

Conversation

tooptoop4 commented Dec 26, 2019 • edited Loading

Jira

Description

Tests

Commits

Documentation

potiuk commented Dec 27, 2019

tooptoop4 commented Dec 27, 2019

potiuk commented Dec 27, 2019 • edited Loading

codecov-io commented Dec 27, 2019 • edited Loading

Codecov Report

RosterIn commented Dec 27, 2019

potiuk commented Dec 27, 2019 • edited Loading

potiuk left a comment

Choose a reason for hiding this comment

tooptoop4 commented Dec 27, 2019

potiuk left a comment

Choose a reason for hiding this comment

mik-laj Dec 30, 2019

Choose a reason for hiding this comment

potiuk Dec 30, 2019

Choose a reason for hiding this comment

mik-laj Dec 30, 2019

Choose a reason for hiding this comment

tooptoop4 commented Mar 6, 2020

NBardelot commented Mar 12, 2020

dephusluke commented Mar 16, 2020

renaldrozario commented Oct 14, 2020

mik-laj commented Oct 14, 2020

mik-laj commented Oct 14, 2020

tooptoop4 commented Dec 26, 2019 •

edited

Loading

potiuk commented Dec 27, 2019 •

edited

Loading

codecov-io commented Dec 27, 2019 •

edited

Loading

potiuk commented Dec 27, 2019 •

edited

Loading