To: KaiGai Kohei <kaigai@ak.jp.nec.com>
cc: Stephen Frost <sfrost@snowman.net>, robertmhaas@gmail.com,
	pgsql-hackers@postgresql.org, kaigai@kaigai.gr.jp
Subject: Re: Reworks for Access Control facilities (r2363) 
In-reply-to: <4AD54082.9050001@ak.jp.nec.com> 
References: <4AC5A14F.6050306@ak.jp.nec.com>
	<20091012024604.GW17756@tamriel.snowman.net>
	<4AD54082.9050001@ak.jp.nec.com>
Comments: In-reply-to KaiGai Kohei <kaigai@ak.jp.nec.com>
	message dated "Wed, 14 Oct 2009 12:07:46 +0900"
Date: Thu, 15 Oct 2009 13:22:02 -0400
Message-ID: <10495.1255627322@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
> [ patch r2363 ]

I promised I would review this today, but I just can't make myself do it
in any detail.  This is too large, too ugly, and it is going in a
direction that I do not like or want to spend any of my time on.

The overwhelming impression after a brief read-through is that the
code has been hacked apart with a chainsaw and reassembled into a
Frankenstein's monster --- it's alive, but man is it ugly.  Code
comments that refer to something "above" or "below" are still there,
but the referent is no longer close enough for that to be a reasonable
way of referring to it.  It's impossible to follow what's going on or
why, either in the shim functions or in the callers --- in the original
coding there was context for the aclcheck calls, now there isn't.

I don't have any confidence that this is a sane way to proceed forward.
The shim layer knows everything about everything --- there may still
be a few backend .h files it doesn't include, but that's not for lack
of trying.  The direction this is heading in is an unmaintainable
giant-bowl-of-spaghetti security module, rather than something that can
be divided into understandable parts.  And I don't think it's really
removed any complexity from the callers, nor do I believe that it's
going to be a useful basis for imposing a different security policy
than the one we have now.  Two specific examples of why not:

* The "skip permissions checks" arguments that have been added to
various random functions suggest strongly that the factoring still isn't
right --- I especially don't believe in that in the context of
performDeletion and friends.

* There are two special-purpose shims, ac_database_calculate_size and
ac_tablespace_calculate_size, that got added for the benefit of
utils/adt/dbsize.c.  What if that code were still in contrib?  How is it
different from a lot of the code that is in contrib now, eg dblink or
pgrowlocks, to say nothing of third-party modules?  Presuming that the
shim layer can know explicitly about each individual permission-checking
requirement is a dead-end design.

Maybe if I weren't burned out after a month of CommitFesting, I could
muster a more positive reaction, but right now I just can't summon any
enthusiasm for this.

			regards, tom lane