Thread: Patch to be verbose about being unable to read ~/.pgpasss...

Patch to be verbose about being unable to read ~/.pgpasss...

From
Sean Chittenden
Date:
Howdy.  Quick chump patch: if libpq finds a ~/.pgpass but can't stat
it, print something to stderr letting the user know.  If someone went
out of their way to put a .pgpass file in place, if they can't read
it, it seems worth while to alert them to the fact that it's not being
used (ex: root creates a .pgpass file but forgets to chown it).

-sc

--
Sean Chittenden

Attachment

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Tom Lane
Date:
Sean Chittenden <sean@chittenden.org> writes:
> Howdy.  Quick chump patch: if libpq finds a ~/.pgpass but can't stat
> it, print something to stderr letting the user know.

Isn't this gonna complain when the file doesn't exist at all?

            regards, tom lane

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Sean Chittenden
Date:
> > Howdy.  Quick chump patch: if libpq finds a ~/.pgpass but can't stat
> > it, print something to stderr letting the user know.
>
> Isn't this gonna complain when the file doesn't exist at all?

*blush*  Not with the attached patch.  -sc

--
Sean Chittenden

Attachment

Re: Patch to be verbose about being unable to read

From
Peter Eisentraut
Date:
Sean Chittenden writes:

> Howdy.  Quick chump patch: if libpq finds a ~/.pgpass but can't stat
> it, print something to stderr letting the user know.  If someone went
> out of their way to put a .pgpass file in place, if they can't read
> it, it seems worth while to alert them to the fact that it's not being
> used (ex: root creates a .pgpass file but forgets to chown it).

You cannot assume that stderr is meaningful in all applications using
libpq.  (Consider PHP.)  I think you need to report this using the normal
error reporting mechanism.

--
Peter Eisentraut   peter_e@gmx.net


Re: Patch to be verbose about being unable to read

From
Tom Lane
Date:
Peter Eisentraut <peter_e@gmx.net> writes:
> Sean Chittenden writes:
>> Howdy.  Quick chump patch: if libpq finds a ~/.pgpass but can't stat
>> it, print something to stderr letting the user know.  If someone went
>> out of their way to put a .pgpass file in place, if they can't read
>> it, it seems worth while to alert them to the fact that it's not being
>> used (ex: root creates a .pgpass file but forgets to chown it).

> You cannot assume that stderr is meaningful in all applications using
> libpq.  (Consider PHP.)  I think you need to report this using the normal
> error reporting mechanism.

But it's not an error; we must not fail the connection attempt just
because of this.

I'd suggest using the NOTICE mechanism, except that during connection
setup the client app has not yet had a chance to set a notice processor,
so Peter's objection still holds.

On the whole, I'm not sure we have a problem we need to fix there.  If
.pgpass isn't readable, it's not going to take that long for the user to
figure it out.

OTOH --- there already is a print-to-stderr in the code for the case
where .pgpass exists and has insecure permissions.  Taking Peter's
complaint into account, I wonder whether we shouldn't make that case a
hard error (connect failure) to ensure that the user notices the problem
and does something about it promptly.  IIRC, the postmaster refuses to
start if $PGDATA has insecure permissions, so there's precedent.

            regards, tom lane

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Sean Chittenden
Date:
> > Howdy.  Quick chump patch: if libpq finds a ~/.pgpass but can't stat
> > it, print something to stderr letting the user know.  If someone went
> > out of their way to put a .pgpass file in place, if they can't read
> > it, it seems worth while to alert them to the fact that it's not being
> > used (ex: root creates a .pgpass file but forgets to chown it).
>
> You cannot assume that stderr is meaningful in all applications
> using libpq.  (Consider PHP.)  I think you need to report this using
> the normal error reporting mechanism.

stderr is hooked up to the error logs under Apache, which is the
normal error reporting mechanism.  I thought about adding a test to
see if stderr is writable, but given that better than 99% of the time
it will be, I didn't see the point, esp since the above case regarding
the permissions, doesn't.

> But it's not an error; we must not fail the connection attempt just
> because of this.

Agreed, though one train of thought is that if someone did put a
.pgpass file in place that isn't readable, it should be known that
their efforts are being wasted in vein.  In the case of an unreadable
.pgpass, I'd like to know about it even if its stale that way I can
remove it or make it usable.  Having any kind of password hanging
around on the file system isn't a good policy and only invites trouble
in the event of a breach.

> On the whole, I'm not sure we have a problem we need to fix there.
> If .pgpass isn't readable, it's not going to take that long for the
> user to figure it out.

I agree it's not a huge problem, but if there's one in place for a
given user and it's not usable or is being improperly used, then the
library _should_ point it out in some fashion.  Since the user has the
ability to chmod the file, it seems prudent to point this out (even
possibly chmod(2) the file for them given we're already know what the
result is going to be).  If root chown'ed the .pgpass file, however,
it _doesn't_ make sense to continue in the connection process in the
event that a user is no longer given access to the DB, but it also
makes sense to point out to the user that the reason they're being
asked for their password is because their .pgpass file is unreadable.
It would have saved me a few minutes instead of double checking that I
had the file format setup correctly.  Soooo, I figured I'd correct
this to prevent future bits of time from being wasted hunting down
brain dead mistakes: very likely there are huge numbers of admins that
are less competent, skilled, or familiar with libpq than I or those
reviewing the patch so it may take others much more time to diagnose
the problem.  It's little niceties like this that make using a library
pleasant to use, IMHO.

> OTOH --- there already is a print-to-stderr in the code for the case
> where .pgpass exists and has insecure permissions.  Taking Peter's
> complaint into account, I wonder whether we shouldn't make that case
> a hard error (connect failure) to ensure that the user notices the
> problem and does something about it promptly.  IIRC, the postmaster
> refuses to start if $PGDATA has insecure permissions, so there's
> precedent.

This wouldn't be a bad idea, though frustrating for those that are
caught off guard by it... worth a mention in the release notes for
sure and can likely only be done at a major release point (7.4).

-sc

--
Sean Chittenden

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Tom Lane
Date:
Sean Chittenden <sean@chittenden.org> writes:
> stderr is hooked up to the error logs under Apache, which is the
> normal error reporting mechanism.  I thought about adding a test to
> see if stderr is writable, but given that better than 99% of the time
> it will be, I didn't see the point, esp since the above case regarding
> the permissions, doesn't.

If you are inside a GUI application, it's entirely likely that stderr
is hooked up to /dev/null.  I think Peter's complaint has great force
--- libpq has no business assuming it's okay to write on stderr.  The
entire reason we provided a notice-processor hook is to get away from
that assumption.

>> But it's not an error; we must not fail the connection attempt just
>> because of this.

> Agreed, though one train of thought is that if someone did put a
> .pgpass file in place that isn't readable, it should be known that
> their efforts are being wasted in vein.

Yeah, in fact I've been reconsidering that.  If we fail to stat the
file, we should just return quietly; but if we can stat it but not
open it, it's reasonable to assume there's something wrong that ought
to be fixed.  It's difficult to say that this shouldn't be a hard error
if you accept my suggestion that world readability should be an error.

If we do take that viewpoint then the problem goes away, since we can
just report all the reportable conditions as connection-failure
messages.

            regards, tom lane

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Sean Chittenden
Date:
> > stderr is hooked up to the error logs under Apache, which is the
> > normal error reporting mechanism.  I thought about adding a test to
> > see if stderr is writable, but given that better than 99% of the time
> > it will be, I didn't see the point, esp since the above case regarding
> > the permissions, doesn't.
>
> If you are inside a GUI application, it's entirely likely that
> stderr is hooked up to /dev/null.  I think Peter's complaint has
> great force --- libpq has no business assuming it's okay to write on
> stderr.  The entire reason we provided a notice-processor hook is to
> get away from that assumption.

Do you mean setting something in conn->errorMessage.data, use
defaultNoticeProcessor(), or fprintf(conn->Pfdebug....) ??  If the 1st
or 3rd, conn isn't setup, is it?  In the 2nd, it's just an
fprintf(stderr...) call with no way of overriding the output stream.
Would you like me to add an interface that'd allow stderr to be
redirected in the case of defaultNoticeProcessor()?  Problem is this
interface wouldn't be thread safe then.  I'd think it'd be nice to
have something that'd take variable length arguments for error
reporting too.

> >> But it's not an error; we must not fail the connection attempt just
> >> because of this.
>
> > Agreed, though one train of thought is that if someone did put a
> > .pgpass file in place that isn't readable, it should be known that
> > their efforts are being wasted in vein.
>
> Yeah, in fact I've been reconsidering that.  If we fail to stat the
> file, we should just return quietly; but if we can stat it but not
> open it, it's reasonable to assume there's something wrong that
> ought to be fixed.  It's difficult to say that this shouldn't be a
> hard error if you accept my suggestion that world readability should
> be an error.
>
> If we do take that viewpoint then the problem goes away, since we
> can just report all the reportable conditions as connection-failure
> messages.

We can stat a file that is owned by root and not readable to the user.
In such a case, because the user isn't able to read the file, all
libpq applications will fail making it effectively possible for the
system admin to shut off all applications that use libpq.  In the
other case of the permissions being unsafe, however, the user should
have the ability to chmod the file.  I actually think libpq _should_
chmod(2) the file for the user, issue a warning if it succeeds stating
that it chmod(2)'ed the file, or if it fails, then it should abort the
connection process.  Rationale being that if it exists, and the user
can't chmod it because they don't have access to it, then again,
you've DoS'ed the user trying to use an application that uses libpq.
If the ~/.pgpass file has inappropriate permissions and isn't owned by
the user, then libpq should not use the .pgpass file, continue on its
merry way, issue a warning, but not abort the connection process.

Hrm, this is almost turning into a mother knows best kind of scenario
where libpq is trying to out think the DBA....  is there a flag/env
var that can be set so that libpq drops its I know better than you
attitude in the event of a complex setup?  A real world example from
an Oracle shop I used to work at: ~/.pgpass is owned by the user,
group owned by the dba group, and password rotation is managed by the
dba group.

-sc

--
Sean Chittenden

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Tom Lane
Date:
Sean Chittenden <sean@chittenden.org> writes:
> Would you like me to add an interface that'd allow stderr to be
> redirected in the case of defaultNoticeProcessor()?

No, the application is supposed to set a notice processor if it doesn't
want stuff going to stderr.  I don't think that stuff needs to be
redesigned.

> We can stat a file that is owned by root and not readable to the user.
> In such a case, because the user isn't able to read the file, all
> libpq applications will fail making it effectively possible for the
> system admin to shut off all applications that use libpq.

Hardly, since the user can simply remove or rename the file.  It's in
his home directory, remember?  (If he can't write his home directory
then whether libpq works is the least of his troubles.)

            regards, tom lane

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Sean Chittenden
Date:
> > Would you like me to add an interface that'd allow stderr to be
> > redirected in the case of defaultNoticeProcessor()?
>
> No, the application is supposed to set a notice processor if it
> doesn't want stuff going to stderr.  I don't think that stuff needs
> to be redesigned.

Ehh... I'd say it's kinda broken as a logging interface in its current
encarnation.  Whoever does the autoconf magic should test for
stdarg.h.  Since we're on the verge of a minor version change, might
not be a bad time to adjust this.  There should be no source code
incompatabilities introduced in this change, but things will have to
be recompiled as the function ABI has changed.

> > We can stat a file that is owned by root and not readable to the
> > user.  In such a case, because the user isn't able to read the
> > file, all libpq applications will fail making it effectively
> > possible for the system admin to shut off all applications that
> > use libpq.
>
> Hardly, since the user can simply remove or rename the file.  It's
> in his home directory, remember?  (If he can't write his home
> directory then whether libpq works is the least of his troubles.)

*shrug* Whatever floats whoever's boat, I'm easy, just playing devils
advocate.  Now that I think about it though, there's no graceful way
to get PasswordFromFile() to tell the application to abort and nor
should it.  I think the warnings and bailing out of PasswordFromFile()
is sufficient.  Comments on the revised patch?  -sc

--
Sean Chittenden

Attachment

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Tom Lane
Date:
Sean Chittenden <sean@chittenden.org> writes:
> -typedef void (*PQnoticeProcessor) (void *arg, const char *message);
> +typedef void (*PQnoticeProcessor) (void *arg, const char *message, ...);

This isn't going to happen.  It would break every existing application
that uses a notice processor, since they are not expecting to have to
treat the message as a format string (quite aside from your unportable
assumption that this doesn't change the function's ABI).

            regards, tom lane

Re: Patch to be verbose about being unable to read

From
Peter Eisentraut
Date:
Sean Chittenden writes:

> Agreed, though one train of thought is that if someone did put a
> .pgpass file in place that isn't readable, it should be known that
> their efforts are being wasted in vein.

I tried making several dot-files I found in my home directory unreadable
(or unsearchable, in case of a directory): acrorc, antrc, bashrc, cvsrc,
fetchmailrc, inputrc, licq, mozilla, psqlrc, rpmmacros, Xmodmap, xemacs,
xmms.  Bash gave me warning, fetchmail gave me an error (which is
reasonable, because fetchmail doesn't work without configuration), and the
rest just ignored the file.

Open password and private key files might give different results, but as
far as unreadable files go, I say ignore them.

--
Peter Eisentraut   peter_e@gmx.net


Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Sean Chittenden
Date:
> > -typedef void (*PQnoticeProcessor) (void *arg, const char *message);
> > +typedef void (*PQnoticeProcessor) (void *arg, const char *message, ...);
>
> This isn't going to happen.  It would break every existing
> application that uses a notice processor, since they are not
> expecting to have to treat the message as a format string (quite
> aside from your unportable assumption that this doesn't change the
> function's ABI).

Um, I said it would change the ABI and I said as much in my previous
post.  When would be a better time to have this go in?  The major
version number of libpq is going to get bumped because of the various
changes that have gone in (isn't it?)... all the same, this shouldn't
break applications that are recompiled with libpq provided their error
message doesn't contain a format string: normal strings will work as
expected, it is code compatible provided people aren't using any %
escape strings in their error messages (if there aren't any format
strings, there won't be any need to modify the program), and it should
be easier for people to write error messages with some kind of context
(using vfprintf() is much easier than sprintf()'ing a message into a
buffer and passing the buffer to the handler - 1 LOC vs at the minimum
5).  In the release notes, ask people to use -Wformat if they're
trying to lint for old code.  The risk seems low to me.  *shrug*

-sc

--
Sean Chittenden

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Tom Lane
Date:
Sean Chittenden <sean@chittenden.org> writes:
> Um, I said it would change the ABI and I said as much in my previous
> post.

The ABI is only the smaller part of the problem; you'd be forcing people
to change their source code, in a rather nonobvious way.

> it is code compatible provided people aren't using any %
> escape strings in their error messages (if there aren't any format
> strings, there won't be any need to modify the program),

And if there aren't any format strings, then we don't need to change the
interface.  I don't see your point at all.  You cannot make use of the
added functionality without breaking client applications.

I could envision a helper procedure, known only within libpq, that has
a signature like formatNotice(PGconn* conn, const char *fmtstring, ...)
and encapsulates the work needed to handle a format string.  But I see
no reason to push that work out to client applications.

            regards, tom lane

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Sean Chittenden
Date:
> > Um, I said it would change the ABI and I said as much in my
> > previous post.
>
> The ABI is only the smaller part of the problem; you'd be forcing
> people to change their source code, in a rather nonobvious way.

*sigh* This isn't worth it at this point.  Having written several
logging interfaces recently, I thought it'd be a nice addition and
certainly one that I've appreciated as a consumer of my own APIs.
Regarding your point, however, it shouldn't require any changes:

func1(void *arg, char *msg) is written as:
    func1(NULL, "foobar\n");
    func1(NULL, "some static msg\n");
    func1(NULL, "some error\n");
    func1(NULL, "such and such is 100% full\n");

func2(void *arg, char *fmt, ...) is written as:
    func2(NULL, "foobar\n");
    func2(NULL, "some static msg\n");
    func2(NULL, "some error\n");
    func2(NULL, "blah happened at iteration %d in %s\n", i, "baz");
    func2(NULL, "such and such is 100%% full\n");

Only in the last example there does it require any modification of the
source code if you want to be completely spec compliant, however, even
in the last case, you'd have to provide an argument to % in order for
it to break, at which point the compile would fail if -Wformat is used
(maybe even without -Wformat, but I always have it on):

fprintf(3):
     %       A `%' is written.  No argument is converted.  The complete con-
             version specification is `%%'.

gcc(1):
       -Wformat
              Check calls to printf and scanf, etc., to make sure that the ar-
              guments supplied have types appropriate  to  the  format  string
              specified.

> > it is code compatible provided people aren't using any % escape
> > strings in their error messages (if there aren't any format
> > strings, there won't be any need to modify the program),
>
> And if there aren't any format strings, then we don't need to change
> the interface.  I don't see your point at all.  You cannot make use
> of the added functionality without breaking client applications.

See above.  With libpq's version being bumped (is it?), apps will have
to be recompiled to make use of the new protocol, there shouldn't be
any extra mangling of app source code unless their strings contain
"%[\w]{1,2}", which is a relatively uncommon pattern to run across.

Like I said though, no biggie, just something that's a nice to have
for logging interfaces, so in the advent that we want all cases of
fprintf() to be removed from libpq (ex: PasswordFromFile()), the above
seems like an appropriate and low risk option.  -sc

--
Sean Chittenden

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Sean Chittenden
Date:
> I could envision a helper procedure, known only within libpq, that has
> a signature like formatNotice(PGconn* conn, const char *fmtstring, ...)
> and encapsulates the work needed to handle a format string.  But I see
> no reason to push that work out to client applications.

I could make fnoticeProc private if you'd like, but that doesn't seem
smart if the user can override noticeFormat but can't with the
internal bits used by libpq... unless you're envisioning a private
method that constructs a msg for you, calls noticeProc with the buffer
as the arg, then then free()'s the result.

All regression tests pass with this case and no ABI or source
incompatabilities are introduced.

-sc

--
Sean Chittenden

Attachment

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Sean Chittenden
Date:
> > All regression tests pass with this case and no ABI or source
> > incompatabilities are introduced.
>
> You're still playing extremely fast and loose with that claim of "no
> incompatibilities introduced".  This may not *directly* break an
> application that uses PQsetNoticeProcessor, but it breaks the purpose
> that the app is using PQsetNoticeProcessor for, namely to intercept
> notices.  If it doesn't also override the fnoticeProc then it hasn't
> intercepted all the notices.

Those notices were sent to setderr earlier, they were already broken:
at least now there's a ghost of a chance at the app at picking up
those errors.

> What I was envisioning was a simple little subroutine that takes a
> format string and some args, formats into a work buffer, and then
> passes the formatted string to the existing notice hook.
> Essentially, moving the five lines you're unhappy about into a
> common subroutine.  This gets the job done without *any*
> modification of the application API.

*nods*

> A bigger problem with the whole approach is that there's no way for
> the application to install a notice processor before
> PasswordFromFile is invoked, because that occurs during PQconnectdb.
> So any notices emitted by same will necessarily fall into the lap of
> the default notice processor.

Which is why I thought fprinting to stderr isn't that bad of an idea
given there are only two instances inside of PasswordFromFile that
make use of the noticeProc. :)

> I didn't hear any strong objection to the idea of treating the
> weak-protection complaint as a hard error (connection failure), so
> we can fix the already-existing problem by doing that.

libpq's a library used by many many applications ... do you envison
calling abort(), exit() or _exit()?  Seems really broken to halt the
app based on FS permissions.  Imagine an ISP web based environment
with PHP using libpq to connect to PostgreSQL and an insecure .pgpass
file.  Issuing a warning should be plenty sufficient.  Since this is a
case of "mother knows best," IMHO, it'd probably be wise to only print
these warnings if a debugging variable was set inside of libpq.

> As for the issue of whether to complain about a file that exists but
> isn't readable, Peter exhibited lots of precedent for not doing so.
> I'm inclined to think we should drop that part.

*shrug* Whatever the consensus is, raising awareness isn't a bad
thing, halting execution can be fatal.  The app that carries the most
weight in terms of precedent with me is gnupg because gnupg
intentionally deals with secure content/passwords.  Here are the perm
checks that gnupg does:

*) make sure the homedir, .gnupg dir, and libdir are owned by the
 user/root
*) make sure the homedir, .gnupg dir, and libdir are "safe" for the
 user/root

It's not extensive, but, in the event of a problem, it only prints to
stderr and it doesn't bomb out (gnupg-1.2.2/g10/g10.c:885).  With
GnuPG as the extreme in terms of security and its list of checks a bit
more extensive (though not exhaustive), bombing out seems like a bad
idea.

Here's another case to not bomb out.  An admin in a php web env types:

% vi ~www/.pgpass
[enters in a .pgpass file]
% chown www ~www/.pgpass
% chmod 0600 ~www/.pgpass

Game over.  During the period of time from when the admin writes the
file to chmod 0600's the file, all database requests will be
denied... which could be substantial in a web environment, and bad for
PostgreSQL's image, esp with the talk of MySQL alienating its users
and Pg trying to pick up steam in the PHP community.

-sc


PS I really will lay off the topic, just trying to make a good case
   for a usable and secure library.  :)

--
Sean Chittenden

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Tom Lane
Date:
Sean Chittenden <sean@chittenden.org> writes:
> All regression tests pass with this case and no ABI or source
> incompatabilities are introduced.

You're still playing extremely fast and loose with that claim of "no
incompatibilities introduced".  This may not *directly* break an
application that uses PQsetNoticeProcessor, but it breaks the purpose
that the app is using PQsetNoticeProcessor for, namely to intercept
notices.  If it doesn't also override the fnoticeProc then it hasn't
intercepted all the notices.

What I was envisioning was a simple little subroutine that takes a
format string and some args, formats into a work buffer, and then passes
the formatted string to the existing notice hook.  Essentially, moving
the five lines you're unhappy about into a common subroutine.  This
gets the job done without *any* modification of the application API.

A bigger problem with the whole approach is that there's no way for
the application to install a notice processor before PasswordFromFile
is invoked, because that occurs during PQconnectdb.  So any notices
emitted by same will necessarily fall into the lap of the default
notice processor.

I didn't hear any strong objection to the idea of treating the
weak-protection complaint as a hard error (connection failure),
so we can fix the already-existing problem by doing that.  As
for the issue of whether to complain about a file that exists but
isn't readable, Peter exhibited lots of precedent for not doing so.
I'm inclined to think we should drop that part.

            regards, tom lane

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Tom Lane
Date:
Sean Chittenden <sean@chittenden.org> writes:
> Those notices were sent to setderr earlier, they were already broken:
> at least now there's a ghost of a chance at the app at picking up
> those errors.

The notice in PasswordFromFile is broken, yes, but it is a recent
addition; it has not been there long and has no seniority in my mind.
And no, there's no "ghost of a chance" for the app to pick it up because
PasswordFromFile runs before the app has any opportunity to install a
notice hook into the new connection object.

>> I didn't hear any strong objection to the idea of treating the
>> weak-protection complaint as a hard error (connection failure), so
>> we can fix the already-existing problem by doing that.

> libpq's a library used by many many applications ... do you envison
> calling abort(), exit() or _exit()?

No, I envision returning a failed connection.

> Seems really broken to halt the
> app based on FS permissions.  Imagine an ISP web based environment
> with PHP using libpq to connect to PostgreSQL and an insecure .pgpass
> file.  Issuing a warning should be plenty sufficient.

There are environments in which attempting to print on stderr leads to
a core dump (Windows is or at least used to be that way).  Returning a
failed connection should not be something the app can't deal with ---
if it is, that app has got problems anyway.  But insisting that we can
print on stderr may well be something it cannot overcome.

            regards, tom lane

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Sean Chittenden
Date:
> > Those notices were sent to setderr earlier, they were already
> > broken: at least now there's a ghost of a chance at the app at
> > picking up those errors.
>
> The notice in PasswordFromFile is broken, yes, but it is a recent
> addition; it has not been there long and has no seniority in my
> mind.  And no, there's no "ghost of a chance" for the app to pick it
> up because PasswordFromFile runs before the app has any opportunity
> to install a notice hook into the new connection object.

Bah, this is nuts: I have too many balls in the air at the moment and
need to spend a half minute thinking about things.  My apologies.

In the current form it's not possible to catch unless stderr is going
to a log file or the terminal, which is what I had in mind.  In the
case of Qt or some other GUI, writes to stderr where stderr is closed
doesn't kill the app.

> There are environments in which attempting to print on stderr leads
> to a core dump (Windows is or at least used to be that way).

Doesn't sound like that's the case any more, though I don't have a
win32 machine to test with.  My understanding is that mingw handles
this correctly, likely same with cygwin.  Native MSVC, however, I have
no clue.

> Returning a failed connection should not be something the app can't
> deal with --- if it is, that app has got problems anyway.  But
> insisting that we can print on stderr may well be something it
> cannot overcome.

If the app can't, and to deal with the case of PasswordFromFile()
being called before the connection is returned to the user, would you
object to adding an optional function to libpq that lets you allocate
a pg_conn and then pass pg_conn to PGconnectdb2()?

conn = PGnewconn();
conn->hostname = "foo";
conn->noticeHooks.noticeProc = myHandler;
/* setup the connection struct manually */
if (PGconnectdb2(conn) == true) {
  /* Connected */
} else {
  /* Not connected */
}

This wouldn't affect existing code and would still allow calling
PGconnectdb() to get a new connection handle.  I'll make the
fnoticeProc introduced above private and have it pass the resulting
buffer to noticeProc.

I maintain that having a security concern in PasswordFromFile() cause
the connection to abort as the default behavior is a bad idea.  It can
be the default with PostgreSQL's CLI utilities that make use of libpq
(would need to add a paranoid flag to the connection), but it
shouldn't be the default.

-sc

--
Sean Chittenden

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Tom Lane
Date:
Sean Chittenden <sean@chittenden.org> writes:
> I maintain that having a security concern in PasswordFromFile() cause
> the connection to abort as the default behavior is a bad idea.

That is a legitimate concern.  Doesn't seem like we have any really
clean way to satisfy all the needs here.

One idea I was toying with was to have PasswordFromFile set a flag in
the PGconn struct indicating that .pgpass has permissions problems.
Then some later routine could check the flag and issue the notice if
needed, giving the app a chance to install its notice handler
beforehand.  However, this begs the question of *which* later routine
should do this.  PQexec would once have been the obvious choice, but
as of 7.4 it's quite possible that some apps would never use it.
And I don't want to sprinkle the issue through a bunch of different
routines.  Maybe PQgetResult would be a safe bet that all apps would
go through (directly or indirectly).  Comments?

            regards, tom lane

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Bruce Momjian
Date:
Is there a TODO here?  Tom has added the printf functionality already.


---------------------------------------------------------------------------

Tom Lane wrote:
> Sean Chittenden <sean@chittenden.org> writes:
> > I maintain that having a security concern in PasswordFromFile() cause
> > the connection to abort as the default behavior is a bad idea.
>
> That is a legitimate concern.  Doesn't seem like we have any really
> clean way to satisfy all the needs here.
>
> One idea I was toying with was to have PasswordFromFile set a flag in
> the PGconn struct indicating that .pgpass has permissions problems.
> Then some later routine could check the flag and issue the notice if
> needed, giving the app a chance to install its notice handler
> beforehand.  However, this begs the question of *which* later routine
> should do this.  PQexec would once have been the obvious choice, but
> as of 7.4 it's quite possible that some apps would never use it.
> And I don't want to sprinkle the issue through a bunch of different
> routines.  Maybe PQgetResult would be a safe bet that all apps would
> go through (directly or indirectly).  Comments?
>
>             regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
>       joining column's datatypes do not match
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Tom Lane
Date:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Is there a TODO here?  Tom has added the printf functionality already.

We still have to do something about the fprintf(stderr) call that's in
PasswordFromFile already.  What do you think of my suggestion of a
delayed notice?

            regards, tom lane

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Bruce Momjian
Date:
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Is there a TODO here?  Tom has added the printf functionality already.
>
> We still have to do something about the fprintf(stderr) call that's in
> PasswordFromFile already.  What do you think of my suggestion of a
> delayed notice?

That seemed pretty complicated because it added a structure member.  I
don't have problems sending the output to stderr myself.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Re: Patch to be verbose about being unable to read ~/.pgpasss...

From
Tom Lane
Date:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I don't have problems sending the output to stderr myself.

That's because you don't run libpq in any contexts where that behavior
is inappropriate ... but there are plenty such contexts.  Most GUI
applications will want to trap any and all notices and put them up in
a window; sending to stderr is likely to mean sending to /dev/null,
or at best a logfile that the user will never look at.

            regards, tom lane