-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Sensitive variables don't get masked when rendered with airflow tasks test #17476
Comments
I can taking a look into this. Will try to reproduce. |
I repeated above instructions and was able reproduce. So it looks like the airflow/util/log/secrets_masker.py:should_hide_value_for_key() is where the check for hiding a sensitive variable happens. Looking into the workflow of running DAGs from cli and if it hits this function/class |
So from [1]: "The automatic masking is triggered by Connection or Variable access. This means that if you pass a sensitive value via XCom or any other side-channel it will not be masked when printed in the downstream task." and going into task-command::task_test it looks like logs are being propagated so they are no longer masked. Seems to be WAI |
Hello! |
@artful88533 -> suggestion - maybe you or @ShakaibKhan can take a look and provide fix for that? If you care about it, this is the most "certain" way to make it land in 2.2.3 - way more certain than pinging here - and becoming one of the > 1800 contributors to Airflow is a great way to pay back for the free software you use. IMHO it is not crirtical (when you have access to the CLI you already can read all the secretes in whatever way you want) but if you need to get it for your CI process, maybe that's a good incentive for you to fix it ? It seems that this is just a question of adding secret masker as filter when the CLI commands are run. |
Actually, I think this might be more critical than it looks at first glance @potiuk. By running the |
Why would you want to run "airflow tasks test" in DAG? Is this a valid casethat is likely? Maybe I am not understnding something, but I am not sure I see the case when it could be used in "production" in a valid scenario? Just to give a bit of context - there are many ways you could print the unmasked values for connections, variables, even secrets. For example you could easily launch a subprocess calling "python -c print(Connection.get('conn_id'))" or just running "airlfow connection list" a as a command to print unmasked paswords. The way how masking is done currently will not prevent this if the Connection is not used in the task before. So "Masking" is not "total prevention" of showing the secret values, it just prevents from accidental printing of those in the "regular use cases". There are many ways how DAG writer could print those and bypass secrets masker deliberately. So my question is - how likely and "normal" it is to run 'airlfow test" inside the "execute" method of a task. I think very unlikely. And BTW in the future when we implement DB-less mode and maybe even (as a follow up) we will further harden Airlfow to not be able to reach out to read Airflow DB at all, this might be more "hardened" but as of currently we have no mechanism to prevent the DAG writers to print any secret they want to the task log. That's simply impossible. |
That's very true. Thanks for explaining your reasoning about this. |
…he#17476) Add a context manager to secrets_masker inside of which stdout is captured, redacted according to the filters on 'airflow.task' logger and then spit back into stdout. Filters stdout that doesn't go through logger according to airflow.task logger's filters.
Apache Airflow version: 2.1.2
Kubernetes version (if you are using kubernetes) (use
kubectl version
): NoEnvironment:
uname -a
): -What happened:
With the following code:
By executing the command
airflow tasks test my_dag extract 2021-01-01
The value of the variable my_dag_partner_secret gets rendered in the logs whereas it shouldn't
What you expected to happen:
The value should be masked like on the UI or in the logs
How to reproduce it:
DAG given above
Anything else we need to know:
Nop
The text was updated successfully, but these errors were encountered: