Re: Reworks for Access Control facilities (r2363)

Tom Lane wrote:
> 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.

Quite frankly, I felt disappointed that we have to repeat these kind of
design level issues again. :-(

The purpose of this patch is to provide function entrypoints for the
upcoming SE-PostgreSQL feature, because I got a few comments that we
hesitate to put sepgsql_xxx() hooks on the main routines directly in
the first commit fest. In addition, I already tried to put SE-PG hooks
within pg_xxx_aclchecks() in this CF, but it was failed due to the
differences in the security models.
Then, we made a direction to add an abstraction layer for the purpose
of access controls which can be available both of DAC and MAC.

Apart from this patch, we need to consider the preferable way to host
additional security models in PostgreSQL again.

> I don't have any confidence that this is a sane way to proceed forward.

Indeed, ac_xxx_*() routines needs a large scale changes for the core.
However, we didn't have any other way, if both of security model have
to use common entry points.

In the original design, I put sepgsqlCheckXXX() hooks which does not
affect anything if disabled on the main routines, and it works well.
I would like to consider why reviewers felt these hooks are (possibly)
hard to maintain again.

One reason was the hooks reflected individual SELinux permissions.

e.g) sepgsqlCheckProcedureInstall(Oid procOid);

It checks user's privilege to use a certain function as a system internal
stuff which is executed on runtime without individual execution permission
checks, like an implementation of conversion.

However, it may need future contributors to understand the intention
why the hooks were deployed here, without enough knowledge about SELinux.
In other word, the hooks represented how SELinux makes its decision
(method), not what SELinux make its decision on (purpose).

Instead of this ac_xxx_*() routines and previous sepgsqlCheckXXX()
routines, I would like to propose SE-PG hooks which reflects the
purpose of security checks.

e.g) sepgsql_relation_create(char *relName, Oid namespace_oid, ...);

It internally compute the default security context of the new table
and checks permission on the table itself and the namespace to be
created on. The series of checks consists of a permission check to
create a new table in totally.

I think it is not a major issue whether this patch is applied, or not.
What is important is to point out the right direction to host SELinux
security model correctly.

At the PGcon2008 keynote, Bruce talked that our road to the summit is
similar to a bendy road. It means our development does not always
go into the right direction, but we are certainly getting near to the
summit.
I've tried several approaches for more than two years, but I cannot
feel we are getting near to the summit yet.

At least, we need to decide where we should go on the next at the
end of this commit fest.

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

The reason why we needed to put permission checks on the routines in
dependency.c is that we cannot know what objects are dropped due to
the cascaded deletion.
But some of purely internal stuffs (such as cleaning up temporary
objects) uses the routines without any necessity of permission checks.
So, the flag to control permission check is necessary.

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

Back to the definition of access controls (or reference monitor).
It prevents violated accesses launched by user's requests (SQL).
It is not a job to protect something from malicious internal modules.
The loadable kernel module is a good analogy. It is allowed anything
because kernel module can access directly without system-call invocations.
However, OS checks whether the admin has an appropriate privilege, or not,
when the kernel module tries to be loaded.
It is also DBA's decision whether he allows to load a third-party module,
or not.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>