Re: SE-PostgreSQL/Lite Review

2009/12/12 KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>:
> I'd like to summary about the framework.

I am not necessarily in agreement with many of the points listed in
this email.

> * Functionalities
>
> The ACE framework hosts both of the default PG checks and upcoming
> enhanced securities. We can build a binary with multiple enhanced
> security features, but user can choose one from them at most due
> to the security label management.

Just yesterday, Stephen Frost and I were discussing whether to stick a
layer of interdirection into the proposed interface layer using
function pointers. We agreed to defer that decision to a later date.
If, however, we eventually decide to do that, it will clearly mean
that multiple security frameworks are possible simultaneously and that
they can be implemented as loadable modules. On the other hand, we
might decide that that design does NOT make sense, in which case we
might end up with something more like the code snippet you proposed
below, although I'm not sure you have the details right.

We have also not decided whether to support a single security label or
multiple security labels per object. I tend to think that the case
for multiple labels is pretty thin, but on the other hand it's not
unmangeably difficult to do so. It could be coded up using an array
representation, just as we now do for reloptions. Again, I want to
defer this decision until later. Right now, I want to focus on seeing
whether it is all possible for us to get community buy-in on just
centralizing the existing checks, without making any commitment to
what may come after that.

> So, it basically has the following structure:
>
>  Value *
>  ac_database_create([arguments ...])
>  {
>      Value *retval = NULL;
>
>      /*
>       * The default PG checks here. It is never disabled.
>       */
>
>      switch (ace_feature)
>      {
>  #ifdef HAVE_SELINUX
>      case ACE_SELINUX_SUPPORT:
>          if (sepgsql_is_enabled())
>              retval = sepgsql_database_create(...);
>          break;
>  #endif
>  #ifdef HAVE_SMACK
>      case ACE_SMACK_SUPPORT:
>          if (smack_is_enabled())
>              retval = smack_database_create(...);
>          break;
>  #endif
>      default:
>           break;
>      }
>      return retval;
>  }

Again, if we decided on a function-pointer interface, it would be up
to the loaded security module whether or not to still apply the
default PG checks. We have made no decision on that one way or the
other.

> * Namespace
>
> The framework is stored in the src/backend/security/ace.
> As a general rule, Each source file has this naming convension:
>  src/backend/security/ace/<object_class>_ace.c

Maybe. Generally I think if there's a common prefix it should go at
the beginning of the filename, not the end, but I don't want to get
into source code organization too much at this point. It's too early
to make those decisions.

> Uncategolized hooks (such as cascaded-deletion called from dependency
> system) are stored in ace/sysobj_ace.c. It is an exception.

The uncategorized hooks are one of the big design decisions we need to
deal with. Let's put our energy into thinking about how to make a
tight, clean interface there, rather than worrying about where we put
the code.

> All the framework functions (except for static) have "ace_" prefix
> to distinguish from any other internal routines.

I'm not convinced. Probably there will be a prefix, but since I'm
envisioning the first phase of this as strictly code cleanup, it
doesn't seem right to give it a prefix that means access control
*extensions*.

> The security providers (SELinux, upcoming SMACK, ...) specific files
> are stored in a certain directory in src/backend/security/ace/.
> SE-PgSQL shall use "sepgsql" directory.

It's way too early to decide this, IMO.

> * Documentation
>
> I don't plan to provide SGML documentation about ACE itself, except for
> internal section, because it is an internal infrastructure.
> For developers, src/backend/security/ace/README will provide a brief
> overview of the ACE framework, and every security hooks have source
> code comments to introduce the specification of itself.

That's probably about right, but I don't want to prejudge that without
further discussion in the community.

> * Patches
>
> As a general rule, a patch is organized for each object classes, except
> for very tiny object classes.
> e.g)
>  pgsql-ace-02-database.patch
>  pgsql-ace-03-schema.patch
>     :
>
> I'll try to submit a set of patches related to the following object classes
> and functionalities by the 12/16.

Stephen Frost is working on a patch for just one object type. I think
that is a better approach than writing code for everything and then
trying to review it all at once. We need to try to get consensus on
the basic design before we write a lot of code. I do agree however
that **if** we are able to get consensus on one object type, we should
probably design the whole thing as a stack of patches that apply on
top of each other, one per object type, for easier review. That's not
an approach I usually favor, but in this case I think it makes sense.

...Robert