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: Fujii Masao on 9 Mar 2010 22:31 On Wed, Mar 10, 2010 at 11:52 AM, Bruce Momjian <bruce(a)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 -- 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 9 Mar 2010 22:39 Bruce Momjian <bruce(a)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 -- 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 10 Mar 2010 20:01 Tom Lane wrote: > Bruce Momjian <bruce(a)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(a)momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
From: Tom Lane on 10 Mar 2010 20:07 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. 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 -- 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 10 Mar 2010 20:09
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? -- 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 |