SEO

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
> Tom Lane wrote:
>> * It does not come close to passing the regression tests. I saw a lot of
>> ! ERROR: unrecognized node type: 903
>> which suggests that something's been screwed up about parse analysis
>> (903 = T_A_Const, which shouldn't get further than parse analysis),

> Could you tell me what queries hit these errors?

I remember seeing it on some EXECUTEs, but you really ought to run the
tests for yourself. A *minimum* requirement on any submitted patch
is that it should pass the regression tests.

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

Andrew, Marc,

> FWIW, I support and think important the row- and column- level access
> controls this seems to be proposing, at least in principle. Whether
> that's a support that will extend to 2x overhead on everything is
> rather a different matter. Also, I am more than prepared to trade
> away some cases in order to get a broadly useful functionality (so if
> you can't hide the existence of a table, but all efforts to learn its
> contents don't work, I might be willing to support that trade-off).

Well, there are two different goals we can satisfy. One is to help
support the kind of VPS functionality that Veil is designed for, and the
majority of users want. The second goal is upholding the kind of
security systems demanded by highly secure environments which have
statutory requirements about how security should work.

That is, while Veil-like funcitonality is what most developers want,
it's not what NSA/Banks/military want, who have their own ideas about
security. I think we can conceivably capture both.

I do think that SE functionality which goes beyond reasonable SQL
requirements should be a build-time options because I don't feasably see
ways to implement them that won't cause a big performance hit.

Also, I think you should be aware that for serious multilevel security
hackers (one of whom will be working on Postgres soon) SEPostgres is the
beginning and not the end. One of the requirements of many militaries,
for example, is not merely data hiding by data substitution, where the
row contents you see depend on your security clearance.

Also, re: pg_dump: it's actually a desired feature that, for example,
some users only be able to dump a subset of the database. Including
some DBAs. One of the issues which SE/Mulitlevel tries to address is
"what happens if you don't trust your DBA 100%?" So if we can retain
that, it's actually a feature and not a bug.

--Josh





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

>> 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.
>
> I also think "early security" codes are ad-hoc. :-(
> Pushing it into initdb seems me a good idea.
> I'll try to consider whether it is possible, or not.

The purpose of "early security" code is to manage relationship security
identifier and text representation before pg_security creation.

Therefore, we can make it redundant if initializing security attributes
can be done later. In the next patch set, I'll inject a hook to initialize
them at the last of bootstraping phase, and remove "early security" code.

Any tuples inserted during bootstraping mode are unlabeled, and this
operation is always allowed. Then, this hook is invoked after setting
up all system catalog, and it labels whole of database, as if "restorecon"
command initialize security context of filesystem objects.

How do you think about this design?
--
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

Tom Lane wrote:
> KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
>> Tom Lane wrote:
>>> * It does not come close to passing the regression tests. I saw a lot of
>>> ! ERROR: unrecognized node type: 903
>>> which suggests that something's been screwed up about parse analysis
>>> (903 = T_A_Const, which shouldn't get further than parse analysis),
>
>> Could you tell me what queries hit these errors?
>
> I remember seeing it on some EXECUTEs, but you really ought to run the
> tests for yourself. A *minimum* requirement on any submitted patch
> is that it should pass the regression tests.

Some of the test fails contains minor differences from expected results, like:

| SELECT '' AS "xxx", *
| FROM J1_TBL t1 (a, b, c) NATURAL JOIN J2_TBL t2 (d, a);
| xxx | a | b | c | d
| -----+---+---+------+---
| - | 0 | | zero |
| | 2 | 3 | two | 2
| | 4 | 1 | four | 2
| + | 0 | | zero |
| (3 rows)

and, some of them are trivial ones, like:

| SELECT p1.oid, p1.typname
| FROM pg_type as p1
| WHERE p1.typtype in ('b','e') AND p1.typname NOT LIKE E'\\_%' AND NOT EXISTS
| (SELECT 1 FROM pg_type as p2
| WHERE p2.typname = ('_' || p1.typname)::name AND
| p2.typelem = p1.oid and p1.typarray = p2.oid);
| - oid | typname
| ------+---------
| - 210 | smgr
| - 705 | unknown
| -(2 rows)
| + oid | typname
| +------+----------------
| + 210 | smgr
| + 705 | unknown
| + 3403 | security_label
| +(3 rows)

Isn't it necessary to consider them as regressions?

Thanks,
--
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

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
> Some of the test fails contains minor differences from expected results, like:

> | SELECT '' AS "xxx", *
> | FROM J1_TBL t1 (a, b, c) NATURAL JOIN J2_TBL t2 (d, a);
> | xxx | a | b | c | d
> | -----+---+---+------+---
> | - | 0 | | zero |
> | | 2 | 3 | two | 2
> | | 4 | 1 | four | 2
> | + | 0 | | zero |
> | (3 rows)

Yeah, I remember those. What needs to be looked at here is *why* the
output is changing. For a patch that allegedly does not touch the
planner, it's fairly disturbing that you don't get the same results.

> and, some of them are trivial ones, like:

> | SELECT p1.oid, p1.typname
> | FROM pg_type as p1
> | WHERE p1.typtype in ('b','e') AND p1.typname NOT LIKE E'\\_%' AND NOT EXISTS
> | (SELECT 1 FROM pg_type as p2
> | WHERE p2.typname = ('_' || p1.typname)::name AND
> | p2.typelem = p1.oid and p1.typarray = p2.oid);
> | - oid | typname
> | ------+---------
> | - 210 | smgr
> | - 705 | unknown
> | -(2 rows)
> | + oid | typname
> | +------+----------------
> | + 210 | smgr
> | + 705 | unknown
> | + 3403 | security_label
> | +(3 rows)

Are you sure that the security_label type should not have an array type?
I do not offhand see a good argument for that. If it really shouldn't,
we can change the expected output here, but you've got to make that
case first.

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

Tom Lane wrote:
> KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
>> Some of the test fails contains minor differences from expected results, like:
>
>> | SELECT '' AS "xxx", *
>> | FROM J1_TBL t1 (a, b, c) NATURAL JOIN J2_TBL t2 (d, a);
>> | xxx | a | b | c | d
>> | -----+---+---+------+---
>> | - | 0 | | zero |
>> | | 2 | 3 | two | 2
>> | | 4 | 1 | four | 2
>> | + | 0 | | zero |
>> | (3 rows)
>
> Yeah, I remember those. What needs to be looked at here is *why* the
> output is changing. For a patch that allegedly does not touch the
> planner, it's fairly disturbing that you don't get the same results.

SE-PostgreSQL does not touch the planner, but it modifies given query
to filter violated tuples for the current user.
Thus, the above query is dealt as if the following query is inputed.

SELECT '' AS "xxx", *
FROM J1_TBL t1 (a, b, c) NATURAL JOIN J2_TBL t2 (d, a)
ON sepgsql_tuple_perms(t1.security_context, SEPGSQL_PERMS_SELECT, ...)
and sepgsql_tuple_perms(t2.security_context, SEPGSQL_PERMS_SELECT, ...);

sepgsql_tuple_perms() returns true, if the security policy allows user
to access a given tuple. It returns false, if not so.

The result of EXPLAIN shows it more clearly:

| kaigai=# EXPLAIN SELECT '' AS "xxx", * FROM J1_TBL t1 (a, b, c) NATURAL JOIN J2_TBL t2 (d, a);
| QUERY PLAN
| -----------------------------------------------------------------------------------------------
| Hash Join (cost=29023.54..82599.93 rows=1380 width=44)
| Hash Cond: (t2.a = t1.a)
| -> Seq Scan on j2_tbl t2 (cost=0.00..53526.05 rows=713 width=8)
| Filter: pg_catalog.sepgsql_tuple_perms(tableoid, security_context, 12288, t2.*)
| -> Hash (cost=29018.70..29018.70 rows=387 width=40)
| -> Seq Scan on j1_tbl t1 (cost=0.00..29018.70 rows=387 width=40)
| Filter: pg_catalog.sepgsql_tuple_perms(tableoid, security_context, 12288, t1.*)
| (7 rows)

>> and, some of them are trivial ones, like:
>
>> | SELECT p1.oid, p1.typname
>> | FROM pg_type as p1
>> | WHERE p1.typtype in ('b','e') AND p1.typname NOT LIKE E'\\_%' AND NOT EXISTS
>> | (SELECT 1 FROM pg_type as p2
>> | WHERE p2.typname = ('_' || p1.typname)::name AND
>> | p2.typelem = p1.oid and p1.typarray = p2.oid);
>> | - oid | typname
>> | ------+---------
>> | - 210 | smgr
>> | - 705 | unknown
>> | -(2 rows)
>> | + oid | typname
>> | +------+----------------
>> | + 210 | smgr
>> | + 705 | unknown
>> | + 3403 | security_label
>> | +(3 rows)
>
> Are you sure that the security_label type should not have an array type?
> I do not offhand see a good argument for that. If it really shouldn't,
> we can change the expected output here, but you've got to make that
> case first.

Yes, security_label type should not have an array type.
It is defined with typelem=0 and typarray=0.
The purpose of this type is to represent the new system column of
security attribute ("security_context" column).

So, I think it is a case that a new expented output should be modified.

Thanks,
--
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

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
> Tom Lane wrote:
>> Yeah, I remember those. What needs to be looked at here is *why* the
>> output is changing. For a patch that allegedly does not touch the
>> planner, it's fairly disturbing that you don't get the same results.

> SE-PostgreSQL does not touch the planner, but it modifies given query
> to filter violated tuples for the current user.

Hmm. Is that really a good idea, compared to hard-wiring the checks
into nodeSeqscan and friends? I didn't look at the query-rewriting
portion of the patch in any detail, but I'd tend not to trust such
a technique very far: getting it right is going to be quite complex
and probably bug prone.

>> Are you sure that the security_label type should not have an array type?

> Yes, security_label type should not have an array type.

You didn't provide one ounce of justification for making it not obey the
expected behavior, so I'm not accepting this position. It doesn't seem
to me to be all that unlikely that users would want to compute with
arrays of security labels. As an example:
select ... where security_label in ('foo', 'bar')
which will become an = ANY(ARRAY[]) construct under the hood.

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

Tom Lane wrote:
> KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
>
>> Tom Lane wrote:
>>
>>> Yeah, I remember those. What needs to be looked at here is *why* the
>>> output is changing. For a patch that allegedly does not touch the
>>> planner, it's fairly disturbing that you don't get the same results.
>>>
>
>
>> SE-PostgreSQL does not touch the planner, but it modifies given query
>> to filter violated tuples for the current user.
>>
>
> Hmm. Is that really a good idea, compared to hard-wiring the checks
> into nodeSeqscan and friends? I didn't look at the query-rewriting
> portion of the patch in any detail, but I'd tend not to trust such
> a technique very far: getting it right is going to be quite complex
> and probably bug prone.
>
>

My eyebrows went up when I read this too. Presumably, if it's hardwired
like you suggest then the planner can't take any account of the filter,
though. Do we want it to?

OTOH, I'm not happy about silently rewriting queries, either - it would
make optimising queries a lot harder, I suspect.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> Hmm. Is that really a good idea, compared to hard-wiring the checks
>> into nodeSeqscan and friends?

> My eyebrows went up when I read this too. Presumably, if it's hardwired
> like you suggest then the planner can't take any account of the filter,
> though. Do we want it to?

Well, the planner could have hardwired knowledge about the existence of
the hardwired filters --- if anything, that'd probably be easier than
hacking it to have a similar level of knowledge about generic-looking
function calls. But in reality, since we don't know at plan time which
userid will execute the constructed plan, I'm not sure we could come up
with useful estimates anyway.

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

Tom Lane wrote:
> KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
>> Tom Lane wrote:
>>> Yeah, I remember those. What needs to be looked at here is *why* the
>>> output is changing. For a patch that allegedly does not touch the
>>> planner, it's fairly disturbing that you don't get the same results.
>
>> SE-PostgreSQL does not touch the planner, but it modifies given query
>> to filter violated tuples for the current user.
>
> Hmm. Is that really a good idea, compared to hard-wiring the checks
> into nodeSeqscan and friends? I didn't look at the query-rewriting
> portion of the patch in any detail, but I'd tend not to trust suchte
> a technique very far: getting it right is going to be quite complex
> and probably bug prone.

In the prior base version (8.2.x and 8.3.x), I tended to implement
these stuffs in the modular part as far as possible, because massive
patched hanks makes more difficult to follow the mainstreamed PostgreSQL. :-(

However, the hard-wides checks look like a good idea for me.
I tried to implement a prototype of the disign, and currently it works fine.
If we can replace the implementation of tuple-level access controls by this
design, it makes the implementation simpler.

Now, I add a hook into ExecScan(). It can return true or false, to decide
whether a given tuple is filtered or not.
Permissions to be evaluated are delivered via Scan structure. A variable
named as pgaceTuplePerms (uint32) is added to keep permission set for
tuple level access controls into Scan structure.
If the security module put a proper bitmask on pgaceTuplePerms of RangeTblEntry,
it is copied to related Scan structure later.

>>> Are you sure that the security_label type should not have an array type?
>
>> Yes, security_label type should not have an array type.
>
> You didn't provide one ounce of justification for making it not obey the
> expected behavior, so I'm not accepting this position. It doesn't seem
> to me to be all that unlikely that users would want to compute with
> arrays of security labels. As an example:
> select ... where security_label in ('foo', 'bar')
> which will become an = ANY(ARRAY[]) construct under the hood.

Ah.., I didn't intend such kind of usage, so security_label type does not
have operators to use it directly, not only array support.

I'll add it in the next patch set.

Thanks,
--
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