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

2009/7/14 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> 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...)

I don't have a strong feeling on it.  As I said before, what I mostly
care about is eliminating the heap tuple header stuff.  I haven't
looked at the patch in enough detail yet to figure out what should
replace it.  If you think that's the best way to go, give it a shot.

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

Yes: to repeat what has been said multiple times previously, you
should postpone everything that isn't a mirror of the current security
model: there should only be permission checks in places where there
are permissions checks now, and they should be mirror images of the
current DAC checks.

As for the rest, I think you've likely got it backwards: we shouldn't
start by not cluttering the entire code-base with sepgsql permission
checks, and then go and do it later after the basic functionality is
in.  We shouldn't EVER clutter the whole code-base with sepgsql
permission checks.  If we want to expose other sepgsql functionality,
we should do that by putting in more kinds of pg_xxx_aclcheck()-type
calls in other places, and the sepgsql can leverage those call sites
too.  But never mind that, because it's irrelevant for right now: what
you need to do is strip this down to the minimal feature set without
worrying about what will or won't happen later.  Otherwise, nothing is
going to happen at all.

> I guess the following permissions can be checked at the first version.
>  - db_database:{access}   ... equivalent to ACL_CONNECT on pg_database
>  - db_procedure:{execute} ... equivalent to ACL_EXECUTE on pg_proc
>  - db_schema:{search}     ... equivalent to ACL_USAGE on pg_namespace

Why not db_database:{connect} and db_schema:{usage}?  It seems to me
that there is zero value in inventing new names for the same things.

>  - db_database:{superuser} to be called from superuser_arg().
>   Should it be postponed to the next phase?

No, I don't see any reason to postpone that.  That seems analagous to
what we do now, and should be included in the first version, I would
think.

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

That seems very reasonable to me.

...Robert