Re: [HACKERS] CATUPDATE confusion?

2015-03-16 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote:
 On Fri, Mar 13, 2015 at 9:52 AM, Stephen Frost sfr...@snowman.net wrote:
  * Stephen Frost (sfr...@snowman.net) wrote:
  I should have been more specific.  I don't believe they've moved to
  using pg_roles completely (which was created specifically to address the
  issue that regular users can't select from pg_authid).
 
  Err, forgot to finish that thought, sorry.  Let's try again:
 
  I should have been more specific.  I don't believe they've moved to
  using pg_roles completely (which was created specifically to address the
  issue that regular users can't select from pg_authid) simply because
  they haven't had reason to.
 
 That's another way of saying removing pg_user would be creating extra
 work for tool authors that otherwise wouldn't need to be done.

Sure, if we never deprecate anything then tool authors would never need
to change their existing code.  I don't think that's actually a viable
solution though; the reason we're discussing the removal of these
particular views is that they aren't really being maintained and, when
they are, they're making work for us.  That's certainly a trade-off to
consider, of course, but in this case I'm coming down on the side of
dropping support and our own maintenance costs associated with these
views in favor of asking the tooling community to complete the migration
to the new views which have been around for the past 10 years.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] CATUPDATE confusion?

2015-03-16 Thread Robert Haas
On Fri, Mar 13, 2015 at 9:52 AM, Stephen Frost sfr...@snowman.net wrote:
 * Stephen Frost (sfr...@snowman.net) wrote:
 I should have been more specific.  I don't believe they've moved to
 using pg_roles completely (which was created specifically to address the
 issue that regular users can't select from pg_authid).

 Err, forgot to finish that thought, sorry.  Let's try again:

 I should have been more specific.  I don't believe they've moved to
 using pg_roles completely (which was created specifically to address the
 issue that regular users can't select from pg_authid) simply because
 they haven't had reason to.

That's another way of saying removing pg_user would be creating extra
work for tool authors that otherwise wouldn't need to be done.

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


Re: [HACKERS] CATUPDATE confusion?

2015-03-16 Thread Adam Brightwell
All,

Sure, if we never deprecate anything then tool authors would never need
 to change their existing code.  I don't think that's actually a viable
 solution though; the reason we're discussing the removal of these
 particular views is that they aren't really being maintained and, when
 they are, they're making work for us.  That's certainly a trade-off to
 consider, of course, but in this case I'm coming down on the side of
 dropping support and our own maintenance costs associated with these
 views in favor of asking the tooling community to complete the migration
 to the new views which have been around for the past 10 years.


Perhaps this is naive or has been attempted in the past without success,
but would it be possible to maintain a list of deprecated features?  I
noticed the following wiki page, (though it hasn't been updated recently)
that I think could be used for this purpose.

https://wiki.postgresql.org/wiki/Deprecated_Features

Using this page as a model, having an official deprecation list that does
the following might be very useful:

* Lists feature that is deprecated.
* Reason it was deprecated.
* What to use instead, perhaps with example.
* Version the feature will be removed.

Or perhaps such a list could be included as part of the official
documentation?  In either case, if it is well known that such a list is
available/exists then tool developers, etc. should have adequate time,
opportunity and information to make the appropriate changes to their
products with a minimal impact.

Thoughts?

-Adam

-- 
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com


Re: [HACKERS] CATUPDATE confusion?

2015-03-13 Thread Robert Haas
On Thu, Mar 12, 2015 at 10:36 PM, Stephen Frost sfr...@snowman.net wrote:
 As near as I can tell, pgAdmin3 does still use pg_user (though I don't
 think it uses pg_group or pg_shadow except when connected to an ancient
 server) in some cases.  Where it is used, based on my quick review at
 least, it looks like it'd be pretty easy to fix.

 The rolcatupdate usage appears to all be associated with pg_authid
 though, and the changes required to remove the places where it shows up
 doesn't look particularly bad either.  There are no references to
 usecatupdate.  Where there are references to 'use*', they'd have to also
 be updated with the change to pg_user, naturally.

 Looking at phppgadmin, there are quite a few more uses of pg_user there,
 along with references to pg_group and even pg_shadow (for 8.0 and
 earlier backends).  Amusingly, the only place 'catupdate' is referenced
 there is in the Polish language file.  Updating phppgadmin to not
 reference pg_user or the other views looks like it'd be a bit more work,
 but not a huge effort either.

 Basically, in my view at least, these programs are likely to continue to
 use these backwards compatibility views until we either formally
 deprecate them or (more likely) until we actually remove them and things
 break.  As such, I'm not sure if this information really helps us make a
 decision here.

After poking at this a bit, I am guessing the reason they still use
pg_user is that regular users don't have permission to access
pg_authid directly.  We don't want to make it impossible for pgAdmin
to work for non-superusers.

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


Re: [HACKERS] CATUPDATE confusion?

2015-03-13 Thread Stephen Frost
Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
 On Thu, Mar 12, 2015 at 10:36 PM, Stephen Frost sfr...@snowman.net wrote:
  Basically, in my view at least, these programs are likely to continue to
  use these backwards compatibility views until we either formally
  deprecate them or (more likely) until we actually remove them and things
  break.  As such, I'm not sure if this information really helps us make a
  decision here.
 
 After poking at this a bit, I am guessing the reason they still use
 pg_user is that regular users don't have permission to access
 pg_authid directly.  We don't want to make it impossible for pgAdmin
 to work for non-superusers.

I should have been more specific.  I don't believe they've moved to
using pg_roles completely (which was created specifically to address the
issue that regular users can't select from pg_authid).  Both of the
tools being discussed use pg_roles also (and, in fact, I believe they
have to for certain fields which pg_user doesn't include..
rolcreaterole being a pretty big one, but also rolconnlimit and
rolinherit, which wouldn't make sense in pg_user anyway..).  I agree
that they can't simply move to pg_authid today since only superusers
have access to that table today.  Of course, with column-level
privileges, we could potentially change that too..

In any case, looking at this again, it seems clear that we've not been
keeping pg_user up to date and no one seems to care.  I don't think it
makes sense to go back and try to add useconnlimit, usecanlogin,
usecreateuser for 9.5 when pg_user was only ever intended for
backwards compatibility and clearly hasn't been getting the love and
attention it would deserve if it was really useful.

As such, I'm coming down on the side of simply removing pg_user,
pg_shadow, and pg_group for 9.5.  Having a half-maintained mish-mash of
things from pg_authid making their way into pg_user/pg_shadow (which
suffers all the same problems of missing important fields) isn't doing
anyone any favors, and pg_group is an inefficient way of getting at the
information that's in pg_auth_members which implies that groups are
somehow different from roles, which hasn't been the case in 10 years.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] CATUPDATE confusion?

2015-03-13 Thread Stephen Frost
* Stephen Frost (sfr...@snowman.net) wrote:
 I should have been more specific.  I don't believe they've moved to
 using pg_roles completely (which was created specifically to address the
 issue that regular users can't select from pg_authid).  

Err, forgot to finish that thought, sorry.  Let's try again:

I should have been more specific.  I don't believe they've moved to
using pg_roles completely (which was created specifically to address the
issue that regular users can't select from pg_authid) simply because
they haven't had reason to.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] CATUPDATE confusion?

2015-03-10 Thread Robert Haas
On Sat, Mar 7, 2015 at 6:40 PM, Adam Brightwell
adam.brightw...@crunchydatasolutions.com wrote:
 pg_shadow, pg_user and pg_group were added when role support was added,
 specifically for backwards compatibility.  I don't believe there was
 ever discussion about keeping them because filtering pg_roles based on
 rolcanlogin was too onerous.  That said, we already decided recently
 that we wanted to keep them updated to match the actual attributes
 available (note that the replication role attribute modified those
 views) and I think that makes sense on the removal side as well as the
 new-attribute side.

 I continue to feel that we really should officially deprecate those
 views as I don't think they're actually all that useful any more and
 maintaining them ends up bringing up all these questions, discussion,
 and ends up being largely busy-work if no one really uses them.

 Deprecation would certainly seem like an appropriate path for 'usecatupd'
 (and the mentioned views).  Though, is there a 'formal' deprecation
 policy/process that the community follows?  I certainly understand and
 support giving client/dependent tools the time and opportunity to update
 accordingly.  Therefore, I think having them read from 'rolsuper' for the
 time being would be ok, provided that they are actually removed at the next
 possible opportunity.  Otherwise, I'd probably lean towards just removing
 them now and getting it over with.

Unless some popular tool like pgAdmin is using these views, I'd say we
should just nuke them outright.  If it breaks somebody's installation,
then they can always recreate the view on their particular system.
That seems like an adequate workaround to me.

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


Re: [HACKERS] CATUPDATE confusion?

2015-03-07 Thread Stephen Frost
* Peter Eisentraut (pete...@gmx.net) wrote:
 On 3/7/15 12:31 AM, Tom Lane wrote:
  Peter Eisentraut pete...@gmx.net writes:
  On 12/29/14 7:16 PM, Adam Brightwell wrote:
  Given this discussion, I have attached a patch that removes CATUPDATE
  for review/discussion.
  
  committed this version
  
  Hmm .. I'm not sure that summarily removing usecatupd from those three
  system views was well thought out.  pg_shadow, especially, has no reason
  to live at all except for backwards compatibility, and clients might well
  expect that column to still be there.  I wonder if we'd not be better off
  to keep the column in the views but have it read from rolsuper.
 
 I doubt anyone is reading the column.  And if they are, they should stop.

Certainly pgAdmin and similar tools are.  From that standpoint though, I
agree that they should be modified to no longer read it, as the whole
point of those kinds of tools is to allow users to modify those
attributes and having CATUPDATE in the view would surely be confusing as
the user wouldn't be able to modify it.

 pg_shadow and pg_user have been kept around because it is plausible that
 a lot of tools want to have a list of users, and requiring all of them
 to change to pg_authid at once was deemed too onerous at the time.  I
 don't think this requires us to keep all the details the same forever.

pg_shadow, pg_user and pg_group were added when role support was added,
specifically for backwards compatibility.  I don't believe there was
ever discussion about keeping them because filtering pg_roles based on
rolcanlogin was too onerous.  That said, we already decided recently
that we wanted to keep them updated to match the actual attributes
available (note that the replication role attribute modified those
views) and I think that makes sense on the removal side as well as the
new-attribute side.

I continue to feel that we really should officially deprecate those
views as I don't think they're actually all that useful any more and
maintaining them ends up bringing up all these questions, discussion,
and ends up being largely busy-work if no one really uses them.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] CATUPDATE confusion?

2015-03-07 Thread Peter Eisentraut
On 3/7/15 12:31 AM, Tom Lane wrote:
 Peter Eisentraut pete...@gmx.net writes:
 On 12/29/14 7:16 PM, Adam Brightwell wrote:
 Given this discussion, I have attached a patch that removes CATUPDATE
 for review/discussion.
 
 committed this version
 
 Hmm .. I'm not sure that summarily removing usecatupd from those three
 system views was well thought out.  pg_shadow, especially, has no reason
 to live at all except for backwards compatibility, and clients might well
 expect that column to still be there.  I wonder if we'd not be better off
 to keep the column in the views but have it read from rolsuper.

I doubt anyone is reading the column.  And if they are, they should stop.

pg_shadow and pg_user have been kept around because it is plausible that
a lot of tools want to have a list of users, and requiring all of them
to change to pg_authid at once was deemed too onerous at the time.  I
don't think this requires us to keep all the details the same forever.



-- 
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] CATUPDATE confusion?

2015-03-07 Thread Adam Brightwell
All,


 pg_shadow, pg_user and pg_group were added when role support was added,
 specifically for backwards compatibility.  I don't believe there was
 ever discussion about keeping them because filtering pg_roles based on
 rolcanlogin was too onerous.  That said, we already decided recently
 that we wanted to keep them updated to match the actual attributes
 available (note that the replication role attribute modified those
 views) and I think that makes sense on the removal side as well as the
 new-attribute side.

 I continue to feel that we really should officially deprecate those
 views as I don't think they're actually all that useful any more and
 maintaining them ends up bringing up all these questions, discussion,
 and ends up being largely busy-work if no one really uses them.


Deprecation would certainly seem like an appropriate path for 'usecatupd'
(and the mentioned views).  Though, is there a 'formal' deprecation
policy/process that the community follows?  I certainly understand and
support giving client/dependent tools the time and opportunity to update
accordingly.  Therefore, I think having them read from 'rolsuper' for the
time being would be ok, provided that they are actually removed at the next
possible opportunity.  Otherwise, I'd probably lean towards just removing
them now and getting it over with.

-Adam

-- 
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com


Re: [HACKERS] CATUPDATE confusion?

2015-03-06 Thread Adam Brightwell
All,

Thanks for all the feedback and review.

 I think I vote for (1).  I do not like (2) because of the argument I made
  upthread that write access on system catalogs is far more dangerous than
  a naive DBA might think.  (0) has some attraction but really CATUPDATE
  is one more concept than we need.

 I agree with #1 on this.


#1 makes sense to me as well.  The current version of the patch takes this
approach.  Also, I have verified against 'master' as of c6ee39b.

Thanks,
Adam

-- 
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com


Re: [HACKERS] CATUPDATE confusion?

2015-03-06 Thread Peter Eisentraut
On 12/29/14 7:16 PM, Adam Brightwell wrote:
 Given this discussion, I have attached a patch that removes CATUPDATE
 for review/discussion.

committed this version



-- 
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] CATUPDATE confusion?

2015-03-06 Thread Tom Lane
Peter Eisentraut pete...@gmx.net writes:
 On 12/29/14 7:16 PM, Adam Brightwell wrote:
 Given this discussion, I have attached a patch that removes CATUPDATE
 for review/discussion.

 committed this version

Hmm .. I'm not sure that summarily removing usecatupd from those three
system views was well thought out.  pg_shadow, especially, has no reason
to live at all except for backwards compatibility, and clients might well
expect that column to still be there.  I wonder if we'd not be better off
to keep the column in the views but have it read from rolsuper.

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] CATUPDATE confusion?

2015-03-05 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Peter Eisentraut pete...@gmx.net writes:
  Any other opinions?
  The options are:
  0) Leave as is.
  1) Remove catupdate and replace with superuser checks.
  2) Remove catupdate and rely on regular table permissions on catalogs.
 
 I think I vote for (1).  I do not like (2) because of the argument I made
 upthread that write access on system catalogs is far more dangerous than
 a naive DBA might think.  (0) has some attraction but really CATUPDATE
 is one more concept than we need.

I agree with #1 on this.

 On the other hand, if Stephen is going to push forward with his plan to
 subdivide superuserness, we might have the equivalent of CATUPDATE right
 back again.  (But at least it would be properly documented and
 supported...)

The subdivision of superuserness is intended only for operations which
can't be used to directly give the user superuser access back and
therefore I don't think we'd ever put back CATUPDATE or an equivilant.

I'd much rather reduce the need for direct catalog modifications by
adding additional syntax for those operations which can't be done
without modifying the catalog directly and, where it makes sense to, add
a way to control access to those operations.

For example, changing a database to not accept connections seems like
something the database owner should be allowed to do.  Perhaps that'd be
interesting to allow users other than the owner to do, perhaps it
doesn't, but that would be an independent question to address.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] CATUPDATE confusion?

2015-03-04 Thread Tom Lane
Peter Eisentraut pete...@gmx.net writes:
 Any other opinions?
 The options are:
 0) Leave as is.
 1) Remove catupdate and replace with superuser checks.
 2) Remove catupdate and rely on regular table permissions on catalogs.

I think I vote for (1).  I do not like (2) because of the argument I made
upthread that write access on system catalogs is far more dangerous than
a naive DBA might think.  (0) has some attraction but really CATUPDATE
is one more concept than we need.

On the other hand, if Stephen is going to push forward with his plan to
subdivide superuserness, we might have the equivalent of CATUPDATE right
back again.  (But at least it would be properly documented and
supported...)

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] CATUPDATE confusion?

2015-03-04 Thread Peter Eisentraut
Any other opinions?

The options are:

0) Leave as is.

1) Remove catupdate and replace with superuser checks.

2) Remove catupdate and rely on regular table permissions on catalogs.



-- 
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] CATUPDATE confusion?

2015-03-03 Thread Peter Eisentraut
On 2/28/15 6:32 PM, Stephen Frost wrote:
 This isn't really /etc/shadow though, this is more like direct access to
 the filesystem through the device node- and you'll note that Linux
 certainly has got an independent set of permissions for that called the
 capabilities system.  That's because messing with those pieces can crash
 the kernel.  You're not going to crash the kernel if you goof up
 /etc/shadow.

I think this characterization is incorrect.  The Linux capability system
does not exist because the actions are scary or can crash the kernel.
Capabilities exist because they are not attached to file system objects
and can therefore not be represented using the usual permission system.

Note that one can write directly to raw devices or the kernel memory
through various /dev and /proc files.  No capability protects against
that.  It's only the file permissions, possibly in combination with
mount options.



-- 
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] CATUPDATE confusion?

2015-02-28 Thread Stephen Frost
Peter,

* Peter Eisentraut (pete...@gmx.net) wrote:
 On 2/25/15 10:05 PM, Stephen Frost wrote:
  Agreed, but I'd also like to get rid of any reason, beyond emergency
  cases, for people to modify the catalog directly.  There's a few places
  which we aren't yet doing that, but I'd rather fix those cases than
  encourage people to give out rights to modify them and end up making
  things like:
  
  UPDATE pg_database SET datallowconn = false where datname = 'xyz';
  
  an accepted interface.
 
 I'm not sure those things are related.

Eh, I feel they are, but either way.

 Getting rid of the *reasons* for updating catalogs directly might be
 worthwhile, but I don't see why we need to install (or maintain) a
 special invisible permission trap for it.  We have a permission system,
 and it works pretty well.

We have this invisible permission trap known as checking if you're a
superuser all over the place.  I'm not against revisiting those cases
and considering using the GRANT permission system to manage access, but
that's certainly a larger project to work on.  What I'm referring to
here are all the functions that check if you're a superuser, instead of
just revoking EXECUTE from public and letting the user manage the
permission.

 The Unix kernels don't have special traps for someone to modify
 /etc/shadow or similar directly.  That file has appropriate permissions,
 and that's it.  I think that works pretty well.

This isn't really /etc/shadow though, this is more like direct access to
the filesystem through the device node- and you'll note that Linux
certainly has got an independent set of permissions for that called the
capabilities system.  That's because messing with those pieces can crash
the kernel.  You're not going to crash the kernel if you goof up
/etc/shadow.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] CATUPDATE confusion?

2015-02-28 Thread Peter Eisentraut
On 2/25/15 10:05 PM, Stephen Frost wrote:
 * Peter Eisentraut (pete...@gmx.net) wrote:
 On 2/25/15 3:39 PM, Stephen Frost wrote:
 I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.

 Err, wouldn't this make it possible to grant normal users the ability to
 modify system catalogs?  I realize that they wouldn't have that
 initially, but I'm not sure we want the superuser to be able to grant
 that to non-superusers..

 Why not?  I thought we are trying to get rid of special superuser behavior.
 
 Agreed, but I'd also like to get rid of any reason, beyond emergency
 cases, for people to modify the catalog directly.  There's a few places
 which we aren't yet doing that, but I'd rather fix those cases than
 encourage people to give out rights to modify them and end up making
 things like:
 
 UPDATE pg_database SET datallowconn = false where datname = 'xyz';
 
 an accepted interface.

I'm not sure those things are related.

Getting rid of the *reasons* for updating catalogs directly might be
worthwhile, but I don't see why we need to install (or maintain) a
special invisible permission trap for it.  We have a permission system,
and it works pretty well.

The Unix kernels don't have special traps for someone to modify
/etc/shadow or similar directly.  That file has appropriate permissions,
and that's it.  I think that works pretty well.



-- 
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] CATUPDATE confusion?

2015-02-25 Thread Stephen Frost
* Peter Eisentraut (pete...@gmx.net) wrote:
 On 12/29/14 7:16 PM, Adam Brightwell wrote:
  Given this discussion, I have attached a patch that removes CATUPDATE
  for review/discussion.
  
  One of the interesting behaviors (or perhaps not) is how
  'pg_class_aclmask' handles an invalid role id when checking permissions
  against 'rolsuper' instead of 'rolcatupdate'.
 
 I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.

Err, wouldn't this make it possible to grant normal users the ability to
modify system catalogs?  I realize that they wouldn't have that
initially, but I'm not sure we want the superuser to be able to grant
that to non-superusers..

I'm fine with making it if system table and not superuser, error.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] CATUPDATE confusion?

2015-02-25 Thread Stephen Frost
* Peter Eisentraut (pete...@gmx.net) wrote:
 On 2/25/15 3:39 PM, Stephen Frost wrote:
  I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.
  
  Err, wouldn't this make it possible to grant normal users the ability to
  modify system catalogs?  I realize that they wouldn't have that
  initially, but I'm not sure we want the superuser to be able to grant
  that to non-superusers..
 
 Why not?  I thought we are trying to get rid of special superuser behavior.

Agreed, but I'd also like to get rid of any reason, beyond emergency
cases, for people to modify the catalog directly.  There's a few places
which we aren't yet doing that, but I'd rather fix those cases than
encourage people to give out rights to modify them and end up making
things like:

UPDATE pg_database SET datallowconn = false where datname = 'xyz';

an accepted interface.

 After all, superusers can also make the other user a superuser to bypass
 this issue.

Sure, but that gives us the option to write off whatever happens next as
not our fault.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] CATUPDATE confusion?

2015-02-25 Thread Peter Eisentraut
On 2/25/15 3:39 PM, Stephen Frost wrote:
 I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.
 
 Err, wouldn't this make it possible to grant normal users the ability to
 modify system catalogs?  I realize that they wouldn't have that
 initially, but I'm not sure we want the superuser to be able to grant
 that to non-superusers..

Why not?  I thought we are trying to get rid of special superuser behavior.

After all, superusers can also make the other user a superuser to bypass
this issue.



-- 
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] CATUPDATE confusion?

2015-02-24 Thread Michael Paquier
On Fri, Feb 20, 2015 at 8:08 AM, Adam Brightwell 
adam.brightw...@crunchydatasolutions.com wrote:

 Thanks for the review and feedback.

  One of the interesting behaviors (or perhaps not) is how
  'pg_class_aclmask' handles an invalid role id when checking permissions
  against 'rolsuper' instead of 'rolcatupdate'.

 I'd get rid of that whole check, not just replace rolcatupdate by
 rolsuper.


 Ah yes, that's a good point.  I have updated and attached a new version of
 the patch.


I just had a look at this patch and it works as intended, simply removing
catupdate and all its code paths. As everybody on this thread seems to
agree about the removal of rolcatupdate, I think that we could let a
committer have a look at it.
Thoughts?
-- 
Michael


Re: [HACKERS] CATUPDATE confusion?

2015-02-19 Thread Peter Eisentraut
On 12/29/14 7:16 PM, Adam Brightwell wrote:
 Given this discussion, I have attached a patch that removes CATUPDATE
 for review/discussion.
 
 One of the interesting behaviors (or perhaps not) is how
 'pg_class_aclmask' handles an invalid role id when checking permissions
 against 'rolsuper' instead of 'rolcatupdate'.

I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.


-- 
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] CATUPDATE confusion?

2015-02-19 Thread Adam Brightwell
Peter,

Thanks for the review and feedback.

 One of the interesting behaviors (or perhaps not) is how
  'pg_class_aclmask' handles an invalid role id when checking permissions
  against 'rolsuper' instead of 'rolcatupdate'.

 I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.


Ah yes, that's a good point.  I have updated and attached a new version of
the patch.

Thanks,
Adam

-- 
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com


remove-catupdate-v2.patch
Description: Binary data

-- 
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] CATUPDATE confusion?

2015-01-19 Thread Adam Brightwell

 * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote:
  Given this discussion, I have attached a patch that removes CATUPDATE for
  review/discussion.

 Thanks!


I've added this patch (up-thread) to the next CommitFest (2015-02).

-Adam

-- 
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com


Re: [HACKERS] CATUPDATE confusion?

2014-12-29 Thread Adam Brightwell
All,

On Sat, Dec 27, 2014 at 6:31 PM, Noah Misch n...@leadboat.com wrote:

 On Sat, Dec 27, 2014 at 06:26:02PM -0500, Tom Lane wrote:
  Stephen Frost sfr...@snowman.net writes:
   * Tom Lane (t...@sss.pgh.pa.us) wrote:
   Plan C (remove CATUPDATE altogether) also has some merit.  But adding
 a
   superuser override there would be entirely pointless.
 
   This is be my recommendation.  I do not see the point of carrying the
   option around just to confuse new users of pg_authid and reviewers of
   the code.
 
  Yeah ... if no one's found it interesting in the 20 years since the
  code left Berkeley, it's unlikely that interest will emerge in the
  future.

 No objection here.


Given this discussion, I have attached a patch that removes CATUPDATE for
review/discussion.

One of the interesting behaviors (or perhaps not) is how 'pg_class_aclmask'
handles an invalid role id when checking permissions against 'rolsuper'
instead of 'rolcatupdate'.  This is demonstrated by the
'has_table_privilege' regression test expected results.  In summary,
'has_rolcatupdate' raises an error when an invalid OID is provided,
however, as documented in the source 'superuser_arg' does not, simply
returning false.  Therefore, when checking for superuser-ness of an
non-existent role, the result will be false and not an error.  Perhaps this
is OK, but my concern would be on the expected behavior around items like
'has_table_privilege'.  My natural thought would be that we would want to
preserve that specific functionality, though short of adding a
'has_rolsuper' function that will raise an appropriate error, I'm uncertain
of an approach.  Thoughts?

Thanks,
Adam

-- 
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
new file mode 100644
index 9ceb96b..635032d
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
***
*** 1416,1430 
   /row
  
   row
-   entrystructfieldrolcatupdate/structfield/entry
-   entrytypebool/type/entry
-   entry
-Role can update system catalogs directly.  (Even a superuser cannot do
-this unless this column is true)
-   /entry
-  /row
- 
-  row
entrystructfieldrolcanlogin/structfield/entry
entrytypebool/type/entry
entry
--- 1416,1421 
*** SELECT * FROM pg_locks pl LEFT JOIN pg_p
*** 8479,8494 
   /row
  
   row
-   entrystructfieldrolcatupdate/structfield/entry
-   entrytypebool/type/entry
-   entry/entry
-   entry
-Role can update system catalogs directly.  (Even a superuser cannot do
-this unless this column is true)
-   /entry
-  /row
- 
-  row
entrystructfieldrolcanlogin/structfield/entry
entrytypebool/type/entry
entry/entry
--- 8470,8475 
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
new file mode 100644
index d30612c..18623ef
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*** aclcheck_error_type(AclResult aclerr, Oi
*** 3423,3448 
  }
  
  
- /* Check if given user has rolcatupdate privilege according to pg_authid */
- static bool
- has_rolcatupdate(Oid roleid)
- {
- 	bool		rolcatupdate;
- 	HeapTuple	tuple;
- 
- 	tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
- 	if (!HeapTupleIsValid(tuple))
- 		ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
-  errmsg(role with OID %u does not exist, roleid)));
- 
- 	rolcatupdate = ((Form_pg_authid) GETSTRUCT(tuple))-rolcatupdate;
- 
- 	ReleaseSysCache(tuple);
- 
- 	return rolcatupdate;
- }
- 
  /*
   * Relay for the various pg_*_mask routines depending on object kind
   */
--- 3423,3428 
*** pg_class_aclmask(Oid table_oid, Oid role
*** 3620,3627 
  
  	/*
  	 * Deny anyone permission to update a system catalog unless
! 	 * pg_authid.rolcatupdate is set.   (This is to let superusers protect
! 	 * themselves from themselves.)  Also allow it if allowSystemTableMods.
  	 *
  	 * As of 7.4 we have some updatable system views; those shouldn't be
  	 * protected in this way.  Assume the view rules can take care of
--- 3600,3606 
  
  	/*
  	 * Deny anyone permission to update a system catalog unless
! 	 * pg_authid.rolsuper is set.  Also allow it if allowSystemTableMods.
  	 *
  	 * As of 7.4 we have some updatable system views; those shouldn't be
  	 * protected in this way.  Assume the view rules can take care of
*** pg_class_aclmask(Oid table_oid, Oid role
*** 3630,3636 
  	if ((mask  (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE)) 
  		IsSystemClass(table_oid, classForm) 
  		classForm-relkind != RELKIND_VIEW 
! 		!has_rolcatupdate(roleid) 
  		!allowSystemTableMods)
  	{
  #ifdef ACLDEBUG
--- 3609,3615 
  	if ((mask  (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE)) 
  		

Re: [HACKERS] CATUPDATE confusion?

2014-12-29 Thread Stephen Frost
Adam,

* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote:
 Given this discussion, I have attached a patch that removes CATUPDATE for
 review/discussion.

Thanks!

 One of the interesting behaviors (or perhaps not) is how 'pg_class_aclmask'
 handles an invalid role id when checking permissions against 'rolsuper'
 instead of 'rolcatupdate'.  This is demonstrated by the
 'has_table_privilege' regression test expected results.  In summary,
 'has_rolcatupdate' raises an error when an invalid OID is provided,
 however, as documented in the source 'superuser_arg' does not, simply
 returning false.  Therefore, when checking for superuser-ness of an
 non-existent role, the result will be false and not an error.  Perhaps this
 is OK, but my concern would be on the expected behavior around items like
 'has_table_privilege'.  My natural thought would be that we would want to
 preserve that specific functionality, though short of adding a
 'has_rolsuper' function that will raise an appropriate error, I'm uncertain
 of an approach.  Thoughts?

This role oid check is only getting called in this case because it's
pg_authid which is getting checked (because it's a system catalog table)
and it's then falling into this one case.  I don't think we need to
preserve that.

If you do the same query with that bogus OID but a normal table, you get
the same 'f' result that the regression test gives with this change.
We're not back-patching this and I seriously doubt anyone is relying on
this special response for system catalog tables.

So, unless anyone wants to object, I'll continue to look over this patch
for eventual application against master.  If there are objections, it'd
be great if they could be voiced sooner rather than later.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] CATUPDATE confusion?

2014-12-27 Thread Noah Misch
On Mon, Nov 24, 2014 at 04:28:46PM -0500, Adam Brightwell wrote:
 So, where I find this confusing is that the documentation supports this
 functionality and the check keeps superuser separate from CATUPDATE...
 however... comments and implementation in user.c state/do the opposite and
 couple them together.
 
 Documentation:
 http://www.postgresql.org/docs/9.4/static/catalog-pg-authid.html - Role
 can update system catalogs directly. (Even a superuser cannot do this
 unless this column is true)
 
 src/backend/commands/user.c
 
 /* superuser gets catupdate right by default */
 new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper);
 
 and...
 
 /*
  * issuper/createrole/catupdate/etc
  *
  * XXX It's rather unclear how to handle catupdate.  It's probably best to
  * keep it equal to the superuser status, otherwise you could end up with
  * a situation where no existing superuser can alter the catalogs,
  * including pg_authid!
  */
 if (issuper = 0)
 {
   new_record[Anum_pg_authid_rolsuper - 1] = BoolGetDatum(issuper  0);
   new_record_repl[Anum_pg_authid_rolsuper - 1] = true;
 
   new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper  0);
   new_record_repl[Anum_pg_authid_rolcatupdate - 1] = true;
 }

That documentation is correct as far it goes.  It does neglect to mention
that, as you have discovered, any CREATE ROLE or ALTER ROLE [NO]SUPERUSER will
change rolcatupdate to match.

 Perhaps this is not an issue but it seemed odd to me, especially after
 giving Peter's comment more thought.  So, I suppose the question is whether
 or not a superuser check should be added to 'has_rolcatupdate' or not?  I

I would not.  PostgreSQL has had this feature since day one (original name
usecatupd).  It has two use cases, (1) giving effect to non-superuser
catalog grants and (2) preventing a narrow class of superuser mistakes.  We
wouldn't add such a thing today, but one can safely ignore its existence.
Making has_rolcatupdate() approve superusers unconditionally would exclude use
case (2).  Neither use case is valuable, but if I had to pick, (2) is more
valuable than (1).

 believe I understand the reasoning for coupling the two at role
 creation/alter time, however, is there really a case where a superuser
 wouldn't be allowed to bypass this check?  Based on the comments, there
 seems like there is potentially enough concern to allow it.  And if it is
 allowed, couldn't CATUPDATE then be treated like every other attribute and
 the coupling with superuser removed?  Thoughts?

A superuser always has _some_ way to bypass the check, if nothing else by
loading a C-language function to update pg_authid.  The user.c code you quoted
ensures that, if the DBA manages to remove rolcatupdate from every user, a
simple CREATE ROLE or ALTER ROLE can fix things.


-- 
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] CATUPDATE confusion?

2014-12-27 Thread Stephen Frost
* Noah Misch (n...@leadboat.com) wrote:
 On Mon, Nov 24, 2014 at 04:28:46PM -0500, Adam Brightwell wrote:
  Perhaps this is not an issue but it seemed odd to me, especially after
  giving Peter's comment more thought.  So, I suppose the question is whether
  or not a superuser check should be added to 'has_rolcatupdate' or not?  I
 
 I would not.  PostgreSQL has had this feature since day one (original name
 usecatupd).  It has two use cases, (1) giving effect to non-superuser
 catalog grants and (2) preventing a narrow class of superuser mistakes.  We
 wouldn't add such a thing today, but one can safely ignore its existence.
 Making has_rolcatupdate() approve superusers unconditionally would exclude use
 case (2).  Neither use case is valuable, but if I had to pick, (2) is more
 valuable than (1).

What's confusing about this is that use-case (2) appears to have been
the intent of the option, but because it's tied directly to superuser
and isn't an independently controllable option, the only way change it
is to do an 'update pg_authid ...'.  Even when set that way, it isn't
visible that it's been set through \du, you have to query the catalog
directly.

I wonder if the option had originally been intended to be independent
and *not* given to superusers by default, but because of the concern
about dealing with corrupt systems, etc, it was never actually done.
I'm fine with the line of thinking that says we can't deny superusers
this power because of corruption/debugging concerns, but if that's the
case, then there is no use-case (2) and we should just remove the option
entirely.

I don't buy into use-case (1) above being at all reasonable.  With it,
one can trivially make themselves a superuser, and in many ways the
catupdate capability is *more* dangerous than superuser- if you have
catupdate but not superuser, you'd be tempted to hack at the catalog to
make the changes you want instead of using the regular ALTER TABLE, etc,
commands.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] CATUPDATE confusion?

2014-12-27 Thread Tom Lane
Stephen Frost sfr...@snowman.net writes:
 * Noah Misch (n...@leadboat.com) wrote:
 On Mon, Nov 24, 2014 at 04:28:46PM -0500, Adam Brightwell wrote:
 Perhaps this is not an issue but it seemed odd to me, especially after
 giving Peter's comment more thought.  So, I suppose the question is whether
 or not a superuser check should be added to 'has_rolcatupdate' or not?  I

 I would not.  PostgreSQL has had this feature since day one (original name
 usecatupd).  It has two use cases, (1) giving effect to non-superuser
 catalog grants and (2) preventing a narrow class of superuser mistakes.  We
 wouldn't add such a thing today, but one can safely ignore its existence.
 Making has_rolcatupdate() approve superusers unconditionally would exclude 
 use
 case (2).  Neither use case is valuable, but if I had to pick, (2) is more
 valuable than (1).

 What's confusing about this is that use-case (2) appears to have been
 the intent of the option,

Yes.  Noah's claim that anybody intended (1) seems to be mere historical
revisionism, especially since as you note CATUPDATE could be trivially
parlayed into superuser if someone were to grant it to a non-superuser.

 but because it's tied directly to superuser
 and isn't an independently controllable option, the only way change it
 is to do an 'update pg_authid ...'.  Even when set that way, it isn't
 visible that it's been set through \du, you have to query the catalog
 directly.

This is not hard to understand either: the option dates from long before
there was any concerted effort to invent bespoke SQL commands for every
interesting catalog manipulation.  If CATUPDATE had any significant use,
we'd have extended ALTER USER (and \du, and pg_dump ...) at some point
to make it more easily controllable.  But since it's of such marginal
usefulness, nobody ever bothered; and I doubt we should bother now.

In short, I agree with Noah's conclusion (do nothing) though not his
reasoning.

Plan C (remove CATUPDATE altogether) also has some merit.  But adding a
superuser override there would be entirely pointless.

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] CATUPDATE confusion?

2014-12-27 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Plan C (remove CATUPDATE altogether) also has some merit.  But adding a
 superuser override there would be entirely pointless.

This is be my recommendation.  I do not see the point of carrying the
option around just to confuse new users of pg_authid and reviewers of
the code.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] CATUPDATE confusion?

2014-12-27 Thread Tom Lane
Stephen Frost sfr...@snowman.net writes:
 * Tom Lane (t...@sss.pgh.pa.us) wrote:
 Plan C (remove CATUPDATE altogether) also has some merit.  But adding a
 superuser override there would be entirely pointless.

 This is be my recommendation.  I do not see the point of carrying the
 option around just to confuse new users of pg_authid and reviewers of
 the code.

Yeah ... if no one's found it interesting in the 20 years since the
code left Berkeley, it's unlikely that interest will emerge in the
future.

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] CATUPDATE confusion?

2014-12-27 Thread Noah Misch
On Sat, Dec 27, 2014 at 06:26:02PM -0500, Tom Lane wrote:
 Stephen Frost sfr...@snowman.net writes:
  * Tom Lane (t...@sss.pgh.pa.us) wrote:
  Plan C (remove CATUPDATE altogether) also has some merit.  But adding a
  superuser override there would be entirely pointless.
 
  This is be my recommendation.  I do not see the point of carrying the
  option around just to confuse new users of pg_authid and reviewers of
  the code.
 
 Yeah ... if no one's found it interesting in the 20 years since the
 code left Berkeley, it's unlikely that interest will emerge in the
 future.

No objection here.


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


[HACKERS] CATUPDATE confusion?

2014-11-24 Thread Adam Brightwell
All,

While reviewing the supporting documentation and functions for role
attributes I noticed that there seems to be some confusion (at least for
me) with CATUPDATE.

This was prompted by the following comment from Peter about
'has_rolcatupdate' not having a superuser check.

http://www.postgresql.org/message-id/54590bbf.1080...@gmx.net

So, where I find this confusing is that the documentation supports this
functionality and the check keeps superuser separate from CATUPDATE...
however... comments and implementation in user.c state/do the opposite and
couple them together.

Documentation:
http://www.postgresql.org/docs/9.4/static/catalog-pg-authid.html - Role
can update system catalogs directly. (Even a superuser cannot do this
unless this column is true)

src/backend/commands/user.c

/* superuser gets catupdate right by default */
new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper);

and...

/*
 * issuper/createrole/catupdate/etc
 *
 * XXX It's rather unclear how to handle catupdate.  It's probably best to
 * keep it equal to the superuser status, otherwise you could end up with
 * a situation where no existing superuser can alter the catalogs,
 * including pg_authid!
 */
if (issuper = 0)
{
  new_record[Anum_pg_authid_rolsuper - 1] = BoolGetDatum(issuper  0);
  new_record_repl[Anum_pg_authid_rolsuper - 1] = true;

  new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper  0);
  new_record_repl[Anum_pg_authid_rolcatupdate - 1] = true;
}

Perhaps this is not an issue but it seemed odd to me, especially after
giving Peter's comment more thought.  So, I suppose the question is whether
or not a superuser check should be added to 'has_rolcatupdate' or not?  I
believe I understand the reasoning for coupling the two at role
creation/alter time, however, is there really a case where a superuser
wouldn't be allowed to bypass this check?  Based on the comments, there
seems like there is potentially enough concern to allow it.  And if it is
allowed, couldn't CATUPDATE then be treated like every other attribute and
the coupling with superuser removed?  Thoughts?

Thanks,
Adam

--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com