Re: SE-PostgreSQL?

Robert Haas wrote:
> 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.

As you know, I'm not a native English user, so I need to admit it is
not avoidable the documentations are not idiomatic more or less.
If you consider it is unclear what the documentation actually said,
could you please tell me. I'll revise it soon.

I believe PostgreSQL community is internationally open.

> 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.

IIRC, I introduced the way to label files and directories installed
on the later section?

> 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?

If we can have more graceful interface design, I can agree to replace
the current design. However, several kind of permission checks requires
extra informations rather than OID of database.
For example, when we modify the security label of the database, SELinux
shall requires to check db_database:{relabelfrom} on the database with
older label and db_database:{relabelto} on the database with newer label.
In this case, the security hook need the user given security label rather
than OID of the database.

When I designed the interfaces, a part of security hooks requires only
OID of the database object, but rest of them are not.
So, I considered that it is confusable for developers some of hooks are
common for various kind of permissions but others are not so.

> 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.

IIRC, at the v8.4 development cycle, Peter Eisentraut asked to me what
feature is the fundamental principle of the SE-PostgreSQL and what is
not separatable. I replied the system-wide consistency in access controls
and mandatory access controls are the essence in SE-PostgreSQL. It is the
security model in other word.
Rest of features are not as significant as the security model, such as
granularity of access controls, extra permissions (largeobejcts, ...).
So, I could agree to postpone a part of features in the later version
to reduce the scale of patches.

Needless to say, it is not something fundamental things how to implement
them and what coding style is acceptable. I'm always flexible for them.
Please remind that older SE-PgSQL wrapped all the security hooks with
LSM like interfaces called PGACE. But, it was pointed out we don't need
any in-code framework if the feature tries to be mainlined, then I removed
all the pgace_xxxx() hooks from the patch.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>