astro-sdk icon indicating copy to clipboard operation
astro-sdk copied to clipboard

Support postgres without a password

Open dimberman opened this issue 3 years ago • 3 comments
trafficstars

The current TempPostgresHook returns an error if a user creates a postgres instance because of string parsing

File "/usr/local/lib/python3.9/site-packages/airflow/hooks/dbapi.py", line 118, in get_sqlalchemy_engine
    return create_engine(self.get_uri(), **engine_kwargs)
  File "/usr/local/lib/python3.9/site-packages/astro/sql/operators/temp_hooks.py", line 51, in get_uri
    login = f"{quote_plus(conn.login)}:{quote_plus(conn.password)}@"
  File "/usr/local/lib/python3.9/urllib/parse.py", line 887, in quote_plus
    string = quote(string, safe + space, encoding, errors)
  File "/usr/local/lib/python3.9/urllib/parse.py", line 871, in quote
    return quote_from_bytes(string, safe)
  File "/usr/local/lib/python3.9/urllib/parse.py", line 896, in quote_from_bytes
    raise TypeError("quote_from_bytes() expected bytes")
TypeError: quote_from_bytes() expected bytes

We should check whether the PG has a password or not and handle appropriately

dimberman avatar Jan 19 '22 05:01 dimberman

Let's be secure out of the box and let's NOT support connecting without a password, but make it a nice error message saying you need to create a secure connection.

:-)

bolkedebruin avatar Jan 20 '22 10:01 bolkedebruin

@bolkedebruin we've had users complain because the default local postgres does not have passwords. cc: @ashb

dimberman avatar Jan 26 '22 22:01 dimberman

I understand that, but then we need to educate our users to have a safe setup. Postgres sets up a password less 'postgres' user which is basically a root account. Postgres documentation advises to set a password for this user. So users go against the recommended settings (maybe knowingless), want to use postgres in a insecure setup (thou shall not use the postgres account for ordinary activities) and want us to accomodate that?

I suggest only supporting this, if really really required, in Airflow debug mode or with an envionment variable "AIRFLOW_RUN_INSECURE" or something along those lines (DEBUG, LOCAL_DEVELOPMENT). One exception could be to allow this to happen through unix domain sockets as that does allow for authentication (the other end is able to get the effective user id), but we might consider that going down the rabbit hole.

bolkedebruin avatar Jan 27 '22 10:01 bolkedebruin