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

Robert Haas wrote:
> 2009/7/13 KaiGai Kohei <kaigai@ak.jp.nec.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.

What's your opinion to separate it _at the first stage_?
It can reduce about 800L.

I can implement without security identifiers while we don't have
row level security. I'll update it.
(Please don't count changeset in catalog/pg_proc.h, about 8KL...)

>>  [kaigai@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@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."

Are you suggesting me to move the hooks into pg_xxx_aclcheck()
_at the first stage_, aren't you?

If so, I can postpone some of permission checks outside of the
pg_xxx_aclcheck(). However, SELinux's security model often
requires different criteria to make its decision.
(Please note that I never say either of them is better or worse.)
Thus, we will need to add security hooks outside of the pg_xxx_aclcheck()
in the future, although it can be done step-by-step.

I guess the following permissions can be checked at the first version.- db_database:{access}   ... equivalent to
ACL_CONNECTon pg_database- db_procedure:{execute} ... equivalent to ACL_EXECUTE on pg_proc- db_schema:{search}     ...
equivalentto ACL_USAGE on pg_namespace
 
- db_database:{superuser} to be called from superuser_arg().  Should it be postponed to the next phase?

BTW, SELinux needs objects to be labeled correctly on its creation time.
At least, we have to put hooks to provide security labels to be assigned
on the DefineRelation() and so on, although it does not check permission
to create the objects.

Thanks,

> 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.

-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>