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

Add support for pgpass #666

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

surister
Copy link
Contributor

@surister surister commented Jul 27, 2024

We follow the spec in https://www.postgresql.org/docs/current/libpq-pgpass.html,

Order of passfile path is:

  1. passfile path specified in connection string, e.g. postgresql://localhost:5432?passfile=/home/someuser/secret/.pgpass
  2. default path ~/.pgpass in *nix, %APPDATA%\postgresql\pgpass.conf in windows.
  • pgpass file needs to have safe permissions 0600 or we raise an exception.
  • pgpass needs to follow the spec format, host:port:db_name:user_name:password, so
    *:*:*:*:password is valid, just password is not.

pgpassfile has precedence over the given connection string.

Fixes: #567

Co-authored-by: Aimilios Tsouvelekakis <[email protected]>
@surister
Copy link
Contributor Author

@wangxiaoying Adding a bunch of new code to support specific postgres functionality into the __init__ feels bloat, how about we add new code in a new file? maybe something like postgres/pgpass.py?

__init__.py
postgres/pgpass.py

@@ -247,6 +247,66 @@ def read_sql(
) -> pl.DataFrame: ...


def get_passfile_content(path: pathlib.Path) -> dict:
# host:port:db_name:user_name:password
with open(path) as f:
Copy link
Contributor

@aimtsou aimtsou Jul 28, 2024

Choose a reason for hiding this comment

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

How about we do something different? the content we need from pgpass is the password only the restof the elements are used for retrieving it. So:

            for line in f:
                contents = line.read().split(':')
                if len(contents) != 5:
                    raise Exception('Pgpass content should follow: host:port:db_name:user_name:password')
                else:
                    if ((contents['host'] == host or contents['host'] == '*') and
                        (contents['port'] == port or contents['port'] == '*') and
                        (contents['database'] == database or contents['database'] == '*') and
                        (contents['user'] == user or contents['user'] == '*')):
                        return contents['password']

So in this way we do also multiple lines and we return the first line matched.

def get_pgpass(conn) -> dict:
# check param or (PGPASS env or DEFAULT)
# DEFAULT = linux or windows
passfile = '%APPDATA%\postgresql\pgpass.conf' if sys.platform == 'windows' else os.path.expanduser(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of sys.platform maybe we can use os.name == 'nt'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it better?


passfile_path = pathlib.Path(passfile)

if sys.platform != 'windows':
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: os.name != 'nt'

# We rewrite the netloc from scratch here e.g.
# netloc = contents['username'] or o.username + ':' + contents['password'] or o.password + ...
parsed_conn = urllib.parse.urlparse(conn)
return str(parsed_conn._replace(**contents))
Copy link
Contributor

Choose a reason for hiding this comment

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

We can do the following:
database = parsed_conn.path[1:] or os.environ.get('PGDATABASE', os.getenv('USER'))
host = parsed_conn.hostname or os.environ.get('PGHOST', 'localhost')
port = parsed_conn.port or os.environ.get('PGPORT', '5432')
user = parsed_conn.username or os.environ.get('PGUSER', os.getenv('USER'))
password = parsed_conn.password

We parse 2 times the URI so maybe we need to rethink that maybe we call pgpass from inside reconstruct? That would allow something like that also:

query_params = parse_qs(parsed_con.query)

passfile = query_params.get('passfile', [None])[0] or os.environ.get('PGPASSFILE')
default_pgpass_path = os.path.expanduser('~/.pgpass') if os.name != 'nt' else os.path.join(os.getenv('APPDATA', ''), 'postgresql', 'pgpass.conf')

# Determine the password precedence
if not password:
    if 'PGPASSWORD' in os.environ:
        password = os.environ['PGPASSWORD']
    else:
        pgpass_path = passfile or default_pgpass_path
        password = get_passfile_content(pgpass_path, host or '*', str(port or '*'), database or '*', user or '*')

And with all these information we can get the new connection string.


def run_per_database(conn):
# Todo rename to something better.
if 'postgresql' in conn:
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we ought to change it to postgres

@wangxiaoying
Copy link
Contributor

wangxiaoying commented Jul 29, 2024

Thanks for the PR and review @surister @aimtsou !

@wangxiaoying Adding a bunch of new code to support specific postgres functionality into the __init__ feels bloat, how about we add new code in a new file? maybe something like postgres/pgpass.py?

__init__.py postgres/pgpass.py

I think moving the logic into an independent file would be great!

@wangxiaoying
Copy link
Contributor

I just pushed a commit to fix the error that cause the fail of the CI. Please feel free to rebase on that one.

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.

Postgresql: .pgpass support
3 participants