|From:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>|
|To:||KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>|
|Cc:||Stephen Frost <sfrost(at)snowman(dot)net>, robertmhaas(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org, kaigai(at)kaigai(dot)gr(dot)jp|
|Subject:||Re: Reworks for Access Control facilities (r2363)|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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
|Next Message||Josh Berkus||2009-10-15 17:22:52||Re: Could regexp_matches be immutable?|
|Previous Message||Robert Haas||2009-10-15 17:19:10||Re: Rejecting weak passwords|