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>
- <entry><structfield>rolcatupdate</structfield></entry>
- <entry><type>bool</type></entry>
- <entry>
- Role can update system catalogs directly. (Even a superuser cannot do
- this unless this column is true)
- </entry>
- </row>
-
- <row>
<entry><structfield>rolcanlogin</structfield></entry>
<entry><type>bool</type></entry>
<entry>
--- 1416,1421 ----
*************** SELECT * FROM pg_locks pl LEFT JOIN pg_p
*** 8479,8494 ****
</row>
<row>
- <entry><structfield>rolcatupdate</structfield></entry>
- <entry><type>bool</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>
<entry><structfield>rolcanlogin</structfield></entry>
<entry><type>bool</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)) &&
IsSystemClass(table_oid, classForm) &&
classForm->relkind != RELKIND_VIEW &&
! !superuser_arg(roleid) &&
!allowSystemTableMods)
{
#ifdef ACLDEBUG
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
new file mode 100644
index 22b8cee..fc3fa9e
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*************** CREATE VIEW pg_roles AS
*** 13,19 ****
rolinherit,
rolcreaterole,
rolcreatedb,
- rolcatupdate,
rolcanlogin,
rolreplication,
rolconnlimit,
--- 13,18 ----
*************** CREATE VIEW pg_shadow AS
*** 31,37 ****
pg_authid.oid AS usesysid,
rolcreatedb AS usecreatedb,
rolsuper AS usesuper,
- rolcatupdate AS usecatupd,
rolreplication AS userepl,
rolpassword AS passwd,
rolvaliduntil::abstime AS valuntil,
--- 30,35 ----
*************** CREATE VIEW pg_user AS
*** 56,62 ****
usesysid,
usecreatedb,
usesuper,
- usecatupd,
userepl,
'********'::text as passwd,
valuntil,
--- 54,59 ----
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
new file mode 100644
index 1a73fd8..d8b3898
*** a/src/backend/commands/user.c
--- b/src/backend/commands/user.c
*************** CreateRole(CreateRoleStmt *stmt)
*** 368,375 ****
new_record[Anum_pg_authid_rolinherit - 1] = BoolGetDatum(inherit);
new_record[Anum_pg_authid_rolcreaterole - 1] = BoolGetDatum(createrole);
new_record[Anum_pg_authid_rolcreatedb - 1] = BoolGetDatum(createdb);
- /* superuser gets catupdate right by default */
- new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper);
new_record[Anum_pg_authid_rolcanlogin - 1] = BoolGetDatum(canlogin);
new_record[Anum_pg_authid_rolreplication - 1] = BoolGetDatum(isreplication);
new_record[Anum_pg_authid_rolconnlimit - 1] = Int32GetDatum(connlimit);
--- 368,373 ----
*************** AlterRole(AlterRoleStmt *stmt)
*** 734,753 ****
MemSet(new_record_repl, false, sizeof(new_record_repl));
/*
! * 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;
}
if (inherit >= 0)
--- 732,743 ----
MemSet(new_record_repl, false, sizeof(new_record_repl));
/*
! * issuper/createrole/etc
*/
if (issuper >= 0)
{
new_record[Anum_pg_authid_rolsuper - 1] = BoolGetDatum(issuper > 0);
new_record_repl[Anum_pg_authid_rolsuper - 1] = true;
}
if (inherit >= 0)
diff --git a/src/include/catalog/pg_authid.h b/src/include/catalog/pg_authid.h
new file mode 100644
index 3b63d2b..6defde5
*** a/src/include/catalog/pg_authid.h
--- b/src/include/catalog/pg_authid.h
*************** CATALOG(pg_authid,1260) BKI_SHARED_RELAT
*** 49,55 ****
bool rolinherit; /* inherit privileges from other roles? */
bool rolcreaterole; /* allowed to create more roles? */
bool rolcreatedb; /* allowed to create databases? */
- bool rolcatupdate; /* allowed to alter catalogs manually? */
bool rolcanlogin; /* allowed to log in as session user? */
bool rolreplication; /* role used for streaming replication */
bool rolbypassrls; /* allowed to bypass row level security? */
--- 49,54 ----
*************** typedef FormData_pg_authid *Form_pg_auth
*** 74,92 ****
* compiler constants for pg_authid
* ----------------
*/
! #define Natts_pg_authid 12
#define Anum_pg_authid_rolname 1
#define Anum_pg_authid_rolsuper 2
#define Anum_pg_authid_rolinherit 3
#define Anum_pg_authid_rolcreaterole 4
#define Anum_pg_authid_rolcreatedb 5
! #define Anum_pg_authid_rolcatupdate 6
! #define Anum_pg_authid_rolcanlogin 7
! #define Anum_pg_authid_rolreplication 8
! #define Anum_pg_authid_rolbypassrls 9
! #define Anum_pg_authid_rolconnlimit 10
! #define Anum_pg_authid_rolpassword 11
! #define Anum_pg_authid_rolvaliduntil 12
/* ----------------
* initial contents of pg_authid
--- 73,90 ----
* compiler constants for pg_authid
* ----------------
*/
! #define Natts_pg_authid 11
#define Anum_pg_authid_rolname 1
#define Anum_pg_authid_rolsuper 2
#define Anum_pg_authid_rolinherit 3
#define Anum_pg_authid_rolcreaterole 4
#define Anum_pg_authid_rolcreatedb 5
! #define Anum_pg_authid_rolcanlogin 6
! #define Anum_pg_authid_rolreplication 7
! #define Anum_pg_authid_rolbypassrls 8
! #define Anum_pg_authid_rolconnlimit 9
! #define Anum_pg_authid_rolpassword 10
! #define Anum_pg_authid_rolvaliduntil 11
/* ----------------
* initial contents of pg_authid
*************** typedef FormData_pg_authid *Form_pg_auth
*** 95,101 ****
* user choices.
* ----------------
*/
! DATA(insert OID = 10 ( "POSTGRES" t t t t t t t t -1 _null_ _null_));
#define BOOTSTRAP_SUPERUSERID 10
--- 93,99 ----
* user choices.
* ----------------
*/
! DATA(insert OID = 10 ( "POSTGRES" t t t t t t t -1 _null_ _null_));
#define BOOTSTRAP_SUPERUSERID 10
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
new file mode 100644
index 5359dd8..f84e7c7
*** a/src/test/regress/expected/privileges.out
--- b/src/test/regress/expected/privileges.out
*************** ERROR: role "nosuchuser" does not exist
*** 645,651 ****
select has_table_privilege('pg_authid','sel');
ERROR: unrecognized privilege type: "sel"
select has_table_privilege(-999999,'pg_authid','update');
! ERROR: role with OID 4293967297 does not exist
select has_table_privilege(1,'select');
has_table_privilege
---------------------
--- 645,655 ----
select has_table_privilege('pg_authid','sel');
ERROR: unrecognized privilege type: "sel"
select has_table_privilege(-999999,'pg_authid','update');
! has_table_privilege
! ---------------------
! f
! (1 row)
!
select has_table_privilege(1,'select');
has_table_privilege
---------------------
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
new file mode 100644
index 80c3351..c4ecfe6
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** pg_roles| SELECT pg_authid.rolname,
*** 1409,1415 ****
pg_authid.rolinherit,
pg_authid.rolcreaterole,
pg_authid.rolcreatedb,
- pg_authid.rolcatupdate,
pg_authid.rolcanlogin,
pg_authid.rolreplication,
pg_authid.rolconnlimit,
--- 1409,1414 ----
*************** pg_shadow| SELECT pg_authid.rolname AS u
*** 1610,1616 ****
pg_authid.oid AS usesysid,
pg_authid.rolcreatedb AS usecreatedb,
pg_authid.rolsuper AS usesuper,
- pg_authid.rolcatupdate AS usecatupd,
pg_authid.rolreplication AS userepl,
pg_authid.rolpassword AS passwd,
(pg_authid.rolvaliduntil)::abstime AS valuntil,
--- 1609,1614 ----
*************** pg_user| SELECT pg_shadow.usename,
*** 2064,2070 ****
pg_shadow.usesysid,
pg_shadow.usecreatedb,
pg_shadow.usesuper,
- pg_shadow.usecatupd,
pg_shadow.userepl,
'********'::text AS passwd,
pg_shadow.valuntil,
--- 2062,2067 ----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
