SE-PgSQL patch review - Mailing list pgsql-hackers

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
itjust "context". It might be useful if the context is designed for more general purpose. For example, we could develop
context-basedlogging or contex-aware setting parameters. I think "Application Name" patch is one of the implementations
ofcontext-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'
becausewe 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:    =>
permissiondenied to rename database  Error messages from SE-PgSQL    => SE-PgSQL prevents to modify "pg_class" by hand
looksvery 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
featurewill 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
labelsas 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