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 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:
[email protected]


Reply via email to