Thread

Warning about invalid .pgpass passwords

Bruce Momjian <bruce@momjian.us> — 2010-03-10T02:52:39Z

Bruce Momjian wrote:
> Tom Lane wrote:
> > Alvaro Herrera <alvherre@commandprompt.com> writes:
> > > It had to do with me having a bogus password in .pgpass (so psql was
> > > first trying empty password, then the one in .pgpass, and both failing).
> > > Pilot error.  However, I'd say that we ought to give a notice if the
> > > password in .pgpass fails.
> > 
> > Can we do something like
> > 	ERROR: password authentication failed (using password from .pgpass)
> > ie, just tack on a comment to the error message?
> 
> I looked into that but found it difficult to implement because only
> libpq knows about pgpass, while the message is printed by psql.

I just got confused for +10 minutes by an incorrect .pgpass password, so
I found new interest in improving this reported behavior.  ;-)

The attached patch reports the fact that .pgpass was used if the libpq
connection fails:

	$ psql -h localhost test
	psql: FATAL:  password authentication failed for user "postgres"
	(password retrieved from .pgpass)

I am not sure if I like the parentheses or not.  Ideally I would report
this only for password failures but that information is not passed back
from the server except as an error string.

I am thinking this could be in 9.0.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do

Re: Warning about invalid .pgpass passwords

Fujii Masao <masao.fujii@gmail.com> — 2010-03-10T03:31:56Z

On Wed, Mar 10, 2010 at 11:52 AM, Bruce Momjian <bruce@momjian.us> wrote:
> The attached patch reports the fact that .pgpass was used if the libpq
> connection fails:

+	/*
+	 *	If the connection failed, we should mention that
+	 *	we got the password from .pgpass in case that
+	 *	password is wrong.
+	 */
+	if (conn->used_dot_pgpass && conn->password_needed)
+		appendPQExpBufferStr(&conn->errorMessage,
+			libpq_gettext("(password retrieved from .pgpass)\n"));

I think that this should be in PQconnectPoll() rather than connectDBComplete().
Otherwise, only when we make a connection to the server in a nonblocking manner
(i.e., use PQconnectStart() and PQconnectPoll()), that HINT message is
not output.
This looks odd for me. Thought?

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Warning about invalid .pgpass passwords

Tom Lane <tgl@sss.pgh.pa.us> — 2010-03-10T03:39:27Z

Bruce Momjian <bruce@momjian.us> writes:
> The attached patch reports the fact that .pgpass was used if the libpq
> connection fails:

The test is in a very inappropriate place --- it will be missed by
several fully-supported code paths.

> I am not sure if I like the parentheses or not.

I don't like 'em.  If you don't have enough confidence in the message to
be replacing the normal error string, you probably shouldn't be doing
this at all.  We'll look silly if we attach such a comment to a message
that indicates a network failure, for example; and cases where the
comment is actively misleading would be even worse.

I'm inclined to think that maybe we should make the server return a
distinct SQLSTATE for "bad password", and have libpq check for that
rather than just assuming that the failure must be bad password.
The main argument against this is probably that it would tell a bad
guy that he's got a valid username but not a valid password.  Not
sure if that's a serious issue or not --- I seem to recall that we
are exposing validity of user names already (or was that database
names?).  It would also mean that the new message only triggers when
talking to a 9.0+ server, but since we've gotten along without it
till now, that aspect doesn't bother me at all.

A compromise would be to check for
ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION, which in combination
with the knowledge that we got asked for a password would give
fairly high confidence though not certainty that the problem is a bad
password.

			regards, tom lane

Re: Warning about invalid .pgpass passwords

Bruce Momjian <bruce@momjian.us> — 2010-03-11T01:01:05Z

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > The attached patch reports the fact that .pgpass was used if the libpq
> > connection fails:
> 
> The test is in a very inappropriate place --- it will be missed by
> several fully-supported code paths.

Agreed.  I moved it to the error return label ("error_return") in
PQconnectPoll(), and placed the code in a new function.

> > I am not sure if I like the parentheses or not.
> 
> I don't like 'em.  If you don't have enough confidence in the message to

OK, parentheses removed.

> be replacing the normal error string, you probably shouldn't be doing
> this at all.  We'll look silly if we attach such a comment to a message
> that indicates a network failure, for example; and cases where the
> comment is actively misleading would be even worse.
> 
> I'm inclined to think that maybe we should make the server return a
> distinct SQLSTATE for "bad password", and have libpq check for that
> rather than just assuming that the failure must be bad password.
> The main argument against this is probably that it would tell a bad
> guy that he's got a valid username but not a valid password.  Not
> sure if that's a serious issue or not --- I seem to recall that we
> are exposing validity of user names already (or was that database
> names?).  It would also mean that the new message only triggers when
> talking to a 9.0+ server, but since we've gotten along without it
> till now, that aspect doesn't bother me at all.

Modifying the backend to issue this hint seems like overkill, unless we
have some other use for it.

> A compromise would be to check for
> ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION, which in combination
> with the knowledge that we got asked for a password would give
> fairly high confidence though not certainty that the problem is a bad
> password.

I originally considered using the SQLSTATE but found few uses of it in
the frontend code.  However, I agree it is the proper solution so I now
coded it that way, including a loop to convert from the 6-bit sqlstate
to base-10.  (FYI, the same C file hardcodes ERRCODE_APPNAME_UNKNOWN as
a string for historical reasons, but I didn't do that.)

Updated patch attached.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do

Re: Warning about invalid .pgpass passwords

Tom Lane <tgl@sss.pgh.pa.us> — 2010-03-11T01:07:08Z

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> I'm inclined to think that maybe we should make the server return a
>> distinct SQLSTATE for "bad password", and have libpq check for that
>> rather than just assuming that the failure must be bad password.

> Modifying the backend to issue this hint seems like overkill, unless we
> have some other use for it.

I wouldn't suggest it if I thought it were only helpful for this
particular message.  It seems to me that we've spent a lot of time
kluging around the lack of certainty about whether a connection failure
is a password issue.  Admittedly a lot of that was between libpq and its
client, but the state of affairs on the wire isn't great either.

I'm not convinced we have to do it that way, but now is definitely
the time to think about it before we implement yet another
sort-of-good-enough kluge.  Which is what this is.

			regards, tom lane

Re: Warning about invalid .pgpass passwords

Bruce Momjian <bruce@momjian.us> — 2010-03-11T01:09:04Z

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> I'm inclined to think that maybe we should make the server return a
> >> distinct SQLSTATE for "bad password", and have libpq check for that
> >> rather than just assuming that the failure must be bad password.
> 
> > Modifying the backend to issue this hint seems like overkill, unless we
> > have some other use for it.
> 
> I wouldn't suggest it if I thought it were only helpful for this
> particular message.  It seems to me that we've spent a lot of time
> kluging around the lack of certainty about whether a connection failure
> is a password issue.  Admittedly a lot of that was between libpq and its
> client, but the state of affairs on the wire isn't great either.

Yes, I have seen that myself in psql.

> I'm not convinced we have to do it that way, but now is definitely
> the time to think about it before we implement yet another
> sort-of-good-enough kluge.  Which is what this is.

True.  Should we just hold this all for 9.1 or should I code it and
let's look at the size of the patch?

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do

Re: Warning about invalid .pgpass passwords

Bruce Momjian <bruce@momjian.us> — 2010-03-11T21:19:18Z

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce@momjian.us> writes:
> > > Tom Lane wrote:
> > >> I'm inclined to think that maybe we should make the server return a
> > >> distinct SQLSTATE for "bad password", and have libpq check for that
> > >> rather than just assuming that the failure must be bad password.
> > 
> > > Modifying the backend to issue this hint seems like overkill, unless we
> > > have some other use for it.
> > 
> > I wouldn't suggest it if I thought it were only helpful for this
> > particular message.  It seems to me that we've spent a lot of time
> > kluging around the lack of certainty about whether a connection failure
> > is a password issue.  Admittedly a lot of that was between libpq and its
> > client, but the state of affairs on the wire isn't great either.
> 
> Yes, I have seen that myself in psql.
> 
> > I'm not convinced we have to do it that way, but now is definitely
> > the time to think about it before we implement yet another
> > sort-of-good-enough kluge.  Which is what this is.
> 
> True.  Should we just hold this all for 9.1 or should I code it and
> let's look at the size of the patch?

With no one replying, I decide to code up a patch that adds a new
SQLSTATE (28001) to report invalid/missing passwords.  With this code,
the warning will only appear when connecting to 9.0 servers.  The output
still looks the same, but will only appear for a password failure:

	$ sql -h localhost test
	psql: FATAL:  password authentication failed for user "postgres"
	password retrieved from .pgpass

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do

Re: Warning about invalid .pgpass passwords

Alvaro Herrera <alvherre@commandprompt.com> — 2010-03-11T21:24:47Z

Bruce Momjian wrote:

> + static void
> + dot_pg_pass_warning(PGconn *conn)
> + {
> + 	/* If it was 'invalid authorization', add .pgpass mention */
> + 	if (conn->dot_pgpass_used && conn->password_needed && conn->result &&
> + 		/* only works with >= 9.0 servers */
> + 		atoi(PQresultErrorField(conn->result, PG_DIAG_SQLSTATE)) ==
> + 			SQLSTATE_TO_BASE10(ERRCODE_INVALID_PASSWORD_SPECIFICATION))
> + 		appendPQExpBufferStr(&conn->errorMessage,
> + 			libpq_gettext("password retrieved from .pgpass\n"));
> + }

This bit seems strange ...  I think we just do strcmp() to compare sqlstates?

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Re: Warning about invalid .pgpass passwords

Bruce Momjian <bruce@momjian.us> — 2010-03-11T21:35:40Z

Alvaro Herrera wrote:
> Bruce Momjian wrote:
> 
> > + static void
> > + dot_pg_pass_warning(PGconn *conn)
> > + {
> > + 	/* If it was 'invalid authorization', add .pgpass mention */
> > + 	if (conn->dot_pgpass_used && conn->password_needed && conn->result &&
> > + 		/* only works with >= 9.0 servers */
> > + 		atoi(PQresultErrorField(conn->result, PG_DIAG_SQLSTATE)) ==
> > + 			SQLSTATE_TO_BASE10(ERRCODE_INVALID_PASSWORD_SPECIFICATION))
> > + 		appendPQExpBufferStr(&conn->errorMessage,
> > + 			libpq_gettext("password retrieved from .pgpass\n"));
> > + }
> 
> This bit seems strange ...  I think we just do strcmp() to compare sqlstates?

Uh, the PQresultErrorField is a string, while
ERRCODE_INVALID_PASSWORD_SPECIFICATI is a 4-byte value in base-64.  
Yea, it's true.  For excitement, see the MAKE_SQLSTATE macro.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do

Re: Warning about invalid .pgpass passwords

Tom Lane <tgl@sss.pgh.pa.us> — 2010-03-11T21:55:22Z

Bruce Momjian <bruce@momjian.us> writes:
> Alvaro Herrera wrote:
>> This bit seems strange ...  I think we just do strcmp() to compare sqlstates?

> Uh, the PQresultErrorField is a string, while
> ERRCODE_INVALID_PASSWORD_SPECIFICATI is a 4-byte value in base-64.  
> Yea, it's true.  For excitement, see the MAKE_SQLSTATE macro.

I don't particularly believe in doing things this way: I think that
libpq should just hardwire the string the same as it's doing for the
other specific sqlstate it's checking for.  If we do this at all,
then we are effectively embedding that sqlstate value in the protocol.
We are not going to be able to change it later.  Doing it like this
opens us up to randomly using errcodes in the frontend without realizing
that we've thereby lost the freedom to change them.

Even if you insist on including errcodes.h into the frontend code
despite that, this is an ugly brute-force way of solving the problem.
The intended way of solving the problem was to redefine MAKE_SQLSTATE
before including the file, in .c files where you need a different
representation.

			regards, tom lane

Re: Warning about invalid .pgpass passwords

Bruce Momjian <bruce@momjian.us> — 2010-03-11T22:42:56Z

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Alvaro Herrera wrote:
> >> This bit seems strange ...  I think we just do strcmp() to compare sqlstates?
> 
> > Uh, the PQresultErrorField is a string, while
> > ERRCODE_INVALID_PASSWORD_SPECIFICATION is a 4-byte value in base-64.  
> > Yea, it's true.  For excitement, see the MAKE_SQLSTATE macro.
> 
> I don't particularly believe in doing things this way: I think that
> libpq should just hardwire the string the same as it's doing for the
> other specific sqlstate it's checking for.  If we do this at all,
> then we are effectively embedding that sqlstate value in the protocol.
> We are not going to be able to change it later.  Doing it like this
> opens us up to randomly using errcodes in the frontend without realizing
> that we've thereby lost the freedom to change them.
> 
> Even if you insist on including errcodes.h into the frontend code
> despite that, this is an ugly brute-force way of solving the problem.
> The intended way of solving the problem was to redefine MAKE_SQLSTATE
> before including the file, in .c files where you need a different
> representation.

OK, just defined it as a constant string. I am not a big fan of
redefining macros, especially ones that are referenced in later include
files.

Is this going to cause problems for client applications that only test
for ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION?  I doubt many of them
are testing for just the first two digits.

Is there anywhere else we should be testing for this new sqlstate value?

Updated patch attached.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do

Re: Warning about invalid .pgpass passwords

Tom Lane <tgl@sss.pgh.pa.us> — 2010-03-11T23:38:23Z

Bruce Momjian <bruce@momjian.us> writes:
>>>> ERRCODE_INVALID_PASSWORD_SPECIFICATION

BTW, why not just "ERRCODE_INVALID_PASSWORD"?  The extra word doesn't
seem to promote anything except carpal tunnel syndrome.

			regards, tom lane

Re: Warning about invalid .pgpass passwords

Tom Lane <tgl@sss.pgh.pa.us> — 2010-03-11T23:40:12Z

Bruce Momjian <bruce@momjian.us> writes:
> + #define ERRCODE_INVALID_PASSWORD_SPECIFICATION MAKE_SQLSTATE('2','8', '0','0','1')

Oh, another thought: you're infringing on SQL-committee-controlled code
space there.  Probably 28P01 would be a safer choice.

			regards, tom lane

Re: Warning about invalid .pgpass passwords

Bruce Momjian <bruce@momjian.us> — 2010-03-12T00:42:33Z

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > + #define ERRCODE_INVALID_PASSWORD_SPECIFICATION MAKE_SQLSTATE('2','8', '0','0','1')
> 
> Oh, another thought: you're infringing on SQL-committee-controlled code
> space there.  Probably 28P01 would be a safer choice.

OK, updated patch with code "28P01", and I shorted the name (now
ERRCODE_INVALID_PASSWORD).

I thought carpal tunnel was the goal of these error codes.  ;-)

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do

Re: Warning about invalid .pgpass passwords

Robert Haas <robertmhaas@gmail.com> — 2010-03-12T02:51:08Z

On Thu, Mar 11, 2010 at 4:19 PM, Bruce Momjian <bruce@momjian.us> wrote:
>        $ sql -h localhost test
>        psql: FATAL:  password authentication failed for user "postgres"
>        password retrieved from .pgpass

I find this not quite explicit enough for my taste.  Maybe something like this?

the failing password was retrieved from .pgpass

...Robert

Re: Warning about invalid .pgpass passwords

Bruce Momjian <bruce@momjian.us> — 2010-03-12T03:00:23Z

Robert Haas wrote:
> On Thu, Mar 11, 2010 at 4:19 PM, Bruce Momjian <bruce@momjian.us> wrote:
> > ? ? ? ?$ sql -h localhost test
> > ? ? ? ?psql: FATAL: ?password authentication failed for user "postgres"
> > ? ? ? ?password retrieved from .pgpass
> 
> I find this not quite explicit enough for my taste.  Maybe something like this?
> 
> the failing password was retrieved from .pgpass

Uh, not sure.  That doesn't match the style of many of our other
messages.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do

Re: Warning about invalid .pgpass passwords

Andrew Dunstan <andrew@dunslane.net> — 2010-03-12T03:39:35Z

Bruce Momjian wrote:
> + 	/* If it was 'invalid authorization', add .pgpass mention */
> + 	if (conn->dot_pgpass_used && conn->password_needed && conn->result &&
> + 		/* only works with >= 9.0 servers */
> + 		strcmp(PQresultErrorField(conn->result, PG_DIAG_SQLSTATE),
> + 			ERRCODE_INVALID_PASSWORD_SPECIFICATION) == 0)
> + 		appendPQExpBufferStr(&conn->errorMessage,
> + 			libpq_gettext("password retrieved from .pgpass\n"));
>   

Surely we should use the name of the actual file from which the password 
was retrieved here, which could be quite different from ".pgpass" (see 
PGPASSFILE environment setting) and is different by default on Windows 
anyway. Using a hardcoded ".pgpass" in those situations could be quite 
confusing.

cheers

andrew

Re: Warning about invalid .pgpass passwords

Bruce Momjian <bruce@momjian.us> — 2010-03-12T16:54:29Z

Andrew Dunstan wrote:
> 
> 
> Bruce Momjian wrote:
> > + 	/* If it was 'invalid authorization', add .pgpass mention */
> > + 	if (conn->dot_pgpass_used && conn->password_needed && conn->result &&
> > + 		/* only works with >= 9.0 servers */
> > + 		strcmp(PQresultErrorField(conn->result, PG_DIAG_SQLSTATE),
> > + 			ERRCODE_INVALID_PASSWORD_SPECIFICATION) == 0)
> > + 		appendPQExpBufferStr(&conn->errorMessage,
> > + 			libpq_gettext("password retrieved from .pgpass\n"));
> >   
> 
> Surely we should use the name of the actual file from which the password 
> was retrieved here, which could be quite different from ".pgpass" (see 
> PGPASSFILE environment setting) and is different by default on Windows 
> anyway. Using a hardcoded ".pgpass" in those situations could be quite 
> confusing.

Agreed, very good idea.  Update patch attached.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do

Re: Warning about invalid .pgpass passwords

Bruce Momjian <bruce@momjian.us> — 2010-03-13T14:56:18Z

Applied.

---------------------------------------------------------------------------

Bruce Momjian wrote:
> Andrew Dunstan wrote:
> > 
> > 
> > Bruce Momjian wrote:
> > > + 	/* If it was 'invalid authorization', add .pgpass mention */
> > > + 	if (conn->dot_pgpass_used && conn->password_needed && conn->result &&
> > > + 		/* only works with >= 9.0 servers */
> > > + 		strcmp(PQresultErrorField(conn->result, PG_DIAG_SQLSTATE),
> > > + 			ERRCODE_INVALID_PASSWORD_SPECIFICATION) == 0)
> > > + 		appendPQExpBufferStr(&conn->errorMessage,
> > > + 			libpq_gettext("password retrieved from .pgpass\n"));
> > >   
> > 
> > Surely we should use the name of the actual file from which the password 
> > was retrieved here, which could be quite different from ".pgpass" (see 
> > PGPASSFILE environment setting) and is different by default on Windows 
> > anyway. Using a hardcoded ".pgpass" in those situations could be quite 
> > confusing.
> 
> Agreed, very good idea.  Update patch attached.
> 
> -- 
>   Bruce Momjian  <bruce@momjian.us>        http://momjian.us
>   EnterpriseDB                             http://enterprisedb.com
> 
>   PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do

[ text/x-diff is unsupported, treating like TEXT/PLAIN ]

> Index: doc/src/sgml/errcodes.sgml
> ===================================================================
> RCS file: /cvsroot/pgsql/doc/src/sgml/errcodes.sgml,v
> retrieving revision 1.28
> diff -c -c -r1.28 errcodes.sgml
> *** doc/src/sgml/errcodes.sgml	7 Dec 2009 05:22:21 -0000	1.28
> --- doc/src/sgml/errcodes.sgml	12 Mar 2010 16:53:22 -0000
> ***************
> *** 761,766 ****
> --- 761,772 ----
>   <entry>invalid_authorization_specification</entry>
>   </row>
>   
> + <row>
> + <entry><literal>28P01</literal></entry>
> + <entry>INVALID PASSWORD</entry>
> + <entry>invalid_password</entry>
> + </row>
> + 
>   
>   <row>
>   <entry spanname="span13"><emphasis role="bold">Class 2B &mdash; Dependent Privilege Descriptors Still Exist</></entry>
> Index: src/backend/libpq/auth.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/backend/libpq/auth.c,v
> retrieving revision 1.195
> diff -c -c -r1.195 auth.c
> *** src/backend/libpq/auth.c	26 Feb 2010 02:00:42 -0000	1.195
> --- src/backend/libpq/auth.c	12 Mar 2010 16:53:24 -0000
> ***************
> *** 232,238 ****
>   auth_failed(Port *port, int status)
>   {
>   	const char *errstr;
> ! 
>   	/*
>   	 * If we failed due to EOF from client, just quit; there's no point in
>   	 * trying to send a message to the client, and not much point in logging
> --- 232,239 ----
>   auth_failed(Port *port, int status)
>   {
>   	const char *errstr;
> ! 	int		errcode_return = ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION;
> ! 	
>   	/*
>   	 * If we failed due to EOF from client, just quit; there's no point in
>   	 * trying to send a message to the client, and not much point in logging
> ***************
> *** 269,274 ****
> --- 270,277 ----
>   		case uaMD5:
>   		case uaPassword:
>   			errstr = gettext_noop("password authentication failed for user \"%s\"");
> + 			/* We use it to indicate if a .pgpass password failed. */
> + 			errcode_return = ERRCODE_INVALID_PASSWORD;
>   			break;
>   		case uaPAM:
>   			errstr = gettext_noop("PAM authentication failed for user \"%s\"");
> ***************
> *** 285,291 ****
>   	}
>   
>   	ereport(FATAL,
> ! 			(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
>   			 errmsg(errstr, port->user_name)));
>   	/* doesn't return */
>   }
> --- 288,294 ----
>   	}
>   
>   	ereport(FATAL,
> ! 			(errcode(errcode_return),
>   			 errmsg(errstr, port->user_name)));
>   	/* doesn't return */
>   }
> Index: src/include/utils/errcodes.h
> ===================================================================
> RCS file: /cvsroot/pgsql/src/include/utils/errcodes.h,v
> retrieving revision 1.31
> diff -c -c -r1.31 errcodes.h
> *** src/include/utils/errcodes.h	2 Jan 2010 16:58:10 -0000	1.31
> --- src/include/utils/errcodes.h	12 Mar 2010 16:53:24 -0000
> ***************
> *** 194,199 ****
> --- 194,200 ----
>   
>   /* Class 28 - Invalid Authorization Specification */
>   #define ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION MAKE_SQLSTATE('2','8', '0','0','0')
> + #define ERRCODE_INVALID_PASSWORD MAKE_SQLSTATE('2','8', 'P','0','1')
>   
>   /* Class 2B - Dependent Privilege Descriptors Still Exist */
>   #define ERRCODE_DEPENDENT_PRIVILEGE_DESCRIPTORS_STILL_EXIST		MAKE_SQLSTATE('2','B', '0','0','0')
> Index: src/interfaces/libpq/fe-connect.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
> retrieving revision 1.389
> diff -c -c -r1.389 fe-connect.c
> *** src/interfaces/libpq/fe-connect.c	3 Mar 2010 20:31:09 -0000	1.389
> --- src/interfaces/libpq/fe-connect.c	12 Mar 2010 16:53:25 -0000
> ***************
> *** 91,96 ****
> --- 91,99 ----
>    */
>   #define ERRCODE_APPNAME_UNKNOWN "42704"
>   
> + /* This is part of the protocol so just define it */
> + #define ERRCODE_INVALID_PASSWORD "28P01"
> + 
>   /*
>    * fall back options if they are not specified by arguments or defined
>    * by environment variables
> ***************
> *** 284,289 ****
> --- 287,294 ----
>   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 void default_threadlock(int acquire);
>   
>   
> ***************
> *** 652,657 ****
> --- 657,664 ----
>   										conn->dbName, conn->pguser);
>   		if (conn->pgpass == NULL)
>   			conn->pgpass = strdup(DefaultPassword);
> + 		else
> + 			conn->dot_pgpass_used = true;
>   	}
>   
>   	/*
> ***************
> *** 2133,2138 ****
> --- 2140,2147 ----
>   
>   error_return:
>   
> + 	dot_pg_pass_warning(conn);
> + 	
>   	/*
>   	 * We used to close the socket at this point, but that makes it awkward
>   	 * for those above us if they wish to remove this socket from their own
> ***************
> *** 2191,2196 ****
> --- 2200,2206 ----
>   	conn->verbosity = PQERRORS_DEFAULT;
>   	conn->sock = -1;
>   	conn->password_needed = false;
> + 	conn->dot_pgpass_used = false;
>   #ifdef USE_SSL
>   	conn->allow_ssl_try = true;
>   	conn->wait_ssl_try = false;
> ***************
> *** 4323,4329 ****
>   	FILE	   *fp;
>   	char		pgpassfile[MAXPGPATH];
>   	struct stat stat_buf;
> - 	char	   *passfile_env;
>   
>   #define LINELEN NAMEDATALEN*5
>   	char		buf[LINELEN];
> --- 4333,4338 ----
> ***************
> *** 4349,4365 ****
>   	if (port == NULL)
>   		port = DEF_PGPORT_STR;
>   
> ! 	if ((passfile_env = getenv("PGPASSFILE")) != NULL)
> ! 		/* use the literal path from the environment, if set */
> ! 		strlcpy(pgpassfile, passfile_env, sizeof(pgpassfile));
> ! 	else
> ! 	{
> ! 		char		homedir[MAXPGPATH];
> ! 
> ! 		if (!pqGetHomeDirectory(homedir, sizeof(homedir)))
> ! 			return NULL;
> ! 		snprintf(pgpassfile, MAXPGPATH, "%s/%s", homedir, PGPASSFILE);
> ! 	}
>   
>   	/* If password file cannot be opened, ignore it. */
>   	if (stat(pgpassfile, &stat_buf) != 0)
> --- 4358,4365 ----
>   	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)
> ***************
> *** 4426,4431 ****
> --- 4426,4476 ----
>   #undef LINELEN
>   }
>   
> + 
> + static bool getPgPassFilename(char *pgpassfile)
> + {
> + 	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);
> + 	}
> + 	return true;
> + }
> + 
> + /*
> +  *	If the connection failed, we should mention if
> +  *	we got the password from .pgpass in case that
> +  *	password is wrong.
> +  */
> + static void
> + dot_pg_pass_warning(PGconn *conn)
> + {
> + 	/* If it was 'invalid authorization', add .pgpass mention */
> + 	if (conn->dot_pgpass_used && conn->password_needed && conn->result &&
> + 		/* only works with >= 9.0 servers */
> + 		strcmp(PQresultErrorField(conn->result, PG_DIAG_SQLSTATE),
> + 			ERRCODE_INVALID_PASSWORD) == 0)
> + 	{
> + 		char		pgpassfile[MAXPGPATH];
> + 
> + 		if (!getPgPassFilename(pgpassfile))
> + 			return;
> + 		appendPQExpBufferStr(&conn->errorMessage,
> + 			libpq_gettext("password retrieved from "));
> + 		appendPQExpBufferStr(&conn->errorMessage, pgpassfile);
> + 		appendPQExpBufferChar(&conn->errorMessage, '\n');
> + 	}
> + }
> + 
> + 	
>   /*
>    * Obtain user's home directory, return in given buffer
>    *
> Index: src/interfaces/libpq/libpq-int.h
> ===================================================================
> RCS file: /cvsroot/pgsql/src/interfaces/libpq/libpq-int.h,v
> retrieving revision 1.149
> diff -c -c -r1.149 libpq-int.h
> *** src/interfaces/libpq/libpq-int.h	26 Feb 2010 02:01:33 -0000	1.149
> --- src/interfaces/libpq/libpq-int.h	12 Mar 2010 16:53:25 -0000
> ***************
> *** 343,348 ****
> --- 343,349 ----
>   	ProtocolVersion pversion;	/* FE/BE protocol version in use */
>   	int			sversion;		/* server version, e.g. 70401 for 7.4.1 */
>   	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? */
>   
> Index: src/pl/plpgsql/src/plerrcodes.h
> ===================================================================
> RCS file: /cvsroot/pgsql/src/pl/plpgsql/src/plerrcodes.h,v
> retrieving revision 1.20
> diff -c -c -r1.20 plerrcodes.h
> *** src/pl/plpgsql/src/plerrcodes.h	2 Jan 2010 16:58:13 -0000	1.20
> --- src/pl/plpgsql/src/plerrcodes.h	12 Mar 2010 16:53:25 -0000
> ***************
> *** 368,373 ****
> --- 368,377 ----
>   },
>   
>   {
> + 	"invalid_password", ERRCODE_INVALID_PASSWORD
> + },
> + 
> + {
>   	"dependent_privilege_descriptors_still_exist", ERRCODE_DEPENDENT_PRIVILEGE_DESCRIPTORS_STILL_EXIST
>   },
>   

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

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do