Re: [PATCH] ACE Framework - Database, Schema - Mailing list pgsql-hackers

Robert, Thanks for your detailed comments.

> Just to name a few really obvious problems (I only looked at the
> 01-database patch):
>
> 1. We have been talking for several days about the need to make the
> initial patch in this area strictly a code cleanup patch.  Is this
> cleaner than the code that it is replacing?  Is it even making an
> attempt to conform to that mandate?

Even if it is unclear whether the current form is more clear than the
current inlined pg_xxx_aclcheck() form, or not, it will obviously
provide a set of common entry points for upcoming enhanced security
providers.
Eventually, it is more clear than enumeration of #ifdef ... #endif
blocks for SELinux, Smack, Solaris-TX and others.

> 2. What will happen when someone runs pgindent against this?
>
> Perhaps you only intended this to be a starting point for discussion,
> in which case that's fine, but I don't think I can really contribute
> anything useful until it gets a little further along.

If someone runs pgindent, I'll have to fix up a series of patches
mechanically to resolve conflictions, if necessary.
Indeed, this framework does not provide any additional user visible
features by itself, but necessary to accept upcoming anything useful.


Robert Haas wrote:
> I read through the database patch a little more.  Here are some
> further thoughts.
> 
> ace_database_create() calls have_createdb_privilege(), but of course
> what ace_database_create() is supposed to be checking is whether you
> have privileges to create the DB.  This is extremely confusing.  The
> calling sequence for ace_database_create() seems to indicate a failure
> of abstraction.  The srcIsTemplate argument seems quite problematic.
> It is one of many values that are returned by get_db_info(), and it's
> passed to ace_database_create() because the current PG model happens
> to care.  It seems like a near-certainty that future security models
> will care about something else.  (Incidentally, the interface to the
> existing get_db_info function seems to me to be rather poor.  By the
> time we get up to 10 out parameters, I think we ought to be using a
> structure.  This might be material for a standalone patch.)

The have_createdb_privilege() is a copy & paste from the dbcommand.c.
It lookups AUTHOID syscache and returns pg_authid.rolcreatedb, but
indeed, its name is confusable. I'll consider another name.

The srcIsTemplate might be pulled out from DATABASEOID syscache
using the given srcDatOid. I guess the reason why get_db_info()
is it tries to resolve dbname -> db_oid and fetch its properties
in same time. But OID of the source database is already given,
so security provider (including the default PG) can lookup this
as neccessity.

> I don't understand the modifications to restrict_and_check_grant().
> It seems to me that this is going in the wrong direction, although
> since I don't understand the motivation for the change, I might be
> wrong.  If we have a piece of code that centralizes permissions
> checking now, splitting that up into a bunch of ace_*_grant()
> functions and duplicating it doesn't seem like an improvement.

From perspective of the enhanced security providers, GRANT/REVOKE is
a kind of modification of properties on a certain database object.
So, it has a motivation to check GRANT/REVOKE using its access control
rules.
If we don't modify restrict_and_check_grant(), the caller (aclchk.c)
correctly handles the default PG checks, so rest of enhanced security
providers will apply additional checks in the ace_*_grant().
In this case, we keep ace_*_grant() empty at the moment, and it shall
be a special purpose hook for enhanced security providers.
I don't have any preference on either of them.

> ace_database_alter() appears to never be called with more than one
> argument that is non-NULL/InvalidOid, which is usually a sign that the
> interface is wrong.  There are two calls to this function where,
> apparently, we're checking alter database permissions without
> specifying any details whatsoever about exactly what's going to get
> altered.  That sounds like we don't really know what things we want to
> pass across the interface layer, so we're just passing the ones that
> we happen to care about at the moment.  It certainly looks like we
> need separate functions for alter, rename, and alter-set.  In some
> cases it might make sense to pass the parsenode as an argument rather
> than trying to decide which bits of data contained within it are
> sufficiently interesting to be worth sending over.

It makes sense for me. I'll separate it as follows:

- ace_database_alter_rename()   ... ALTER DATABASE xxx RENAME TO
- ace_database_alter_owner()    ... ALTER DATABASE xxx OWNER TO
- ace_database_alter()          ... ALTER DATABASE with other options

When we support security label on database, it will also necessary:

- ace_database_alter_relabel()

> Tom objected to ace_database_calculate_size() before.  It seems to me
> that in order to keep this managable we have to settle on a finite set
> of permissions and stick with it.  For example, in this case, we might
> decide that in EVERY security model, calculating the database size
> requires the same permissions as connect.  The individual security
> models might want to make different decisions about how to check that
> permission, but they all have to treat it the same way they would
> treat an actual connection attempt.  It seems to me that if every
> security model is allowed to introduce not only new logic for making
> checks but also new kinds of checks, we might as well give up on
> designing an interface layer.

It againsts to the purpose of the framework. It provides a common set
of access controls for multiple security providers, but it does not
enforce the way to make its access control decision.

It seems to me ace_database_calculate_size() might be a bad name
because of too specific naming for a certin functionality.
We can also consider this hook checks permission to obtain attributes
of a certain database.
So, we can also generalize it as follows:
 void ace_database_getattr(Oid datOid);

It checks permission to reference attributes (not contents) of a certain
database. The database size is a part of attribute, and the default PG
model checks ACL_CONNECT permission for the purpose.
Any other upcoming enhanced security provider may make its decision based
on different basis. But there is no matter.


> (Similarly, skipping back to ace_database_create() for a minute ...
> since I asked for a patch that only deals with one object type, I
> won't now complain about having gotten one.  But I will note that I
> think where ace_database_create() calls pg_tablespace_aclchk() it
> probably eventually needs to call some ace_tablespace_...() function.
> Although frankly I'm not sure which one.  Would
> ace_tablespace_create() mean permission to create a tablespace or
> permission to create tables within that tablespace?)

ace_tablespace_create() means permission to create a tablespace itself.
It shall be checked on CREATE TABLESPACE statement, not an alternative
of pg_tablespace_aclcheck(..., ACL_CREATE).

In the default PG model, it makes its access control decision based on
the following rules:- Current database user must has pg_authid.rolsuper or pg_authid.rolcreatedb- If not superuser,
currentdatabase user must have membership of the  owner of the new database.- If source database is not template, the
currentdatabase user must  have ownership of the new database.- If an explicit tablespace is given, the current
databaseuser must  have CREATE privilege on the target tablespace.
 

It is the way to make access control decision in the default PG model.
However, any other enhanced security providers can apply thier rules.

E.g, SE-PgSQL tries to assign a default security label on a new database,
and checks permission to create a new database (db_database:{create})
between the client and the database with this label.

This framework provides a set of common entrypoints for various kind of
security providers, but does not enforce a certain security model.

> security/ace.h, like a good number of other things in this patch, does
> not conform to project indentation style.  All of the parameter blocks
> (parameter : description) don't either; unless I'm mistaken, pgindent
> will do something awful to those.  The comments generally need some
> editing help from a native English speaker, and I spotted at least two
> typos (defatult for default, provides for providers).  They also make
> reference to multiple security providers, which is not within the
> purview of this patch.

I'll pay my efforts as possible as I can.

> I am not very happy with the ace_<object-type>_<operation> naming
> convention.  Most PG functions are named a little more descriptively
> than that.  Like I can pretty much guess that AlterDatabase() is going
> to alter a database, and get_db_info() is going to get info about a
> DB, and ExecHashJoin() is going to execute a hash join.  It's not
> possible to look at ace_database_create() and have any idea what the
> function does, unless you have the preexisting knowledge that ACE
> stands for "access control extensions", and then you still don't know
> what the function does because "access control extensions database
> create" is a sentence with no verb.  Granted, we have some poorly
> named functions in the system already, but I'd like to not increase
> the number.  Apart from all of the above, I still believe that for a
> first phase of this we should just be looking to centralize access
> control decisions without promising to ever do anything more, and
> "access control extensions" doesn't send that message.  I don't know
> exactly what the best way to structure this is but I think putting the
> word "check" somewhere in there would be a good start (or maybe the
> already-used-elsewhere abbreviation "chk").

OK. Indeed, it is not a verb.
However, the "check" does not means by itself what should be checked.
It may be permission, integrity, syntax or others.

What about "acechk_*" to mean access control extension checks <something>
to be <action>'ed? For example, acechk_database_create() means "access
control extension checks <database> to be <created>".

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