Re: SE-PostgreSQL?

On Sat, Jul 18, 2009 at 1:19 PM, Josh Berkus<josh(at)agliodbs(dot)com> wrote:
> b) Efficient constraint-based row-level security (as opposed to individual
> row labelling)[1]
[...]
> [1] For an explanation of the two ways to do row-level security, see here:
> http://it.toolbox.com/blogs/database-soup/thinking-about-row-level-security-part-1-30732
> http://it.toolbox.com/blogs/database-soup/thinking-about-row-level-security-part-2-30757

Yeah. That would be teh awesome (though admittedly I'm a sucker for a
cool feature), but it's quite beside the point of the original email
here, which is whether there's any point in continuing with the
current SE-Linux patches. I think there isn't.

Quite beyond the fact that we never seem to be able to get a patch
that implements a reasonable first set of features, the amount of work
that's going to be required to get these patches committable is going
to be enormous. Just to cite a few examples, here is the
documentation for the "sepostgresql_mctrans" documentation.

% Enables to turn on/off Mcstrans feature on SE-PostgreSQL. It is on
by default. Every users can set this
% parameter on their sessision without any limitations. SELinux
provides a feature to translate a part of security
% context between raw format and human-readable format, called
Mcstrans. If the parameter is turned on,
% SE-PostgreSQL translate the security context when it is exported to
or imported from users.

So, one problem is that this is written in poor English. While I'm
willing to do a certain amount of copy-editing as part of the review
process, SE-PostgreSQL is a massive feature and it's unfair to put the
entire burden of making the documentation readable on the shoulders of
the reviewers. I already proofread and corrected these docs once,
back in December, but now they need more reworking because they've
been extensively revised since then, and lots of non-idiomatic
constructs have crept back in. It's worse than that, though: the
above is not only bad English, it's also not a very good description
of the feature. I certainly can't tell from reading it what that GUC
actually does, and there are no references to anyplace where I can
turn for followup reading.

And then there's this, which is unduly RedHat-centric:

% Please note that SELinux requires installed files, directories and
others should be labeled properly. RPM
% installation do it implicitly.

And then look at this patch hunk:

pg_database_aclcheck(Oid db_oid, Oid roleid, AclMode mode)
{
if (pg_database_aclmask(db_oid, roleid, mode, ACLMASK_ANY) != 0)
+ {
+ /* SELinux checks db_database:{connect} permission */
+ if ((mode & ACL_CONNECT) &&
+ !sepgsqlCheckDatabaseConnect(db_oid))
+ return ACLCHECK_NO_PRIV;
+
return ACLCHECK_OK;
+ }
else
return ACLCHECK_NO_PRIV;
}

I don't think I'm stepping too far out on a limb to say that this
looks like a very poor interface design. Surely we don't want a
separate sepgsqlCheck<object-type><privilege> function for every
combination of an object type and a privilege, and shouldn't the check
be inside pg_database_aclmask anyway?

These are just a few examples from reading through the first bit of
the patch. I have no doubt that Kaigai is good at what he does, but I
don't think he understands what the community is looking for from this
patch in terms of features and coding style. I think the question is
not so much whether anyone is interested in the features (I know some
are) but whether anyone who understands what will be acceptable for
PostgreSQL is willling to do the necessary amount of work to get a
committable patch that implements them. I would be willing to work
with someone to fine-tune such a patch set, but the ratio of reviewer
effort to patch quality improvement on this patch set is well above
what I am prepared to put in.

...Robert