You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/07/27 18:17:43 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request, #25346: Add option to mask sensitive data in UI configuration page
ephraimbuddy opened a new pull request, #25346:
URL: https://github.com/apache/airflow/pull/25346
This PR adds an option to mask sensitive data in the UI configuration page, making it possible to view the page with redated data
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r934343830
##########
tests/www/views/test_views_configuration.py:
##########
@@ -0,0 +1,64 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+
+from airflow.configuration import SENSITIVE_CONFIG_VALUES, conf
+from tests.test_utils.config import conf_vars
+from tests.test_utils.www import check_content_in_response, check_content_not_in_response
+
+
+@conf_vars({("webserver", "expose_config"): 'False'})
+def test_user_cant_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ check_content_in_response(
+ "Your Airflow administrator chose not to expose the configuration, "
+ "most likely for security reasons.",
+ resp,
+ )
+
+
+@conf_vars({("webserver", "expose_config"): 'True'})
+def test_user_can_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value:
+ continue
+ check_content_in_response(value, resp)
+
+
+@conf_vars({("webserver", "expose_config"): 'non-sensitive-only'})
+def test_configuration_redacted(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value or value == 'airflow':
+ continue
+ if value.startswith('db+postgresql'): # this is in configuration comment
+ continue
Review Comment:
Yes. It's needed because an example of postgresql connection string was used in the comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r934493847
##########
tests/www/views/test_views_configuration.py:
##########
@@ -0,0 +1,64 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+
+from airflow.configuration import SENSITIVE_CONFIG_VALUES, conf
+from tests.test_utils.config import conf_vars
+from tests.test_utils.www import check_content_in_response, check_content_not_in_response
+
+
+@conf_vars({("webserver", "expose_config"): 'False'})
+def test_user_cant_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ check_content_in_response(
+ "Your Airflow administrator chose not to expose the configuration, "
+ "most likely for security reasons.",
+ resp,
+ )
+
+
+@conf_vars({("webserver", "expose_config"): 'True'})
+def test_user_can_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value:
+ continue
+ check_content_in_response(value, resp)
+
+
+@conf_vars({("webserver", "expose_config"): 'non-sensitive-only'})
+def test_configuration_redacted(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value or value == 'airflow':
+ continue
+ if value.startswith('db+postgresql'): # this is in configuration comment
+ continue
Review Comment:
I added another test instead of removing this one because we also hide those in source files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r932160596
##########
airflow/www/views.py:
##########
@@ -3724,8 +3724,38 @@ def conf(self):
raw = request.args.get('raw') == "true"
title = "Airflow Configuration"
subtitle = AIRFLOW_CONFIG
+
+ expose_config = conf.get('webserver', 'expose_config')
+
# Don't show config when expose_config variable is False in airflow config
- if conf.getboolean("webserver", "expose_config"):
+ # Don't show sensitive config values if expose_config variable is 'non-sensitive-only'
+ # in airflow config
+ if expose_config.lower() == 'non-sensitive-only':
+ from airflow.configuration import SENSITIVE_CONFIG_VALUES
+
+ with open(AIRFLOW_CONFIG) as file:
+ config = file.readlines()
+ for line in config:
+ for _, key in SENSITIVE_CONFIG_VALUES:
Review Comment:
Added a comment on this regard
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r934397951
##########
tests/www/views/test_views_configuration.py:
##########
@@ -0,0 +1,64 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+
+from airflow.configuration import SENSITIVE_CONFIG_VALUES, conf
+from tests.test_utils.config import conf_vars
+from tests.test_utils.www import check_content_in_response, check_content_not_in_response
+
+
+@conf_vars({("webserver", "expose_config"): 'False'})
+def test_user_cant_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ check_content_in_response(
+ "Your Airflow administrator chose not to expose the configuration, "
+ "most likely for security reasons.",
+ resp,
+ )
+
+
+@conf_vars({("webserver", "expose_config"): 'True'})
+def test_user_can_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value:
+ continue
+ check_content_in_response(value, resp)
+
+
+@conf_vars({("webserver", "expose_config"): 'non-sensitive-only'})
+def test_configuration_redacted(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value or value == 'airflow':
+ continue
+ if value.startswith('db+postgresql'): # this is in configuration comment
+ continue
Review Comment:
Then we should make the test better.
Instead of:
```python
check_content_not_in_response(value, resp)
```
something like this should test that the value isn't showing in the running config section
```python
check_content_not_in_response('<td>' + html.escape(value), resp)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r932795885
##########
tests/www/views/test_views_configuration.py:
##########
@@ -0,0 +1,65 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.configuration import SENSITIVE_CONFIG_VALUES, conf
+from tests.test_utils.config import conf_vars
+from tests.test_utils.www import check_content_in_response, check_content_not_in_response
+
+
+@conf_vars({("webserver", "expose_config"): 'False'})
+def test_user_cant_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ check_content_in_response(
+ "Your Airflow administrator chose not to expose the configuration, "
+ "most likely for security reasons.",
+ resp,
+ )
+
+
+@conf_vars({("webserver", "expose_config"): 'True'})
+def test_user_can_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value or 'mssql' in value:
+ # TODO: investigate why mssql db can't be found
Review Comment:
Resolved
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r934342199
##########
tests/www/views/test_views_configuration.py:
##########
@@ -0,0 +1,64 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+
+from airflow.configuration import SENSITIVE_CONFIG_VALUES, conf
+from tests.test_utils.config import conf_vars
+from tests.test_utils.www import check_content_in_response, check_content_not_in_response
+
+
+@conf_vars({("webserver", "expose_config"): 'False'})
+def test_user_cant_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ check_content_in_response(
+ "Your Airflow administrator chose not to expose the configuration, "
+ "most likely for security reasons.",
+ resp,
+ )
+
+
+@conf_vars({("webserver", "expose_config"): 'True'})
+def test_user_can_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value:
+ continue
+ check_content_in_response(value, resp)
+
+
+@conf_vars({("webserver", "expose_config"): 'non-sensitive-only'})
+def test_configuration_redacted(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value or value == 'airflow':
+ continue
+ if value.startswith('db+postgresql'): # this is in configuration comment
+ continue
Review Comment:
I don't think this is needed is it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r932542462
##########
airflow/www/views.py:
##########
@@ -3727,8 +3727,41 @@ def conf(self):
raw = request.args.get('raw') == "true"
title = "Airflow Configuration"
subtitle = AIRFLOW_CONFIG
+
+ expose_config = conf.get('webserver', 'expose_config')
+
# Don't show config when expose_config variable is False in airflow config
- if conf.getboolean("webserver", "expose_config"):
+ # Don't show sensitive config values if expose_config variable is 'non-sensitive-only'
+ # in airflow config
+ if expose_config.lower() == 'non-sensitive-only':
+ from airflow.configuration import SENSITIVE_CONFIG_VALUES
+
+ with open(AIRFLOW_CONFIG) as file:
+ config = file.readlines()
+ for line in config:
+ for _, key in SENSITIVE_CONFIG_VALUES:
+ # this masks the keys wherever it's found not
+ # minding the section
+ if key in line and not line.startswith('#'):
+ config[config.index(line)] = key + ' = ***\n'
+ break
+
+ config = ''.join(config)
+
+ running_conf = conf.as_dict(True, True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ running_conf_value = running_conf[section].get(key, None)
+ if running_conf_value:
+ new = ('***', running_conf_value[1])
+ running_conf[section][key] = new
Review Comment:
Yeah, found out that the `display_sensitive` option was made to hide sensitive fields but was not hidden those that come from airflow.cfg in the running config. Fixed now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r932775953
##########
tests/www/views/test_views_configuration.py:
##########
@@ -0,0 +1,65 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.configuration import SENSITIVE_CONFIG_VALUES, conf
+from tests.test_utils.config import conf_vars
+from tests.test_utils.www import check_content_in_response, check_content_not_in_response
+
+
+@conf_vars({("webserver", "expose_config"): 'False'})
+def test_user_cant_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ check_content_in_response(
+ "Your Airflow administrator chose not to expose the configuration, "
+ "most likely for security reasons.",
+ resp,
+ )
+
+
+@conf_vars({("webserver", "expose_config"): 'True'})
+def test_user_can_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value or 'mssql' in value:
+ # TODO: investigate why mssql db can't be found
Review Comment:
It was because connection string was quoted. So the UI is not showing the actual value set in the env. I feel this is a bug. I let the test pass by quoting the value for now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r933762790
##########
tests/www/views/test_views_configuration.py:
##########
@@ -0,0 +1,65 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.configuration import SENSITIVE_CONFIG_VALUES, conf
+from tests.test_utils.config import conf_vars
+from tests.test_utils.www import check_content_in_response, check_content_not_in_response
+
+
+@conf_vars({("webserver", "expose_config"): 'False'})
+def test_user_cant_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ check_content_in_response(
+ "Your Airflow administrator chose not to expose the configuration, "
+ "most likely for security reasons.",
+ resp,
+ )
+
+
+@conf_vars({("webserver", "expose_config"): 'True'})
+def test_user_can_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value or 'mssql' in value:
+ # TODO: investigate why mssql db can't be found
Review Comment:
It seems like there's some masking going on when we get the config value because the failed test shows some asterisks:
```python
> check_content_in_response(value, resp)
tests/www/views/test_views_configuration.py:41:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
text = '***mssql:1433/airflow?driver=ODBC+Driver+18+for+SQL+Server&Encrypt=No'
resp = <WrapperTestResponse 263724 bytes [200 OK]>, resp_code = 200
def check_content_in_response(text, resp, resp_code=200):
resp_html = resp.data.decode('utf-8')
assert resp_code == resp.status_code
if isinstance(text, list):
for line in text:
assert line in resp_html
else:
> assert text in resp_html
E AssertionError
tests/test_utils/www.py:37: AssertionError
```
I'm not sure why the value is starting with asterisks:
`'***mssql:1433/airflow?driver=ODBC+Driver+18+for+SQL+Server&Encrypt=No'`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] norm commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
norm commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r931922836
##########
airflow/www/views.py:
##########
@@ -3724,8 +3724,38 @@ def conf(self):
raw = request.args.get('raw') == "true"
title = "Airflow Configuration"
subtitle = AIRFLOW_CONFIG
+
+ expose_config = conf.get('webserver', 'expose_config')
+
# Don't show config when expose_config variable is False in airflow config
- if conf.getboolean("webserver", "expose_config"):
+ # Don't show sensitive config values if expose_config variable is 'non-sensitive-only'
+ # in airflow config
+ if expose_config.lower() == 'non-sensitive-only':
+ from airflow.configuration import SENSITIVE_CONFIG_VALUES
+
+ with open(AIRFLOW_CONFIG) as file:
+ config = file.readlines()
+ for line in config:
+ for _, key in SENSITIVE_CONFIG_VALUES:
Review Comment:
This is making an assumption that the name is unique amongst all sections. I don't think it's a problem (better to redact too much than not enough) but there is a possibility of this catching other settings.
As I said, not a problem per se, but I would add a comment to that effect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r932422157
##########
airflow/www/views.py:
##########
@@ -3727,8 +3727,41 @@ def conf(self):
raw = request.args.get('raw') == "true"
title = "Airflow Configuration"
subtitle = AIRFLOW_CONFIG
+
+ expose_config = conf.get('webserver', 'expose_config')
+
# Don't show config when expose_config variable is False in airflow config
- if conf.getboolean("webserver", "expose_config"):
+ # Don't show sensitive config values if expose_config variable is 'non-sensitive-only'
+ # in airflow config
+ if expose_config.lower() == 'non-sensitive-only':
+ from airflow.configuration import SENSITIVE_CONFIG_VALUES
+
+ with open(AIRFLOW_CONFIG) as file:
+ config = file.readlines()
+ for line in config:
+ for _, key in SENSITIVE_CONFIG_VALUES:
+ # this masks the keys wherever it's found not
+ # minding the section
+ if key in line and not line.startswith('#'):
+ config[config.index(line)] = key + ' = ***\n'
+ break
+
+ config = ''.join(config)
+
+ running_conf = conf.as_dict(True, True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ running_conf_value = running_conf[section].get(key, None)
+ if running_conf_value:
+ new = ('***', running_conf_value[1])
+ running_conf[section][key] = new
Review Comment:
I wonder if this behaviour should live as an option inside conf as_dict
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r933883223
##########
tests/www/views/test_views_configuration.py:
##########
@@ -0,0 +1,65 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.configuration import SENSITIVE_CONFIG_VALUES, conf
+from tests.test_utils.config import conf_vars
+from tests.test_utils.www import check_content_in_response, check_content_not_in_response
+
+
+@conf_vars({("webserver", "expose_config"): 'False'})
+def test_user_cant_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ check_content_in_response(
+ "Your Airflow administrator chose not to expose the configuration, "
+ "most likely for security reasons.",
+ resp,
+ )
+
+
+@conf_vars({("webserver", "expose_config"): 'True'})
+def test_user_can_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value or 'mssql' in value:
+ # TODO: investigate why mssql db can't be found
Review Comment:
Ok. I think this is because of log masking
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] norm commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
norm commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r931931404
##########
airflow/www/views.py:
##########
@@ -3724,8 +3724,38 @@ def conf(self):
raw = request.args.get('raw') == "true"
title = "Airflow Configuration"
subtitle = AIRFLOW_CONFIG
+
+ expose_config = conf.get('webserver', 'expose_config')
+
# Don't show config when expose_config variable is False in airflow config
- if conf.getboolean("webserver", "expose_config"):
+ # Don't show sensitive config values if expose_config variable is 'non-sensitive-only'
+ # in airflow config
+ if expose_config.lower() == 'non-sensitive-only':
+ from airflow.configuration import SENSITIVE_CONFIG_VALUES
+
+ with open(AIRFLOW_CONFIG) as file:
+ config = file.readlines()
+ for line in config:
+ for _, key in SENSITIVE_CONFIG_VALUES:
+ if key in line and not line.startswith('#'):
Review Comment:
I think configparser allows lines to be indented, which means when it *is* commented out but *also* indented, it would be marked as active and redacted rather than left as a comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r933066759
##########
airflow/www/views.py:
##########
@@ -3724,8 +3724,38 @@ def conf(self):
raw = request.args.get('raw') == "true"
title = "Airflow Configuration"
subtitle = AIRFLOW_CONFIG
+
+ expose_config = conf.get('webserver', 'expose_config')
+
# Don't show config when expose_config variable is False in airflow config
- if conf.getboolean("webserver", "expose_config"):
+ # Don't show sensitive config values if expose_config variable is 'non-sensitive-only'
+ # in airflow config
+ if expose_config.lower() == 'non-sensitive-only':
+ from airflow.configuration import SENSITIVE_CONFIG_VALUES
+
+ with open(AIRFLOW_CONFIG) as file:
+ config = file.readlines()
+ for line in config:
+ for _, key in SENSITIVE_CONFIG_VALUES:
+ if key in line and not line.startswith('#'):
Review Comment:
I'd suggest that rather than trying to parse the format ourselves we should use a module to handle this for us.
https://github.com/pyscaffold/configupdater is one that I've found that a) does the job, b) preserves comments. The code to use it is
```python
updater = configupdater.ConfigUpdater()
updater.read(AIRFLOW_CONFIG)
for sect, key in SENSITIVE_CONFIG_VALUES:
if updater.has_option(sect, key):
updater[sect][key].value = '< hidden >'
config = str(updater)
```
(This module also has no extra deps which is a nice bonus.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on PR #25346:
URL: https://github.com/apache/airflow/pull/25346#issuecomment-1199217647
> This PR doesn't work if you are using the `_CMD` option to give values. For example:
>
> ```
> AIRFLOW__DATABASE__SQL_ALCHEMY_CONN_CMD='echo postgresl:///airflow' airflow webserver
> ```
>
> will fail because the value is replaced with `< hidden >` _before_ it tries to run the command, meaning it tries to run `"<" "hidden" ">"` as a command!
I have rewritten the `display-sensitive` logic. This bug is in current main. Basically, if you set environment variable and also provide command line equivalent then `conf.as_dict(True, display_sensitive=False)` won't work because the `_include_envs` method of `AirflowConfigParser` hides sensitive envs and updates the `config_sources`. In the `_include_cmds` method, which is called after `_include_envs`, we just look up the section and key in the config_sources and we get `< hidden >` which is now run as command.
What I have done is to hide sensitive values at the end of all these updates of the `config_sources`. Another option could be to look up envs with `CMD` at the end in `_include_cmds` instead of getting the value from the `config_sources`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r933883223
##########
tests/www/views/test_views_configuration.py:
##########
@@ -0,0 +1,65 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.configuration import SENSITIVE_CONFIG_VALUES, conf
+from tests.test_utils.config import conf_vars
+from tests.test_utils.www import check_content_in_response, check_content_not_in_response
+
+
+@conf_vars({("webserver", "expose_config"): 'False'})
+def test_user_cant_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ check_content_in_response(
+ "Your Airflow administrator chose not to expose the configuration, "
+ "most likely for security reasons.",
+ resp,
+ )
+
+
+@conf_vars({("webserver", "expose_config"): 'True'})
+def test_user_can_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value or 'mssql' in value:
+ # TODO: investigate why mssql db can't be found
Review Comment:
Ok. I think this is because of log masking. The *** is just what was hidden by the failure log
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25346:
URL: https://github.com/apache/airflow/pull/25346
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r932159976
##########
airflow/www/views.py:
##########
@@ -3724,8 +3724,38 @@ def conf(self):
raw = request.args.get('raw') == "true"
title = "Airflow Configuration"
subtitle = AIRFLOW_CONFIG
+
+ expose_config = conf.get('webserver', 'expose_config')
+
# Don't show config when expose_config variable is False in airflow config
- if conf.getboolean("webserver", "expose_config"):
+ # Don't show sensitive config values if expose_config variable is 'non-sensitive-only'
+ # in airflow config
+ if expose_config.lower() == 'non-sensitive-only':
+ from airflow.configuration import SENSITIVE_CONFIG_VALUES
+
+ with open(AIRFLOW_CONFIG) as file:
+ config = file.readlines()
+ for line in config:
+ for _, key in SENSITIVE_CONFIG_VALUES:
+ if key in line and not line.startswith('#'):
Review Comment:
Added a test to ensure it's handled
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r932419145
##########
tests/www/views/test_views_configuration.py:
##########
@@ -0,0 +1,65 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.configuration import SENSITIVE_CONFIG_VALUES, conf
+from tests.test_utils.config import conf_vars
+from tests.test_utils.www import check_content_in_response, check_content_not_in_response
+
+
+@conf_vars({("webserver", "expose_config"): 'False'})
+def test_user_cant_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ check_content_in_response(
+ "Your Airflow administrator chose not to expose the configuration, "
+ "most likely for security reasons.",
+ resp,
+ )
+
+
+@conf_vars({("webserver", "expose_config"): 'True'})
+def test_user_can_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value or 'mssql' in value:
+ # TODO: investigate why mssql db can't be found
Review Comment:
This seems fishy. What is going on here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #25346: Add option to mask sensitive data in UI configuration page
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #25346:
URL: https://github.com/apache/airflow/pull/25346#discussion_r933062828
##########
tests/www/views/test_views_configuration.py:
##########
@@ -0,0 +1,65 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from airflow.configuration import SENSITIVE_CONFIG_VALUES, conf
+from tests.test_utils.config import conf_vars
+from tests.test_utils.www import check_content_in_response, check_content_not_in_response
+
+
+@conf_vars({("webserver", "expose_config"): 'False'})
+def test_user_cant_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ check_content_in_response(
+ "Your Airflow administrator chose not to expose the configuration, "
+ "most likely for security reasons.",
+ resp,
+ )
+
+
+@conf_vars({("webserver", "expose_config"): 'True'})
+def test_user_can_view_configuration(admin_client):
+ resp = admin_client.get('configuration', follow_redirects=True)
+ for section, key in SENSITIVE_CONFIG_VALUES:
+ value = conf.get(section, key, fallback='')
+ if not value or 'mssql' in value:
+ # TODO: investigate why mssql db can't be found
Review Comment:
Not resolved...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org