Fabien Coelho wrote:
A few detailed comments:
Beware of spacing:
. "if(" -> "if (" (2 instances)
. " =='\0')" -> " == '\0')" (at least one instance)
Elsewhere:
+ if (conn->pgpassfile_used && conn->password_needed && conn->result &&
+ conn->pgpassfile && conn->pgpassfile[0]!='\0' &&
ISTM that if pgpassfile_used is true then pgpassfile is necessarily
defined, so the second line tests are redundant? Or am I missing
something?
I've adressed those spacing errors.
You are right, if pgpassfile_used is true, it SHOULD be defined, I just
like to be careful whenever I'm working with strings. But I guess in
this scenario I can trust the caller and omit those checks.
On 11/22/2016 07:04 AM, Mithun Cy wrote:
> Independently of your patch, while testing I concluded that the
multi-host feature and documentation should be improved:
> - If a connection fails, it does not say for which host/port.
Thanks I will re-test same.
> In fact they are tried in turn if the network connection fails, but not
> if the connection fails for some other reason such as the auth.
> The documentation is not precise enough.
If we fail due to authentication, then I think we should notify the
client instead of trying next host. I think it is expected genuine
user have right credentials for making the connection. So it's better
if we notify same to client immediately rather than silently ignoring
them. Otherwise the host node which the client failed to connect will
be permanently unavailable until client corrects itself. But I agree
documentation and error message as you said need improvements. I will
try to correct same
I agree with those criticisms of the multi-host feature and notifying
the client in case of an authentification error rather than trying other
hosts seems sensible to me.
But I think fixes for those should be part of different patches, as this
patch's aim was only to expand the existing pgpassfile functionality to
be used with a parameter.
regards,
Julian
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 0f375bf..c806aeb 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -943,7 +943,7 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
Note that authentication is likely to fail if <literal>host</>
is not the name of the server at network address <literal>hostaddr</>.
Also, note that <literal>host</> rather than <literal>hostaddr</>
- is used to identify the connection in <filename>~/.pgpass</> (see
+ is used to identify the connection in a password file (see
<xref linkend="libpq-pgpass">).
</para>
@@ -1002,6 +1002,16 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-pgpassfile" xreflabel="pgpassfile">
+ <term><literal>pgpassfile</literal></term>
+ <listitem>
+ <para>
+ Specifies the name of the file used to lookup passwords.
+ Defaults to the password file (see <xref linkend="libpq-pgpass">).
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-connect-connect-timeout" xreflabel="connect_timeout">
<term><literal>connect_timeout</literal></term>
<listitem>
@@ -6886,9 +6896,8 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
<indexterm>
<primary><envar>PGPASSFILE</envar></primary>
</indexterm>
- <envar>PGPASSFILE</envar> specifies the name of the password file to
- use for lookups. If not set, it defaults to <filename>~/.pgpass</>
- (see <xref linkend="libpq-pgpass">).
+ <envar>PGPASSFILE</envar> behaves the same as the <xref
+ linkend="libpq-connect-pgpassfile"> connection parameter.
</para>
</listitem>
@@ -7160,13 +7169,15 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
</indexterm>
<para>
- The file <filename>.pgpass</filename> in a user's home directory or the
- file referenced by <envar>PGPASSFILE</envar> can contain passwords to
+ The file <filename>.pgpass</filename> in a user's home directory can contain passwords to
be used if the connection requires a password (and no password has been
specified otherwise). On Microsoft Windows the file is named
<filename>%APPDATA%\postgresql\pgpass.conf</> (where
<filename>%APPDATA%</> refers to the Application Data subdirectory in
the user's profile).
+ Alternatively, a password file can be specified
+ using the connection parameter <xref linkend="libpq-connect-pgpassfile">
+ or the environment variable <envar>PGPASSFILE</envar>.
</para>
<para>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 3e9c45b..ffc341d 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -184,6 +184,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"Database-Password", "*", 20,
offsetof(struct pg_conn, pgpass)},
+ {"pgpassfile", "PGPASSFILE", NULL, NULL,
+ "Database-Password-File", "", 64,
+ offsetof(struct pg_conn, pgpassfile)},
+
{"connect_timeout", "PGCONNECT_TIMEOUT", NULL, NULL,
"Connect-timeout", "", 10, /* strlen(INT32_MAX) == 10 */
offsetof(struct pg_conn, connect_timeout)},
@@ -374,10 +378,10 @@ static int parseServiceFile(const char *serviceFile,
PQExpBuffer errorMessage,
bool *group_found);
static char *pwdfMatchesString(char *buf, char *token);
-static char *PasswordFromFile(char *hostname, char *port, char *dbname,
- char *username);
-static bool getPgPassFilename(char *pgpassfile);
-static void dot_pg_pass_warning(PGconn *conn);
+static char *passwordFromFile(char *hostname, char *port, char *dbname,
+ char *username, char *pgpassfile);
+static bool fillDefaultPGPassFile(PGconn *conn);
+static void pgpassfileWarning(PGconn *conn);
static void default_threadlock(int acquire);
@@ -954,14 +958,25 @@ connectOptions2(PGconn *conn)
conn->pgpass = strdup(DefaultPassword);
if (!conn->pgpass)
goto oom_error;
+
+ if (!conn->pgpassfile || conn->pgpassfile[0] == '\0')
+ {
+ /* Fail if we couldn't fill in the default pgpassfile */
+ if (fillDefaultPGPassFile(conn) == false)
+ return false;
+ }
+
for (i = 0; i < conn->nconnhost; ++i)
{
+ /* We'll pass conn->pgpassfile regardless of it's contents - checks happen in passwordFromFile() */
conn->connhost[i].password =
- PasswordFromFile(conn->connhost[i].host,
+ passwordFromFile(conn->connhost[i].host,
conn->connhost[i].port,
- conn->dbName, conn->pguser);
- if (conn->connhost[i].password != NULL)
- conn->dot_pgpass_used = true;
+ conn->dbName, conn->pguser,
+ conn->pgpassfile);
+ /* Check if conn->pgpassfile_used is already true so we can skip this each following iteration */
+ if (!conn->pgpassfile_used && conn->connhost[i].password != NULL)
+ conn->pgpassfile_used = true;
}
}
@@ -2830,7 +2845,7 @@ keep_going: /* We will come back to here until there is
error_return:
- dot_pg_pass_warning(conn);
+ pgpassfileWarning(conn);
/*
* We used to close the socket at this point, but that makes it awkward
@@ -2961,7 +2976,7 @@ makeEmptyPGconn(void)
conn->sock = PGINVALID_SOCKET;
conn->auth_req_received = false;
conn->password_needed = false;
- conn->dot_pgpass_used = false;
+ conn->pgpassfile_used = false;
#ifdef USE_SSL
conn->allow_ssl_try = true;
conn->wait_ssl_try = false;
@@ -3070,6 +3085,8 @@ freePGconn(PGconn *conn)
free(conn->pguser);
if (conn->pgpass)
free(conn->pgpass);
+ if (conn->pgpassfile)
+ free(conn->pgpassfile);
if (conn->keepalives)
free(conn->keepalives);
if (conn->keepalives_idle)
@@ -5947,12 +5964,12 @@ pwdfMatchesString(char *buf, char *token)
return NULL;
}
-/* Get a password from the password file. Return value is malloc'd. */
+/* Get a password from the password file or the user-specified pgpassfile.
+ Return value is malloc'd. */
static char *
-PasswordFromFile(char *hostname, char *port, char *dbname, char *username)
+passwordFromFile(char *hostname, char *port, char *dbname, char *username, char *pgpassfile)
{
FILE *fp;
- char pgpassfile[MAXPGPATH];
struct stat stat_buf;
#define LINELEN NAMEDATALEN*5
@@ -5979,9 +5996,6 @@ PasswordFromFile(char *hostname, char *port, char *dbname, char *username)
if (port == NULL)
port = DEF_PGPORT_STR;
- if (!getPgPassFilename(pgpassfile))
- return NULL;
-
/* If password file cannot be opened, ignore it. */
if (stat(pgpassfile, &stat_buf) != 0)
return NULL;
@@ -6076,21 +6090,19 @@ PasswordFromFile(char *hostname, char *port, char *dbname, char *username)
static bool
-getPgPassFilename(char *pgpassfile)
+fillDefaultPGPassFile(PGconn *conn)
{
- char *passfile_env;
-
- if ((passfile_env = getenv("PGPASSFILE")) != NULL)
- /* use the literal path from the environment, if set */
- strlcpy(pgpassfile, passfile_env, MAXPGPATH);
- else
- {
- char homedir[MAXPGPATH];
-
- if (!pqGetHomeDirectory(homedir, sizeof(homedir)))
- return false;
- snprintf(pgpassfile, MAXPGPATH, "%s/%s", homedir, PGPASSFILE);
- }
+ char homedir[MAXPGPATH];
+
+ if (conn->pgpassfile)
+ free(conn->pgpassfile);
+
+ conn->pgpassfile = malloc(MAXPGPATH);
+
+ if (!pqGetHomeDirectory(homedir, sizeof(homedir)))
+ return false;
+ snprintf(conn->pgpassfile, MAXPGPATH, "%s/%s", homedir, PGPASSFILE);
+
return true;
}
@@ -6100,21 +6112,17 @@ getPgPassFilename(char *pgpassfile)
* password is wrong.
*/
static void
-dot_pg_pass_warning(PGconn *conn)
+pgpassfileWarning(PGconn *conn)
{
- /* If it was 'invalid authorization', add .pgpass mention */
+ /* If it was 'invalid authorization', add pgpassfile mention */
/* only works with >= 9.0 servers */
- if (conn->dot_pgpass_used && conn->password_needed && conn->result &&
+ if (conn->pgpassfile_used && conn->password_needed && conn->result &&
strcmp(PQresultErrorField(conn->result, PG_DIAG_SQLSTATE),
ERRCODE_INVALID_PASSWORD) == 0)
{
- char pgpassfile[MAXPGPATH];
-
- if (!getPgPassFilename(pgpassfile))
- return;
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("password retrieved from file \"%s\"\n"),
- pgpassfile);
+ conn->pgpassfile);
}
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 854ec89..a60ebea 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -343,6 +343,7 @@ struct pg_conn
char *replication; /* connect as the replication standby? */
char *pguser; /* Postgres username and password, if any */
char *pgpass;
+ char *pgpassfile; /* path to a file containing the password */
char *keepalives; /* use TCP keepalives? */
char *keepalives_idle; /* time between TCP keepalives */
char *keepalives_interval; /* time between TCP keepalive
@@ -404,7 +405,7 @@ struct pg_conn
bool auth_req_received; /* true if any type of auth req
* received */
bool password_needed; /* true if server demanded a password */
- bool dot_pgpass_used; /* true if used .pgpass */
+ bool pgpassfile_used; /* true if a password from a pgpassfile was used */
bool sigpipe_so; /* have we masked SIGPIPE via SO_NOSIGPIPE? */
bool sigpipe_flag; /* can we mask SIGPIPE via MSG_NOSIGNAL? */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
