Re: Warning about invalid .pgpass passwords
Bruce Momjian <bruce@momjian.us>
From: Bruce Momjian <bruce@momjian.us>
To: Bruce Momjian <bruce@momjian.us>
Cc: Andrew Dunstan <andrew@dunslane.net>, Tom Lane <tgl@sss.pgh.pa.us>, Alvaro Herrera <alvherre@commandprompt.com>, Eamonn Martin <mas01em@gold.ac.uk>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2010-03-13T14:56:18Z
Lists: pgsql-hackers
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