Re: [HACKERS] [PATCH] pgpassfile connection option

[Tom Lane]

Fabien COELHO  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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Haribabu Kommi]

On Tue, Nov 29, 2016 at 2:53 AM, Fabien COELHO  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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Fabien COELHO]

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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Julian Markwort]

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 host
 is not the name of the server at network address hostaddr.
 Also, note that host rather than hostaddr
-is used to identify the connection in ~/.pgpass (see
+is used to identify the connection in a password file (see
 ).

 
@@ -1002,6 +1002,16 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
  
 
+ 
+  pgpassfile
+  
+  
+Specifies the name of the file used to lookup passwords.
+Defaults to the password file (see ).
+  
+  
+ 
+
  
   connect_timeout
   
@@ -6886,9 +6896,8 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
   
PGPASSFILE
   
-  PGPASSFILE specifies the name of the password file to
-  use for lookups.  If not set, it defaults to ~/.pgpass
-  (see ).
+  PGPASSFILE behaves the same as the  connection parameter.
  
 
 
@@ -7160,13 +7169,15 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
   
 
   
-   The file .pgpass in a user's home directory or the
-   file referenced by PGPASSFILE can contain passwords to
+   The file .pgpass 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
%APPDATA%\postgresql\pgpass.conf (where
%APPDATA% refers to the Application Data subdirectory in
the user's profile).
+   Alternatively, a password file can be specified
+   using the connection parameter 
+   or the environment variable PGPASSFILE.
   
 
   
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 

Re: [HACKERS] [PATCH] pgpassfile connection option

[Mithun Cy]

 > 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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Fabien COELHO]

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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Stephen Frost]

All,

* Robert Haas (robertmh...@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


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] pgpassfile connection option

[Julian Markwort]

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

pgpassfile_v4.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] pgpassfile connection option

[Julian Markwort]

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.

Re: [HACKERS] [PATCH] pgpassfile connection option

[Fabien COELHO]

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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Julian Markwort]

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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Julian Markwort]

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
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 host
 is not the name of the server at network address hostaddr.
 Also, note that host rather than hostaddr
-is used to identify the connection in ~/.pgpass (see
+is used to identify the connection in a password file (see
 ).

 
@@ -986,6 +986,16 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
  
 
+ 
+  pgpassfile
+  
+  
+Specifies the name of the file used to lookup passwords.
+Defaults to the password file (see ).
+  
+  
+ 
+
  
   connect_timeout
   
@@ -6862,9 +6872,8 @@ myEventProc(PGEventId evtId, void *evtInfo, void 
*passThrough)
   
PGPASSFILE
   
-  PGPASSFILE specifies the name of the password file to
-  use for lookups.  If not set, it defaults to ~/.pgpass
-  (see ).
+  PGPASSFILE behaves the same as the  connection parameter.
  
 
 
@@ -7136,13 +7145,15 @@ myEventProc(PGEventId evtId, void *evtInfo, void 
*passThrough)
   
 
   
-   The file .pgpass in a user's home directory or the
-   file referenced by PGPASSFILE can contain passwords to
+   The file .pgpass 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
%APPDATA%\postgresql\pgpass.conf (where
%APPDATA% refers to the Application Data subdirectory in
the user's profile).
+   Alternatively, a password file can be specified
+   using the connection parameter 
+   or the environment variable PGPASSFILE.
   
 
   
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);
 

Re: [HACKERS] [PATCH] pgpassfile connection option

[Fabien COELHO]

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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Julian Markwort]

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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Fabien COELHO]

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) 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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Julian Markwort]

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) 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

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 host
 is not the name of the server at network address hostaddr.
 Also, note that host rather than hostaddr
-is used to identify the connection in ~/.pgpass (see
+is used to identify the connection in a password file (see
 ).

 
@@ -986,6 +986,16 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
  
 
+ 
+  pgpassfile
+  
+  
+Specifies the name of the file used to lookup passwords.
+Defaults to the password file (see ).
+  
+  
+ 
+
  
   connect_timeout
   
@@ -6862,9 +6872,8 @@ myEventProc(PGEventId evtId, void *evtInfo, void 
*passThrough)
   
PGPASSFILE
   
-  PGPASSFILE specifies the name of the password file to
-  use for lookups.  If not set, it defaults to ~/.pgpass
-  (see ).
+  PGPASSFILE behaves the same as the  connection parameter.
  
 
 
@@ -7136,13 +7145,15 @@ myEventProc(PGEventId evtId, void *evtInfo, void 
*passThrough)
   
 
   
-   The file .pgpass in a user's home directory or the
-   file referenced by PGPASSFILE can contain passwords to
+   The file .pgpass 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
%APPDATA%\postgresql\pgpass.conf (where
%APPDATA% refers to the Application Data subdirectory in
the user's profile).
+   Alternatively, a password file can be specified
+   using the connection parameter 
+   or the environment variable PGPASSFILE.
   
 
   
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 

Re: [HACKERS] [PATCH] pgpassfile connection option

[Robert Haas]

On Tue, Oct 11, 2016 at 5:06 PM, Oskari Saarenmaa  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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Fabien COELHO]

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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Oskari Saarenmaa]

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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Robert Haas]

On Tue, Oct 4, 2016 at 7:42 AM, Julian Markwort
 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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Julian Markwort]

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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Robert Haas]

On Thu, Sep 22, 2016 at 11:34 AM, Julian Markwort
 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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Julian Markwort]

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

Re: [HACKERS] [PATCH] pgpassfile connection option

[Andrew Dunstan]

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

[HACKERS] [PATCH] pgpassfile connection option

[Julian Markwort]

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.markw...@uni-muenster.de

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, _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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription: