Re: SE-PostgreSQL/Lite Review - Mailing list pgsql-hackers

KaiGai,

* KaiGai Kohei (kaigai@kaigai.gr.jp) wrote:
> As Rober Haas already suggested in another message, my patch in the last
> commit fest is too large. It tried to rework anything in a single patch.
> The "per-object-type" basis make sense for me.

Agreed.

> In my cosmetic preference, "ace_" is better than "ac_". The 'e' means
> extendable, and "ace" feels like something cool. :-)

No complaints here..  I just hope this doesn't end up being *exactly*
the same as your original PGACE patches..  I'd feel terrible if we
weren't able to at least improve something with this extremely long and
drawn our process!

> And I'd like to store these files in "backend/security/ace/*.c", because
> "backend/security" will also store other security modules!

This is perfectly reasonable in my view.  No complaints here.

> src/
>  + backend/
>     + security/
>        + ace/         ... access control framework
>        + sepgsql/     ... selinux specific code
>        + smack/       ... (upcoming?) smack specific code
>        + solaristx/   ... (upcoming?) solaris-tx specific code

Looks good to me.  Perhaps we'll have a smack/ patch showing up very
shortly as well..

> The reason why I prefer the default PG check + one enhanced security at
> most is simplification of the security label management.
> If two label-based enhanced securities are enabled at same time,
> we need two field on pg_class and others to store security context.

Ah, yes, I see your point must more clearly now.  This sounds reasonable
to me.

> In addition, OS allows to choose one enhanced security at most eventually.
>
> In my image, the hook should be as:
>
>   Value *
>   ac_database_create([arguments ...])
>   {
>       /*
>        * The default PG checks here.
>        * It is never disabled
>        */
>
>       /* "enhanced" security as follows */
>   #if defined(HAVE_SELINUX)
>       if (sepgsql_is_enabled())
>           return sepgsql_database_create(...);
>   #elif defined(HAVE_SMACK)
>       if (smack_is_enabled())
>           return smack_database_create(...);
>   #elif defined(HAVE_SOLARISTX)
>       if (soltx_is_enabled())
>           return soltx_database_create(...);
>   #endif
>       return NULL;
>   }
>
> We can share a common image image?

If all of these security modules make sense as "only-more-restrictive",
then I have no problem with this approach.  Honestly, I'm fine with the
initial hooks looking as above in any case, since it provides a clear
way to deal with switching out the 'default PG checks' if someone
desires it- you could #if around that block as well.  As it's the
principle/primary, and I could see "only-more-restrictive" being more
popular, I don't mind having that code in-line in the hook.  The check
itself is still being brought out and into the security/ framework.

I do think that, technically, there's no reason we couldn't allow for
multiple "only-more-restrictive" models to be enabled and built in a
single binary for systems which support it.  As such, I would make those
just "#if defined()" rather than "#elif".  Let it be decided at runtime
which are actually used, otherwise it becomes a much bigger problem for
packagers too.
Thanks!
    Stephen