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

Mask Password in Log table when using the CLI #11468

Merged
merged 4 commits into from
Oct 12, 2020

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Oct 12, 2020

Before:

image

After:

image


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@kaxil kaxil requested a review from ashb October 12, 2020 13:24
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

It's also possible to do --password=foobar (but not -p=foobar) which this PR won't catch.

@paolaperaza paolaperaza added this to the Airflow 1.10.13 milestone Oct 12, 2020
@kaxil
Copy link
Member Author

kaxil commented Oct 12, 2020

It's also possible to do --password=foobar (but not -p=foobar) which this PR won't catch.

Fixed in 40b1c04

I tested with -p=foobar and I was able to create the user with it too

Example:

root@6b30e511a6ac:/opt/airflow# airflow users  create --role Admin --username admin5 --email admin333 --firstname admin --lastname admin -p=admin

image

So I have handled that case too and added test for it

@ashb
Copy link
Member

ashb commented Oct 12, 2020

Should we add --conn-password to this list?

And for bonus points the password part of the URI style?

@kaxil
Copy link
Member Author

kaxil commented Oct 12, 2020

Should we add --conn-password to this list?
And for bonus points the password part of the URI style?

Added --conn-password, will address URI style later in a separate PR as we can do some cleanup of other parts where we mask Password in Conn URI too

technically this is a password of =test I think, (or it's not actually accepted by argparse) and it would have to be given as -p test.

But I think as an edge case this is okay if we don't worry about it

Looks like it works fine though

In [1]: from airflow.cli.cli_parser import get_parser
In [2]: parser = get_parser()
In [8]: parser.parse_args(["users", "create","-e" ,"airflow" , "-p=10", "-r", "Admin", "-f", "as", "-l", "asd", "-u", "as"])
Out[8]: Namespace(email='airflow', firstname='as', func=<function lazy_load_command.<locals>.command at 0x7fd196603dd0>, lastname='asd', password='10', role='Admin', subcommand='create', use_random_password=False, username='as')

@ashb ashb changed the title Mask Password in Log table when creating users via CLI Mask Password in Log table when using the CLI Oct 12, 2020
@kaxil kaxil merged commit 4e32546 into apache:master Oct 12, 2020
@kaxil kaxil deleted the mask-password branch October 12, 2020 18:27
kaxil added a commit that referenced this pull request Nov 18, 2020
kaxil added a commit to astronomer/airflow that referenced this pull request Nov 24, 2020
(cherry picked from commit 4e32546)
(cherry picked from commit 3cd9e9f)
kaxil added a commit to astronomer/airflow that referenced this pull request Nov 24, 2020
(cherry picked from commit 4e32546)
(cherry picked from commit 3cd9e9f)
(cherry picked from commit a092010)
(cherry picked from commit 9a901d198be9b6577bdee16eb86a5abb08a35798)
kaxil added a commit to astronomer/airflow that referenced this pull request Nov 27, 2020
(cherry picked from commit 4e32546)
(cherry picked from commit 3cd9e9f)
kaxil added a commit to astronomer/airflow that referenced this pull request Dec 1, 2020
(cherry picked from commit 4e32546)
(cherry picked from commit 3cd9e9f)
(cherry picked from commit a092010)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants