Warning about invalid .pgpass passwords
in [PgSql]
|
Prev: Access violation from palloc, Visual Studio 2005, C-language function
Next: [HACKERS] walreceiver is uninterruptible on win32
From: Bruce Momjian on 11 Mar 2010 16:19 Bruce Momjian wrote: > Tom Lane wrote: > > Bruce Momjian <bruce(a)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(a)momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
From: Alvaro Herrera on 11 Mar 2010 16:24 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Bruce Momjian on 11 Mar 2010 16:35 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(a)momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 11 Mar 2010 16:55 Bruce Momjian <bruce(a)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 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Bruce Momjian on 11 Mar 2010 17:42
Tom Lane wrote: > Bruce Momjian <bruce(a)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(a)momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do |