Re: [HACKERS] libpq 9.4 requires /etc/passwd?
Re: Tom Lane 2015-01-11 <13609.1420998...@sss.pgh.pa.us> > > The problem isn't present in 9.3 and earlier (at least with > > postfix-pgsql), so there's no need to go back further. > > I've committed a fix for this in HEAD and 9.4. I've just tested with the HEAD libpq and the issue is fixed. Thanks! In the first iteration of the test the database was still down and I got this error message: Jan 11 19:08:53 lehmann postfix/trivial-rewrite[3453]: warning: connect to pgsql server localhost:5432: could not connect to server: Connection refused??Is the server running on host "localhost" (::1) and accepting??TCP/IP connections on port 5432??could not connect to server: Connection refused??Is the server running on host "localhost" (127.0.0.1) and accepting??TCP/IP connections on port 5432?? The ?? are probably postfix and/or syslog messing with newlines or the like. At first I was confused that the PG part of the error message is duplicated, but then I figured that's two different "localhost" addresses, so all is fine. Christoph -- c...@df7cb.de | http://www.df7cb.de/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq 9.4 requires /etc/passwd?
Christoph Berg writes: > Re: Tom Lane 2015-01-10 <22432.1420915...@sss.pgh.pa.us> >> So what I propose we do with this is patch HEAD and 9.4 only. >> We need to do *something* in 9.4 to address Christoph's complaint, and >> that branch is new enough that we can probably get away with changing >> officially-unsupported APIs. The lack of other field complaints makes >> me okay with not trying to fix these issues further back. > The problem isn't present in 9.3 and earlier (at least with > postfix-pgsql), so there's no need to go back further. I've committed a fix for this in HEAD and 9.4. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq 9.4 requires /etc/passwd?
On Sat, Jan 10, 2015 at 02:02:54PM -0500, Tom Lane wrote: > Not entirely sure what to do about this, but I predict this won't be > the last complaint unless we find some way to improve test coverage > in this area. Or perhaps we could turn PQsetdbLogin into a > ***very*** thin wrapper around PQconnectdbParams? +1 for this. Having a single point of truth here would be a big win. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq 9.4 requires /etc/passwd?
Christoph Berg writes: > Re: Tom Lane 2015-01-10 <22432.1420915...@sss.pgh.pa.us> >> So what I propose we do with this is patch HEAD and 9.4 only. >> We need to do *something* in 9.4 to address Christoph's complaint, and >> that branch is new enough that we can probably get away with changing >> officially-unsupported APIs. The lack of other field complaints makes >> me okay with not trying to fix these issues further back. > The problem isn't present in 9.3 and earlier (at least with > postfix-pgsql), so there's no need to go back further. Right, the specific issue you're complaining of is new in 9.4. The general issue that failures in /etc/passwd lookups aren't well reported has been there more or less forever, but your immediate point is that no such failure should be reported to the user at all if he's supplied a login name to use. It looks like the bad handling of Windows' GetUserName() failures has been there a long time too, but given the lack of field reports I don't feel a need to back-patch that very far either. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq 9.4 requires /etc/passwd?
Re: Tom Lane 2015-01-10 <22432.1420915...@sss.pgh.pa.us> > So what I propose we do with this is patch HEAD and 9.4 only. > We need to do *something* in 9.4 to address Christoph's complaint, and > that branch is new enough that we can probably get away with changing > officially-unsupported APIs. The lack of other field complaints makes > me okay with not trying to fix these issues further back. The problem isn't present in 9.3 and earlier (at least with postfix-pgsql), so there's no need to go back further. As for the number of complaints, I've received two independent reports on IRC, and upon googling the problem had been seen as early as in July [1] and August [2]. All four reports are for postfix-pgsql on Debian, but that's probably just because we pushed 9.4 into the next-release branch very early. (And I wish someone had told me about the problem, instead of only reporting it for postfix...) [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=756627 [2] https://workaround.org/comment/3415#comment-3415 Christoph -- c...@df7cb.de | http://www.df7cb.de/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq 9.4 requires /etc/passwd?
One other point here: I realized while testing my patch that it's actually impossible to provoke the failure mode Christoph is unhappy about in psql. You can only see it in an application that uses PQsetdb/PQsetdbLogin, which of course psql doesn't anymore. The reason is that in the PQconnect family of functions, conninfo_add_defaults() is only applied after filling in all available caller-supplied parameters, so if the user has in one way or another specified the role name to use, we never invoke pg_fe_getauthname() at all. It's only called if we have to use the default role name, and in that context of course a hard failure is appropriate. But in the PQsetdbLogin code path, we do pg_fe_getauthname first and then overwrite (some) values from the parameters, so a failure during conninfo_add_defaults() is premature. This is a tad disturbing, because we are not using and therefore not testing PQsetdb/PQsetdbLogin anywhere, which means that any failure modes that path acquires that don't exist in the PQconnect code path are guaranteed to go undetected in our testing. Now, I rather doubt that we'd have found the problem with doesn't-handle-lack-of-/etc/passwd-well even if we had been testing that code path, but we certainly won't find problems in paths we don't test. Not entirely sure what to do about this, but I predict this won't be the last complaint unless we find some way to improve test coverage in this area. Or perhaps we could turn PQsetdbLogin into a ***very*** thin wrapper around PQconnectdbParams? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq 9.4 requires /etc/passwd?
On Fri, Jan 09, 2015 at 06:57:02PM -0500, Tom Lane wrote: > Commit a4c8f14364c27508233f8a31ac4b10a4c90235a9 turned > failure of pg_fe_getauthname() into a hard connection failure, whereas > previously it was harmless as long as the caller provided a username. > > I wonder if we shouldn't just revert that commit in toto. Yeah, > identifying an out-of-memory error might be useful, but this cure > seems a lot worse than the disease. What's more, this coding reports > *any* pg_fe_getauthname failure as "out of memory", which is much worse > than useless. +1 for reverting it as the next step > Alternatively, maybe don't try to resolve username this way until > we've found that the caller isn't providing any username. and for subsequently pursuing this. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq 9.4 requires /etc/passwd?
Bruce Momjian writes: > On Fri, Jan 9, 2015 at 06:57:02PM -0500, Tom Lane wrote: >> Hmm ... actually, I'll bet it's not $HOME that's at issue, but the name >> of the user. Commit a4c8f14364c27508233f8a31ac4b10a4c90235a9 turned >> failure of pg_fe_getauthname() into a hard connection failure, whereas >> previously it was harmless as long as the caller provided a username. >> >> I wonder if we shouldn't just revert that commit in toto. Yeah, >> identifying an out-of-memory error might be useful, but this cure >> seems a lot worse than the disease. What's more, this coding reports >> *any* pg_fe_getauthname failure as "out of memory", which is much worse >> than useless. >> >> Alternatively, maybe don't try to resolve username this way until >> we've found that the caller isn't providing any username. > I have developed the attached patch which documents why the user name > lookup might fail, and sets the failure string to "". It preserves the > memory allocation failure behavior. I'm unimpressed with that patch. It does nothing to fix the fundamental problem, which is that error handling in this area is many bricks shy of a load. And to the extent that it addresses Christoph's complaint, it does so only in a roundabout and utterly undocumented way. I think we need to suck it up and fix pg_fe_getauthname to have proper error reporting, which in turns means fixing pqGetpwuid (since the API for that is incapable of distinguishing "no such user" from "error during lookup"). Accordingly, I propose the attached patch instead. Some notes: * According to Microsoft's documentation, Windows' GetUserName() does not set errno (it would be mildly astonishing if it did...). The existing error-handling code in src/common/username.c is therefore wrong too. * port.h seems to think it should declare pqGetpwuid() if __CYGWIN__, but noplace else agrees with that. * I made pg_fe_getauthname's Windows username[] array 256 bytes for consistency with username.c, where some actual research seems to have been expended on how large it ought to be. * pqGetpwuid thinks there was once a four-argument spec for getpwuid_r. That must have been in the late bronze age, because even in 1992's Single Unix Spec it's five arguments. And the SUS is our standard benchmark for Unix portability. So I think we could rip that out, along with the corresponding configure test, at least in HEAD. I've not done so here though. In principle, changing the API specs for pg_fe_getauthname and pqGetpwuid should be safe because we don't officially export those functions. In practice, on platforms that don't honor the export restriction list it's theoretically possible that somebody is calling those from third-party code. So what I propose we do with this is patch HEAD and 9.4 only. We need to do *something* in 9.4 to address Christoph's complaint, and that branch is new enough that we can probably get away with changing officially-unsupported APIs. The lack of other field complaints makes me okay with not trying to fix these issues further back. Comments? regards, tom lane diff --git a/src/common/username.c b/src/common/username.c index ee5ef1c..ac9a898 100644 *** a/src/common/username.c --- b/src/common/username.c *** get_user_name(char **errstr) *** 58,64 if (!GetUserName(username, &len)) { ! *errstr = psprintf(_("user name lookup failure: %s"), strerror(errno)); return NULL; } --- 58,65 if (!GetUserName(username, &len)) { ! *errstr = psprintf(_("user name lookup failure: error code %lu"), ! GetLastError()); return NULL; } diff --git a/src/include/port.h b/src/include/port.h index 1d53e4e..26d7fcd 100644 *** a/src/include/port.h --- b/src/include/port.h *** extern void srandom(unsigned int seed); *** 433,439 /* thread.h */ extern char *pqStrerror(int errnum, char *strerrbuf, size_t buflen); ! #if !defined(WIN32) || defined(__CYGWIN__) extern int pqGetpwuid(uid_t uid, struct passwd * resultbuf, char *buffer, size_t buflen, struct passwd ** result); #endif --- 433,439 /* thread.h */ extern char *pqStrerror(int errnum, char *strerrbuf, size_t buflen); ! #ifndef WIN32 extern int pqGetpwuid(uid_t uid, struct passwd * resultbuf, char *buffer, size_t buflen, struct passwd ** result); #endif diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 179793e..5a6a1e4 100644 *** a/src/interfaces/libpq/fe-auth.c --- b/src/interfaces/libpq/fe-auth.c *** pg_fe_sendauth(AuthRequest areq, PGconn *** 714,735 /* * pg_fe_getauthname * ! * Returns a pointer to dynamic space containing whatever name the user ! * has authenticated to the system. If there is an error, return NULL. */ char * ! pg_fe_getauthname(void) { const char *name = NULL; - char *authn; #ifdef WIN32 ! char username[128]; DWORD namesize = sizeof(username)
Re: [HACKERS] libpq 9.4 requires /etc/passwd?
On Fri, Jan 9, 2015 at 06:57:02PM -0500, Tom Lane wrote: > I wrote: > > Christoph Berg writes: > >> libpq wants the user home directory to find .pgpass and > >> .pg_service.conf files, but apparently the behavior to require the > >> existence of the passwd file (or nss equivalent) is new in 9.4. > > > There is demonstrably no direct reference to /etc/passwd in the PG code. > > It's possible that we've added some new expectation that $HOME can be > > identified, but a quick look through the code shows no such checks that > > don't look like they've been there for some time. > > Hmm ... actually, I'll bet it's not $HOME that's at issue, but the name > of the user. Commit a4c8f14364c27508233f8a31ac4b10a4c90235a9 turned > failure of pg_fe_getauthname() into a hard connection failure, whereas > previously it was harmless as long as the caller provided a username. > > I wonder if we shouldn't just revert that commit in toto. Yeah, > identifying an out-of-memory error might be useful, but this cure > seems a lot worse than the disease. What's more, this coding reports > *any* pg_fe_getauthname failure as "out of memory", which is much worse > than useless. > > Alternatively, maybe don't try to resolve username this way until > we've found that the caller isn't providing any username. Seems we both found it at the same time. Here is the thread were we discussed it, and you were concerned about the memory allocation failure too: http://www.postgresql.org/message-id/flat/20140320154905.gc7...@momjian.us#20140320154905.gc7...@momjian.us In particular, it appears to me that if the strdup in pg_fe_getauthname fails, we'll just let that pass without comment, which is inconsistent with all the other out-of-memory cases in conninfo_add_defaults. (I wonder whether any code downstream of this supposes that we always have a value for the "user" option. It's a pretty safe bet that the case is hardly ever tested.) I have developed the attached patch which documents why the user name lookup might fail, and sets the failure string to "". It preserves the memory allocation failure behavior. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c new file mode 100644 index 179793e..b9155b4 *** a/src/interfaces/libpq/fe-auth.c --- b/src/interfaces/libpq/fe-auth.c *** pg_fe_sendauth(AuthRequest areq, PGconn *** 715,726 * pg_fe_getauthname * * Returns a pointer to dynamic space containing whatever name the user ! * has authenticated to the system. If there is an error, return NULL. */ char * pg_fe_getauthname(void) { ! const char *name = NULL; char *authn; #ifdef WIN32 --- 715,731 * pg_fe_getauthname * * Returns a pointer to dynamic space containing whatever name the user ! * has authenticated to the system. Returns NULL on memory allocation ! * failure, and a zero-length string on user name lookup failure. */ char * pg_fe_getauthname(void) { ! /* ! * We might be in a chroot environment where we can't look up our own ! * user name, so return "". ! */ ! const char *name = ""; char *authn; #ifdef WIN32 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq 9.4 requires /etc/passwd?
On Fri, Jan 9, 2015 at 06:42:19PM -0500, Tom Lane wrote: > Christoph Berg writes: > > libpq wants the user home directory to find .pgpass and > > .pg_service.conf files, but apparently the behavior to require the > > existence of the passwd file (or nss equivalent) is new in 9.4. > > There is demonstrably no direct reference to /etc/passwd in the PG code. > It's possible that we've added some new expectation that $HOME can be > identified, but a quick look through the code shows no such checks that > don't look like they've been there for some time. > > Are the complainants doing anything that would result in SSL certificate > checking? More generally, it'd be useful to see an exact example of > what are the connection parameters and environment that result in a > failure. I see similar error strings, but nothing that ends with a question mark: "out of memory?". However, I wonder if the crux of the problem is that they are running in a chroot environment where the user id can't be looked up. Looking at libpq's pg_fe_getauthname(), I see that if it can't get the OS user name of the effective user, it assumes it failed and returns NULL: /* * We document PQconndefaults() to return NULL for a memory allocation * failure. We don't have an API to return a user name lookup failure, so * we just assume it always succeeds. */ #ifdef WIN32 if (GetUserName(username, &namesize)) name = username; #else if (pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pw) == 0) name = pw->pw_name; #endif authn = name ? strdup(name) : NULL; and conninfo_add_defaults() assumes a pg_fe_getauthname() failure is a memory allocation failure: option->val = pg_fe_getauthname(); if (!option->val) { if (errorMessage) printfPQExpBuffer(errorMessage, libpq_gettext("out of memory\n")); return false; It was my 9.4 commit that changed this: commit a4c8f14364c27508233f8a31ac4b10a4c90235a9 Author: Bruce Momjian Date: Thu Mar 20 11:48:31 2014 -0400 libpq: pass a memory allocation failure error up to PQconndefaults() Previously user name memory allocation failures were ignored and the default user name set to NULL. I am thinking we have to now assume that user name lookups can fail for other reasons. I am unclear how this worked in the past though. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq 9.4 requires /etc/passwd?
I wrote: > Christoph Berg writes: >> libpq wants the user home directory to find .pgpass and >> .pg_service.conf files, but apparently the behavior to require the >> existence of the passwd file (or nss equivalent) is new in 9.4. > There is demonstrably no direct reference to /etc/passwd in the PG code. > It's possible that we've added some new expectation that $HOME can be > identified, but a quick look through the code shows no such checks that > don't look like they've been there for some time. Hmm ... actually, I'll bet it's not $HOME that's at issue, but the name of the user. Commit a4c8f14364c27508233f8a31ac4b10a4c90235a9 turned failure of pg_fe_getauthname() into a hard connection failure, whereas previously it was harmless as long as the caller provided a username. I wonder if we shouldn't just revert that commit in toto. Yeah, identifying an out-of-memory error might be useful, but this cure seems a lot worse than the disease. What's more, this coding reports *any* pg_fe_getauthname failure as "out of memory", which is much worse than useless. Alternatively, maybe don't try to resolve username this way until we've found that the caller isn't providing any username. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] libpq 9.4 requires /etc/passwd?
Christoph Berg writes: > libpq wants the user home directory to find .pgpass and > .pg_service.conf files, but apparently the behavior to require the > existence of the passwd file (or nss equivalent) is new in 9.4. There is demonstrably no direct reference to /etc/passwd in the PG code. It's possible that we've added some new expectation that $HOME can be identified, but a quick look through the code shows no such checks that don't look like they've been there for some time. Are the complainants doing anything that would result in SSL certificate checking? More generally, it'd be useful to see an exact example of what are the connection parameters and environment that result in a failure. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] libpq 9.4 requires /etc/passwd?
Hi, I've got several reports that postfix's pgsql lookup tables are broken with 9.4's libpq5, while 9.3's libpq5 works just fine. The error message looks like this: Jan 10 00:11:40 lehmann postfix/trivial-rewrite[29960]: warning: connect to pgsql server localhost:5432: out of memory? Jan 10 00:11:40 lehmann postfix/trivial-rewrite[29960]: warning: pgsql:/etc/postfix/pgsqltest lookup error for "*" The "out of memory?" message comes from PQsetdbLogin and PQerrorMessage in postfix-2.11.3/src/global/dict_pgsql.c: if ((host->db = PQsetdbLogin(host->name, host->port, NULL, NULL, dbname, username, password)) == NULL || PQstatus(host->db) != CONNECTION_OK) { msg_warn("connect to pgsql server %s: %s", host->hostname, PQerrorMessage(host->db)); There are two ways around the problem on the postfix side: not running the postfix service chrooted, or using a "proxy" map which effectively does the lookup also from outside the chroot. Debian bug https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=756627 mentions another solution: creation of an /etc/passwd file inside the postfix chroot. libpq wants the user home directory to find .pgpass and .pg_service.conf files, but apparently the behavior to require the existence of the passwd file (or nss equivalent) is new in 9.4. I've tried to make sense of the fe-connect.c code to find the issue but couldn't spot it. Can someone explain what's going on there? Is this a bug in libpq? (It's certainly a regression.) Christoph -- c...@df7cb.de | http://www.df7cb.de/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers