Re: Audit of logout

On Mon, Jun 23, 2014 at 5:42 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway <mail(at)joeconway(dot)com> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 06/13/2014 07:29 AM, Tom Lane wrote:
>>> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
>>>> On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
>>>> <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>>> Some users enable log_disconnections in postgresql.conf to
>>>>> audit all logouts. But since log_disconnections is defined with
>>>>> PGC_BACKEND, it can be changed at connection start. This means
>>>>> that any client (even nonsuperuser) can freely disable
>>>>> log_disconnections not to log his or her logout even when the
>>>>> system admin enables it in postgresql.conf. Isn't this
>>>>> problematic for audit?
>>>
>>>> That's harmful for audit purpose. I think that we should make
>>>> log_disconnections PGC_SUSET rather than PGC_BACKEND in order to
>>>> forbid non-superusers from changing its setting. Attached patch
>>>> does this.
>>
>> This whole argument seems wrong unless I'm missing something:
>>
>> test=# set log_connections = on;
>> ERROR: parameter "log_connections" cannot be set after connection start
>> test=# set log_disconnections = off;
>> ERROR: parameter "log_disconnections" cannot be set after connection
>> start
>
> You can change log_connections/disconnections via connection option as follows
>
> $ grep log_disconnections $PGDATA/postgresql.conf
> log_disconnections = on
>
> $ psql -U hoge -d "options='-c log_disconnections=off'"
> => show log_disconnections ;
> log_disconnections
> --------------------
> off
> (1 row)
>
> => \du
> List of roles
> Role name | Attributes | Member of
> -----------+------------------------------------------------+-----------
> hoge | | {}
> postgres | Superuser, Create role, Create DB, Replication | {}
>
>
>>> I wonder whether we should just get rid of log_disconnections as a
>>> separate variable, instead logging disconnections when
>>> log_connections is set.
>>
>>
>> That might be a good idea though.
>
> David pointed the merit of keeping those two parameters separate upthread
> and I understand his thought.
> http://www.postgresql.org/message-id/1402675662004-5807224.post@n5.nabble.com

Let me explain the problem which I'd like to address here, again.

The problem is that any client (even non-superuser) can change the setting of
log_disconnections when it connects to the server. For example, you can do by
using "options" connection parameter as follows.

$ psql -U hoge -d "options='-c log_disconnections=off'"
=> show log_disconnections ;
log_disconnections
--------------------
off
(1 row)

This means that, even when DBA enables log_disconnections in postgresql.conf
in order to audit all the logouts, a client can freely avoid logging
of his or her
logout for some reasons. I think this situation problematic especially for audit
purpose.

You may think that logging logins is enough for audit purpose and
logging logouts
is not so important. But imagine the case where all logins are logged but some
corresponding logouts are not logged. This would make it difficult to check and
verify the validities of audit logs. It's not easy to handle such
users that only
login is logged.

Any client can enable/disable log_disconnections because it's defined with
PGC_BACKEND context. By definition, GUC parameters with PGC_BACKEND can
be changed at connection startup. But it cannot be changed after connection is
established.

BTW, log_connections is also defined with PGC_BACKEND, so any client can
change its setting at connection startup. But *fortunately* it's used
by the server
before the changed value is given from a client at connection startup. So, the
server always uses the setting value in postgresql.conf, whether a
client tries to
change log_connections or not. Therefore log_connections doesn't cause the
problem which I described the above at all. That's good for audit purpose. OTOH,
ISTM that defining log_connections with PGC_BACKEND is a bit strange since
a client cannot change it at all.

To address the problem, I'm thinking to change the GUC context of
log_disconnections from PGC_BACKEND to PGC_SIGHUP. This prevents a client
from changing the setting. In the top of the thread, I posted the patch which
changed the context to PGC_SUSET, but now I'm thinking PGC_SIGHUP is better.
Since log_disconnections is used (if it's enabled, the callback
function which logs
lotout is registered) just after connection is established, using PGC_SUSET and
allowing superuser to change the setting by SET command is useless.

Changing the GUC context of log_disconnections cause two behavior changes.

(1) As I explained the above, it prevents a client from changing the setting.
Yeah, this is what I want.

(2) It allows DBA to change log_disconnections of running backends by
reloading the configuration file. But such changed value has no effect on
whether logout is logged or not because running backend will never use it
except SHOW command. As I explained the above, log_disconnections is
*fixed* only just after connection is established.

SHOW command displays the changed value. But it might not be the value
which the running backend really uses. You may think that this
inconsistency
is a problem. For this, we can add new GUC flag which forbid the reload of
configuration file to change the setting, and redefine log_disconnections
with that flag. Do we need this?

I don't think that my idea causes serious behavior changes which can break
existing application. So I think that it's not problematic to change the GUC
context of log_disconnections to PGC_SIGHUP. Thought?

Regards,

--
Fujii Masao