Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Adam Brightwell]

> Pushed, with one cosmetic change (update comment on formrdesc).  I also
> bumped the catversion, but didn't verify whether this is critical.

Excellent! Thanks!

-Adam


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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Adam Brightwell]

> So this looks like a bugfix that we should backpatch, but on closer
> inspection it turns out that we need the rowtype OID to be fixed, which
> it isn't unless this:
>
>> -CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
>> +CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_ROWTYPE_OID(4066) 
>> BKI_WITHOUT_OIDS BKI_SCHEMA_MACRO
>
> so I'm afraid this cannot be backpatched at all; if we did, the rowtype
> wouldn't match for already-initdb'd installations.
>
> I'm gonna push this to master only, which means you won't be able to
> rely on pg_shseclabel until 9.6.

I thinks that's fair.

-Adam


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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Alvaro Herrera]

Alvaro Herrera wrote:

> I'm gonna push this to master only, which means you won't be able to
> rely on pg_shseclabel until 9.6.

Pushed, with one cosmetic change (update comment on formrdesc).  I also
bumped the catversion, but didn't verify whether this is critical.

-- 
Álvaro Herrerahttp://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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Alvaro Herrera]

Adam Brightwell wrote:

> While working on an auth hook, I found that I was unable to access the
> pg_shseclabel system table while processing the hook.
[ ... ]
> Given that the shared relations currently exposed can also have
> security labels that can be used for auth purposes, I believe it makes
> sense to make those available as well.  I have attached a patch that
> adds this functionality for review/discussion.  If this functionality
> makes sense I'll add it to the commitfest.

So this looks like a bugfix that we should backpatch, but on closer
inspection it turns out that we need the rowtype OID to be fixed, which
it isn't unless this:

> -CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
> +CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_ROWTYPE_OID(4066) 
> BKI_WITHOUT_OIDS BKI_SCHEMA_MACRO

so I'm afraid this cannot be backpatched at all; if we did, the rowtype
wouldn't match for already-initdb'd installations.

I'm gonna push this to master only, which means you won't be able to
rely on pg_shseclabel until 9.6.

-- 
Álvaro Herrerahttp://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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Tom Lane]

I wrote:
> When I checked the behavior of 5d1ff6bd559ea8df, I must have only
> tried it for unshared catalogs.  Those are set up by
> RelationCacheInitializePhase3, which is post-authentication, so the
> message comes out and causes regression test failures as expected.

> This is kind of annoying :-(.  As noted in elog.c, it doesn't seem
> like a terribly good idea to send WARNING messages while the client
> is still in authentication mode; we can't be very sure that clients
> will react desirably.  So we can't fix it by mucking with that.

> One answer is to promote the case to an ERROR.  We could (probably) keep
> a bad initfile from becoming a permanent lockout condition by unlinking
> the initfile before reporting ERROR, but this way still seems like a
> reliability hazard that could be worse than the original problem.

After sleeping on it, the best compromise I can think of is to add an
"Assert(false)" after the WARNING report for the shared-catalogs case.
This will make the failure un-missable in any development build, while
not breaking production builds' ability to recover from corner cases
we might not've foreseen.

Of course, if you run an assert-enabled build in production, you might
possibly lose.  But that's never been recommended practice.

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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Joe Conway]

On 11/11/2015 11:31 AM, Tom Lane wrote:
> After sleeping on it, the best compromise I can think of is to add an
> "Assert(false)" after the WARNING report for the shared-catalogs case.
> This will make the failure un-missable in any development build, while
> not breaking production builds' ability to recover from corner cases
> we might not've foreseen.

That sounds like a good answer to me.

> Of course, if you run an assert-enabled build in production, you might
> possibly lose.  But that's never been recommended practice.

Yeah, there are plenty of other ways you would lose on that proposition.

Joe

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Adam Brightwell]

> In commit 5d1ff6bd559ea8df I'd expected that the
> WARNINGs would certainly show up in regression test output, and I thought
> I'd verified that that was the case --- did that not happen for you?

I just doubled checked with both 'check' and 'check-world' and neither
seemed to have an issue with it.  Though, I do see the warning show up
in 'regress/log/postmaster.log'.

-Adam


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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Adam Brightwell]

On Tue, Nov 10, 2015 at 9:18 AM, Adam Brightwell
 wrote:
>> I'm with Alvaro: the most interesting question here is why that mistake
>> did not blow up on you immediately.  I thought we had enough safeguards
>> in place to catch this type of error.
>
> Ok, I'll explore that a bit further as I was able to build and use
> with my hook without any issue. :-/

Ok, I have verified that it does indeed eventually raise a warning
about this.  It took a few for it to occur/appear in my logs. I was
expecting it to be more "immediate".  At any rate, I don't believe
there are any issues with the safeguards in place.

-Adam


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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Tom Lane]

Adam Brightwell  writes:
> On Tue, Nov 10, 2015 at 9:18 AM, Adam Brightwell
>  wrote:
>>> I'm with Alvaro: the most interesting question here is why that mistake
>>> did not blow up on you immediately.  I thought we had enough safeguards
>>> in place to catch this type of error.

>> Ok, I'll explore that a bit further as I was able to build and use
>> with my hook without any issue. :-/

> Ok, I have verified that it does indeed eventually raise a warning
> about this.  It took a few for it to occur/appear in my logs. I was
> expecting it to be more "immediate".

Hm.  Salesforce had also run into the issue that the warnings are just
warnings and relatively easy to miss.  What we did there was to change
the elog(WARNING)s near the bottom of load_relcache_init_file to
elog(ERROR), but I'm not sure if I want to recommend doing that in the
community sources.  In commit 5d1ff6bd559ea8df I'd expected that the
WARNINGs would certainly show up in regression test output, and I thought
I'd verified that that was the case --- did that not happen for you?

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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Tom Lane]

Adam Brightwell  writes:
>> In commit 5d1ff6bd559ea8df I'd expected that the
>> WARNINGs would certainly show up in regression test output, and I thought
>> I'd verified that that was the case --- did that not happen for you?

> I just doubled checked with both 'check' and 'check-world' and neither
> seemed to have an issue with it.  Though, I do see the warning show up
> in 'regress/log/postmaster.log'.

I poked around a bit and figured out what is wrong: for shared catalogs,
this message would be emitted during RelationCacheInitializePhase2(),
which is executed before we perform client authentication (as indeed
is rather your point here).  That means ClientAuthInProgress is still
true, which means elog.c doesn't honor client_min_messages --- it
will only send ERROR or higher messages to the client.  So the message
goes to the postmaster log, but not to the client.

When I checked the behavior of 5d1ff6bd559ea8df, I must have only
tried it for unshared catalogs.  Those are set up by
RelationCacheInitializePhase3, which is post-authentication, so the
message comes out and causes regression test failures as expected.

This is kind of annoying :-(.  As noted in elog.c, it doesn't seem
like a terribly good idea to send WARNING messages while the client
is still in authentication mode; we can't be very sure that clients
will react desirably.  So we can't fix it by mucking with that.

One answer is to promote the case to an ERROR.  We could (probably) keep
a bad initfile from becoming a permanent lockout condition by unlinking
the initfile before reporting ERROR, but this way still seems like a
reliability hazard that could be worse than the original problem.

Another idea, which seems pretty grotty but might be workable, is
to save aside some state about the failure during
RelationCacheInitializePhase2 and then actually send the WARNING
during RelationCacheInitializePhase3.  But that seems a little Rube
Goldberg-ish, and who's to say it couldn't break?

But I don't think I want to just do nothing.  We already found out
how easy it is to not realize that we have a bug here, leading to
a serious backend-startup-performance issue.

Thoughts?

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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Kouhei Kaigai]

> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Joe Conway
> Sent: Tuesday, November 10, 2015 3:08 AM
> To: Craig Ringer; Adam Brightwell
> Cc: PostgreSQL Hackers
> Subject: Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization
> 
> On 11/08/2015 11:17 PM, Craig Ringer wrote:
> > On 9 November 2015 at 12:40, Adam Brightwell
> > <adam.brightw...@crunchydata.com> wrote:
> >> Hi All,
> >>
> >> While working on an auth hook, I found that I was unable to access the
> >> pg_shseclabel system table while processing the hook.  I discovered
> >> that the only tables that were bootstrapped and made available at this
> >> stage of the the auth process were pg_database, pg_authid and
> >> pg_auth_members.  Unfortunately, this is problematic if you have
> >> security labels that are associated with a role which are needed to
> >> determine auth decisions/actions.
> >>
> >> Given that the shared relations currently exposed can also have
> >> security labels that can be used for auth purposes, I believe it makes
> >> sense to make those available as well.  I have attached a patch that
> >> adds this functionality for review/discussion.  If this functionality
> >> makes sense I'll add it to the commitfest.
> >
> > Your reasoning certainly makes sense to me. I'm a little surprised
> > this didn't cause issues for SEPostgreSQL already.
> 
> Currently sepgsql at least does not support security labels on roles,
> even though nominally postgres does. If the label provider does not
> support them (as in sepgsql) you just get a "feature not supported" type
> of error when trying to create the label. I'm not sure if there are any
> other label providers in the wild other than sepgsql, but I should think
> they would all benefit from this change.
>
The sepgsql does not support and its security model intends to assign
client's privileges orthogonal to database role, thus, it didn't touch
pg_shseclabel at that time. However, we can easily expect another label
provider that references database role.

> +1 for adding to the next commitfest.
>
Me also.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kai...@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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Adam Brightwell]

> I'm with Alvaro: the most interesting question here is why that mistake
> did not blow up on you immediately.  I thought we had enough safeguards
> in place to catch this type of error.

Ok, I'll explore that a bit further as I was able to build and use
with my hook without any issue. :-/

-Adam


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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Adam Brightwell]

>> +1 for adding to the next commitfest.
>>
> Me also.

Done.

-Adam


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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Tom Lane]

Adam Brightwell  writes:
>>> Need to bump this #define?  If you didn't get the error that this is
>>> supposed to throw, perhaps there's a bug somewhere worth investigating.

> Whoops!  It must be getting late... updated patch attached.

I'm with Alvaro: the most interesting question here is why that mistake
did not blow up on you immediately.  I thought we had enough safeguards
in place to catch this type of error.

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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Adam Brightwell]

>> @@ -3365,6 +3370,8 @@ RelationCacheInitializePhase3(void)
>>   AuthIdRelationId);
>>   load_critical_index(AuthMemMemRoleIndexId,
>>   AuthMemRelationId);
>> + load_critical_index(SharedSecLabelObjectIndexId,
>> + 
>> SharedSecLabelRelationId);
>>
>>  #define NUM_CRITICAL_SHARED_INDEXES 5/* fix if you change list 
>> above */
>>
>
> Need to bump this #define?  If you didn't get the error that this is
> supposed to throw, perhaps there's a bug somewhere worth investigating.

Hmm... I thought that I had, are you not seeing the following change?

-#define NUM_CRITICAL_SHARED_RELS3/* fix if you change list above */
+#define NUM_CRITICAL_SHARED_RELS4/* fix if you change list above */

-Adam


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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Andres Freund]

On 2015-11-09 23:38:57 -0500, Adam Brightwell wrote:
> >> @@ -3365,6 +3370,8 @@ RelationCacheInitializePhase3(void)
> >>   AuthIdRelationId);
> >>   load_critical_index(AuthMemMemRoleIndexId,
> >>   AuthMemRelationId);
> >> + load_critical_index(SharedSecLabelObjectIndexId,
> >> + 
> >> SharedSecLabelRelationId);
> >>
> >>  #define NUM_CRITICAL_SHARED_INDEXES 5/* fix if you change list 
> >> above */
> >>
> >
> > Need to bump this #define?  If you didn't get the error that this is
> > supposed to throw, perhaps there's a bug somewhere worth investigating.
> 
> Hmm... I thought that I had, are you not seeing the following change?
> 
> -#define NUM_CRITICAL_SHARED_RELS3/* fix if you change list above */
> +#define NUM_CRITICAL_SHARED_RELS4/* fix if you change list above */

NUM_CRITICAL_SHARED_RELS != NUM_CRITICAL_SHARED_INDEXES


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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Adam Brightwell]

>> >>  #define NUM_CRITICAL_SHARED_INDEXES 5/* fix if you change list 
>> >> above */
>> >>
>> >
>> > Need to bump this #define?  If you didn't get the error that this is
>> > supposed to throw, perhaps there's a bug somewhere worth investigating.
>>
>> Hmm... I thought that I had, are you not seeing the following change?
>>
>> -#define NUM_CRITICAL_SHARED_RELS3/* fix if you change list above */
>> +#define NUM_CRITICAL_SHARED_RELS4/* fix if you change list above */
>
> NUM_CRITICAL_SHARED_RELS != NUM_CRITICAL_SHARED_INDEXES

Whoops!  It must be getting late... updated patch attached.

-Adam
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 9c3d096..b701d82 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -51,6 +51,7 @@
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_rewrite.h"
+#include "catalog/pg_shseclabel.h"
 #include "catalog/pg_tablespace.h"
 #include "catalog/pg_trigger.h"
 #include "catalog/pg_type.h"
@@ -98,6 +99,7 @@ static const FormData_pg_attribute Desc_pg_database[Natts_pg_database] = {Schema
 static const FormData_pg_attribute Desc_pg_authid[Natts_pg_authid] = {Schema_pg_authid};
 static const FormData_pg_attribute Desc_pg_auth_members[Natts_pg_auth_members] = {Schema_pg_auth_members};
 static const FormData_pg_attribute Desc_pg_index[Natts_pg_index] = {Schema_pg_index};
+static const FormData_pg_attribute Desc_pg_shseclabel[Natts_pg_shseclabel] = {Schema_pg_shseclabel};
 
 /*
  *		Hash tables that index the relation cache
@@ -3187,13 +3189,14 @@ RelationCacheInitialize(void)
 /*
  *		RelationCacheInitializePhase2
  *
- *		This is called to prepare for access to shared catalogs during startup.
- *		We must at least set up nailed reldescs for pg_database, pg_authid,
- *		and pg_auth_members.  Ideally we'd like to have reldescs for their
- *		indexes, too.  We attempt to load this information from the shared
- *		relcache init file.  If that's missing or broken, just make phony
- *		entries for the catalogs themselves.  RelationCacheInitializePhase3
- *		will clean up as needed.
+ *		This is called to prepare for access to shared catalogs during
+ *		startup.  We must at least set up nailed reldescs for
+ *		pg_database, pg_authid, pg_auth_members, and pg_shseclabel.
+ *		Ideally we'd like to have reldescs for their indexes, too.  We
+ *		attempt to load this information from the shared relcache init
+ *		file.  If that's missing or broken, just make phony entries for
+ *		the catalogs themselves.  RelationCacheInitializePhase3 will
+ *		clean up as needed.
  */
 void
 RelationCacheInitializePhase2(void)
@@ -3229,8 +3232,10 @@ RelationCacheInitializePhase2(void)
   true, Natts_pg_authid, Desc_pg_authid);
 		formrdesc("pg_auth_members", AuthMemRelation_Rowtype_Id, true,
   false, Natts_pg_auth_members, Desc_pg_auth_members);
+		formrdesc("pg_shseclabel", SharedSecLabelRelation_Rowtype_Id, true,
+  false, Natts_pg_shseclabel, Desc_pg_shseclabel);
 
-#define NUM_CRITICAL_SHARED_RELS	3	/* fix if you change list above */
+#define NUM_CRITICAL_SHARED_RELS	4	/* fix if you change list above */
 	}
 
 	MemoryContextSwitchTo(oldcxt);
@@ -3365,8 +3370,10 @@ RelationCacheInitializePhase3(void)
 			AuthIdRelationId);
 		load_critical_index(AuthMemMemRoleIndexId,
 			AuthMemRelationId);
+		load_critical_index(SharedSecLabelObjectIndexId,
+			SharedSecLabelRelationId);
 
-#define NUM_CRITICAL_SHARED_INDEXES 5	/* fix if you change list above */
+#define NUM_CRITICAL_SHARED_INDEXES 6	/* fix if you change list above */
 
 		criticalSharedRelcachesBuilt = true;
 	}
diff --git a/src/include/catalog/pg_shseclabel.h b/src/include/catalog/pg_shseclabel.h
index 0ff41f3..d8334bf 100644
--- a/src/include/catalog/pg_shseclabel.h
+++ b/src/include/catalog/pg_shseclabel.h
@@ -18,9 +18,10 @@
  *		typedef struct FormData_pg_shseclabel
  * 
  */
-#define SharedSecLabelRelationId		3592
+#define SharedSecLabelRelationId			3592
+#define SharedSecLabelRelation_Rowtype_Id	4066
 
-CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
+CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_ROWTYPE_OID(4066) BKI_WITHOUT_OIDS BKI_SCHEMA_MACRO
 {
 	Oid			objoid;			/* OID of the shared object itself */
 	Oid			classoid;		/* OID of table containing the shared object */
@@ -31,6 +32,8 @@ CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
 #endif
 } FormData_pg_shseclabel;
 
+typedef FormData_pg_shseclabel *Form_pg_shseclabel;
+
 /* 
  *		compiler constants for pg_shseclabel
  * 

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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Alvaro Herrera]

Adam Brightwell wrote:

> @@ -3365,6 +3370,8 @@ RelationCacheInitializePhase3(void)
>   AuthIdRelationId);
>   load_critical_index(AuthMemMemRoleIndexId,
>   AuthMemRelationId);
> + load_critical_index(SharedSecLabelObjectIndexId,
> + 
> SharedSecLabelRelationId);
>  
>  #define NUM_CRITICAL_SHARED_INDEXES 5/* fix if you change list above 
> */
>  

Need to bump this #define?  If you didn't get the error that this is
supposed to throw, perhaps there's a bug somewhere worth investigating.

-- 
Álvaro Herrerahttp://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

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Joe Conway]

On 11/08/2015 11:17 PM, Craig Ringer wrote:
> On 9 November 2015 at 12:40, Adam Brightwell
>  wrote:
>> Hi All,
>>
>> While working on an auth hook, I found that I was unable to access the
>> pg_shseclabel system table while processing the hook.  I discovered
>> that the only tables that were bootstrapped and made available at this
>> stage of the the auth process were pg_database, pg_authid and
>> pg_auth_members.  Unfortunately, this is problematic if you have
>> security labels that are associated with a role which are needed to
>> determine auth decisions/actions.
>>
>> Given that the shared relations currently exposed can also have
>> security labels that can be used for auth purposes, I believe it makes
>> sense to make those available as well.  I have attached a patch that
>> adds this functionality for review/discussion.  If this functionality
>> makes sense I'll add it to the commitfest.
> 
> Your reasoning certainly makes sense to me. I'm a little surprised
> this didn't cause issues for SEPostgreSQL already.

Currently sepgsql at least does not support security labels on roles,
even though nominally postgres does. If the label provider does not
support them (as in sepgsql) you just get a "feature not supported" type
of error when trying to create the label. I'm not sure if there are any
other label providers in the wild other than sepgsql, but I should think
they would all benefit from this change.

+1 for adding to the next commitfest.

Joe

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization

[Craig Ringer]

On 9 November 2015 at 12:40, Adam Brightwell
 wrote:
> Hi All,
>
> While working on an auth hook, I found that I was unable to access the
> pg_shseclabel system table while processing the hook.  I discovered
> that the only tables that were bootstrapped and made available at this
> stage of the the auth process were pg_database, pg_authid and
> pg_auth_members.  Unfortunately, this is problematic if you have
> security labels that are associated with a role which are needed to
> determine auth decisions/actions.
>
> Given that the shared relations currently exposed can also have
> security labels that can be used for auth purposes, I believe it makes
> sense to make those available as well.  I have attached a patch that
> adds this functionality for review/discussion.  If this functionality
> makes sense I'll add it to the commitfest.

Your reasoning certainly makes sense to me. I'm a little surprised
this didn't cause issues for SEPostgreSQL already.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, 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

[HACKERS] bootstrap pg_shseclabel in relcache initialization

[Adam Brightwell]

Hi All,

While working on an auth hook, I found that I was unable to access the
pg_shseclabel system table while processing the hook.  I discovered
that the only tables that were bootstrapped and made available at this
stage of the the auth process were pg_database, pg_authid and
pg_auth_members.  Unfortunately, this is problematic if you have
security labels that are associated with a role which are needed to
determine auth decisions/actions.

Given that the shared relations currently exposed can also have
security labels that can be used for auth purposes, I believe it makes
sense to make those available as well.  I have attached a patch that
adds this functionality for review/discussion.  If this functionality
makes sense I'll add it to the commitfest.

Thanks,
Adam
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 9c3d096..c38a8ac 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -51,6 +51,7 @@
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_rewrite.h"
+#include "catalog/pg_shseclabel.h"
 #include "catalog/pg_tablespace.h"
 #include "catalog/pg_trigger.h"
 #include "catalog/pg_type.h"
@@ -98,6 +99,7 @@ static const FormData_pg_attribute Desc_pg_database[Natts_pg_database] = {Schema
 static const FormData_pg_attribute Desc_pg_authid[Natts_pg_authid] = {Schema_pg_authid};
 static const FormData_pg_attribute Desc_pg_auth_members[Natts_pg_auth_members] = {Schema_pg_auth_members};
 static const FormData_pg_attribute Desc_pg_index[Natts_pg_index] = {Schema_pg_index};
+static const FormData_pg_attribute Desc_pg_shseclabel[Natts_pg_shseclabel] = {Schema_pg_shseclabel};
 
 /*
  *		Hash tables that index the relation cache
@@ -3187,13 +3189,14 @@ RelationCacheInitialize(void)
 /*
  *		RelationCacheInitializePhase2
  *
- *		This is called to prepare for access to shared catalogs during startup.
- *		We must at least set up nailed reldescs for pg_database, pg_authid,
- *		and pg_auth_members.  Ideally we'd like to have reldescs for their
- *		indexes, too.  We attempt to load this information from the shared
- *		relcache init file.  If that's missing or broken, just make phony
- *		entries for the catalogs themselves.  RelationCacheInitializePhase3
- *		will clean up as needed.
+ *		This is called to prepare for access to shared catalogs during
+ *		startup.  We must at least set up nailed reldescs for
+ *		pg_database, pg_authid, pg_auth_members, and pg_shseclabel.
+ *		Ideally we'd like to have reldescs for their indexes, too.  We
+ *		attempt to load this information from the shared relcache init
+ *		file.  If that's missing or broken, just make phony entries for
+ *		the catalogs themselves.  RelationCacheInitializePhase3 will
+ *		clean up as needed.
  */
 void
 RelationCacheInitializePhase2(void)
@@ -3229,8 +3232,10 @@ RelationCacheInitializePhase2(void)
   true, Natts_pg_authid, Desc_pg_authid);
 		formrdesc("pg_auth_members", AuthMemRelation_Rowtype_Id, true,
   false, Natts_pg_auth_members, Desc_pg_auth_members);
+		formrdesc("pg_shseclabel", SharedSecLabelRelation_Rowtype_Id, true,
+  false, Natts_pg_shseclabel, Desc_pg_shseclabel);
 
-#define NUM_CRITICAL_SHARED_RELS	3	/* fix if you change list above */
+#define NUM_CRITICAL_SHARED_RELS	4	/* fix if you change list above */
 	}
 
 	MemoryContextSwitchTo(oldcxt);
@@ -3365,6 +3370,8 @@ RelationCacheInitializePhase3(void)
 			AuthIdRelationId);
 		load_critical_index(AuthMemMemRoleIndexId,
 			AuthMemRelationId);
+		load_critical_index(SharedSecLabelObjectIndexId,
+			SharedSecLabelRelationId);
 
 #define NUM_CRITICAL_SHARED_INDEXES 5	/* fix if you change list above */
 
diff --git a/src/include/catalog/pg_shseclabel.h b/src/include/catalog/pg_shseclabel.h
index 0ff41f3..d8334bf 100644
--- a/src/include/catalog/pg_shseclabel.h
+++ b/src/include/catalog/pg_shseclabel.h
@@ -18,9 +18,10 @@
  *		typedef struct FormData_pg_shseclabel
  * 
  */
-#define SharedSecLabelRelationId		3592
+#define SharedSecLabelRelationId			3592
+#define SharedSecLabelRelation_Rowtype_Id	4066
 
-CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
+CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_ROWTYPE_OID(4066) BKI_WITHOUT_OIDS BKI_SCHEMA_MACRO
 {
 	Oid			objoid;			/* OID of the shared object itself */
 	Oid			classoid;		/* OID of table containing the shared object */
@@ -31,6 +32,8 @@ CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
 #endif
 } FormData_pg_shseclabel;
 
+typedef FormData_pg_shseclabel *Form_pg_shseclabel;
+
 /* 
  *		compiler constants for pg_shseclabel
  * 

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