Обсуждение: Warning about invalid .pgpass passwords
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
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 10 Mar 2010 02:37:20 -0000
***************
*** 652,657 ****
--- 652,659 ----
conn->dbName, conn->pguser);
if (conn->pgpass == NULL)
conn->pgpass = strdup(DefaultPassword);
+ else
+ conn->used_dot_pgpass = true;
}
/*
***************
*** 1144,1149 ****
--- 1146,1161 ----
default:
/* Just in case we failed to set it in PQconnectPoll */
conn->status = CONNECTION_BAD;
+
+ /*
+ * 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"));
+
return 0;
}
***************
*** 2191,2196 ****
--- 2203,2209 ----
conn->verbosity = PQERRORS_DEFAULT;
conn->sock = -1;
conn->password_needed = false;
+ conn->used_dot_pgpass = false;
#ifdef USE_SSL
conn->allow_ssl_try = true;
conn->wait_ssl_try = false;
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 10 Mar 2010 02:37:20 -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 used_dot_pgpass; /* true if used .pgpass */
bool sigpipe_so; /* have we masked SIGPIPE via SO_NOSIGPIPE? */
bool sigpipe_flag; /* can we mask SIGPIPE via MSG_NOSIGNAL? */
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
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
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
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 11 Mar 2010 00:53:00 -0000
***************
*** 71,76 ****
--- 71,78 ----
#include "libpq/ip.h"
#include "mb/pg_wchar.h"
+ #include "utils/elog.h"
+ #include "utils/errcodes.h"
#ifndef FD_CLOEXEC
#define FD_CLOEXEC 1
***************
*** 284,289 ****
--- 286,292 ----
static char *pwdfMatchesString(char *buf, char *token);
static char *PasswordFromFile(char *hostname, char *port, char *dbname,
char *username);
+ static void dot_pg_pass_warning(PGconn *conn);
static void default_threadlock(int acquire);
***************
*** 652,657 ****
--- 655,662 ----
conn->dbName, conn->pguser);
if (conn->pgpass == NULL)
conn->pgpass = strdup(DefaultPassword);
+ else
+ conn->dot_pgpass_used = true;
}
/*
***************
*** 2133,2138 ****
--- 2138,2145 ----
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 ****
--- 2198,2204 ----
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;
***************
*** 4426,4431 ****
--- 4434,4471 ----
#undef LINELEN
}
+
+ /*
+ * 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)
+ {
+ int invalid_authorization_specification_integer = 0;
+ int i;
+
+ if (!conn->dot_pgpass_used || !conn->password_needed)
+ return; /* quick exit */
+
+ /* Convert 6-bit packed sqlstate to a base-10 integer */
+ for (i = 0; i < 5; i++)
+ {
+ invalid_authorization_specification_integer *= 10;
+ invalid_authorization_specification_integer +=
+ (ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION >> (i * 6)) & 0x3F;
+ }
+
+ /* If it was 'invalid authorization', add .pgpass mention */
+ if (conn->result &&
+ atoi(PQresultErrorField(conn->result, PG_DIAG_SQLSTATE)) ==
+ invalid_authorization_specification_integer)
+ appendPQExpBufferStr(&conn->errorMessage,
+ libpq_gettext("password retrieved from .pgpass\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 11 Mar 2010 00:53:00 -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? */
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
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
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
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 11 Mar 2010 21:07:09 -0000
***************
*** 761,766 ****
--- 761,772 ----
<entry>invalid_authorization_specification</entry>
</row>
+ <row>
+ <entry><literal>28001</literal></entry>
+ <entry>INVALID PASSWORD SPECIFICATION</entry>
+ <entry>invalid_password_specification</entry>
+ </row>
+
<row>
<entry spanname="span13"><emphasis role="bold">Class 2B — 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 11 Mar 2010 21:07:16 -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,280 ----
case uaMD5:
case uaPassword:
errstr = gettext_noop("password authentication failed for user \"%s\"");
+ /*
+ * This might require the client to prompt for a password, so we use
+ * a distinct error code.
+ */
+ errcode_return = ERRCODE_INVALID_PASSWORD_SPECIFICATION;
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 */
}
--- 291,297 ----
}
ereport(FATAL,
! (errcode(errcode_return),
errmsg(errstr, port->user_name)));
/* doesn't return */
}
Index: src/include/utils/elog.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/utils/elog.h,v
retrieving revision 1.102
diff -c -c -r1.102 elog.h
*** src/include/utils/elog.h 2 Jan 2010 16:58:10 -0000 1.102
--- src/include/utils/elog.h 11 Mar 2010 21:07:16 -0000
***************
*** 61,66 ****
--- 61,73 ----
(PGSIXBIT(ch1) + (PGSIXBIT(ch2) << 6) + (PGSIXBIT(ch3) << 12) + \
(PGSIXBIT(ch4) << 18) + (PGSIXBIT(ch5) << 24))
+ #define SQLSTATE_TO_BASE10(state) \
+ (((state) & 0x3F) * 10000 + \
+ (((state) >> 6) & 0x3F) * 1000 + \
+ (((state) >> 12) & 0x3F) * 100 + \
+ (((state) >> 18) & 0x3F) * 10 + \
+ (((state) >> 24) & 0x3F))
+
/* These macros depend on the fact that '0' becomes a zero in SIXBIT */
#define ERRCODE_TO_CATEGORY(ec) ((ec) & ((1 << 12) - 1))
#define ERRCODE_IS_CATEGORY(ec) (((ec) & ~((1 << 12) - 1)) == 0)
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 11 Mar 2010 21:07:16 -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_SPECIFICATION MAKE_SQLSTATE('2','8', '0','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 11 Mar 2010 21:07:17 -0000
***************
*** 71,76 ****
--- 71,78 ----
#include "libpq/ip.h"
#include "mb/pg_wchar.h"
+ #include "utils/elog.h"
+ #include "utils/errcodes.h"
#ifndef FD_CLOEXEC
#define FD_CLOEXEC 1
***************
*** 284,289 ****
--- 286,292 ----
static char *pwdfMatchesString(char *buf, char *token);
static char *PasswordFromFile(char *hostname, char *port, char *dbname,
char *username);
+ static void dot_pg_pass_warning(PGconn *conn);
static void default_threadlock(int acquire);
***************
*** 652,657 ****
--- 655,662 ----
conn->dbName, conn->pguser);
if (conn->pgpass == NULL)
conn->pgpass = strdup(DefaultPassword);
+ else
+ conn->dot_pgpass_used = true;
}
/*
***************
*** 2133,2138 ****
--- 2138,2145 ----
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 ****
--- 2198,2204 ----
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;
***************
*** 4426,4431 ****
--- 4434,4458 ----
#undef LINELEN
}
+
+ /*
+ * 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 */
+ atoi(PQresultErrorField(conn->result, PG_DIAG_SQLSTATE)) ==
+ SQLSTATE_TO_BASE10(ERRCODE_INVALID_PASSWORD_SPECIFICATION))
+ appendPQExpBufferStr(&conn->errorMessage,
+ libpq_gettext("password retrieved from .pgpass\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 11 Mar 2010 21:07:19 -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 11 Mar 2010 21:07:19 -0000
***************
*** 368,373 ****
--- 368,377 ----
},
{
+ "invalid_password_specification", ERRCODE_INVALID_PASSWORD_SPECIFICATION
+ },
+
+ {
"dependent_privilege_descriptors_still_exist", ERRCODE_DEPENDENT_PRIVILEGE_DESCRIPTORS_STILL_EXIST
},
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
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
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
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 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 11 Mar 2010 22:40:24 -0000 *************** *** 761,766 **** --- 761,772 ---- <entry>invalid_authorization_specification</entry> </row> + <row> + <entry><literal>28001</literal></entry> + <entry>INVALID PASSWORD SPECIFICATION</entry> + <entry>invalid_password_specification</entry> + </row> + <row> <entry spanname="span13"><emphasis role="bold">Class 2B — 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 11 Mar 2010 22:40: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_SPECIFICATION; 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 11 Mar 2010 22:40:25 -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_SPECIFICATION MAKE_SQLSTATE('2','8', '0','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 11 Mar 2010 22:40: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_SPECIFICATION "28001" + /* * fall back options if they are not specified by arguments or defined * by environment variables *************** *** 284,289 **** --- 287,293 ---- static char *pwdfMatchesString(char *buf, char *token); static char *PasswordFromFile(char *hostname, char *port, char *dbname, char *username); + static void dot_pg_pass_warning(PGconn *conn); static void default_threadlock(int acquire); *************** *** 652,657 **** --- 656,663 ---- conn->dbName, conn->pguser); if (conn->pgpass == NULL) conn->pgpass = strdup(DefaultPassword); + else + conn->dot_pgpass_used = true; } /* *************** *** 2133,2138 **** --- 2139,2146 ---- 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 **** --- 2199,2205 ---- 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; *************** *** 4426,4431 **** --- 4435,4459 ---- #undef LINELEN } + + /* + * 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_SPECIFICATION) == 0) + appendPQExpBufferStr(&conn->errorMessage, + libpq_gettext("password retrieved from .pgpass\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 11 Mar 2010 22:40: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 11 Mar 2010 22:40:25 -0000 *************** *** 368,373 **** --- 368,377 ---- }, { + "invalid_password_specification", ERRCODE_INVALID_PASSWORD_SPECIFICATION + }, + + { "dependent_privilege_descriptors_still_exist", ERRCODE_DEPENDENT_PRIVILEGE_DESCRIPTORS_STILL_EXIST },
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
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
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
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 00:37:47 -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 — 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 00:37:49 -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 00:37:50 -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 00:37:51 -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,293 ----
static char *pwdfMatchesString(char *buf, char *token);
static char *PasswordFromFile(char *hostname, char *port, char *dbname,
char *username);
+ static void dot_pg_pass_warning(PGconn *conn);
static void default_threadlock(int acquire);
***************
*** 652,657 ****
--- 656,663 ----
conn->dbName, conn->pguser);
if (conn->pgpass == NULL)
conn->pgpass = strdup(DefaultPassword);
+ else
+ conn->dot_pgpass_used = true;
}
/*
***************
*** 2133,2138 ****
--- 2139,2146 ----
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 ****
--- 2199,2205 ----
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;
***************
*** 4426,4431 ****
--- 4435,4459 ----
#undef LINELEN
}
+
+ /*
+ * 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)
+ appendPQExpBufferStr(&conn->errorMessage,
+ libpq_gettext("password retrieved from .pgpass\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 00:37:51 -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 00:37:51 -0000
***************
*** 368,373 ****
--- 368,377 ----
},
{
+ "invalid_password", ERRCODE_INVALID_PASSWORD
+ },
+
+ {
"dependent_privilege_descriptors_still_exist", ERRCODE_DEPENDENT_PRIVILEGE_DESCRIPTORS_STILL_EXIST
},
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
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
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
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
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 — 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
},
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 — 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