8000 fix password handling in safeURI by acritox · Pull Request #522 · dimitri/pgcopydb · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix password handling in safeURI #522

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 2, 2023
Merged

fix password handling in safeURI #522

merged 5 commits into from
Nov 2, 2023

Conversation

acritox
Copy link
Contributor
@acritox acritox commented Oct 31, 2023

when the passwords of source and target differ it will always fail on the first connection attempt because it tries to log in with the password for the other DB:

reproducible test case:

$ git switch main && make -s bin
Already on 'main'
Your branch is up to date with 'origin/main'.
$ env|grep PGCOPYDB_
PGCOPYDB_SOURCE_PGURI=postgres://testuser1:testPassword@localhost/testdb1
PGCOPYDB_TARGET_PGURI=postgres://testuser2:testPassword@localhost/testdb2
$ ./src/bin/pgcopydb/pgcopydb clone --restart |& grep connect
$ psql -e postgres://postgres@localhost --command="ALTER ROLE testuser2 PASSWORD 'a_different_password';"
ALTER ROLE testuser2 PASSWORD 'a_different_password';
ALTER ROLE
Time: 12.275 ms
$ export PGCOPYDB_TARGET_PGURI="postgres://testuser2:a_different_password@localhost/testdb2"
$ ./src/bin/pgcopydb/pgcopydb clone --restart |& grep connect
2023-10-31 16:21:24 2172643 WARN   pgsql.c:608               Failed to connect to "postgres://testuser2@localhost/testdb2?keepalives=1&keepalives_idle=10&keepalives_interval=10&keepalives_count=60", retrying until the server is ready
2023-10-31 16:21:24 2172643 INFO   pgsql.c:693               Successfully connected to "postgres://testuser2@localhost/testdb2?keepalives=1&keepalives_idle=10&keepalives_interval=10&keepalives_count=60" after 2 attempts in 88 ms.
2023-10-31 16:21:25 2172643 WARN   pgsql.c:608               Failed to connect to "postgres://testuser2@localhost/testdb2?keepalives=1&keepalives_idle=10&keepalives_interval=10&keepalives_count=60", retrying until the server is ready
2023-10-31 16:21:25 2172643 INFO   pgsql.c:693               Successfully connected to "postgres://testuser2@localhost/testdb2?keepalives=1&keepalives_idle=10&keepalives_interval=10&keepalives_count=60" after 2 attempts in 92 ms.
2023-10-31 16:21:25 2172662 WARN   pgsql.c:608               Failed to connect to "postgres://testuser2@localhost/testdb2?keepalives=1&keepalives_idle=10&keepalives_interval=10&keepalives_count=60", retrying until the server is ready
2023-10-31 16:21:25 2172662 INFO   pgsql.c:693               Successfully connected to "postgres://testuser2@localhost/testdb2?keepalives=1&keepalives_idle=10&keepalives_interval=10&keepalives_count=60" after 2 attempts in 102 ms.
$ git switch fix_different_passwords; make -s bin
Switched to branch 'fix_different_passwords'
Your branch is up to date with 'origin/fix_different_passwords'.
$ ./src/bin/pgcopydb/pgcopydb clone --restart |& grep connect
$

no output from grep = no issue, therefore this commit fixes the bug

8000
@acritox
fix password handling in safeURI
771e30e 
when the passwords of source and target differ it will always fail on
the first connection attempt because it tries to log in with the
password for the other DB:

WARN   Failed to connect to "postgres://…", retrying until the server is ready
INFO   Successfully connected to "postgres://…" after 2 attempts in 125 ms.
@dimitri
Copy link
Owner
dimitri commented Nov 2, 2023

Thanks for this contribution!

After careful consideration I see that we rely on changing the environment internally within a process, without starting a new command, and that's undefined behavior / unsupported. We should just skip the PGPASSWORD environment processing altogether and use pgsql->connectionString here really.

@acritox
Copy link
Contributor Author
acritox commented Nov 2, 2023

yes that sounds good, too - I think pgsql->connectionString is what the second connection attempt uses and why it succeeds then

@acritox
Copy link
Contributor Author
acritox commented Nov 2, 2023

yes, pgsql_retry_open_connection() uses pgsql->connectionString, so the change in this line was probably an error and I'll change it back.

I found PR #328 which might explain why there was some PGPASSWORD environment processing in the implemented first place:

we set the password in the environment of the child processes when running pg_dump etc commands

so it makes sense having that in pgcmd.c but in pgsql.c it shouldn't be needed, or should it?

there's actually a bit of a cleanup left that should be done now from PR#328, e.g. the comments for uri_grab_password()
(it's not uri_contains_password()) and parse_and_scrub_connection_string() are not correct anymore, PASSWORD_MASK is not used anywhere currently

I'll change those, try it out and will update this PR if it works.

@acritox
remove PGPASSWORD environment processing from pgsql.c
4d0d558 
instead use `pgsql->connectionString` containing the password
and adjust related function comments and removed unused PASSWORD_MASK
@dimitri
Copy link
Owner
dimitri commented Nov 2, 2023

so it makes sense having that in pgcmd.c but in pgsql.c it shouldn't be needed, or should it?

Not needed in pgsql.c ; it's undefined behaviour and we'd rather not count on that and simply use .connectionString instead.

@acritox
Copy link
Contributor Author
acritox commented Nov 2, 2023

PR updated - took a bit because I stumbled into another bug while testing 😆

pgcopydb clone just silently exited at step 3 in some cases I tested…

why? because the pg_restore subprocess segfaulted when the Source URI had a password but the Target URI did not (so it used $PGPASSWORD from env), i.e.:

export PGPASSWORD=testPassword
export PGCOPYDB_SOURCE_PGURI=postgres://testuser1:testPassword@localhost/testdb1
export PGCOPYDB_TARGET_PGURI=postgres://testuser2@localhost/testdb2
pgcopydb clone

found out that safeSourcePGURI and safeTargetPGURI were interchanged in two places in pg_restore_db()

@dimitri dimitri merged commit 44d6fdf into dimitri:main Nov 2, 2023
@acritox acritox deleted the fix_different_passwords branch November 3, 2023 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimitri dimitri dimitri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@acritox @dimitri
0