Unix Technical Forum

SEO

vBulletin Search Engine Optimization


Go Back   Unix Technical Forum > Database Server Software > PostgreSQL > pgsql Hackers

Register FAQ Members List Calendar Search Today's Posts Mark Forums Read
  #1 (permalink)  
Old 05-02-2008, 05:06 AM
KaiGai Kohei
 
Posts: n/a
Default Re: [0/4] Proposal of SE-PostgreSQL patches

I updated the series of SE-PostgreSQL patches for the latest pgsql-8.4devel tree.

[1/4] sepostgresql-pgace-8.4devel-3-r739.patch
provides PGACE (PostgreSQL Access Control Extension) framework.
http://sepgsql.googlecode.com/files/...l-3-r739.patch

[2/4] sepostgresql-sepgsql-8.4devel-3-r739.patch
provides SE-PostgreSQL feature, based on PGACE framework.
http://sepgsql.googlecode.com/files/...l-3-r739.patch

[3/4] sepostgresql-pg_dump-8.4devel-3-r739.patch
enables to dump databases with security attribute.
http://sepgsql.googlecode.com/files/...l-3-r739.patch

[4/4] sepostgresql-policy-8.4devel-3-r739.patch
provides the default security policy of SE-PostgreSQL.
http://sepgsql.googlecode.com/files/...l-3-r739.patch

We provide a quick overview for SE-PostgreSQL at:
http://code.google.com/p/sepgsql/wik...IsSEPostgreSQL

Thanks,

KaiGai Kohei wrote:
> The series of patches are the proposal of Security-Enhanced PostgreSQL (SE-PostgreSQL)
> for the upstreamed PostgreSQL 8.4 development cycle.
>
> [1/4] sepostgresql-pgace-8.4devel-3.patch
> provides PGACE (PostgreSQL Access Control Extension) framework
> http://sepgsql.googlecode.com/files/...l-3-r704.patch
>
> [2/4] sepostgresql-sepgsql-8.4devel-3.patch
> provides SE-PostgreSQL feature, based on PGACE framework.
> http://sepgsql.googlecode.com/files/...l-3-r704.patch
>
> [3/4] sepostgresql-pg_dump-8.4devel-3.patch
> enables pg_dump to dump database with security attribute.
> http://sepgsql.googlecode.com/files/...l-3-r704.patch
>
> [4/4] sepostgresql-policy-8.4devel-3.patch
> provides the default security policy for SE-PostgreSQL.
> http://sepgsql.googlecode.com/files/...l-3-r704.patch
>
> We can provide a quick overview for SE-PostgreSQL at:
> http://code.google.com/p/sepgsql/wik...IsSEPostgreSQL
>
> ENVIRONMENT
> -----------
> Please confirm your environment.
> The followings are requriements of SE-PostgreSQL.
> * Fedora 8 or later system
> * SELinux is enabled and working
> * kernel-2.6.24 or later
> * selinux-policy and selinux-policy-devel v3.0.8 or later
> * libselinux, policycoreutils
>
> INSTALLATION
> ------------
> $ tar jxvf postgresql-snapshot.tar.bz2
> $ cd postgresql-snapshot
> $ patch -p1 < ../sepostgresql-pgace-8.4devel-3.patch
> $ patch -p1 < ../sepostgresql-sepgsql-8.4devel-3.patch
> $ patch -p1 < ../sepostgresql-pg_dump-8.4devel-3.patch
> $ patch -p1 < ../sepostgresql-policy-8.4devel-3.patch
>
> $ ./configure --enable-selinux
> $ make
> $ make -C contrib/sepgsql-policy
> $ su
> # make install
>
> # /usr/sbin/semodule -i contrib/sepgsql-policy/sepostgresql.pp
> (NOTE: semodule is a utility to load/unload security policy modules.)
>
> # /sbin/restorecon -R /usr/local/pgsql
> (NOTE: restorecon is a utilicy to initialize security context of files.)
>
> SETUP
> -----
> # mkdir -p /opt/sepgsql
> # chown foo_user:var_group /opt/sepgsql
> # chcon -t postgresql_db_t /opt/sepgsql
> (NOTE: chcon is a utility to set up security context of files.)
> # exit
>
> $ /usr/sbin/run_init /usr/local/pgsql/bin/initdb -D /opt/sepgsql
> (NOTE: run_init is a utility to start a program, as if it is branched from init script.)
> $ /usr/local/pgsql/bin/pg_ctl -D /opt/sepgsql start
>
>
> SUMMARYS FOR EVERY PATCHES
> --------------------------
> [1/4] - sepostgresql-pgace-8.4devel-3.patch
>
> This patch provides PGACE (PostgreSQL Access Control Extension) framework.
>
> It has a similar idea of LSM (Linu Security Module).
> It can provide a guest module several hooks at strategic points.
> The guest module can make its decision whether required actions should be
> allowed, or not.
> In addition, PGACE also provides falicilites to manage security attribute
> of database objects. Any tuple can have a its security attribute, and the
> guest module can refer it to control accesses.
>
> A more conprehensive memo at:
> http://code.google.com/p/sepgsql/wiki/WhatIsPGACE
>
> [2/4] - sepostgresql-sepgsql-8.4devel-3.patch
>
> This patch provides SE-PostgreSQL facilities based on PGACE.
>
> Security-Enhanced PostgreSQL (SE-PostgreSQL) is a security extension
> built in PostgreSQL, to provide system-wide consistency in access
> controls. It enables to apply a single unigied security policy of
> SELinux for both operating system and database management system.
> In addition, it also provides fine-grained mandatory access which
> includes column-/row- level non-bypassable access control even if
> privileged database users.
>
> Quick overview at:
> http://code.google.com/p/sepgsql/wik...IsSEPostgreSQL
>
> [3/4] - sepostgresql-pg_dump-8.4devel-3.patch
>
> This patch gives us a feature to dump database with security attribute.
> It is turned on with '--enable-selinux' option at pg_dump/pg_dumpall,
> when the server works as SE- version.
> No need to say, users need to have enough capabilities to dump whole of
> database. It it same when they tries to restore the database.
>
> [4/4] - sepostgresql-policy-8.4devel-3.patch
>
> This patch gives us the default security policy for SE-PostgreSQL.
> You can build it as a security policy module. It can be linked with
> the existing distributor's policy, and reloaded.
>



--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Digg this Post!Add Post to del.icio.usBookmark Post in TechnoratiFurl this Post!
Reply With Quote
  #2 (permalink)  
Old 05-02-2008, 05:06 AM
Josh Berkus
 
Posts: n/a
Default Re: [0/4] Proposal of SE-PostgreSQL patches

Folks,

For hackers who don't understand security frameworks, I'm going to make a
strong case for KaiGai's patch. Because of my current presentation
series, I've been talking to PostgreSQL users about security features
around the world for the last several months, and there's a *lot* of
interest in security framework support. Not only are existing users
interested in it, but some potential users (security agencies, banks) who
didn't use PostgreSQL before have come to talk to me becuase of
SE-Postgres.

Further, I've asked the TrustedSolaris folks to take a look at KaiGai's
implementation to see if it was "generic" enough for them to build on as a
test of whether SE-Postgres was too specific to SE-Linux; the answer has
been a tentative "yes, it's generic".

So it would be much better to have this functionality be "mainstream"
rather than a fork. If it does get bounced, please do it becuase of code
quality and not because "nobody is asking for this".

Thanks!

--
--Josh

Josh Berkus
PostgreSQL @ Sun
San Francisco

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Digg this Post!Add Post to del.icio.usBookmark Post in TechnoratiFurl this Post!
Reply With Quote
  #3 (permalink)  
Old 05-02-2008, 05:06 AM
Joshua D. Drake
 
Posts: n/a
Default Re: [0/4] Proposal of SE-PostgreSQL patches

Josh Berkus wrote:
> Folks,


> So it would be much better to have this functionality be "mainstream"
> rather than a fork. If it does get bounced, please do it becuase of code
> quality and not because "nobody is asking for this".


+1

Joshua D. Drake

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Digg this Post!Add Post to del.icio.usBookmark Post in TechnoratiFurl this Post!
Reply With Quote
  #4 (permalink)  
Old 05-02-2008, 05:06 AM
Andrej Ricnik-Bay
 
Posts: n/a
Default Re: [0/4] Proposal of SE-PostgreSQL patches

On 01/05/2008, Josh Berkus <josh@agliodbs.com> wrote:
> Further, I've asked the TrustedSolaris folks to take a look at KaiGai's
> implementation to see if it was "generic" enough for them to build on as a
> test of whether SE-Postgres was too specific to SE-Linux; the answer has
> been a tentative "yes, it's generic".


> So it would be much better to have this functionality be "mainstream"
> rather than a fork. If it does get bounced, please do it becuase of code
> quality and not because "nobody is asking for this".


Not a hacker, just a curious reader ... are there equivalent frameworks
for the other supported platforms? E.g. MacOS, *BSD, Windows? Are
the similarities between those (if they exist) close enough not to introduce
a maintenance nightmare?


Cheers,
Andrej

--
Please don't top post, and don't use HTML e-Mail :} Make your quotes concise.

http://www.american.edu/econ/notes/htmlmail.htm

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Digg this Post!Add Post to del.icio.usBookmark Post in TechnoratiFurl this Post!
Reply With Quote
  #5 (permalink)  
Old 05-02-2008, 05:06 AM
Greg Smith
 
Posts: n/a
Default Re: [0/4] Proposal of SE-PostgreSQL patches

On Thu, 1 May 2008, Andrej Ricnik-Bay wrote:

> Not a hacker, just a curious reader ... are there equivalent frameworks
> for the other supported platforms? E.g. MacOS, *BSD, Windows?


SELinux is a Linux implementation of ideas from an earlier NSA project
named Flask. There is port of another variant of that, Flask/TE, that is
making its way into the BSD variants via a project called SEBSD.
TrustedBSD, Darwin (OS X), and OpenSolaris all have projects in this area
already (the Solaris one just launched last month). A good starter page
is http://www.trustedbsd.org/sebsd.html

Particularly given the common heritage, I suspect that the PostgreSQL side
of all these projects will be similar, and that once those hooks are in
place it will just be a matter of tying them into the higher levels of the
other framework. It would be too ambitious to target all of them all at
once for a first pass, but it may be worth a look at the fundamentals of
SEBSD to make sure the right hooks look like they're in place.

Windows has this thing called "Group Policy" that's supposedly leaped
forward for Windows Server 2008. They are now advertising it as like
SELinux, but better. The presentation PDF I just read on that subject
sounds like something written by the crazy guy at Broadway & 57th street I
used to walk by, as he talked on fruit as if they were his cell phone.
It's such a deluded and wildly misguided bit of sales fluff that you can't
take it seriously, and the whole thing just leaves me feeling sorry for
them instead.

--
* Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Digg this Post!Add Post to del.icio.usBookmark Post in TechnoratiFurl this Post!
Reply With Quote
  #6 (permalink)  
Old 05-02-2008, 05:06 AM
Greg Smith
 
Posts: n/a
Default Re: [0/4] Proposal of SE-PostgreSQL patches

On Wed, 30 Apr 2008, KaiGai Kohei wrote:

> [1/4] sepostgresql-pgace-8.4devel-3-r739.patch
> provides PGACE (PostgreSQL Access Control Extension) framework.
> http://sepgsql.googlecode.com/files/...l-3-r739.patch


For those overwhelmed by sheer volume here, this is the patch to start
with, because it's got all the core changes to the server. I'm also in
the camp that would like to see this feature added, but rather than just
giving it a +1 I started looking at it.

The overall code is nice: easy to understand, structured modularly. I
have some concerns though. The first two things that jump out at me on an
initial review appear right from the beginning for those who want to take
a look:

-I'm a bit unnerved by both the performance and reliability implications
from how the security check calls are done in every case, even if there is
no SELinux support included. Those checks are sitting in some pretty low
level tuple and heap calls.

The approach taken here is to put all the "#ifdef" logic into the
underlying ACE interface (see patch [2/4]), so that the caller doesn't
have to care. If SELinux support is off then the calls turns into

void x(y) {} or
bool a(b) { return true; }

This is a very clean design, but it's putting extra (possibly optimized
away) calls into a lot of places. While it would be uglier, it might make
sense to put that on/off logic in all the places where the calls are made,
so that when you turn SELinux support off most of the code really does go
completely away rather than just turning into stubs.

-The only error reporting and handling method used is "elog(ERROR,...".
That seems a bit heavy handed for something that can be expected to happen
all the time.

If I understand this correctly, when you're scanning a table with 1000
rows where you're only allowed to see 50% of them, that's going to be 500
call to elog(), one for each tuple you can't see. Having a tuple get
screened out isn't really an error per se, and while I can see how
sensitive installs would want those all reported there are others where
this volume of log activity would be too much. Just because someone with
classified clearance is looking at a big table that also has a lot of
secret info in it, not all installs will want a million errors reported
just because there's data that person can't see available.

At a minimum, this needs some finer log control, and maybe a rethinking
altogether of how to handle error cases.

--
* Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Digg this Post!Add Post to del.icio.usBookmark Post in TechnoratiFurl this Post!
Reply With Quote
  #7 (permalink)  
Old 05-02-2008, 05:06 AM
Gregory Stark
 
Posts: n/a
Default Re: [0/4] Proposal of SE-PostgreSQL patches

"Greg Smith" <gsmith@gregsmith.com> writes:

> The approach taken here is to put all the "#ifdef" logic into the underlying
> ACE interface (see patch [2/4]), so that the caller doesn't have to care. If
> SELinux support is off then the calls turns into
>
> void x(y) {} or
> bool a(b) { return true; }
>
> This is a very clean design, but it's putting extra (possibly optimized away)
> calls into a lot of places. While it would be uglier, it might make sense to
> put that on/off logic in all the places where the calls are made, so that when
> you turn SELinux support off most of the code really does go completely away
> rather than just turning into stubs.


It's nicer to do it the way they have but we don't generally trust compilers
to inline functions. Is it hard to make those functions into macros?

> -The only error reporting and handling method used is "elog(ERROR,...". That
> seems a bit heavy handed for something that can be expected to happen all the
> time.
>
> If I understand this correctly, when you're scanning a table with 1000 rows
> where you're only allowed to see 50% of them, that's going to be 500 call to
> elog(), one for each tuple you can't see. Having a tuple get screened out
> isn't really an error per se, and while I can see how sensitive installs would
> want those all reported there are others where this volume of log activity
> would be too much. Just because someone with classified clearance is looking
> at a big table that also has a lot of secret info in it, not all installs will
> want a million errors reported just because there's data that person can't see
> available.


I don't understand, if it's ERROR it would throw an error and stop the current
query. Or is this all within a PG_TRY() ?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's RemoteDBA services!

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Digg this Post!Add Post to del.icio.usBookmark Post in TechnoratiFurl this Post!
Reply With Quote
  #8 (permalink)  
Old 05-02-2008, 05:06 AM
KaiGai Kohei
 
Posts: n/a
Default Re: [0/4] Proposal of SE-PostgreSQL patches

Greg Smith wrote:
> On Wed, 30 Apr 2008, KaiGai Kohei wrote:
>
>> [1/4] sepostgresql-pgace-8.4devel-3-r739.patch
>> provides PGACE (PostgreSQL Access Control Extension) framework.
>>
>> http://sepgsql.googlecode.com/files/...l-3-r739.patch
>>

>
> For those overwhelmed by sheer volume here, this is the patch to start
> with, because it's got all the core changes to the server. I'm also in
> the camp that would like to see this feature added, but rather than just
> giving it a +1 I started looking at it.
>
> The overall code is nice: easy to understand, structured modularly. I
> have some concerns though. The first two things that jump out at me on
> an initial review appear right from the beginning for those who want to
> take a look:


Thanks for your attention & reviewing.

> -I'm a bit unnerved by both the performance and reliability implications
> from how the security check calls are done in every case, even if there
> is no SELinux support included. Those checks are sitting in some pretty
> low level tuple and heap calls.
>
> The approach taken here is to put all the "#ifdef" logic into the
> underlying ACE interface (see patch [2/4]), so that the caller doesn't
> have to care. If SELinux support is off then the calls turns into
>
> void x(y) {} or
> bool a(b) { return true; }
>
> This is a very clean design, but it's putting extra (possibly optimized
> away) calls into a lot of places. While it would be uglier, it might
> make sense to put that on/off logic in all the places where the calls
> are made, so that when you turn SELinux support off most of the code
> really does go completely away rather than just turning into stubs.


Your concern is fair enough. I indeed used inline function to avoid unnecessary
invokation in the previous version, as follows:
http://code.google.com/p/sepgsql/sou...curity/pgace.h

However, I changed this design, because it requires any security modules
have to provides its implementation for any hooks, including just a stub.
In the current design, author of another security modules can leave unused
hooks unchanged. It improves code maintenancability.

But an inspiration hit me just now.
If we declare any hooks as static inline functions which includes #ifdef
block, it can resolve these matters as follows:
---- EXAMPLE ----
static inline bool pgaceHeapTupleInsert(Relation rel, HeapTuple tuple,
bool is_internal, bool with_returning) {
#ifdef HAVE_SELINUX
if (sepgsqlIsEnabled())
return sepgsqlHeapTupleInsert(rel, tuple, is_internal, with_returning);
#endif
return true;
}
----------------
Is it reasonable idea?

> -The only error reporting and handling method used is "elog(ERROR,...".
> That seems a bit heavy handed for something that can be expected to
> happen all the time.


SE-PostgreSQL invokes "elog(ERROR,...", when a given query tries to access
violated columns, tables and so on, but not for each tuple.

In row-level access controls, it filters violated tuples without aborting
query execution. However, "elog(NOTICE,..." can be invoked, if the security
policy requires to generate access denied logs on row-level.

> If I understand this correctly, when you're scanning a table with 1000
> rows where you're only allowed to see 50% of them, that's going to be
> 500 call to elog(), one for each tuple you can't see.


The security policy can control whether access denied logs should be
printed, or not.
In the default security policy provided by the patch [4/4], row-level
access denied logs are disabled to avoid flood of logs as you noticed.

If you want to turn on/off row-level logs, use the following command:
# setsebool -P sepgsql_enable_audittuple=(1|0)
It changes the internal state of security policy, and SE-PostgreSQL
works according to this one.

> Having a tuple
> get screened out isn't really an error per se, and while I can see how
> sensitive installs would want those all reported there are others where
> this volume of log activity would be too much. Just because someone
> with classified clearance is looking at a big table that also has a lot
> of secret info in it, not all installs will want a million errors
> reported just because there's data that person can't see available.
>
> At a minimum, this needs some finer log control, and maybe a rethinking
> altogether of how to handle error cases.


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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Digg this Post!Add Post to del.icio.usBookmark Post in TechnoratiFurl this Post!
Reply With Quote
  #9 (permalink)  
Old 05-07-2008, 10:17 AM
Tom Lane
 
Posts: n/a
Default Re: [0/4] Proposal of SE-PostgreSQL patches

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
> I updated the series of SE-PostgreSQL patches for the latest pgsql-8.4devel tree.


I tried to do a bit of testing of this, but it does not work on current
Fedora 8, because the policy module doesn't build:

[tgl@rh2 sepgsql-policy]$ make
make -f /usr/share/selinux/devel/Makefile NAME=targeted
make[1]: Entering directory `/home/tgl/sepgsql/contrib/sepgsql-policy'
Compiling targeted sepostgresql module
/usr/bin/checkmodule: loading policy configuration from tmp/sepostgresql.tmp
sepostgresql.te:349:ERROR 'syntax error' at token 'corenet_tcp_recvfrom_labeled' on line 5675:
# NOTE: These changes are to be merged in the later releases.
corenet_tcp_recvfrom_labeled(sepgsql_server_type, sepgsql_client_type)
/usr/bin/checkmodule: error(s) encountered while parsing configuration
make[1]: *** [tmp/sepostgresql.mod] Error 1
make[1]: Leaving directory `/home/tgl/sepgsql/contrib/sepgsql-policy'
make: *** [sepostgresql.pp] Error 2
[tgl@rh2 sepgsql-policy]$


In the meantime, here are some random comments after my failed test
build and a very fast scan through the patch:

The patch tries to re-add ipcclean to the source tree --- probably a
merge error?

autoconf complains about the description-free AC_DEFINEs

Doesn't compile warning-free if selinux not enabled ... for that matter,
it doesn't compile warning-free if selinux IS enabled.

No documentation updates whatsoever :-(

About half of the patch seems to be conditional on
#ifdef SECURITY_SYSATTR_NAME
and the other half on
#ifdef HAVE_SELINUX
This seems bizarre: is there really any chance that there are two
independently usable chunks of code here? And why is it conditional
on a macro that is a field name, rather than conditional on a feature
macro? That is, I'd expect to see something like
#ifdef ENABLE_SEPOSTGRES
throughout.

For that matter, what is the point of treating SECURITY_SYSATTR_NAME
as a configurable thing in the first place? I can hardly imagine a
worse idea than a security-critical column having different names in
different installations.

The patch hasn't got a mode in which SELinux support is compiled in but
not active. This is a good way to ensure that no one will ever ship
standard RPMs with the feature compiled in, because they will be entirely
nonfunctional for people who aren't interested in setting up SELinux.
I think you need an "enable_sepostgres" GUC, or something like that.
(Of course, the overhead of the per-row security column would probably
discourage anyone from wanting to use such a configuration anyway,
so maybe the point is moot.)

sepgsql-policy has got usability problems:
* It should pay attention to the configured installation PREFIX instead of
hardwiring a couple of random possible installation locations
* It can only support the build machine's SELINUXTYPE --- how am I supposed
to produce RPMs that support all available types?

The contents and use of sepgsqlGetTupleName() make it look like the
entire security scheme is based on object name alone. That doesn't
even account for schemas, let alone overloaded function names. Please
tell me this is not as broken-by-design as it looks.

I occasionally tell people "try to make the patch look like it's always
been there". This is pretty far from meeting that goal. Random bits
of code that are commented "PGACE:" are obviously not intended to just
fit in. You've generally ignored the Postgres code layout conventions
(pgindent will help to some extent but it's far from a panacea) and our
commenting conventions --- eg, hardly any of the functions have header
comments, and the ones that do follow conventions seen noplace in the
Postgres code, like using "@" on parameter names. In general the number
and quality of the comments is far below the standard for Postgres code,
and the lack of any implementation documentation isn't helping.

Another big problem, which I understand your motivation for but that
doesn't make the code any less ugly, is that you've got trivial bits
of code that're separated by two(!) levels of hook calls from where
they're actually being used. Not only does that make it unreadable
but the files that actually do the work combine bits of code that
should be scattered across a lot of modules, causing those files to
be just horrid from a modularity standpoint --- they've got their
fingers stuck in everyplace. If you want this to get applied you need
to start thinking of it as an integral part of the code, not an add-on.

Some other bits of add-on-itis:
* If you need a dedicated LWLock, declare it in lwlock.h.
* If you need a node type, declare it in nodes.h (T_SEvalItem is utterly
broken)

Why in the world would you have security restrictions associated with
TOAST tuples? Seems like all the interesting restrictions should be
on the parent table.

Don't randomly invent your own style of management of a postmaster
child process. For one thing, this code doesn't cope with either
unexpected death of the postmaster or unexpected death of the child.
If you need another child, manage it in postmaster.c the same way
the other children are managed.

The code in hooks.c looks suspiciously not-HOT-aware, eg use of
ItemIdIsUsed() for what probably needs to be ItemIdIsNormal().
(Not that this code ought to be fetching the tuple for itself
in the first place --- probable big performance loss there...)

pgaceHooks.c seems to be a useless layer of indirection
--- lose it all, and inline into callers instead.

Is the hard-wired shmem cache size really adequate? Why are you
using such a cache in shared memory at all, rather than backend-local?
The locking implications likely take away more performance than you
save by not having each session need to load up its cache.
(We don't use shared catalog caches, in general.)

If we're going to support assignment to system columns, we probably
want a general solution that will work for OID not only the security
column. Also, I'm unconvinced that setting resjunk = true for such
targetlist entries is a good idea.

The whole "early security" business looks like a mess :-(. I suspect
you should rip all that out of the backend and add a step to initdb
that fills in those tables.

The idea of input functions that alter system tables scares me.

elog() should not be used for user-facing errors. I couldn't easily
tell just which of the messages are likely to be seen by users and
which ones should be "can't happen" cases, but certainly there are
a whole lot of these that need to be ereport()s. Likely there need
to be some new ERRCODEs too.

__lookupRelationForm() strikes me as a complete crock. It probably
means that you've selected the wrong places to call its callers from.
This gets back to the point above that adding additional fetches of
tuples isn't good for performance anyway.

Don't use identifiers with a leading double-underscore. These are
reserved for system-private identifiers according to the C standard.

Use of a function in genbki.sh is very likely not portable.

use of flock() is probably not portable and even less probably necessary.

Declaring a function foo() rather than foo(void) is poor style ---
at least some compilers will complain about that.

I see a lot of "Copyright 2007 KaiGai Kohei" notices. Are you
willing to assign those copyrights to the Postgres Global Development
Group? If not, we'll at least need statements along the line of
"Distributed under the PostgreSQL license".

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Digg this Post!Add Post to del.icio.usBookmark Post in TechnoratiFurl this Post!
Reply With Quote
  #10 (permalink)  
Old 05-07-2008, 10:17 AM
Greg Smith
 
Posts: n/a
Default Re: [0/4] Proposal of SE-PostgreSQL patches

On Mon, 5 May 2008, Tom Lane wrote:

> elog() should not be used for user-facing errors. I couldn't easily
> tell just which of the messages are likely to be seen by users and
> which ones should be "can't happen" cases, but certainly there are
> a whole lot of these that need to be ereport()s. Likely there need
> to be some new ERRCODEs too.


And it would be a nice step toward the scenarios I was asking about if
there was a GUC variable for what level to log security violations at. I
realize now the tuple-level warnings are going into the SELinux logs
rather than the PostgreSQL ones, but it should be easier to change policy
violations that impact the server to something other than just ERROR.

--
* Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Digg this Post!Add Post to del.icio.usBookmark Post in TechnoratiFurl this Post!
Reply With Quote
Reply


Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

vB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are On



All times are GMT. The time now is 02:05 PM.


Powered by vBulletin® Version 3.6.5
Copyright ©2000 - 2008, Jelsoft Enterprises Ltd.
Search Engine Optimization by vBSEO 3.1.0

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191 192 193 194 195 196 197 198 199 200 201 202 203 204 205 206 207 208 209 210 211 212 213 214 215 216 217 218 219 220 221 222 223 224 225 226 227 228 229 230 231 232 233 234 235 236 237 238 239 240 241 242 243 244 245 246 247 248 249 250 251 252 253 254 255 256 257 258 259 260 261 262 263 264 265 266 267 268 269 270 271 272 273 274 275 276 277 278 279 280 281 282 283 284 285 286 287 288 289 290 291 292 293 294 295 296 297 298 299 300 301 302 303 304 305 306 307 308 309 310 311 312 313 314 315 316 317 318 319 320 321 322 323 324 325 326 327 328 329 330 331 332 333 334 335 336 337 338 339 340 341 342 343 344 345 346 347 348 349 350 351 352 353 354 355 356 357 358 359 360 361 362 363 364 365 366 367 368 369 370 371 372 373 374 375 376 377 378 379 380 381 382 383 384 385 386 387 388 389 390 391 392 393 394 395 396 397 398 399 400 401 402 403 404 405 406 407 408 409 410 411 412 413 414 415 416 417 418 419 420 421 422 423 424 425 426 427 428 429 430 431 432 433 434 435 436 437 438 439 440 441 442 443 444 445 446 447 448 449 450 451 452