partitioned tables and contrib/sepgsql

On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

While going over the contrib modules, I noticed that sepgsql was not
updated for partitioned tables. What that appears to mean is that it's
not possible to define labels on partitioned tables. As I recall,
accessing the parent of a table will, similar to the GRANT system, not
perform checkes against the child tables, meaning that there's no way to
have SELinux checks properly enforced when partitioned tables are being
used.

I'll start taking a look at this. Presumably we'd just extend existing
object_access_hooks to cover partitioned tables?

This is an issue which should be resolved for PG10, so I'll add it to
the open items list.

I'll grab it. Thanks.

--Mike

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

Mike,

* Mike Palmiotto (mike.palmiotto@crunchydata.com) wrote:

On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost <sfrost@snowman.net> wrote:

While going over the contrib modules, I noticed that sepgsql was not
updated for partitioned tables. What that appears to mean is that it's
not possible to define labels on partitioned tables. As I recall,
accessing the parent of a table will, similar to the GRANT system, not
perform checkes against the child tables, meaning that there's no way to
have SELinux checks properly enforced when partitioned tables are being
used.

I'll start taking a look at this. Presumably we'd just extend existing
object_access_hooks to cover partitioned tables?

At least on first blush that seems like the right approach. We'll need
to make sure that the SECURITY LABEL system will properly work with
partitioned tables too, of course, and that the checks are called when a
user queries a partitioned table. Then we'll need regression tests to
make sure we get it all correct and don't screw it up in the future. ;)

This is an issue which should be resolved for PG10, so I'll add it to
the open items list.

I'll grab it. Thanks.

Excellent, thanks!

Stephen

On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost <sfrost@snowman.net> wrote:

While going over the contrib modules, I noticed that sepgsql was not
updated for partitioned tables. What that appears to mean is that it's
not possible to define labels on partitioned tables.

It works for me:

rhaas=# load 'dummy_seclabel';
LOAD
rhaas=# create table foo (a int, b text) partition by range (a);
CREATE TABLE
rhaas=# security label on table foo is 'classified';
SECURITY LABEL

What exactly is the problem you're seeing?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Mon, Mar 27, 2017 at 10:47 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost <sfrost@snowman.net> wrote:

While going over the contrib modules, I noticed that sepgsql was not
updated for partitioned tables. What that appears to mean is that it's
not possible to define labels on partitioned tables.

It works for me:

rhaas=# load 'dummy_seclabel';
LOAD
rhaas=# create table foo (a int, b text) partition by range (a);
CREATE TABLE
rhaas=# security label on table foo is 'classified';
SECURITY LABEL

What exactly is the problem you're seeing?

IIRC the initial concern was that contrib/sepgsql was not updated for
RELKIND_PARTITIONED_TABLE, so the post-create hook would not work
properly for partitioned tables.

I've looked into it a bit and saw a post-alter hook in
StoreCatalogInheritance1. It seems like it may just be an issue of
adding the RELKIND_PARTITIONED_TABLE to sepgsql_relation_post_create.

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

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

On Mon, Mar 27, 2017 at 11:22 AM, Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:

On Mon, Mar 27, 2017 at 10:47 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost <sfrost@snowman.net> wrote:

While going over the contrib modules, I noticed that sepgsql was not
updated for partitioned tables. What that appears to mean is that it's
not possible to define labels on partitioned tables.

It works for me:

rhaas=# load 'dummy_seclabel';
LOAD
rhaas=# create table foo (a int, b text) partition by range (a);
CREATE TABLE
rhaas=# security label on table foo is 'classified';
SECURITY LABEL

What exactly is the problem you're seeing?

IIRC the initial concern was that contrib/sepgsql was not updated for
RELKIND_PARTITIONED_TABLE, so the post-create hook would not work
properly for partitioned tables.

I've looked into it a bit and saw a post-alter hook in
StoreCatalogInheritance1. It seems like it may just be an issue of
adding the RELKIND_PARTITIONED_TABLE to sepgsql_relation_post_create.

I thought that kind of thing might be the issue, but it didn't seem to
match Stephen's description. Adding RELKIND_PARTITIONED_TABLE to that
function seems like it would probably be sufficient to make that hook
work, although that would need to be tested, but there are numerous
other references to RELKIND_RELATION in contrib/sepgsql, some of which
probably also need to be updated to consider
RELKIND_PARTITIONED_TABLE.

In my view, this is really an enhancement to sepgsql to handle a new
PostgreSQL feature rather than a defect in the partitioning commit, so
I don't think it falls on Amit Langote (as the author) or me (as the
committer) to fix. There might similarly be updates to sepgsql to do
something with permissions on logical replication's new publication
and subscription objects, but we should similarly regard those as
possible new features, not things that Petr or Peter need to work on.
Note that sepgsql hasn't been updated to work with RLS yet, either,
but we didn't regard that as an open item for RLS, or if we did the
resolution was just to document it. I am not opposed to giving a
little more time to get this straightened out, but if a patch doesn't
show up fairly soon then I think we should just document that sepgsql
doesn't support partitioned tables in v10. sepgsql has a fairly
lengthy list of implementation restrictions already, so one more is
not going to kill anybody -- or if it will then that person should
produce a patch soon.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Mon, Mar 27, 2017 at 11:46 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Mar 27, 2017 at 11:22 AM, Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:

On Mon, Mar 27, 2017 at 10:47 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Mar 9, 2017 at 9:47 AM, Stephen Frost <sfrost@snowman.net> wrote:

While going over the contrib modules, I noticed that sepgsql was not
updated for partitioned tables. What that appears to mean is that it's
not possible to define labels on partitioned tables.

It works for me:

rhaas=# load 'dummy_seclabel';
LOAD
rhaas=# create table foo (a int, b text) partition by range (a);
CREATE TABLE
rhaas=# security label on table foo is 'classified';
SECURITY LABEL

What exactly is the problem you're seeing?

IIRC the initial concern was that contrib/sepgsql was not updated for
RELKIND_PARTITIONED_TABLE, so the post-create hook would not work
properly for partitioned tables.

I've looked into it a bit and saw a post-alter hook in
StoreCatalogInheritance1. It seems like it may just be an issue of
adding the RELKIND_PARTITIONED_TABLE to sepgsql_relation_post_create.

I thought that kind of thing might be the issue, but it didn't seem to
match Stephen's description. Adding RELKIND_PARTITIONED_TABLE to that
function seems like it would probably be sufficient to make that hook
work, although that would need to be tested, but there are numerous
other references to RELKIND_RELATION in contrib/sepgsql, some of which
probably also need to be updated to consider
RELKIND_PARTITIONED_TABLE.

Agreed.

In my view, this is really an enhancement to sepgsql to handle a new
PostgreSQL feature rather than a defect in the partitioning commit, so
I don't think it falls on Amit Langote (as the author) or me (as the
committer) to fix. There might similarly be updates to sepgsql to do
something with permissions on logical replication's new publication
and subscription objects, but we should similarly regard those as
possible new features, not things that Petr or Peter need to work on.

I'll keep these items in mind for future reference. Thanks.

Note that sepgsql hasn't been updated to work with RLS yet, either,
but we didn't regard that as an open item for RLS, or if we did the
resolution was just to document it. I am not opposed to giving a
little more time to get this straightened out, but if a patch doesn't
show up fairly soon then I think we should just document that sepgsql
doesn't support partitioned tables in v10. sepgsql has a fairly
lengthy list of implementation restrictions already, so one more is
not going to kill anybody -- or if it will then that person should
produce a patch soon.

Okay, I'll make sure I get something fleshed out today or tomorrow.

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

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

On Mon, Mar 27, 2017 at 12:09 PM, Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:

On Mon, Mar 27, 2017 at 11:46 AM, Robert Haas <robertmhaas@gmail.com> wrote:

<snip>
Note that sepgsql hasn't been updated to work with RLS yet, either,
but we didn't regard that as an open item for RLS, or if we did the
resolution was just to document it. I am not opposed to giving a
little more time to get this straightened out, but if a patch doesn't
show up fairly soon then I think we should just document that sepgsql
doesn't support partitioned tables in v10. sepgsql has a fairly
lengthy list of implementation restrictions already, so one more is
not going to kill anybody -- or if it will then that person should
produce a patch soon.

Okay, I'll make sure I get something fleshed out today or tomorrow.

Apologies for the delay. I was waffling over whether to reference
PartitionedRelationId in sepgsql, but ended up deciding to just handle
RELKIND_PARTITIONED_TABLE and treat the classOid as
RelationRelationId. Seeing as there is a relid in pg_class which
corresponds to the partitioned table, this chosen route seemed
acceptable.

Here is a demonstration of the partitioned table working with sepgsql hooks:
https://gist.github.com/anonymous/b10f476a95ae9cdd39b83ef872d4b1e6

Attached you will find two patches, which were rebased on master as of
156d388 (applied with `git am --revert [patch file]`). The first gets
rid of some pesky compiler warnings and the second implements the
sepgsql handling of partitioned tables.

Thanks,

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

On Fri, Mar 31, 2017 at 2:14 PM, Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:

On Mon, Mar 27, 2017 at 12:09 PM, Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:

On Mon, Mar 27, 2017 at 11:46 AM, Robert Haas <robertmhaas@gmail.com> wrote:

<snip>
Note that sepgsql hasn't been updated to work with RLS yet, either,
but we didn't regard that as an open item for RLS, or if we did the
resolution was just to document it. I am not opposed to giving a
little more time to get this straightened out, but if a patch doesn't
show up fairly soon then I think we should just document that sepgsql
doesn't support partitioned tables in v10. sepgsql has a fairly
lengthy list of implementation restrictions already, so one more is
not going to kill anybody -- or if it will then that person should
produce a patch soon.

Okay, I'll make sure I get something fleshed out today or tomorrow.

Apologies for the delay. I was waffling over whether to reference
PartitionedRelationId in sepgsql, but ended up deciding to just handle
RELKIND_PARTITIONED_TABLE and treat the classOid as
RelationRelationId. Seeing as there is a relid in pg_class which
corresponds to the partitioned table, this chosen route seemed
acceptable.

Newest patches remove cruft from said waffling. No need to include
pg_partitioned_table.h if we're not referencing PartitionedRelationId.

Here is a demonstration of the partitioned table working with sepgsql hooks:
https://gist.github.com/anonymous/b10f476a95ae9cdd39b83ef872d4b1e6

Attached you will find two patches, which were rebased on master as of
156d388 (applied with `git am --revert [patch file]`). The first gets
rid of some pesky compiler warnings and the second implements the
sepgsql handling of partitioned tables.

That should have read `git am --reject [patch file]`. Apologies for
the inaccuracy.

Thanks,

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

On Fri, Mar 31, 2017 at 2:14 PM, Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:

Attached you will find two patches, which were rebased on master as of
156d388 (applied with `git am --revert [patch file]`). The first gets
rid of some pesky compiler warnings and the second implements the
sepgsql handling of partitioned tables.

0001 has the problem that we have a firm rule against putting any
#includes whatsoever before "postgres.h". This stdbool issue has come
up before, though, and I fear we're going to need to do something
about it.

0002 looks extremely straightforward, but I wonder if we could get one
of the people on this list who knows about sepgsql to have a look?
(Stephen Frost, Joe Conway, KaiGai Kohei...)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Fri, Mar 31, 2017 at 8:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Mar 31, 2017 at 2:14 PM, Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:

Attached you will find two patches, which were rebased on master as of
156d388 (applied with `git am --revert [patch file]`). The first gets
rid of some pesky compiler warnings and the second implements the
sepgsql handling of partitioned tables.

0001 has the problem that we have a firm rule against putting any
#includes whatsoever before "postgres.h". This stdbool issue has come
up before, though, and I fear we're going to need to do something
about it.

Yeah, I recall this rule. The only things I can really think of to
solve the problem are:
1) #define _STDBOOL_H in postgres's c.h once bool and the like have
been defined, so we can avoid re-definition.
2) Enforce that any code utilizing the stdbool header manage the
re-definition with some combination of #undef/#define/#typedef and
document the issue somewhere.

I'd be more inclined to believe 2) is the correct solution, since 1)
is more of a hack than anything.

Thoughts?

0002 looks extremely straightforward, but I wonder if we could get one
of the people on this list who knows about sepgsql to have a look?
(Stephen Frost, Joe Conway, KaiGai Kohei...)

I welcome any and all feedback.

Thanks,
--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

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

On 03/31/2017 05:23 PM, Robert Haas wrote:

On Fri, Mar 31, 2017 at 2:14 PM, Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:

Attached you will find two patches, which were rebased on master as of
156d388 (applied with `git am --revert [patch file]`). The first gets
rid of some pesky compiler warnings and the second implements the
sepgsql handling of partitioned tables.

0001 has the problem that we have a firm rule against putting any
#includes whatsoever before "postgres.h". This stdbool issue has come
up before, though, and I fear we're going to need to do something
about it.

0002 looks extremely straightforward, but I wonder if we could get one
of the people on this list who knows about sepgsql to have a look?
(Stephen Frost, Joe Conway, KaiGai Kohei...)

Will have a look later today.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Mon, Apr 3, 2017 at 12:02 PM, Joe Conway <mail@joeconway.com> wrote:

0002 looks extremely straightforward, but I wonder if we could get one
of the people on this list who knows about sepgsql to have a look?
(Stephen Frost, Joe Conway, KaiGai Kohei...)

Will have a look later today.

I think it is now tomorrow, unless your time zone is someplace very
far away. :-)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 04/04/2017 06:45 AM, Robert Haas wrote:

On Mon, Apr 3, 2017 at 12:02 PM, Joe Conway <mail@joeconway.com> wrote:

0002 looks extremely straightforward, but I wonder if we could get one
of the people on this list who knows about sepgsql to have a look?
(Stephen Frost, Joe Conway, KaiGai Kohei...)

Will have a look later today.

I think it is now tomorrow, unless your time zone is someplace very
far away. :-)

OBE -- I have scheduled time in 30 minutes from now, after I have gotten
my first cup of coffee ;-)

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Tue, Apr 4, 2017 at 10:19 AM, Joe Conway <mail@joeconway.com> wrote:

On 04/04/2017 06:45 AM, Robert Haas wrote:

On Mon, Apr 3, 2017 at 12:02 PM, Joe Conway <mail@joeconway.com> wrote:

0002 looks extremely straightforward, but I wonder if we could get one
of the people on this list who knows about sepgsql to have a look?
(Stephen Frost, Joe Conway, KaiGai Kohei...)

Will have a look later today.

I think it is now tomorrow, unless your time zone is someplace very
far away. :-)

OBE -- I have scheduled time in 30 minutes from now, after I have gotten
my first cup of coffee ;-)

After some discussion off-list, I've rebased and udpated the patches.
Please see attached for further review.

Thanks,
--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

On 04/04/2017 09:55 AM, Mike Palmiotto wrote:

On Tue, Apr 4, 2017 at 10:19 AM, Joe Conway <mail@joeconway.com> wrote:

On 04/04/2017 06:45 AM, Robert Haas wrote:

On Mon, Apr 3, 2017 at 12:02 PM, Joe Conway <mail@joeconway.com> wrote:

0002 looks extremely straightforward, but I wonder if we could get one
of the people on this list who knows about sepgsql to have a look?
(Stephen Frost, Joe Conway, KaiGai Kohei...)

Will have a look later today.

I think it is now tomorrow, unless your time zone is someplace very
far away. :-)

OBE -- I have scheduled time in 30 minutes from now, after I have gotten
my first cup of coffee ;-)

After some discussion off-list, I've rebased and udpated the patches.
Please see attached for further review.

Thanks -- will have another look and test on a machine with selinux
setup. Robert, did you want me to take responsibility to commit on this
or just provide review/feedback?

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On 04/04/2017 10:02 AM, Joe Conway wrote:

On 04/04/2017 09:55 AM, Mike Palmiotto wrote:

After some discussion off-list, I've rebased and udpated the patches.
Please see attached for further review.

Thanks -- will have another look and test on a machine with selinux
setup. Robert, did you want me to take responsibility to commit on this
or just provide review/feedback?

I did some editorializing on these.

In particular I did not like the approach to fixing "warning: ‘tclass’
may be used uninitialized" and ended up just doing it the same as was
done elsewhere in relation.c already (set tclass = 0 in the variable
declaration). Along the way I also changed one instance of tclass from
uint16 to uint16_t for the sake of consistency.

Interestingly we figured out that the warning was present with -Og, but
not present with -O0, -O2, or -O3.

If you want to test, apply 0001a and 0001b before 0002.

Any objections?

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Tue, Apr 4, 2017 at 6:56 PM, Joe Conway <mail@joeconway.com> wrote:

On 04/04/2017 10:02 AM, Joe Conway wrote:

On 04/04/2017 09:55 AM, Mike Palmiotto wrote:

After some discussion off-list, I've rebased and udpated the patches.
Please see attached for further review.

Thanks -- will have another look and test on a machine with selinux
setup. Robert, did you want me to take responsibility to commit on this
or just provide review/feedback?

I did some editorializing on these.

In particular I did not like the approach to fixing "warning: ‘tclass’
may be used uninitialized" and ended up just doing it the same as was
done elsewhere in relation.c already (set tclass = 0 in the variable
declaration). Along the way I also changed one instance of tclass from
uint16 to uint16_t for the sake of consistency.

Interestingly we figured out that the warning was present with -Og, but
not present with -O0, -O2, or -O3.

If you want to test, apply 0001a and 0001b before 0002.

Any objections?

I'm guessing Tom's going to have a strong feeling about whether 0001a
is the right way to address the stdbool issue, but I don't personally
know what the right thing to do is so I will avoid opining on that
topic.

So, nope, no objections here to you committing those.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Apr 4, 2017 at 6:56 PM, Joe Conway <mail@joeconway.com> wrote:

Any objections?

I'm guessing Tom's going to have a strong feeling about whether 0001a
is the right way to address the stdbool issue,

I will? [ looks ... ] Yup, you're right.

I doubt that works at all, TBH. What I'd expect to happen with a
typical compiler is a complaint about redefinition of typedef bool,
because c.h already declared it and here this fragment is doing
so again. It'd make sense to me to do

+ #ifdef bool
+ #undef bool
+ #endif

to get rid of the macro definition of bool that stdbool.h is
supposed to provide. But there should be no reason to declare
our typedef a second time.

Another issue is whether you won't get compiler complaints about
redefinition of the "true" and "false" macros. But those would
likely only be warnings, not flat-out errors.

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

On 4/5/17 00:58, Tom Lane wrote:

Another issue is whether you won't get compiler complaints about
redefinition of the "true" and "false" macros. But those would
likely only be warnings, not flat-out errors.

The complaint about bool is also just a warning.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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