Re: [PATCH] SE-PgSQL/lite rev.2163

2009/7/13 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> The sepgsql/avc.c provides a facility to cache access control decisions
> in userspace, and enables to reduce time of kernel invocations.
> However, its size is the largest one in the SE-PgSQL patch.

I think that caching access control decisions in userspace is a good
thing and I don't think you should remove it. What I'm concerned
about is the fact that you're making the security ID part of the heap
tuple header, like OID, rather than a normal column. These are two
separate issues.

>  [kaigai(at)saba gram]$ wc -l src/backend/security/sepgsql/avc.c
>  829 src/backend/security/sepgsql/avc.c
>
> I think separation of the avc facility contributes to keep code
> size smaller, rather than pg_security mechanism.
>
>  [kaigai(at)saba gram]$ wc -l src/backend/catalog/pg_security.c
>  381 src/backend/catalog/pg_security.c
>
> I guess it enables to reduce 20% of patch scale. (800L/4KL)
>
> I can also add more source code comments, such as when the security
> hooks to be invoked, what is the purpose and so on, instead of ruduced
> lines. It will make clear where the hooks to be placed.

Comments are always good, but I think you're missing the point. The
point here is that everything I'm pointing out now has been pointed
out in previous review rounds, and you've decide not to do it. For
example, consider this:

http://archives.postgresql.org/message-id/498030E8.9030905@gmx.net

I call your attention to the following paragraph in particular:

"If I had to do this, I would first write a patch for #1: A patch that
additionally executes existing privilege checks against an SELinux
policy. Existing privilege checks are a well-defined set: they mostly
happen through pg_xxx_aclcheck() functions. Hook your checks in there.
This patch would already provide useful functionality, but it would
be much easier to review and verify, because we know how permission
checking works in the existing system, so we can compare them. And it
would build confidence among developers and users about the whole
idea, about SELinux integration etc."

Now, here we are five-and-a-half months later, and guess what? I'm
telling you that you need to put your privilege checks in
pg_xxx_aclcheck() rather than having them scattered all over the code.
I didn't even know when I wrote that that Peter had made EXACTLY THE
SAME SUGGESTION back in January, and judging by his comments,
apparently not for the first time.

You need to decide whether you want to maintain this as a private
patch set forever (in which case you can do whatever you want) or
whether you want to try to create something that is acceptable to the
committers (in which case you need to be responsive to their
feedback). You've said multiple times that your goal is the latter,
but you've had five months to rework this patch since the last round
of reviewing, and I am now spending my limited reviewing time
rediscovering the same problems that other people have discovered
previously and told you about before.

...Robert