[PROPOSAL] Client Log Output Filtering

David Steele wrote:

Currently log messages generated by pgaudit can be made visible to the
client simply by altering client_min_messages. While this has not been a
showstopper for anyone it's ideal, either.

The client authentication code sets the global variable ClientAuthInProgress
which causes ereport() to filter client output < ERROR while forcing client
output >= ERROR. This functionality would also work well for pgaudit.

The patch creates a new counter to separate the log filtering from the
authentication functionality. This makes it possible to get the same
filtering in other parts of the code (or extensions) without abusing the
ClientAuthInProgress variable or using the log hook.

Hmm, okay, but this would need a large blinking comment explaining that
the calling code have better set a PG_TRY block when incrementing, so
that on errors it resets to zero again. Or maybe some handling in
AbortOutOfAnyTransaction/AbortCurrentTransaction. or both.

I also considered a new function for ereport (like errhidecontext()) but
this mechanism would not have worked for authentication and so would not
have been used anywhere in core.

But then, you already know that authentication phase is not exactly the
same use case as security auditing, so they don't have to work the same
way necessarily. I think this needs more discussion. If the audit code
calls something that throws an error (other than an audit message -- say
"out of memory"), then it would also be hidden, but you may not want it
to be hidden. I think maybe it's better to have each individual error
message be marked as "don't show to client" rather than a global var.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jan 11, 2016 at 6:50 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

I think maybe it's better to have each individual error
message be marked as "don't show to client" rather than a global var.

+1.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 1/11/16 6:50 PM, Alvaro Herrera wrote:

David Steele wrote:

The patch creates a new counter to separate the log filtering from the
authentication functionality. This makes it possible to get the same
filtering in other parts of the code (or extensions) without abusing the
ClientAuthInProgress variable or using the log hook.

Hmm, okay, but this would need a large blinking comment explaining that
the calling code have better set a PG_TRY block when incrementing, so
that on errors it resets to zero again. Or maybe some handling in
AbortOutOfAnyTransaction/AbortCurrentTransaction. or both.

In the case of pgaudit only the ereport call is wrapped in the
LimitClientLogOutput() calls which I thought was safe - am I wrong about
that?

I also considered a new function for ereport (like errhidecontext()) but
this mechanism would not have worked for authentication and so would not
have been used anywhere in core.

If the audit code
calls something that throws an error (other than an audit message -- say
"out of memory"), then it would also be hidden, but you may not want it
to be hidden.

This shouldn't happen -- see above.

I think maybe it's better to have each individual error

message be marked as "don't show to client" rather than a global var.

That's easy enough to do and I already have the code for it since that's
the first thing I tried. However, there were two reasons I didn't
submit that version:

1) Unless pgaudit is committed there wouldn't be any code calling the
errhidefromclient() function and that seemed like a bad plan.

2) There would be two different ways to suppress client messages but I
was hoping to only have one.

As you say, authentication is a different beast so maybe #2 is not a big
deal. I could combine the alternative ereport patch with the pgaudit
patch to address #1 but I would like to have the capability in core
whether pgaudit is committed or not, which is why I submitted it separately.

Any advice would be greatly appreciated.

Thanks,
--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

David Steele wrote:

On 1/11/16 6:50 PM, Alvaro Herrera wrote:

David Steele wrote:

The patch creates a new counter to separate the log filtering from the
authentication functionality. This makes it possible to get the same
filtering in other parts of the code (or extensions) without abusing the
ClientAuthInProgress variable or using the log hook.

Hmm, okay, but this would need a large blinking comment explaining that
the calling code have better set a PG_TRY block when incrementing, so
that on errors it resets to zero again. Or maybe some handling in
AbortOutOfAnyTransaction/AbortCurrentTransaction. or both.

In the case of pgaudit only the ereport call is wrapped in the
LimitClientLogOutput() calls which I thought was safe - am I wrong about
that?

Yeah, errstart itself could fail. It's not common but it does happen.

1) Unless pgaudit is committed there wouldn't be any code calling the
errhidefromclient() function and that seemed like a bad plan.

Well, you could add a test module to ensure it continues to work.

2) There would be two different ways to suppress client messages but I was
hoping to only have one.

I think they are two different things actually.

I'm closing this as returned with feedback.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2/1/16 5:25 PM, Alvaro Herrera wrote:

David Steele wrote:

2) There would be two different ways to suppress client messages but I was
hoping to only have one.

I think they are two different things actually.

Fair enough - that was my initial reaction as well but then I thought
the other way would be better.

I'm closing this as returned with feedback.

I have attached a patch that adds an ereport() macro to suppress client
output for a single report call (applies cleanly on 1d0c3b3). I'll also
move it to the next CF.

Thanks!
--
-David
david@pgmasters.net

On Mon, Feb 1, 2016 at 7:24 PM, David Steele <david@pgmasters.net> wrote:

On 2/1/16 5:25 PM, Alvaro Herrera wrote:

David Steele wrote:

2) There would be two different ways to suppress client messages but I was
hoping to only have one.

I think they are two different things actually.

Fair enough - that was my initial reaction as well but then I thought
the other way would be better.

I'm closing this as returned with feedback.

I have attached a patch that adds an ereport() macro to suppress client
output for a single report call (applies cleanly on 1d0c3b3). I'll also
move it to the next CF.

I don't see any reason not to accept this.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 03/02/16 05:02, Robert Haas wrote:

On Mon, Feb 1, 2016 at 7:24 PM, David Steele <david@pgmasters.net> wrote:

I have attached a patch that adds an ereport() macro to suppress client
output for a single report call (applies cleanly on 1d0c3b3). I'll also
move it to the next CF.

I don't see any reason not to accept this.

Yes, the idea seems sane.

Looking at the code, this adds bool hide_from_client to edata which is
not initialized in errstart so that needs to be fixed.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 3/9/16 7:37 PM, Petr Jelinek wrote:

On 03/02/16 05:02, Robert Haas wrote:

On Mon, Feb 1, 2016 at 7:24 PM, David Steele <david@pgmasters.net> wrote:

I have attached a patch that adds an ereport() macro to suppress client
output for a single report call (applies cleanly on 1d0c3b3). I'll also
move it to the next CF.

I don't see any reason not to accept this.

Yes, the idea seems sane.

Looking at the code, this adds bool hide_from_client to edata which is
not initialized in errstart so that needs to be fixed.

I figured this would take care of it:

MemSet(edata, 0, sizeof(ErrorData));

Since I want hide_from_client to default to false. Am I missing something?

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

David Steele <david@pgmasters.net> writes:

On 3/9/16 7:37 PM, Petr Jelinek wrote:

Looking at the code, this adds bool hide_from_client to edata which is
not initialized in errstart so that needs to be fixed.

I figured this would take care of it:
MemSet(edata, 0, sizeof(ErrorData));
Since I want hide_from_client to default to false. Am I missing something?

The patch is evidently modeled on errhidestmt and errhidectx, which are
making the same assumption for their fields.

I wonder whether, instead of continuing to proliferate random bool fields
in struct ErrorData, we oughta replace them all with an "int flags" field.
That's probably material for a separate patch though.

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

David Steele <david@pgmasters.net> writes:

I have attached a patch that adds an ereport() macro to suppress client
output for a single report call (applies cleanly on 1d0c3b3). I'll also
move it to the next CF.

This patch fails to add the necessary documentation (see sources.sgml)

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

On 10/03/16 15:08, David Steele wrote:

Looking at the code, this adds bool hide_from_client to edata which is
not initialized in errstart so that needs to be fixed.

I figured this would take care of it:

MemSet(edata, 0, sizeof(ErrorData));

Since I want hide_from_client to default to false. Am I missing something?

Right, missed that, sorry for the noise.

I have another issue though.

The comment above errhidefromclient says "Only log levels below ERROR
can be hidden from the client." but use of the errhidefromclient(true)
actually does hide the error message from client, client just gets
failed query without any message when used with ERROR level.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Petr Jelinek <petr@2ndquadrant.com> writes:

The comment above errhidefromclient says "Only log levels below ERROR
can be hidden from the client." but use of the errhidefromclient(true)
actually does hide the error message from client, client just gets
failed query without any message when used with ERROR level.

Um. That seems pretty broken --- I think it's a violation of the wire
protocol spec.

I notice though that we allow client_min_messages to be set to FATAL,
which would be a different way of violating the protocol. Maybe we
should reduce the max setting of that to ERROR?

libpq/psql seem to more or less survive this situation:

regression=# set client_min_messages to fatal;
SET
regression=# select 2/0;
regression=# select 1;
?column?
----------
1
(1 row)

but it doesn't really seem like a great idea.

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

On 10/03/16 17:07, Tom Lane wrote:

Petr Jelinek <petr@2ndquadrant.com> writes:

The comment above errhidefromclient says "Only log levels below ERROR
can be hidden from the client." but use of the errhidefromclient(true)
actually does hide the error message from client, client just gets
failed query without any message when used with ERROR level.

Um. That seems pretty broken --- I think it's a violation of the wire
protocol spec.

I was thinking that as well. The doc says that on error the
ErrorResponse is sent and connection is closed and we clearly fail to
send the ErrorResponse in this case.

I notice though that we allow client_min_messages to be set to FATAL,
which would be a different way of violating the protocol. Maybe we
should reduce the max setting of that to ERROR?

Hmm yeah that seems to be that way for very long time though so I wonder
if that's intentional although it's also against protocol spec then. In
any case I would be in favor of lowering the max setting to ERROR.

For the patch at hand, it should be sufficient for errhidefromclient()
to check that the edata->elevel is lower than ERROR.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 3/10/16 11:00 AM, Petr Jelinek wrote:

The comment above errhidefromclient says "Only log levels below ERROR
can be hidden from the client." but use of the errhidefromclient(true)
actually does hide the error message from client, client just gets
failed query without any message when used with ERROR level.

Right you are. The v3 patch adds this check.

I also added the documentation to sources.sgml that Tom pointed out was
missing.

Thanks,
--
-David
david@pgmasters.net

On 3/10/16 11:07 AM, Tom Lane wrote:

Petr Jelinek <petr@2ndquadrant.com> writes:

The comment above errhidefromclient says "Only log levels below ERROR
can be hidden from the client." but use of the errhidefromclient(true)
actually does hide the error message from client, client just gets
failed query without any message when used with ERROR level.

Um. That seems pretty broken --- I think it's a violation of the wire
protocol spec.

I notice though that we allow client_min_messages to be set to FATAL,
which would be a different way of violating the protocol. Maybe we
should reduce the max setting of that to ERROR?

This was the same conclusion I came to for the log_level setting in pgaudit.

I'll submit a proposal to hackers after 9.6 to make this change.

--
-David
david@pgmasters.net

On 3/10/16 9:51 AM, Tom Lane wrote:

The patch is evidently modeled on errhidestmt and errhidectx, which are
making the same assumption for their fields.

I wonder whether, instead of continuing to proliferate random bool fields
in struct ErrorData, we oughta replace them all with an "int flags" field.
That's probably material for a separate patch though.

There are currently six boolean flags (if you include my new one) that
all relate to visibility in one way or another. Combining them into a
"flags" field makes sense to me.

I'll submit a proposal to hackers after 9.6 to make this change.

--
-David
david@pgmasters.net

David Steele <david@pgmasters.net> writes:

On 3/10/16 11:00 AM, Petr Jelinek wrote:

The comment above errhidefromclient says "Only log levels below ERROR
can be hidden from the client." but use of the errhidefromclient(true)
actually does hide the error message from client, client just gets
failed query without any message when used with ERROR level.

Right you are. The v3 patch adds this check.

I also added the documentation to sources.sgml that Tom pointed out was
missing.

I set to work on committing this, but was soon rather dissatisfied with
it, because as-implemented there is no way to short-circuit elog.c's
processing if the message is not to be sent to either the client or the
postmaster log. Ideally this would be taken into account when errstart()
figures the output_to_client setting to begin with --- but of course we
can't do that with this API, because errhidefromclient() hasn't run yet.

The patch is buggy even without that consideration, because it would
potentially disable client output of messages even after they've been
promoted to FATAL by pg_re_throw. We could fix that by clearing the flag
in pg_re_throw, but I think that's just another symptom of the shutoff
being done in the wrong place.

I wonder just what the expected usage scenario is for this function, and
whether it would not be better addressed by inventing some other API
rather than modeling it on errhidestmt(), which is by no means the same
kind of thing.

One idea is to invent a new elevel which is never sent to the client ---
analogously to COMMERROR, though probably much higher priority than that,
maybe the same priority as LOG. If there actually is a use for a range of
elevels on errhidefromclient'd messages, that wouldn't work very well of
course. Or we could consider having a flag bit that is OR'd into the
elevel, along the lines of

ereport(LOG | ERR_HIDE_FROM_CLIENT, (errmsg(....)));

so that the information is available at errstart time.

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

On 3/28/16 2:00 PM, Tom Lane wrote:

I set to work on committing this, but was soon rather dissatisfied with
it, because as-implemented there is no way to short-circuit elog.c's
processing if the message is not to be sent to either the client or the
postmaster log. Ideally this would be taken into account when errstart()
figures the output_to_client setting to begin with --- but of course we
can't do that with this API, because errhidefromclient() hasn't run yet.

That's a weakness of this approach but I'm not sure it's a big deal
since there will generally still be output on the server. If both are
suppressed I think that would be a sign of misconfiguration.

The patch is buggy even without that consideration, because it would
potentially disable client output of messages even after they've been
promoted to FATAL by pg_re_throw. We could fix that by clearing the flag
in pg_re_throw, but I think that's just another symptom of the shutoff
being done in the wrong place.

Or elevel could be checked in EmitErrorReport():

if (edata->output_to_client &&
!(edata->hide_from_client && edata->elevel < ERROR))
send_message_to_frontend(edata);

I wonder just what the expected usage scenario is for this function, and
whether it would not be better addressed by inventing some other API
rather than modeling it on errhidestmt(), which is by no means the same
kind of thing.

The particular use case I have in mind is to suppress client output in
pgaudit. The original patch took a different approach by trying to
merge with the logic to suppress messages in auth. Maybe you should
take a look at that patch and see what you think:

/messages/by-id/5655B621.3080906@pgmasters.net

One idea is to invent a new elevel which is never sent to the client ---
analogously to COMMERROR, though probably much higher priority than that,
maybe the same priority as LOG. If there actually is a use for a range of
elevels on errhidefromclient'd messages, that wouldn't work very well of
course. Or we could consider having a flag bit that is OR'd into the
elevel <...>

I think a flag would be more flexible than introducing a new log level.

--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

David Steele <david@pgmasters.net> writes:

On 3/28/16 2:00 PM, Tom Lane wrote:

One idea is to invent a new elevel which is never sent to the client ---
analogously to COMMERROR, though probably much higher priority than that,
maybe the same priority as LOG. If there actually is a use for a range of
elevels on errhidefromclient'd messages, that wouldn't work very well of
course. Or we could consider having a flag bit that is OR'd into the
elevel <...>

I think a flag would be more flexible than introducing a new log level.

I thought about this some more, and while the flag-bit approach definitely
has some attraction, it also has a big problem: there are lots of places
with code like "if (elevel >= ERROR)" which would be at risk of getting
confused, and I'm not confident we'd find them all. We could possibly
dodge that by shifting the elevel constants up a couple of bits and
putting the flag in the LSB rather than a high-order bit; but that's a
bit icky/risky too.

Repurposing COMMERROR is definitely starting to seem like a low-impact
solution compared to these others. Under what circumstances would you
be wanting hide-from-client with an elevel different from LOG, anyway?

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