[GitHub] [airflow] potiuk edited a comment on issue #16007: Masking passwords with empty connection passwords make some logs unreadable in 2.1.0

[GitBox <gi...@apache.org>]

potiuk edited a comment on issue #16007:
URL: https://github.com/apache/airflow/issues/16007#issuecomment-846603509


   > I discovered that if fernet key is not set, you cannot view connections on the UI when load_example dags is True. Maybe it’s related to this issue?
   > 
   > Steps to reproduce:
   > 
   > 1. create virtualenv & activate it
   > 2. Run python setup.py develop & airflow standalone.
   > 3. Visit connections on the UI and you’ll get an error
   
   I think it is not that - the user in Slack discussion mentioned that it was only in some "Task" logs, which suggests that this was when particular connection was used. I am not 100% sure about the path to reproduce it indeed however, but empty "password" looks like a very probable (and quite valid) case.
   
   > I thought I had an `if conn.login` guard. I guess not.
   
   I had a quick look at the PR and have not found any guard - that's why I raised it :). But think it's `if conn.password` case more than `conn.login`. I can imagine case where we have login and no password.
   
   > We could have a minimum length, but I'm generally adverse to adding extra config options without a strong need as each one we add makes understanding airflow harder
   
   I thought more about hard-coding minimum length of password actually. And I think '3' might be a good one especially that it's replaced by '***' :D and we do not want to make logs any longer. But on a more serious note - I can imagine cases where people use some simple connections with very short passwords to test them ( `test`, `admin`, `123` or even `a`, `p` are often used for that purpose). In this case masking logs replacing all `a`s with `***` might lead to a very confusing output. That's why I thought having `len<3` or `len<5` hard-coded might be a good idea. 
   
   Especially that if those "Very short" passwords actually ARE causing a potential security threat mentioned in the PR already. 
   
   Imagine that someone, by mistakes sets 'a' password on a connection. Suddenly all "a`s" get replaced with `***` and it's going to be super-easy to figure out what the password is. That's an interesting vector of attack (very difficult to exploit, more by accident than deliberate action, but still possible). This is less of a problem for longer passwords - figuring out longer passwords replaced in logs might be more difficult. But maybe we should also not mask the password if it is a common "word" in a dictionary? That would decrease the probability of it appearing in the logs and being discovered by accident.
   
   Those are quite low-probability scenarios, so I am not very strong on having them fixed, but if it is an easy thing to do (seems it is) I think we should do it - ideally both: len < 3 + dictionary check.
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org