Patch for PQconnectionUsedPassword brain-damage - Mailing list pgsql-patches

Per the thread here,
http://archives.postgresql.org/pgsql-hackers/2007-12/msg00174.php
the 8.3 patch to create "PQconnectionUsedPassword" is quite a few
bricks shy of a load --- the aforesaid function, which was intended
to serve two different purposes, fails to serve either one correctly.

Attached is my proposed patch to fix this, per the thread discussion.
But given that the misdesign was my fault to begin with, maybe some
other folk better look this over before it goes in :-(

The main bit of ugliness here is that conninfo_parse's API has to
be hacked up, because as it stands it's not possible for the caller
to tell whether a password came from the conninfo string or a
PGPASSWORD environment variable.  It would perhaps be nicer to add
an "option source" enum field to struct PQconninfoOption, but that
would be a libpq ABI break which I don't think we want right now.
So I settled for an ugly special case, instead.

            regards, tom lane

Index: doc/src/sgml/libpq.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.247
diff -c -r1.247 libpq.sgml
*** doc/src/sgml/libpq.sgml    28 Nov 2007 15:42:31 -0000    1.247
--- doc/src/sgml/libpq.sgml    8 Dec 2007 23:13:35 -0000
***************
*** 1146,1156 ****
      </varlistentry>

      <varlistentry>
       <term><function>PQconnectionUsedPassword</function><indexterm><primary>PQconnectionUsedPassword</></></term>
       <listitem>
        <para>
         Returns true (1) if the connection authentication method
!        required a password to be supplied. Returns false (0) if not.

         <synopsis>
          int PQconnectionUsedPassword(const PGconn *conn);
--- 1146,1177 ----
      </varlistentry>

      <varlistentry>
+      <term><function>PQconnectionNeedsPassword</function><indexterm><primary>PQconnectionNeedsPassword</></></term>
+      <listitem>
+       <para>
+        Returns true (1) if the connection authentication method
+        required a password, but none was available.
+        Returns false (0) if not.
+
+        <synopsis>
+         int PQconnectionNeedsPassword(const PGconn *conn);
+        </synopsis>
+
+       </para>
+
+       <para>
+        This function can be applied after a failed connection attempt
+        to decide whether to prompt the user for a password.
+       </para>
+      </listitem>
+     </varlistentry>
+
+     <varlistentry>
       <term><function>PQconnectionUsedPassword</function><indexterm><primary>PQconnectionUsedPassword</></></term>
       <listitem>
        <para>
         Returns true (1) if the connection authentication method
!        used a caller-supplied password. Returns false (0) if not.

         <synopsis>
          int PQconnectionUsedPassword(const PGconn *conn);
***************
*** 1159,1167 ****
        </para>

        <para>
!        This function can be applied after either successful or failed
!        connection attempts.  In the case of failure, it can for example
!        be used to decide whether to prompt the user for a password.
        </para>
       </listitem>
      </varlistentry>
--- 1180,1189 ----
        </para>

        <para>
!        This function detects whether a password supplied to the connection
!        function was actually used.  Passwords obtained from other
!        sources (such as the <filename>.pgpass</> file) are not considered
!        caller-supplied.
        </para>
       </listitem>
      </varlistentry>
Index: doc/src/sgml/release.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/release.sgml,v
retrieving revision 1.563
diff -c -r1.563 release.sgml
*** doc/src/sgml/release.sgml    8 Dec 2007 17:24:03 -0000    1.563
--- doc/src/sgml/release.sgml    8 Dec 2007 23:13:37 -0000
***************
*** 2184,2191 ****

       <listitem>
        <para>
!        Add <function>PQconnectionUsedPassword()</function> that returns
!        true if the server required a password (Joe Conway)
        </para>

        <para>
--- 2184,2192 ----

       <listitem>
        <para>
!        Add <function>PQconnectionNeedsPassword()</function> that returns
!        true if the server required a password but none was supplied
!        (Joe Conway, Tom)
        </para>

        <para>
***************
*** 2197,2202 ****
--- 2198,2216 ----
        </para>
       </listitem>

+      <listitem>
+       <para>
+        Add <function>PQconnectionUsedPassword()</function> that returns
+        true if the supplied password was actually used
+        (Joe Conway, Tom)
+       </para>
+
+       <para>
+        This is useful in some security contexts where it is important
+        to know whether a user-supplied password is actually valid.
+       </para>
+      </listitem>
+
      </itemizedlist>

     </sect3>
Index: src/bin/pg_ctl/pg_ctl.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/pg_ctl/pg_ctl.c,v
retrieving revision 1.90
diff -c -r1.90 pg_ctl.c
*** src/bin/pg_ctl/pg_ctl.c    20 Nov 2007 19:24:26 -0000    1.90
--- src/bin/pg_ctl/pg_ctl.c    8 Dec 2007 23:13:37 -0000
***************
*** 492,498 ****
      {
          if ((conn = PQconnectdb(connstr)) != NULL &&
              (PQstatus(conn) == CONNECTION_OK ||
!              PQconnectionUsedPassword(conn)))
          {
              PQfinish(conn);
              success = true;
--- 492,498 ----
      {
          if ((conn = PQconnectdb(connstr)) != NULL &&
              (PQstatus(conn) == CONNECTION_OK ||
!              PQconnectionNeedsPassword(conn)))
          {
              PQfinish(conn);
              success = true;
Index: src/bin/pg_dump/pg_backup_db.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_backup_db.c,v
retrieving revision 1.76
diff -c -r1.76 pg_backup_db.c
*** src/bin/pg_dump/pg_backup_db.c    8 Jul 2007 19:07:38 -0000    1.76
--- src/bin/pg_dump/pg_backup_db.c    8 Dec 2007 23:13:37 -0000
***************
*** 159,165 ****

          if (PQstatus(newConn) == CONNECTION_BAD)
          {
!             if (!PQconnectionUsedPassword(newConn))
                  die_horribly(AH, modulename, "could not reconnect to database: %s",
                               PQerrorMessage(newConn));
              PQfinish(newConn);
--- 159,165 ----

          if (PQstatus(newConn) == CONNECTION_BAD)
          {
!             if (!PQconnectionNeedsPassword(newConn))
                  die_horribly(AH, modulename, "could not reconnect to database: %s",
                               PQerrorMessage(newConn));
              PQfinish(newConn);
***************
*** 234,240 ****
              die_horribly(AH, modulename, "failed to connect to database\n");

          if (PQstatus(AH->connection) == CONNECTION_BAD &&
!             PQconnectionUsedPassword(AH->connection) &&
              password == NULL &&
              !feof(stdin))
          {
--- 234,240 ----
              die_horribly(AH, modulename, "failed to connect to database\n");

          if (PQstatus(AH->connection) == CONNECTION_BAD &&
!             PQconnectionNeedsPassword(AH->connection) &&
              password == NULL &&
              !feof(stdin))
          {
Index: src/bin/pg_dump/pg_dumpall.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_dumpall.c,v
retrieving revision 1.98
diff -c -r1.98 pg_dumpall.c
*** src/bin/pg_dump/pg_dumpall.c    15 Nov 2007 21:14:42 -0000    1.98
--- src/bin/pg_dump/pg_dumpall.c    8 Dec 2007 23:13:37 -0000
***************
*** 1336,1342 ****
          }

          if (PQstatus(conn) == CONNECTION_BAD &&
!             PQconnectionUsedPassword(conn) &&
              password == NULL &&
              !feof(stdin))
          {
--- 1336,1342 ----
          }

          if (PQstatus(conn) == CONNECTION_BAD &&
!             PQconnectionNeedsPassword(conn) &&
              password == NULL &&
              !feof(stdin))
          {
Index: src/bin/psql/command.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/command.c,v
retrieving revision 1.183
diff -c -r1.183 command.c
*** src/bin/psql/command.c    15 Nov 2007 21:14:42 -0000    1.183
--- src/bin/psql/command.c    8 Dec 2007 23:13:37 -0000
***************
*** 1164,1170 ****
           * Connection attempt failed; either retry the connection attempt with
           * a new password, or give up.
           */
!         if (!password && PQconnectionUsedPassword(n_conn))
          {
              PQfinish(n_conn);
              password = prompt_for_password(user);
--- 1164,1170 ----
           * Connection attempt failed; either retry the connection attempt with
           * a new password, or give up.
           */
!         if (!password && PQconnectionNeedsPassword(n_conn))
          {
              PQfinish(n_conn);
              password = prompt_for_password(user);
Index: src/bin/psql/startup.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/psql/startup.c,v
retrieving revision 1.141
diff -c -r1.141 startup.c
*** src/bin/psql/startup.c    8 Jul 2007 19:07:38 -0000    1.141
--- src/bin/psql/startup.c    8 Dec 2007 23:13:37 -0000
***************
*** 211,217 ****
                                 username, password);

          if (PQstatus(pset.db) == CONNECTION_BAD &&
!             PQconnectionUsedPassword(pset.db) &&
              password == NULL &&
              !feof(stdin))
          {
--- 211,217 ----
                                 username, password);

          if (PQstatus(pset.db) == CONNECTION_BAD &&
!             PQconnectionNeedsPassword(pset.db) &&
              password == NULL &&
              !feof(stdin))
          {
Index: src/bin/scripts/common.c
===================================================================
RCS file: /cvsroot/pgsql/src/bin/scripts/common.c,v
retrieving revision 1.29
diff -c -r1.29 common.c
*** src/bin/scripts/common.c    15 Nov 2007 21:14:42 -0000    1.29
--- src/bin/scripts/common.c    8 Dec 2007 23:13:37 -0000
***************
*** 123,129 ****
          }

          if (PQstatus(conn) == CONNECTION_BAD &&
!             PQconnectionUsedPassword(conn) &&
              password == NULL &&
              !feof(stdin))
          {
--- 123,129 ----
          }

          if (PQstatus(conn) == CONNECTION_BAD &&
!             PQconnectionNeedsPassword(conn) &&
              password == NULL &&
              !feof(stdin))
          {
Index: src/interfaces/libpq/exports.txt
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/exports.txt,v
retrieving revision 1.17
diff -c -r1.17 exports.txt
*** src/interfaces/libpq/exports.txt    13 Oct 2007 20:18:41 -0000    1.17
--- src/interfaces/libpq/exports.txt    8 Dec 2007 23:13:37 -0000
***************
*** 139,141 ****
--- 139,142 ----
  lo_truncate               137
  PQconnectionUsedPassword  138
  pg_valid_server_encoding_id 139
+ PQconnectionNeedsPassword 140
Index: src/interfaces/libpq/fe-auth.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-auth.c,v
retrieving revision 1.134
diff -c -r1.134 fe-auth.c
*** src/interfaces/libpq/fe-auth.c    4 Dec 2007 13:02:53 -0000    1.134
--- src/interfaces/libpq/fe-auth.c    8 Dec 2007 23:13:37 -0000
***************
*** 954,960 ****
          case AUTH_REQ_MD5:
          case AUTH_REQ_CRYPT:
          case AUTH_REQ_PASSWORD:
!             if (conn->pgpass == NULL || *conn->pgpass == '\0')
              {
                  printfPQExpBuffer(&conn->errorMessage,
                                    PQnoPasswordSupplied);
--- 954,961 ----
          case AUTH_REQ_MD5:
          case AUTH_REQ_CRYPT:
          case AUTH_REQ_PASSWORD:
!             conn->password_needed = true;
!             if (conn->pgpass == NULL || conn->pgpass[0] == '\0')
              {
                  printfPQExpBuffer(&conn->errorMessage,
                                    PQnoPasswordSupplied);
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.353
diff -c -r1.353 fe-connect.c
*** src/interfaces/libpq/fe-connect.c    15 Nov 2007 21:14:46 -0000    1.353
--- src/interfaces/libpq/fe-connect.c    8 Dec 2007 23:13:37 -0000
***************
*** 232,238 ****
  static void freePGconn(PGconn *conn);
  static void closePGconn(PGconn *conn);
  static PQconninfoOption *conninfo_parse(const char *conninfo,
!                PQExpBuffer errorMessage);
  static char *conninfo_getval(PQconninfoOption *connOptions,
                  const char *keyword);
  static void defaultNoticeReceiver(void *arg, const PGresult *res);
--- 232,238 ----
  static void freePGconn(PGconn *conn);
  static void closePGconn(PGconn *conn);
  static PQconninfoOption *conninfo_parse(const char *conninfo,
!                PQExpBuffer errorMessage, bool *password_from_string);
  static char *conninfo_getval(PQconninfoOption *connOptions,
                  const char *keyword);
  static void defaultNoticeReceiver(void *arg, const PGresult *res);
***************
*** 376,382 ****
      /*
       * Parse the conninfo string
       */
!     connOptions = conninfo_parse(conninfo, &conn->errorMessage);
      if (connOptions == NULL)
      {
          conn->status = CONNECTION_BAD;
--- 376,383 ----
      /*
       * Parse the conninfo string
       */
!     connOptions = conninfo_parse(conninfo, &conn->errorMessage,
!                                  &conn->pgpass_from_client);
      if (connOptions == NULL)
      {
          conn->status = CONNECTION_BAD;
***************
*** 472,477 ****
--- 473,479 ----
                                          conn->dbName, conn->pguser);
          if (conn->pgpass == NULL)
              conn->pgpass = strdup(DefaultPassword);
+         conn->pgpass_from_client = false;
      }

      /*
***************
*** 555,564 ****
  PQconndefaults(void)
  {
      PQExpBufferData errorBuf;
      PQconninfoOption *connOptions;

      initPQExpBuffer(&errorBuf);
!     connOptions = conninfo_parse("", &errorBuf);
      termPQExpBuffer(&errorBuf);
      return connOptions;
  }
--- 557,567 ----
  PQconndefaults(void)
  {
      PQExpBufferData errorBuf;
+     bool        password_from_string;
      PQconninfoOption *connOptions;

      initPQExpBuffer(&errorBuf);
!     connOptions = conninfo_parse("", &errorBuf, &password_from_string);
      termPQExpBuffer(&errorBuf);
      return connOptions;
  }
***************
*** 659,664 ****
--- 662,668 ----
          if (conn->pgpass)
              free(conn->pgpass);
          conn->pgpass = strdup(pwd);
+         conn->pgpass_from_client = true;
      }

      /*
***************
*** 1718,1727 ****
                   */
                  conn->inStart = conn->inCursor;

-                 /* Save the authentication request type, if first one. */
-                 if (conn->areq == AUTH_REQ_OK)
-                     conn->areq = areq;
-
                  /* Respond to the request if necessary. */

                  /*
--- 1722,1727 ----
***************
*** 1924,1930 ****
      conn->std_strings = false;    /* unless server says differently */
      conn->verbosity = PQERRORS_DEFAULT;
      conn->sock = -1;
!     conn->areq = AUTH_REQ_OK;    /* until we receive something else */
  #ifdef USE_SSL
      conn->allow_ssl_try = true;
      conn->wait_ssl_try = false;
--- 1924,1930 ----
      conn->std_strings = false;    /* unless server says differently */
      conn->verbosity = PQERRORS_DEFAULT;
      conn->sock = -1;
!     conn->password_needed = false;
  #ifdef USE_SSL
      conn->allow_ssl_try = true;
      conn->wait_ssl_try = false;
***************
*** 3064,3072 ****
   * If successful, a malloc'd PQconninfoOption array is returned.
   * If not successful, NULL is returned and an error message is
   * left in errorMessage.
   */
  static PQconninfoOption *
! conninfo_parse(const char *conninfo, PQExpBuffer errorMessage)
  {
      char       *pname;
      char       *pval;
--- 3064,3075 ----
   * If successful, a malloc'd PQconninfoOption array is returned.
   * If not successful, NULL is returned and an error message is
   * left in errorMessage.
+  * *password_from_string is set TRUE if we got a password from the
+  * conninfo string, otherwise FALSE.
   */
  static PQconninfoOption *
! conninfo_parse(const char *conninfo, PQExpBuffer errorMessage,
!                bool *password_from_string)
  {
      char       *pname;
      char       *pval;
***************
*** 3077,3082 ****
--- 3080,3087 ----
      PQconninfoOption *options;
      PQconninfoOption *option;

+     *password_from_string = false;            /* default result */
+
      /* Make a working copy of PQconninfoOptions */
      options = malloc(sizeof(PQconninfoOptions));
      if (options == NULL)
***************
*** 3233,3238 ****
--- 3238,3249 ----
              free(buf);
              return NULL;
          }
+
+         /*
+          * Special handling for password
+          */
+         if (strcmp(option->keyword, "password") == 0)
+             *password_from_string = (option->val[0] != '\0');
      }

      /* Done with the modifiable input string */
***************
*** 3476,3488 ****
  }

  int
  PQconnectionUsedPassword(const PGconn *conn)
  {
      if (!conn)
          return false;
!     if (conn->areq == AUTH_REQ_MD5 ||
!         conn->areq == AUTH_REQ_CRYPT ||
!         conn->areq == AUTH_REQ_PASSWORD)
          return true;
      else
          return false;
--- 3487,3509 ----
  }

  int
+ PQconnectionNeedsPassword(const PGconn *conn)
+ {
+     if (!conn)
+         return false;
+     if (conn->password_needed &&
+         (conn->pgpass == NULL || conn->pgpass[0] == '\0'))
+         return true;
+     else
+         return false;
+ }
+
+ int
  PQconnectionUsedPassword(const PGconn *conn)
  {
      if (!conn)
          return false;
!     if (conn->password_needed && conn->pgpass_from_client)
          return true;
      else
          return false;
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.139
diff -c -r1.139 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h    13 Oct 2007 20:18:42 -0000    1.139
--- src/interfaces/libpq/libpq-fe.h    8 Dec 2007 23:13:37 -0000
***************
*** 263,268 ****
--- 263,269 ----
  extern char *PQerrorMessage(const PGconn *conn);
  extern int    PQsocket(const PGconn *conn);
  extern int    PQbackendPID(const PGconn *conn);
+ extern int    PQconnectionNeedsPassword(const PGconn *conn);
  extern int    PQconnectionUsedPassword(const PGconn *conn);
  extern int    PQclientEncoding(const PGconn *conn);
  extern int    PQsetClientEncoding(PGconn *conn, const char *encoding);
***************
*** 426,432 ****
  #define PQfreeNotify(ptr) PQfreemem(ptr)

  /* Error when no password was given. */
! /* Note: depending on this is deprecated; use PQconnectionUsedPassword(). */
  #define PQnoPasswordSupplied    "fe_sendauth: no password supplied\n"

  /*
--- 427,433 ----
  #define PQfreeNotify(ptr) PQfreemem(ptr)

  /* Error when no password was given. */
! /* Note: depending on this is deprecated; use PQconnectionNeedsPassword(). */
  #define PQnoPasswordSupplied    "fe_sendauth: no password supplied\n"

  /*
Index: src/interfaces/libpq/libpq-int.h
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/libpq-int.h,v
retrieving revision 1.127
diff -c -r1.127 libpq-int.h
*** src/interfaces/libpq/libpq-int.h    15 Nov 2007 21:14:46 -0000    1.127
--- src/interfaces/libpq/libpq-int.h    8 Dec 2007 23:13:37 -0000
***************
*** 291,296 ****
--- 291,297 ----
      char       *dbName;            /* database name */
      char       *pguser;            /* Postgres username and password, if any */
      char       *pgpass;
+     bool        pgpass_from_client;    /* did password come from connect args? */
      char       *sslmode;        /* SSL mode (require,prefer,allow,disable) */
  #if defined(KRB5) || defined(ENABLE_GSS) || defined(ENABLE_SSPI)
      char       *krbsrvname;        /* Kerberos service name */
***************
*** 323,329 ****
      SockAddr    raddr;            /* Remote address */
      ProtocolVersion pversion;    /* FE/BE protocol version in use */
      int            sversion;        /* server version, e.g. 70401 for 7.4.1 */
!     AuthRequest areq;            /* auth type demanded by server */

      /* Transient state needed while establishing connection */
      struct addrinfo *addrlist;    /* list of possible backend addresses */
--- 324,330 ----
      SockAddr    raddr;            /* Remote address */
      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 */

      /* Transient state needed while establishing connection */
      struct addrinfo *addrlist;    /* list of possible backend addresses */