Thread: Proposed patch to remove USERLIMIT
The attached patch removes GUC's USERLIMIT variable category, changing
all the affected variables to be plain SUSET, as per recent discussion.
I also modified postgres.c so that variable settings coming from the
client connection request packet (eg, from PGOPTIONS on the client side)
are processed only after we have determined whether the user is a
superuser. This allows a superuser to set SUSET options from PGOPTIONS,
which has never worked before.
I consider this to be a lot cleaner and more flexible than what we have
now. Comments?
regards, tom lane
Attachment
Tom Lane wrote: > The attached patch removes GUC's USERLIMIT variable category, changing > all the affected variables to be plain SUSET, as per recent discussion. > > I also modified postgres.c so that variable settings coming from the > client connection request packet (eg, from PGOPTIONS on the client side) > are processed only after we have determined whether the user is a > superuser. This allows a superuser to set SUSET options from PGOPTIONS, > which has never worked before. > > I consider this to be a lot cleaner and more flexible than what we have > now. Comments? I would be glad to see USERLIMIT gone, even though I wrote it. However, I feel we are removing user's ability of non-super users to turn logging on and off easily without really having thought through a mechanism to give them that. I just don't see people creating secuirty definer functions easily to do this and perhaps we should ship some capability with our source. I am thinking we should hold until 8.1. I do like the idea of moving things to ALTER USER but I think there is more work for us to do. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > I would be glad to see USERLIMIT gone, even though I wrote it. However, > > I feel we are removing user's ability of non-super users to turn logging > > on and off easily without really having thought through a mechanism to > > give them that. > > I think that is a very weak argument. Remember that the discussion Well, I disagree or I wouldn't have made the argument, would I? > started because setting these variables via ALTER USER fails to work. It fails to work if logging was already on and someone wants to turn it off via ALTER USER, and that matches the expected behavior. > Why is that of less concern than the fact that unprivileged users won't > be able to increase the log level by themselves? I think it's pretty Because it is working as planned, meaning normal users can't turn off logging. > debatable whether the current behavior is a feature at all, rather than > a security hole. > > > I just don't see people creating secuirty definer > > functions easily to do this > > create or replace function enable_logging(bool) returns void as $$ > begin > if $1 then > set log_statement = 'all'; > else > set log_statement = 'ddl'; > end if; > return; > end$$ language plpgsql security definer; > > revoke execute on function enable_logging(bool) from public; > grant execute on function enable_logging(bool) to joe_user; > > (My, that was hard ...) Well, you have just made the system less secure by creating that function. Why didn't you create a function that has the existing behavior of only allowing users to increase the log level? Because it is harder to do, and certainly not something trivial you would write during a debugging session. > You seem to be supposing that anyone who needs this will be needing a > bug-for-bug equivalent to the existing USERLIMIT behavior. I don't What bug? > think so. I think that in the field, people are going to have at most > a couple of logging configurations they want to allow users to select, > and they will use code not significantly more complex than the above. Can you show me the function? > Arguably, this approach is better than directly calling the SET commands > anyway, because it insulates the application code from the changing > details of the specific GUC variables. Need I remind you that any app > that directly issues "SET log_statement" is going to be broken by 8.0? > In 7.4 this variable took "on" and "off", now it does not. The argument > of preserving backwards-compatible behavior looks a bit funny beside > that reality. I realize that but I think we need to give people the existing functionality out of the box, or at least not make such a change so close to final without time to discuss. You should ask on general where we usually do such polls of feature changes? People are already complaining we don't make logging easy enough (pg_stat_activity) and now we are going to make it harder still? Doesn't make sense to me, and I don't see your change as a bug fix. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom Lane wrote:
> >> started because setting these variables via ALTER USER fails to work.
>
> > It fails to work if logging was already on and someone wants to turn it
> > off via ALTER USER, and that matches the expected behavior.
>
> Not if a superuser does it; superusers should be allowed to set it
> however they want. In current code a superuser can't even set it
> that way for himself, let alone for other nonprivileged users.
> You seem to define "expected behavior" as "whatever the code does now",
> but in point of fact it's got lots of deficiencies.
I just turned logging on in my postgresql.conf file, did as super-user:
test=> ALTER USER postgres SET log_statement = 'none';
and it seemed to work fine. Are you saying the problem is that the
super-user can't set log_statement for non-super users via ALTER USER?
Yes, that doesn't work, but I have seen only one user request to do
that, while I have seen a lot of requests of people wanting to turn
log_statement on and off as non-super users in their applications.
> > Well, you have just made the system less secure by creating that
> > function. Why didn't you create a function that has the existing
> > behavior of only allowing users to increase the log level?
>
> I repeat my point: I don't think DBAs actually need that. In the
> function as illustrated, I gave joe_user (and nobody else) the choice
> between 'all' and 'ddl' (and nothing else), where I suppose 'ddl' is the
> global default. This is in fact a whole lot *more* secure than the
> existing behavior, in the sense that I can constrain what's allowed a
> lot more.
>
> If I later decide I want 'mod' as the global default, I can alter the
> function to map it that way, and joe_user's application is entirely
> unaffected. This is *not* true if I expect joe_user's app to set the
> variable directly. He has to go change his app to conform to the new
> global default.
OK, so you are saying your function has to be aligned with the global
default.
> >> think so. I think that in the field, people are going to have at most
> >> a couple of logging configurations they want to allow users to select,
> >> and they will use code not significantly more complex than the above.
>
> > Can you show me the function?
>
> I did.
I want to see one that allows a non-super user to only to increase the
log level without predefining the lowest log level allowed.
> > You should ask on general where
> > we usually do such polls of feature changes?
>
> Where was the poll in which we decided that the logging variables needed
> to be secured at all? In 7.3 these variables were all plain USERSET.
We didn't need a pool because we had repeated user complaints stating
our current setup was insecure, and we weren't in feature freeze or near
final release.
This whole discussion is taking time away from getting us to final.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I would be glad to see USERLIMIT gone, even though I wrote it. However,
> I feel we are removing user's ability of non-super users to turn logging
> on and off easily without really having thought through a mechanism to
> give them that.
I think that is a very weak argument. Remember that the discussion
started because setting these variables via ALTER USER fails to work.
Why is that of less concern than the fact that unprivileged users won't
be able to increase the log level by themselves? I think it's pretty
debatable whether the current behavior is a feature at all, rather than
a security hole.
> I just don't see people creating secuirty definer
> functions easily to do this
create or replace function enable_logging(bool) returns void as $$
begin
if $1 then
set log_statement = 'all';
else
set log_statement = 'ddl';
end if;
return;
end$$ language plpgsql security definer;
revoke execute on function enable_logging(bool) from public;
grant execute on function enable_logging(bool) to joe_user;
(My, that was hard ...)
You seem to be supposing that anyone who needs this will be needing a
bug-for-bug equivalent to the existing USERLIMIT behavior. I don't
think so. I think that in the field, people are going to have at most
a couple of logging configurations they want to allow users to select,
and they will use code not significantly more complex than the above.
Arguably, this approach is better than directly calling the SET commands
anyway, because it insulates the application code from the changing
details of the specific GUC variables. Need I remind you that any app
that directly issues "SET log_statement" is going to be broken by 8.0?
In 7.4 this variable took "on" and "off", now it does not. The argument
of preserving backwards-compatible behavior looks a bit funny beside
that reality.
regards, tom lane
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> started because setting these variables via ALTER USER fails to work.
> It fails to work if logging was already on and someone wants to turn it
> off via ALTER USER, and that matches the expected behavior.
Not if a superuser does it; superusers should be allowed to set it
however they want. In current code a superuser can't even set it
that way for himself, let alone for other nonprivileged users.
You seem to define "expected behavior" as "whatever the code does now",
but in point of fact it's got lots of deficiencies.
> Well, you have just made the system less secure by creating that
> function. Why didn't you create a function that has the existing
> behavior of only allowing users to increase the log level?
I repeat my point: I don't think DBAs actually need that. In the
function as illustrated, I gave joe_user (and nobody else) the choice
between 'all' and 'ddl' (and nothing else), where I suppose 'ddl' is the
global default. This is in fact a whole lot *more* secure than the
existing behavior, in the sense that I can constrain what's allowed a
lot more.
If I later decide I want 'mod' as the global default, I can alter the
function to map it that way, and joe_user's application is entirely
unaffected. This is *not* true if I expect joe_user's app to set the
variable directly. He has to go change his app to conform to the new
global default.
>> think so. I think that in the field, people are going to have at most
>> a couple of logging configurations they want to allow users to select,
>> and they will use code not significantly more complex than the above.
> Can you show me the function?
I did.
> You should ask on general where
> we usually do such polls of feature changes?
Where was the poll in which we decided that the logging variables needed
to be secured at all? In 7.3 these variables were all plain USERSET.
regards, tom lane
The bottom line is that I have no problem with making this change, but I want to be sure it is a full solution we aren't going to have to revisit later. I don't want to make the change and then find out we rushed it and have to make more adjustments in later releases. --------------------------------------------------------------------------- Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Tom Lane wrote: > >> started because setting these variables via ALTER USER fails to work. > > > It fails to work if logging was already on and someone wants to turn it > > off via ALTER USER, and that matches the expected behavior. > > Not if a superuser does it; superusers should be allowed to set it > however they want. In current code a superuser can't even set it > that way for himself, let alone for other nonprivileged users. > You seem to define "expected behavior" as "whatever the code does now", > but in point of fact it's got lots of deficiencies. > > > Well, you have just made the system less secure by creating that > > function. Why didn't you create a function that has the existing > > behavior of only allowing users to increase the log level? > > I repeat my point: I don't think DBAs actually need that. In the > function as illustrated, I gave joe_user (and nobody else) the choice > between 'all' and 'ddl' (and nothing else), where I suppose 'ddl' is the > global default. This is in fact a whole lot *more* secure than the > existing behavior, in the sense that I can constrain what's allowed a > lot more. > > If I later decide I want 'mod' as the global default, I can alter the > function to map it that way, and joe_user's application is entirely > unaffected. This is *not* true if I expect joe_user's app to set the > variable directly. He has to go change his app to conform to the new > global default. > > >> think so. I think that in the field, people are going to have at most > >> a couple of logging configurations they want to allow users to select, > >> and they will use code not significantly more complex than the above. > > > Can you show me the function? > > I did. > > > You should ask on general where > > we usually do such polls of feature changes? > > Where was the poll in which we decided that the logging variables needed > to be secured at all? In 7.3 these variables were all plain USERSET. > > regards, tom lane > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> The bottom line is that I have no problem with making this change, but I
> want to be sure it is a full solution we aren't going to have to revisit
> later. I don't want to make the change and then find out we rushed it
> and have to make more adjustments in later releases.
I would normally not care whether we did it now or postponed it to 8.1,
but I am concerned that we will find ourselves spending time fixing bugs
in the USERLIMIT code if we leave it in 8.0. I already spent half a day
fixing some coredump cases in it not long ago, and I have little
confidence that there aren't more problems.
Anyone else have an opinion on this? Bruce and I clearly aren't
convincing each other.
regards, tom lane