[PATCH] pgpassfile connection option
Hello psql-hackers!
We thought it would be advantageous to be able to specify a 'custom'
pgpassfile within the connection string along the lines of the existing
parameters sslkey and sslcert.
Which is exactly what this very compact patch does.
The patch is minimally invasive - when no pgpassfile attribute is
provided in the connection string, the regular pgpassfile is used.
The security-measures (which are limited to checking the permissions for
0600) are kept, however we could loosen that restriciton to allow group
access as well along the lines of the ssl key file , if this is
preferred. (in case multiple users belonging to the same group would
like to connect using the same file).
The patch applies cleanly to master and compiles and runs as expected
(as there are no critical alterations).
I've not written any documentation as of now, but I'll follow up closely
if there is any interest for this patch.
notes:
- using ~ to denote the user's home directory in the path does not
work, however $HOME works (as this is translated by bash beforehand).
- the notation in the custom pgpassfile should follow the notation of
the 'default' pgpass files:
hostname:port:database:username:password
- this has only been tested on linux so far, however due to the nature
of the changes I suspect that there is nothing that could go wrong in
other environments, although I could test that as well, if deemed necessary.
I'm looking forward to any feedback,
Julian
--
Julian Markwort
Westphalian Wilhelms-University in Münster
julian.markwort@uni-muenster.de
Attachments:
pgpassfile-v1.patchtext/x-patch; name=pgpassfile-v1.patchDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 9b2839b..5c0d88a 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)},
@@ -375,7 +379,7 @@ static int parseServiceFile(const char *serviceFile,
bool *group_found);
static char *pwdfMatchesString(char *buf, char *token);
static char *PasswordFromFile(char *hostname, char *port, char *dbname,
- char *username);
+ char *username, char *pgpassfile);
static bool getPgPassFilename(char *pgpassfile);
static void dot_pg_pass_warning(PGconn *conn);
static void default_threadlock(int acquire);
@@ -806,8 +810,9 @@ connectOptions2(PGconn *conn)
{
if (conn->pgpass)
free(conn->pgpass);
+ /* We'll pass conn->pgpassfile regardless of it's contents - checks happen in PasswordFromFile() */
conn->pgpass = PasswordFromFile(conn->pghost, conn->pgport,
- conn->dbName, conn->pguser);
+ conn->dbName, conn->pguser, conn->pgpassfile);
if (conn->pgpass == NULL)
{
conn->pgpass = strdup(DefaultPassword);
@@ -5699,14 +5704,15 @@ 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];
+ char temp_path[MAXPGPATH];
struct stat stat_buf;
+
#define LINELEN NAMEDATALEN*5
char buf[LINELEN];
@@ -5731,8 +5737,13 @@ PasswordFromFile(char *hostname, char *port, char *dbname, char *username)
if (port == NULL)
port = DEF_PGPORT_STR;
- if (!getPgPassFilename(pgpassfile))
- return NULL;
+ if(!pgpassfile || pgpassfile[0]=='\0')
+ {
+ /* If no pgpassfile was supplied by the user or it is empty, we try to get a global pgpassfile */
+ if (!getPgPassFilename(temp_path))
+ return NULL;
+ pgpassfile = temp_path;
+ }
/* If password file cannot be opened, ignore it. */
if (stat(pgpassfile, &stat_buf) != 0)
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 1183323..ae9317f 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -317,6 +317,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
On 09/22/2016 10:44 AM, Julian Markwort wrote:
Hello psql-hackers!
We thought it would be advantageous to be able to specify a 'custom'
pgpassfile within the connection string along the lines of the
existing parameters sslkey and sslcert.Which is exactly what this very compact patch does.
The patch is minimally invasive - when no pgpassfile attribute is
provided in the connection string, the regular pgpassfile is used.
The security-measures (which are limited to checking the permissions
for 0600) are kept, however we could loosen that restriciton to allow
group access as well along the lines of the ssl key file , if this is
preferred. (in case multiple users belonging to the same group would
like to connect using the same file).The patch applies cleanly to master and compiles and runs as expected
(as there are no critical alterations).
I've not written any documentation as of now, but I'll follow up
closely if there is any interest for this patch.notes:
- using ~ to denote the user's home directory in the path does not
work, however $HOME works (as this is translated by bash beforehand).
- the notation in the custom pgpassfile should follow the notation of
the 'default' pgpass files:
hostname:port:database:username:password
- this has only been tested on linux so far, however due to the
nature of the changes I suspect that there is nothing that could go
wrong in other environments, although I could test that as well, if
deemed necessary.
I'm not necessarily opposed to this, but what is the advantage over the
existing PGPASSFILE environment setting mechanism?
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I haven't really thought about this as I had been asked to make this
work as an additional option to the connection parameters...
Now that I've looked at it - there is really only the benefit of saving
the step of setting the PGPASSFILE environment variable.
However, there might be cases in which setting an environment variable
might not be the easiest option.
Am 22.09.2016 um 17:15 schrieb Andrew Dunstan:
I'm not necessarily opposed to this, but what is the advantage over
the existing PGPASSFILE environment setting mechanism?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Sep 22, 2016 at 11:34 AM, Julian Markwort
<julian.markwort@uni-muenster.de> wrote:
I haven't really thought about this as I had been asked to make this work as
an additional option to the connection parameters...
Now that I've looked at it - there is really only the benefit of saving the
step of setting the PGPASSFILE environment variable.
However, there might be cases in which setting an environment variable might
not be the easiest option.
So, there are some security concerns here in my mind. If a program
running under a particular user ID accepts a connection string from a
source that isn't fully trusted, the user has to accept the risk that
their .pgpass file will be used for authentication to whatever
database the program might try to connect. However, they don't have
to accept the possibility that arbitrary local files readable by the
user ID will be used for authentication and/or disclosed; this patch
would force them to accept that risk. That doesn't seem particularly
good. If an adversary has enough control over my account that they
can set environment variables, it's game over: they win. But if I
merely accept connection strings from them, I shouldn't have to worry
about anything worse than that I might be induced to connect to the
wrong thing.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 09/26/2016 07:51 PM, Robert Haas wrote:
However, they don't have
to accept the possibility that arbitrary local files readable by the
user ID will be used for authentication and/or disclosed; this patch
would force them to accept that risk.
I do agree with you, however we might have to take a look at the
parameter sslkey's implementation here as well - There are no checks in
place to stop you from using rogue sslkey parameters.
I'd like to suggest having both of these parameters behave in a similar
fashion. In order to achieve safe behaviour, we could implement the use
of environment variables prohibiting the use of user-located pgpassfiles
and sslkeys.
How about PGSECRETSLOCATIONLOCK ?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Oct 4, 2016 at 7:42 AM, Julian Markwort
<julian.markwort@uni-muenster.de> wrote:
On 09/26/2016 07:51 PM, Robert Haas wrote:
However, they don't have
to accept the possibility that arbitrary local files readable by the
user ID will be used for authentication and/or disclosed; this patch
would force them to accept that risk.I do agree with you, however we might have to take a look at the parameter
sslkey's implementation here as well - There are no checks in place to stop
you from using rogue sslkey parameters.
I'd like to suggest having both of these parameters behave in a similar
fashion. In order to achieve safe behaviour, we could implement the use of
environment variables prohibiting the use of user-located pgpassfiles and
sslkeys.
How about PGSECRETSLOCATIONLOCK ?
You could do something like that, I guess, but I think it might be a
good idea to wait and see if anyone else has opinions on (1) the
desirability of the basic feature, (2) the severity of the security
hazard it creates, and (3) your proposed remediation method.
So far I don't see anybody actually endorsing your proposal here,
which might mean that any patch you produce will be rejected on the
grounds that nobody has a clear use case for this feature.
Hey, everybody: chime in here...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
05.10.2016, 20:06, Robert Haas kirjoitti:
You could do something like that, I guess, but I think it might be a
good idea to wait and see if anyone else has opinions on (1) the
desirability of the basic feature, (2) the severity of the security
hazard it creates, and (3) your proposed remediation method.
I don't think it's appropriate to compare the proposed pgpassfile option
to sslkey: the key is never transmitted over the network anywhere.
So far I don't see anybody actually endorsing your proposal here,
which might mean that any patch you produce will be rejected on the
grounds that nobody has a clear use case for this feature.Hey, everybody: chime in here...
The original patch didn't come with a description of the problem it was
aiming to solve, but I've wanted something perhaps a bit similar in the
past.
The pghoard backup & restore suite we maintain needs to be able to spawn
pg_basebackup and pg_receivexlog processes and in order to avoid passing
passwords in command-line arguments visible to everyone who can get a
process listing we currently have pghoard edit pgpass files.
Having to edit pgpass files at all is annoying and there isn't really a
good way to do it: we can edit the one at $HOME and hope it doesn't
clash with the expectations of the user running the software, write it
to a custom file somewhere else - copying the password to a location the
user might not expect - or create a new temporary file on every invocation.
I came across some bad advice about passing passwords to libpq today:
there was a recommendation for setting a $PASSWORD environment variable
and including that in the connection string, using shell to expand it.
It got me thinking that maybe such a feature could be implemented in
libpq: have it expand unquoted $variables; for example:
$ PASSWORD=xyz psql 'password=$PASSWORD dbname=foo'
This does have the hazard of making it very easy to accidentally use
double quotes instead of single quotes and have the shell expand the
variable making it visible in process listing though.
/ Oskari
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
My 0.02ￜ:
Patch applies cleanly, make check ok... however AFAICS it only means that
it compiles but it is not tested in anyway... This is is annoying. Well
I'm not sure whether other options are tested either, but they should.
ISTM that this feature is already available through the PGPASSFILE
environment variable, so this is really just about providing the same
feature through a connection string option, if I'm not mistaken. I'm fine
with that.
The documentation needs to be updated.
I'm not much concerned about security issues as discussed on the thread.
If an attacker can control the file, then what... they can provide a
password in my place in a file of their own? If they have the pass then it
works, if they do not then the connection will fail... They cannot change
the database I'm connecting to from the pass file, say to provide other
credentials to an application. So I do not see this as worst that the
current status.
I tested the feature through python/psychopg2 by setting LD_LIBRARY_PATH
to the dev version. I got a strange behavior wrt to errors. With "./foo"
containing the right password it is fine.
import psycopg2 as pg
c = pg.connect("host=localhost dbname=test user=test pgpassfile=./foo")
# ok
c.close()
Now if "./foo" contains a bad password:
import psycopg2 as pg
c = pg.connect("host=localhost dbname=test user=test pgpassfile=./foo")
# error:
OperationalError: FATAL: password authentication failed for user "test"
password retrieved from file "$HOME/.pgpass"
The reported password file is wrong. It is even more funny if ~/.pgpass
contains the right password: the pgpassfile option *is* taken into
account, ./foo is read and it fails, but the error message is not
updated and points to the wrong file. The error message stuff should be
updated to look for the pgpassfile connection string option...
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Oct 11, 2016 at 5:06 PM, Oskari Saarenmaa <os@ohmu.fi> wrote:
$ PASSWORD=xyz psql 'password=$PASSWORD dbname=foo'
This does have the hazard of making it very easy to accidentally use double
quotes instead of single quotes and have the shell expand the variable
making it visible in process listing though.
It has the hazard that environment variables are visible in the
process listing anyway on many platforms. On Linux, try "ps auxeww";
on MacOS X, try "ps -efEww". At a quick glance, it seems that on both
of those platforms you have to either be root or be the same user that
owns the process, but I'm not sure that every platform will have it
locked down that tightly and even that might be more exposure than you
really want.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/16/2016 12:09 PM, Fabien COELHO wrote:
Patch applies cleanly, make check ok... however AFAICS it only means
that it compiles but it is not tested in anyway... This is is
annoying. Well I'm not sure whether other options are tested either,
but they should.
Thanks for taking the time to review my patch! I haven't found much
regarding the testing of connection options. However since there isn't
anything fancy going on in PasswordFromFile() I'm not too worried about
that.
The documentation needs to be updated.
I've written a couple lines now.
I aligned the definition of the connection option and the environment
variable with that of other (conn.opt&env.var.) pairs and added mention
of the different options to the doc of the "Password File".
The reported password file is wrong. It is even more funny if
~/.pgpass contains the right password: the pgpassfile option *is*
taken into account, ./foo is read and it fails, but the error message
is not updated and points to the wrong file. The error message stuff
should be updated to look for the pgpassfile connection string option...
That was indeed an Error on my side, I hadn't updated the errormessages
to inform you which file has been used.
So attached is an updated version of the patch.
I'd like to ask for some input on how to handle invalid files - right
now no message is shown, the user just gets a password prompt as a
result, however I think a message when the custom pgpassfile hasn't been
found would be helpful.
Kind regards,
Julian
--
Julian Markwort
Westphalian Wilhelms-University in Mï¿œnster
julian(dot)markwort(at)uni-muenster(dot)de
Attachments:
pgpassfile_v2.patchtext/plain; charset=UTF-8; name=pgpassfile_v2.patchDownload
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 4e34f00..1bd5597 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -930,7 +930,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>
@@ -986,6 +986,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>
@@ -6862,9 +6872,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>
@@ -7136,13 +7145,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 f3a9e5a..db1de26 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)},
@@ -375,7 +379,7 @@ static int parseServiceFile(const char *serviceFile,
bool *group_found);
static char *pwdfMatchesString(char *buf, char *token);
static char *PasswordFromFile(char *hostname, char *port, char *dbname,
- char *username);
+ char *username, char *pgpassfile);
static bool getPgPassFilename(char *pgpassfile);
static void dot_pg_pass_warning(PGconn *conn);
static void default_threadlock(int acquire);
@@ -806,8 +810,9 @@ connectOptions2(PGconn *conn)
{
if (conn->pgpass)
free(conn->pgpass);
+ /* We'll pass conn->pgpassfile regardless of it's contents - checks happen in PasswordFromFile() */
conn->pgpass = PasswordFromFile(conn->pghost, conn->pgport,
- conn->dbName, conn->pguser);
+ conn->dbName, conn->pguser, conn->pgpassfile);
if (conn->pgpass == NULL)
{
conn->pgpass = strdup(DefaultPassword);
@@ -5703,12 +5708,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];
+ char temp_path[MAXPGPATH];
struct stat stat_buf;
#define LINELEN NAMEDATALEN*5
@@ -5735,8 +5740,13 @@ PasswordFromFile(char *hostname, char *port, char *dbname, char *username)
if (port == NULL)
port = DEF_PGPORT_STR;
- if (!getPgPassFilename(pgpassfile))
- return NULL;
+ if(!pgpassfile || pgpassfile[0]=='\0')
+ {
+ /* If no pgpassfile was supplied by the user or it is empty, we try to get a global pgpassfile */
+ if (!getPgPassFilename(temp_path))
+ return NULL;
+ pgpassfile = temp_path;
+ }
/* If password file cannot be opened, ignore it. */
if (stat(pgpassfile, &stat_buf) != 0)
@@ -5858,11 +5868,14 @@ dot_pg_pass_warning(PGconn *conn)
{
char pgpassfile[MAXPGPATH];
- if (!getPgPassFilename(pgpassfile))
- return;
+ /* If no pgpassfile was specified with the connection, we try to get the default one */
+ if (!conn->pgpassfile){
+ if(!getPgPassFilename(pgpassfile))
+ return;
+ }
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("password retrieved from file \"%s\"\n"),
- pgpassfile);
+ (conn->pgpassfile)?(conn->pgpassfile):pgpassfile);
}
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 7007692..9f5c2f9 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -317,6 +317,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
Hello Julian,
The documentation needs to be updated.
I've written a couple lines now.
I aligned the definition of the connection option and the environment
variable with that of other (conn.opt&env.var.) pairs and added mention of
the different options to the doc of the "Password File".
Ok.
[...] That was indeed an Error on my side, I hadn't updated the
errormessages to inform you which file has been used.So attached is an updated version of the patch.
Patch applies, compiles, "make check" ok (although the feature is not
tested there).
Documentation compiled to html and looks ok.
I tested the feature with psql by resetting the dynamic library path.
The error message error in the previous review is fixed.
I have a problem with how the pass file name is managed: there
are three possible sources:
1) pgpassfile connection string option
2) PGPASSFILE environment variable
3) the default (~/.pgpass or something else on windows)
Now function getPgPassFilename() is a misnomer, as it does not
always return the pass filename, but only in cases 2 and 3.
I think that PGPASSFILE is somehow checked twice: once explicitely
in getPgPassFilename and once because of the struct declaration
"pgpassfile". Once should be enough.
So I think some cleanup would be welcome. The mess is preexisting to your
patch because the PGPASSFILE environment variable was managed differently
from other options.
I would suggest that the struct gets the value (from option, environment
or default) and is always used elsewhere. The getPgPassFilename function
should disappear and PasswordFromFile should be simplified significantly.
I'd like to ask for some input on how to handle invalid files - right
now no message is shown, the user just gets a password prompt as a
result, however I think a message when the custom pgpassfile hasn't been
found would be helpful.
I agree that it is currently unconvincing. I'm unclear about the correct
level of messages that should be displayed for a library. I think that it
should be pretty silent by default if all is well but that some
environment variable should be able to put a more verbose level...
Libpq connection options seem not to be tested anywhere. There should be
TAP tests in src/interfaces/libpq. This remark is independent of your
patch.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thanks for your quick response, Fabien
I think that PGPASSFILE is somehow checked twice
yup, that was unintended redundancy...
I would suggest that the struct gets the value (from option,
environment or default) and is always used elsewhere. The
getPgPassFilename function should disappear and PasswordFromFile
should be simplified significantly.
I agree, however that's easier said than done because the "default" is
only constant by its definition, not by its location.
There is no "default" location as the home directory depends not only on
the system but also on the user. Afaics we can't entirely get rid of a
function to get the location of the default .pgpass file.
Kind regards,
Julian
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I would suggest that the struct gets the value (from option, environment or
default) and is always used elsewhere. The getPgPassFilename function
should disappear and PasswordFromFile should be simplified significantly.I agree, however that's easier said than done because the "default" is only
constant by its definition, not by its location.
There is no "default" location as the home directory depends not only on the
system but also on the user. Afaics we can't entirely get rid of a function
to get the location of the default .pgpass file.
Hmmm... I thought to put the default if not set from the connection option
or the environment variable, say in connectOptions2(), just before getting
the password if it is needed? That is ensure that the pgpassfile field is
not NULL before calling PasswordFromFile, so that PasswordFromFile does
not have to do anything and then the error message can rely on the
pgpassfile field?
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Alright, here goes another one:
1. Cleaned up the clutter with getPgPassFilename - the function is now
named fillDefaultPGPassFile() and only does exactly that.
2. Since a connection option "pgpassfile" or environment variable
"PGPASSFILE" are picked up in conninfo_add_defaults() or in case a
password was needed, but neither a pgpassfile connection option or
environment variable were set, we'd have filled the conn->pgpassfile
field with the "default" ~/.pgpass stuff.
Thus, when returning with an error, if conn->pgpassfile was set and a
password was necessary, we must have tried that pgpassfile, so i got rid
of the field "dot_pgpass_used" in the pg_conn struct and the pgpassfile
string is always used in the error message.
3. Going on, I renamed "dot_pg_pass_warning()" to "PGPassFileWarning()"
Kind regards,
Julian Markwort
Attachments:
pgpassfiles_v3.patchtext/plain; charset=UTF-8; name=pgpassfiles_v3.patchDownload
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 4e34f00..1bd5597 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -930,7 +930,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>
@@ -986,6 +986,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>
@@ -6862,9 +6872,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>
@@ -7136,13 +7145,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 f3a9e5a..e8f8fe1 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)},
@@ -375,9 +379,9 @@ static int parseServiceFile(const char *serviceFile,
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);
+ char *username, char *pgpassfile);
+static bool fillDefaultPGPassFile(PGconn *conn);
+static void PGPassFileWarning(PGconn *conn);
static void default_threadlock(int acquire);
@@ -804,18 +808,22 @@ connectOptions2(PGconn *conn)
*/
if (conn->pgpass == NULL || conn->pgpass[0] == '\0')
{
+ if(!conn->pgpassfile || conn->pgpassfile[0] =='\0')
+ {
+ fillDefaultPGPassFile(conn);
+ }
+
if (conn->pgpass)
free(conn->pgpass);
+ /* We'll pass conn->pgpassfile regardless of it's contents - checks happen in PasswordFromFile() */
conn->pgpass = PasswordFromFile(conn->pghost, conn->pgport,
- conn->dbName, conn->pguser);
+ conn->dbName, conn->pguser, conn->pgpassfile);
if (conn->pgpass == NULL)
{
conn->pgpass = strdup(DefaultPassword);
if (!conn->pgpass)
goto oom_error;
}
- else
- conn->dot_pgpass_used = true;
}
/*
@@ -2661,7 +2669,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
@@ -2792,7 +2800,6 @@ makeEmptyPGconn(void)
conn->sock = PGINVALID_SOCKET;
conn->auth_req_received = false;
conn->password_needed = false;
- conn->dot_pgpass_used = false;
#ifdef USE_SSL
conn->allow_ssl_try = true;
conn->wait_ssl_try = false;
@@ -2888,6 +2895,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)
@@ -5703,12 +5712,11 @@ 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
@@ -5735,9 +5743,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;
@@ -5824,21 +5829,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;
}
@@ -5848,21 +5851,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 && 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 7007692..ec96420 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -317,6 +317,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
@@ -373,7 +374,6 @@ 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 sigpipe_so; /* have we masked SIGPIPE via SO_NOSIGPIPE? */
bool sigpipe_flag; /* can we mask SIGPIPE via MSG_NOSIGNAL? */
Welp, I was in head over heels, sorry for my messy email...
*2. No more "dot_pgpass_used" - we fill the conn->pgpassfile field with
any options that have been provided (connection parameter, environment
variable, "default" ~/.pgpass) and in case there has been an error with
the authentification, we only use conn->pgpassfile in the error message.
Fabien Coelho wrote:
I agree that it is currently unconvincing. I'm unclear about the
correct level of messages that should be displayed for a library. I
think that it should be pretty silent by default if all is well but
that some environment variable should be able to put a more verbose
level...
fe_connect.c in general seems very talkative about it's errors - even if
it's only about files not beeing found, so I'll include an error message
for that case next time.
Julian
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hello Julian,
Alright, here goes another one:
Patch v3 applies, make check ok, feature tested on Linux, one small issue
found, see below.
1. Cleaned up the clutter with getPgPassFilename - the function is now named
fillDefaultPGPassFile() and only does exactly that.
Ok.
2. Since a connection option "pgpassfile" or environment variable
"PGPASSFILE" are picked up in conninfo_add_defaults() or in case a password
was needed, but neither a pgpassfile connection option or environment
variable were set, we'd have filled the conn->pgpassfile field with the
"default" ~/.pgpass stuff.
Ok.
Thus, when returning with an error, if conn->pgpassfile was set and a
password was necessary, we must have tried that pgpassfile, so i got rid of
the field "dot_pgpass_used"
No, you should not have done that, because it changes a feature which was
to warn *only* when the password was coming from file.
in the pg_conn struct and the pgpassfile string is always used in the
error message.
sh> touch dot_pass_empty
sh> LD_LIBRARY_PATH=./src/interfaces/libpq \
psql "dbname=test host=localhost user=test pgpassfile=./dot_pass_empty"
Password: BAD_PASSWORD_TYPED
psql: FATAL: password authentication failed for user "test"
password retrieved from file "./dot_pass_empty"
The warning is wrong, the password was typed directly, not retrieved from
a file. The "dot_pgpass_used" boolean is still required to avoid that.
3. Going on, I renamed "dot_pg_pass_warning()" to "PGPassFileWarning()"
This makes sense, its name is not necessarily ".pgpass".
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Am 01.11.2016 um 15:14 schrieb Fabien COELHO:
Thus, when returning with an error, if conn->pgpassfile was set and a
password was necessary, we must have tried that pgpassfile, so i got
rid of the field "dot_pgpass_used"No, you should not have done that, because it changes a feature which
was to warn *only* when the password was coming from file.The warning is wrong, the password was typed directly, not retrieved
from a file. The "dot_pgpass_used" boolean is still required to avoid
that.
I see... I was too focused on looking for things to declutter, that I
missed that case. I'll address that in the next revision.
I've updated my patch to work with the changes introduced to libpq by allowing multiple hosts.
On Fabien's recommendations, I've kept the variable dot_pgpassfile_used, however I renamed that to pgpassfile_used, to keep a consistent naming scheme.
I'm still not sure about the amount of error messages produced by libpq, I think it would be ideal to report an error while accessing a file provided in the connection string, however I would not report that same type of error when the .pgpass file has been tried to retrieve a password.
(Else, you'd get an error message on every connection string that doesn't specify a pgpassfile or password, since .pgpass will be checked every time, before prompting the user to input the password)
regards,
Julian Markwort
Attachments:
pgpassfile_v4.patchapplication/octet-stream; name=pgpassfile_v4.patchDownload
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index d04dba7..72e1341 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>
@@ -6882,9 +6892,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>
@@ -7156,13 +7165,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 b4f9ad7..abcf1ae 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)},
@@ -375,9 +379,9 @@ static int parseServiceFile(const char *serviceFile,
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);
+ char *username, char *pgpassfile);
+static bool fillDefaultPGPassFile(PGconn *conn);
+static void PGPassFileWarning(PGconn *conn);
static void default_threadlock(int acquire);
@@ -954,14 +958,23 @@ connectOptions2(PGconn *conn)
conn->pgpass = strdup(DefaultPassword);
if (!conn->pgpass)
goto oom_error;
+
+ if(!conn->pgpassfile || conn->pgpassfile[0] =='\0')
+ {
+ fillDefaultPGPassFile(conn);
+ }
+
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,
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 +2843,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 +2974,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 +3083,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)
@@ -5946,12 +5961,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
@@ -5978,9 +5993,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;
@@ -6067,21 +6079,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;
}
@@ -6091,21 +6101,18 @@ 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 &&
+ conn->pgpassfile && conn->pgpassfile[0]!='\0' &&
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? */
All,
* Robert Haas (robertmhaas@gmail.com) wrote:
You could do something like that, I guess, but I think it might be a
good idea to wait and see if anyone else has opinions on (1) the
desirability of the basic feature, (2) the severity of the security
hazard it creates, and (3) your proposed remediation method.
[...]
Hey, everybody: chime in here...
The feature strikes me as pretty reasonable to have and the pghoard
example shows that it can be quite handy in some circumstances. I don't
see much merit behind the security concern raised- the file in question
would have to have the correct format and you would have to be
connecting to a system listed in that file for any disclosure to happen,
no? As such, I don't know that any remediation is necessary for this.
Thanks!
Stephen
Hello Julian,
I've updated my patch to work with the changes introduced to libpq by
allowing multiple hosts.
Ok. Patch applies cleanly, compiles & checks (although yet again the
feature is not tested).
Feature tested and works for me, although I'm not sure how the multi-host
warning about pgpassfile works, but I could not find a wrong warning.
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.
sh> PGPASSFILE=$HOME/.pgpass.test \
LD_LIBRARY_PATH=$PWD/src/interfaces/libpq \
psql -d "host=host1,host2,host3 user=test dbname=test"
psql: FATAL: password authentication failed for user "test"
password retrieved from file "$HOME/.pgpass.test"
- doc says "If multiple host names are specified, each will be tried in
turn in the order given.".
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.
On Fabien's recommendations, I've kept the variable dot_pgpassfile_used,
however I renamed that to pgpassfile_used, to keep a consistent naming
scheme.
Ok.
I'm still not sure about the amount of error messages produced by libpq,
I think it would be ideal to report an error while accessing a file
provided in the connection string, however I would not report that same
type of error when the .pgpass file has been tried to retrieve a
password. (Else, you'd get an error message on every connection string
that doesn't specify a pgpassfile or password, since .pgpass will be
checked every time, before prompting the user to input the password)
Yep. This issue is independent of your patch as it is already there.
There could be a boolean set when the pass file is explicitely provided
that could trigger a warning only in this case.
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?
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com
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
Attachments:
pgpassfile_v5.patchtext/x-patch; name=pgpassfile_v5.patchDownload
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? */
Hello Julian,
I've adressed those spacing errors.
Ok.
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.
Good.
Patch looks ok, applies, compiles & checks, and tested manually.
I've switch in the CF to "ready for committer", and we'll see what the
next level thinks about it:-)
[...] 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.
Sure. I complained about the fuzzy documentation & imprecise warning
message because I stumbled upon that while testing.
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.
Yes.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 29, 2016 at 2:53 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
Hello Julian,
I've adressed those spacing errors.
Ok.
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.Good.
Patch looks ok, applies, compiles & checks, and tested manually.
I've switch in the CF to "ready for committer", and we'll see what the
next level thinks about it:-)[...] 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.Sure. I complained about the fuzzy documentation & imprecise warning
message because I stumbled upon that while testing.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.Yes.
Moved to next commitfest with same status (ready for committer).
Regards,
Hari Babu
Fujitsu Australia
Fabien COELHO <coelho@cri.ensmp.fr> writes:
I've switch in the CF to "ready for committer", and we'll see what the
next level thinks about it:-)
Pushed with a few adjustments:
* It seemed to me that the appropriate parameter name is "passfile"
not "pgpassfile". In all but a couple of legacy cases, the environment
variable's name is the parameter name plus a PG prefix; therefore,
with the environment variable already named PGPASSFILE, we have to use
"passfile" for consistency. Putting a PG prefix on a connection parameter
name is rather pointless anyway, since there's no larger namespace that
it might have a conflict in.
* The error handling in the submitted patch was pretty shoddy; it didn't
check for malloc failure at all, and it didn't report a suitable error on
pqGetHomeDirectory failure either (which is worth doing, if it's going to
be a reason for connection failure). I ended up concluding that the
easiest way to fix that involved just inlining getPgPassFilename into the
one remaining call site.
* I also got rid of the assignment of DefaultPassword to conn->pgpass;
that wasn't doing anything very useful anymore. AFAICS, the only visible
effect was to make PQpass() return an empty string not NULL, but we can
make that happen without paying a malloc/free cycle for it.
* Minor comment and docs cleanups.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers