SE-PgSQL patch review

I'm reviewing SE-PgSQL patch and have some comments.
https://commitfest.postgresql.org/action/patch_view?id=212

Forgive me, but I'm a novice of SELinux. Some of the comments
and question might be nonsense.

==== Patch overview ====
The patch itself is large (>13K), but more than half of it are
well-written regression test, comments and README.

It adds object-level and column-level security checks after normal
permission checks. Row-level checks are not included in the patch.

==== Syntax and identifier names ====
* Can we use "security context" only for securty checking but also
for general "context" handling? If so, we can call it just "context".
It might be useful if the context is designed for more general purpose.
For example, we could develop context-based logging or contex-aware
setting parameters. I think "Application Name" patch is one of the
implementations of context-based logging.
https://commitfest.postgresql.org/action/patch_view?id=195

* CREATE TABLE tbl (...) SECURITY_CONTEXT = '...'
IMHO, '_' and '=' don't suit for SQL.
If there are no conflicts in gram.y, how about removing them?
CREATE TABLE tbl (...) SECURITY CONTEXT '...'

* CREATE TABLE tbl (col integer AS SECURITY_CONTEXT = '...')
Is the syntax "<AS> SECURITY_CONTEXT" natural in English?

* Since SE-PgSQL will be integrated into the core, we can use pg_*
names for the feature. For example, we can rename sepgsql_getcon() to
pg_get_security_context(). Also, we should not use 'con' as an
abbreviated form of 'context' because we often use it for 'connection'.
The same can be said for GUC parameter names.
(ex: "sepostgresql" to be "security_policy")

==== Error messages and error codes ====
* It uses dedicated 'SExxx' error codes, but I think they should belong to
the same family of ERRCODE_INSUFFICIENT_PRIVILEGE (42501).
/* Class SE - SE-PostgreSQL Error (SE-PgSQL-specific error class) */
#define ERRCODE_SELINUX_INTERNAL_ERROR MAKE_SQLSTATE('S','E', '0','0','1')
#define ERRCODE_INVALID_SECURITY_CONTEXT MAKE_SQLSTATE('S','E', '0','0','2')
#define ERRCODE_SELINUX_AUDIT_LOG MAKE_SQLSTATE('S','E', '0','0','3')

* Can we use error messages that looks like existing privilege errors?
ERRCODE_INSUFFICIENT_PRIVILEGE:
=> permission denied to rename database
Error messages from SE-PgSQL
=> SE-PgSQL prevents to modify "pg_class" by hand
looks very different. I'd suggest something like
=> security context denied to rename database

I'll suggest we avoid using the term "SE-PgSQL" in the user visible
message and documentation because because the feature will be integrated
into the core deeply. Of course, we can use "SE-PgSQL" in *promotion*.

==== Internal structures ====
* Are the security labels enough stable?
What will we need when SELinux configuration is modified?
We store security labels as text for each object and column.

* Each security checks are called after pg_*_aclcheck(). Is it possible to
merge SE-PgSQL checks into ACL functions in aclchk.c ?

* What is difference between sepgsql_database_getcon(oid) and
pg_database.datsecon? Why do we need those getcon functions?

That's all comments for now. I'll test the patch in actual machines next.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center