Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if the lower of provided values are sensitives in config endpoint #34712

Merged
merged 4 commits into from Oct 7, 2023

Conversation

hussein-awala
Copy link
Member

No description provided.

@hussein-awala hussein-awala added this to the Airflow 2.7.2 milestone Oct 2, 2023
@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Oct 2, 2023
@hussein-awala hussein-awala added the type:bug-fix Changelog: Bug Fixes label Oct 2, 2023
@uranusjr
Copy link
Member

uranusjr commented Oct 3, 2023

We don’t currently normalise entries in sensitive_config_values to lowercase. Should we do that to ensure things are compared correctly?

@@ -123,7 +123,7 @@ def get_value(section: str, option: str) -> Response:
"Config not found.", detail=f"The option [{section}/{option}] is not found in config."
)

if (section, option) in conf.sensitive_config_values:
if (section.lower(), option.lower()) in conf.sensitive_config_values:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, I think we should do what we do in the webserver

airflow/airflow/www/views.py

Lines 3840 to 3845 in f349fda

updater = configupdater.ConfigUpdater()
updater.read(AIRFLOW_CONFIG)
for sect, key in conf.sensitive_config_values:
if updater.has_option(sect, key):
updater[sect][key].value = "< hidden >"
config = str(updater)
.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot fix the problem, because it checks if the value exists in the configuration as it is. But since we don't read any configuration from the user in this view, it's safe because all the configuration in both side (sensitive dict and configuration dict) are in low format.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @hussein-awala . I think the proposed way is right. The "www/views.py" code displays all the sections/options from the config and then then from sensitive_config_values. What we are preventing here is that differently cased section/option is passed by the user, so the source of section/key are not the config itself, but external to it. We can safely assume that the config has the right sections/keys (that's why the code in views.py works). But we cannot make the same assumption about the values passed by the user so we have to lowercase them.

@ephraimbuddy ephraimbuddy merged commit f044589 into apache:main Oct 7, 2023
43 checks passed
ephraimbuddy pushed a commit that referenced this pull request Oct 7, 2023
…nt (#34712)

* Check if the lower of provided values are sensitives in config endpoint

* update unit test

* ensure that all values in sensitive dict are in lower characters

(cherry picked from commit f044589)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants