Thread: bootstrap pg_shseclabel in relcache initialization
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
Attachment
On 9 November 2015 at 12:40, Adam Brightwell <adam.brightwell@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. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 11/08/2015 11:17 PM, Craig Ringer wrote: > On 9 November 2015 at 12:40, Adam Brightwell > <adam.brightwell@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. +1 for adding to the next commitfest. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>> @@ -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_RELS 3 /* fix if you change list above */ +#define NUM_CRITICAL_SHARED_RELS 4 /* fix if you change list above */ -Adam
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_RELS 3 /* fix if you change list above */ > +#define NUM_CRITICAL_SHARED_RELS 4 /* fix if you change list above */ NUM_CRITICAL_SHARED_RELS != NUM_CRITICAL_SHARED_INDEXES
>> >> #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_RELS 3 /* fix if you change list above */ >> +#define NUM_CRITICAL_SHARED_RELS 4 /* fix if you change list above */ > > NUM_CRITICAL_SHARED_RELS != NUM_CRITICAL_SHARED_INDEXES Whoops! It must be getting late... updated patch attached. -Adam
Attachment
Adam Brightwell <adam.brightwell@crunchydata.com> 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
> -----Original Message----- > From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@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.brightwell@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 <kaigai@ak.jp.nec.com>
> 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
>> +1 for adding to the next commitfest. >> > Me also. Done. -Adam
On Tue, Nov 10, 2015 at 9:18 AM, Adam Brightwell <adam.brightwell@crunchydata.com> 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
Adam Brightwell <adam.brightwell@crunchydata.com> writes:
> On Tue, Nov 10, 2015 at 9:18 AM, Adam Brightwell
> <adam.brightwell@crunchydata.com> 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
> 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
Adam Brightwell <adam.brightwell@crunchydata.com> 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
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
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
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> 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
> 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