Re: Warning about invalid .pgpass passwords

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? */