Re: SE-PostgreSQL/Lite Review

KaiGai,

* KaiGai Kohei (kaigai(at)kaigai(dot)gr(dot)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