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 libpq-compatible password files (passfile=) [second draft]. #766

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mattheww
Copy link

@mattheww mattheww commented May 8, 2021

Previously requested in #729.

Supersedes PR #758.

Adds one new config field: passfile.

Accepts pgpass= in connection strings.

Support is only present when the tokio_postgres runtime feature is enabled. Adds a dependency on the tokio fs feature in that case.

Notes:

I've tried to match libpq's behaviour quite closely.

PostgreSQL docs are at:
https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-PASSFILE
https://www.postgresql.org/docs/current/libpq-pgpass.html

libpq has a default password file location (~/.pgpass or %APPDATA%\postgresql\pgpass.conf). I haven't imitated that; password-file lookup won't happen unless a passfile is explicitly requested.

libpq treats connections to a unix socket in the default directory (whether specified explicitly or implicitly) as connections to 'localhost' for purposes of password-file lookup. I haven't tried to imitate that, because rust-postgres doesn't have a notion of the default unix socket directory.

Adds one new config field: 'passfile'.

Accepts 'pgpass=' in connection strings.

Support is only present when the tokio_postgres 'runtime' feature is enabled.
Adds a dependency on the tokio 'fs' feature in that case.

Adds a dev-dependency on tempfile.
Copy link
Contributor

@jeff-davis jeff-davis left a comment

Choose a reason for hiding this comment

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

Please improve the documentation a bit. Other than that, looks great. Thank you!

@@ -51,6 +51,10 @@ use tokio_postgres::{Error, Socket};
/// * `target_session_attrs` - Specifies requirements of the session. If set to `read-write`, the client will check that
/// the `transaction_read_write` session parameter is set to `on`. This can be used to connect to the primary server
/// in a database cluster as opposed to the secondary read-only mirrors. Defaults to `all`.
/// * `passfile` - Filesystem path of a file storing passwords. Each line should have fields
/// `hostname:port:database:username:password`. Lines beginning with `#` are comments. `*` as a complete field
/// matches anything. `password` takes precedence if both are set. The file is ignored (on Unix only) if its
Copy link
Contributor

Choose a reason for hiding this comment

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

"both are set" is a little ambiguous. Can you clarify?

Also, link to the libpq docs, and have some more general explanation of any user-facing differences.

Copy link
Author

Choose a reason for hiding this comment

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

I've done that.

Comment on lines 48 to 54
struct PassfileEntry {
hostname: Vec<u8>,
port: Vec<u8>,
dbname: Vec<u8>,
user: Vec<u8>,
password: Vec<u8>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You might consider a stronger type representation here, e.g. u16 for the port, and maybe Option to represent the stars.

Maybe this would be counterproductive, so it's just an idea. I'd value compatibility with libpq behavior over type cleanliness.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I've introduced a PassfileField type to represent wildcards cleanly. I don't think representing the port as an integer would be helpful (in particular, we don't want to allow "05432" to match port 5432).

Comment on lines +1 to +2
//! Support for reading password files.
//!
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some more documentation here about compatibility with libpq and various choices you made about corner cases and type representations?

For example, postgres supports many encodings, so we don't know whether the representation of a username is UTF8 or something else, so we have to use bytes.

Are there differences in how libpq vs this PR handle malformed files?

Copy link
Author

Choose a reason for hiding this comment

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

I've added some more text.

I've tried to imitate what libpq does for most malformed files. Unsurprisingly libpq behaves strangely if the file has embedded NULs. I haven't tried to reproduce that but I've included a note about it.

libpq also has a bug where if the file lacks a trailing newline the final line, which would normally be processed, is ignored if it exactly fills the read buffer. I don't think that's worth mentioning.

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.

None yet

2 participants