Re: [HACKERS] security barrier INSERT
Drew, IMHO this is nonintuitive, the intuitive behavior of a security_barrier view should be to forbid inserting rows that can’t appear in the view. Isn't that what WITH CHECK OPTION is meant to accomplish? -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] superuser() shortcuts
Alvaro, I noticed something strange while perusing this patch, but the issue predates the patch. Some messages say must be superuser or replication role to foo, but our longstanding practice is to say permission denied to foo. Why do we have this inconsistency? Should we remove it? If we do want to keep the old wording this patch should use permission denied to in the places that it touches. If we were to make it consistent and use the old wording, what do you think about providing an errhint as well? Perhaps for example in slotfuncs.c#pg_create_physical_replication_stot: errmsg - permission denied to create physical replication slot errhint - You must be superuser or replication role to use replication slots. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] superuser() shortcuts
I noticed something strange while perusing this patch, but the issue predates the patch. Some messages say must be superuser or replication role to foo, but our longstanding practice is to say permission denied to foo. Why do we have this inconsistency? Should we remove it? If we do want to keep the old wording this patch should use permission denied to in the places that it touches. If we were to make it consistent and use the old wording, what do you think about providing an errhint as well? Perhaps for example in slotfuncs.c#pg_create_physical_replication_stot: errmsg - permission denied to create physical replication slot errhint - You must be superuser or replication role to use replication slots. As I started looking at this, there are multiple other places where these types of error messages occur (opclasscmds.c, user.c, postinit.c, miscinit.c are just a few), not just around the changes in this patch. If we change them in one place, wouldn't it be best to change them in the rest? If that is the case, I'm afraid that might distract from the purpose of this patch. Perhaps, if we want to change them, then that should be submitted as a separate patch? -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [TODO] Track number of files ready to be archived in pg_stat_archiver
Julien, Actually, I used the same loop as the archiver one (see backend/postmaster/pgarch.c, function pgarch_readyXlog) to get the exact same number of files. Ah, I see. If we change it in this patch, it would be better to change it everywhere. What do you think ? Hmm... I'd have to defer to the better judgement of a committer on that one. Though, I would think that the general desire would be to keep the patch relevant ONLY to the necessary changes. I would not qualify making those types of changes as relevant, IMHO. I do think this is potential for cleanup, however, I would suspect that would be best done in a separate patch. But again, I'd defer to a committer whether such changes are even necessary/acceptable. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Directory/File Access Permissions for COPY and Generic File Access Functions
Peter,
You patch is missing the files src/include/catalog/pg_diralias.h,
src/include/commands/diralias.h, and src/backend/commands/diralias.c.
(Hint: git add -N)
Yikes, sorry about that, not sure how that happened. Attached is an
updated patch.
-Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index b257b02..8cdc5cb 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -41,7 +41,7 @@ POSTGRES_BKI_SRCS = $(addprefix $(top_srcdir)/src/include/catalog/,\
pg_foreign_data_wrapper.h pg_foreign_server.h pg_user_mapping.h \
pg_foreign_table.h pg_rowsecurity.h \
pg_default_acl.h pg_seclabel.h pg_shseclabel.h pg_collation.h pg_range.h \
- toasting.h indexing.h \
+ pg_diralias.h toasting.h indexing.h \
)
# location of Catalog.pm
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index d30612c..3717bf5 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -30,6 +30,7 @@
#include catalog/pg_collation.h
#include catalog/pg_conversion.h
#include catalog/pg_database.h
+#include catalog/pg_diralias.h
#include catalog/pg_default_acl.h
#include catalog/pg_event_trigger.h
#include catalog/pg_extension.h
@@ -48,6 +49,7 @@
#include catalog/pg_ts_config.h
#include catalog/pg_ts_dict.h
#include commands/dbcommands.h
+#include commands/diralias.h
#include commands/proclang.h
#include commands/tablespace.h
#include foreign/foreign.h
@@ -3183,6 +3185,190 @@ ExecGrant_Type(InternalGrant *istmt)
heap_close(relation, RowExclusiveLock);
}
+/*
+ * ExecuteGrantDirAliasStmt
+ * handles the execution of the GRANT/REVOKE ON DIRALIAS command.
+ *
+ * stmt - the GrantDirAliasStmt that describes the directory aliases and
+ *permissions to be granted/revoked.
+ */
+void
+ExecuteGrantDirAliasStmt(GrantDirAliasStmt *stmt)
+{
+ Relation pg_diralias_rel;
+ Oidgrantor;
+ List *grantee_ids = NIL;
+ AclMode permissions;
+ ListCell *item;
+
+ /* Must be superuser to grant directory alias permissions */
+ if (!superuser())
+ ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg(must be superuser to grant directory alias permissions)));
+
+ /*
+ * Grantor is optional. If it is not provided then set it to the current
+ * user.
+ */
+ if (stmt-grantor)
+ grantor = get_role_oid(stmt-grantor, false);
+ else
+ grantor = GetUserId();
+
+ /* Convert grantee names to oids */
+ foreach(item, stmt-grantees)
+ {
+ PrivGrantee *grantee = (PrivGrantee *) lfirst(item);
+
+ if (grantee-rolname == NULL)
+ grantee_ids = lappend_oid(grantee_ids, ACL_ID_PUBLIC);
+ else
+ {
+ Oid roleid = get_role_oid(grantee-rolname, false);
+ grantee_ids = lappend_oid(grantee_ids, roleid);
+ }
+ }
+
+ permissions = ACL_NO_RIGHTS;
+
+ /* If ALL was provided then set permissions to ACL_ALL_RIGHTS_DIRALIAS */
+ if (stmt-permissions == NIL)
+ permissions = ACL_ALL_RIGHTS_DIRALIAS;
+ else
+ {
+ /* Condense all permissions */
+ foreach(item, stmt-permissions)
+ {
+ AccessPriv *priv = (AccessPriv *) lfirst(item);
+ permissions |= string_to_privilege(priv-priv_name);
+ }
+ }
+
+
+ /*
+ * Though it shouldn't be possible to provide permissions other than READ
+ * and WRITE, check to make sure no others have been set. If they have,
+ * then warn the user and correct the permissions.
+ */
+ if (permissions !((AclMode) ACL_ALL_RIGHTS_DIRALIAS))
+ {
+ ereport(WARNING,
+(errcode(ERRCODE_INVALID_GRANT_OPERATION),
+ errmsg(directory aliases only support READ and WRITE permissions)));
+
+ permissions = ACL_ALL_RIGHTS_DIRALIAS;
+ }
+
+ pg_diralias_rel = heap_open(DirAliasRelationId, RowExclusiveLock);
+
+ /* Grant/Revoke permissions on directory aliases */
+ foreach(item, stmt-directories)
+ {
+ Datum values[Natts_pg_diralias];
+ bool replaces[Natts_pg_diralias];
+ bool nulls[Natts_pg_diralias];
+ ScanKeyData skey[1];
+ HeapScanDesc scandesc;
+ HeapTuple tuple;
+ HeapTuple new_tuple;
+ Datum datum;
+ Oidowner_id;
+ Acl *dir_acl;
+ Acl *new_acl;
+ bool is_null;
+ intnum_old_members;
+ intnum_new_members;
+ Oid *old_members;
+ Oid *new_members;
+ Oiddiralias_id;
+ char *name;
+
+ name = strVal(lfirst(item));
+
+ ScanKeyInit(skey[0],
+ Anum_pg_diralias_dirname,
+ BTEqualStrategyNumber, F_NAMEEQ,
+ CStringGetDatum(name));
+
+ scandesc = heap_beginscan_catalog(pg_diralias_rel, 1, skey);
+
+ tuple = heap_getnext(scandesc, ForwardScanDirection);
+
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, could not find tuple for directory alias \%s\, name);
+
+ /*
+ * Get directory alias owner id. Since all superusers are considered
+ * to be owners of a directory alias, it is safe to assume that the
+ * current user is an owner, given the superuser check above.
+ */
+ owner_id =
Re: [HACKERS] alter user/role CURRENT_USER
Kyotaro,
Food for thought. Couldn't you reduce the following block:
+ if (strcmp(stmt-role, current_user) == 0)
+ {
+ roleid = GetUserId();
+ tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg(roleid %d does not exist, roleid)));
+ }
+ else
+ {
+ tuple = SearchSysCache1(AUTHNAME, PointerGetDatum(stmt-role));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg(role \%s\ does not exist, stmt-role)));
To:
if (strcmp(stmt-role, current_user) == 0)
roleid = GetUserId();
else
roleid = get_role_oid(stmt-role, false);
tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
if (!HeapTupleIsValid(tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg(roleid %d does not exist, roleid)));
I think this makes it a bit cleaner. It also reuses existing code as
'get_role_oid()' already does a valid role name check and will raise the
proper error.
-Adam
On Mon, Oct 20, 2014 at 3:40 AM, Rushabh Lathia rushabh.lat...@gmail.com
wrote:
I gone through patch and here is the review for this patch:
.) patch go applied on master branch with patch -p1 command
(git apply failed)
.) regression make check run fine
.) testcase coverage is missing in the patch
.) Over all coding/patch looks good.
Few comments:
1) Any particular reason for not adding SESSION_USER/USER also made usable
for this command ?
2) I think RoleId_or_curruser can be used for following role:
/* ALTER TABLE name OWNER TO RoleId */
| OWNER TO RoleId
3) In the documentation patch, added for CURRENT_USER but CURRENT_ROLE is
missing.
On Fri, Oct 10, 2014 at 1:57 PM, Kyotaro HORIGUCHI
horiguchi.kyot...@lab.ntt.co.jp wrote:
Hello, on the way considering alter role set .., I found that
ALTER ROLE/USER cannot take CURRENT_USER as the role name.
In addition to that, documents of ALTER ROLE / USER are
inconsistent with each other in spite of ALTER USER is said to be
an alias for ALTER ROLE. Plus, ALTER USER cannot take ALL as user
name although ALTER ROLE can.
This patch does following things,
- ALTER USER/ROLE now takes CURRENT_USER as user name.
- Rewrite sysnopsis of the documents for ALTER USER and ALTER
ROLE so as to they have exactly same syntax.
- Modify syntax of ALTER USER so as to be an alias of ALTER ROLE.
- Added CURRENT_USER/CURRENT_ROLE syntax to both.
- Added ALL syntax as user name to ALTER USER.
- Added IN DATABASE syntax to ALTER USER.
x Integrating ALTER ROLE/USER syntax could not be done because of
shift/reduce error of bison.
x This patch contains no additional regressions. Is it needed?
SESSION_USER/USER also can be made usable for this command, but
this patch doesn't so (yet).
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Rushabh Lathia
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] [TODO] Track number of files ready to be archived in pg_stat_archiver
Julien, The following is an initial review: * Applies cleanly to master (f330a6d). * Regression tests updated and pass, including 'check-world'. * Documentation updated and builds successfully. * Might want to consider replacing the following magic number with a constant or perhaps calculated value. + int basenamelen = (int) strlen(rlde-d_name) - 6; * Wouldn't it be easier, or perhaps more reliable to use strrchr() with the following instead? + strcmp(rlde-d_name + basenamelen, .ready) == 0) char *extension = strrchr(ride-d_name, '.'); ... strcmp(extension, .ready) == 0) I think this approach might also help to resolve the magic number above. For example: char *extension = strrchr(ride-d_name, '.'); int basenamelen = (int) strlen(ride-d_name) - strlen(extension); -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Review of GetUserId() Usage
All,
I'll break them into three pieces- superuser() cleanup, GetUserId() -
has_privs_of_role(), and the additional-role-attributes patch will just
depend on the others.
Attached is a patch for the GetUserId() - has_privs_of_role() cleanup for
review.
-Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
new file mode 100644
index 67539ec..42d9a1f
*** a/src/backend/utils/adt/misc.c
--- b/src/backend/utils/adt/misc.c
***
*** 34,39
--- 34,40
#include storage/pmsignal.h
#include storage/proc.h
#include storage/procarray.h
+ #include utils/acl.h
#include utils/lsyscache.h
#include utils/ruleutils.h
#include tcop/tcopprot.h
*** pg_signal_backend(int pid, int sig)
*** 113,119
return SIGNAL_BACKEND_ERROR;
}
! if (!(superuser() || proc-roleId == GetUserId()))
return SIGNAL_BACKEND_NOPERMISSION;
/*
--- 114,120
return SIGNAL_BACKEND_ERROR;
}
! if (!has_privs_of_role(GetUserId(), proc-roleId))
return SIGNAL_BACKEND_NOPERMISSION;
/*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
new file mode 100644
index 44ccd37..ea2cd1e
*** a/src/backend/utils/adt/pgstatfuncs.c
--- b/src/backend/utils/adt/pgstatfuncs.c
***
*** 20,25
--- 20,26
#include libpq/ip.h
#include miscadmin.h
#include pgstat.h
+ #include utils/acl.h
#include utils/builtins.h
#include utils/inet.h
#include utils/timestamp.h
*** pg_stat_get_activity(PG_FUNCTION_ARGS)
*** 675,681
nulls[15] = true;
/* Values only available to same user or superuser */
! if (superuser() || beentry-st_userid == GetUserId())
{
SockAddr zero_clientaddr;
--- 676,682
nulls[15] = true;
/* Values only available to same user or superuser */
! if (has_privs_of_role(GetUserId(), beentry-st_userid))
{
SockAddr zero_clientaddr;
*** pg_stat_get_backend_activity(PG_FUNCTION
*** 877,883
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
activity = backend information not available;
! else if (!superuser() beentry-st_userid != GetUserId())
activity = insufficient privilege;
else if (*(beentry-st_activity) == '\0')
activity = command string not enabled;
--- 878,884
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
activity = backend information not available;
! else if (!has_privs_of_role(GetUserId(), beentry-st_userid))
activity = insufficient privilege;
else if (*(beentry-st_activity) == '\0')
activity = command string not enabled;
*** pg_stat_get_backend_waiting(PG_FUNCTION_
*** 898,904
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid != GetUserId())
PG_RETURN_NULL();
result = beentry-st_waiting;
--- 899,905
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!has_privs_of_role(GetUserId(), beentry-st_userid))
PG_RETURN_NULL();
result = beentry-st_waiting;
*** pg_stat_get_backend_activity_start(PG_FU
*** 917,923
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid != GetUserId())
PG_RETURN_NULL();
result = beentry-st_activity_start_timestamp;
--- 918,924
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!has_privs_of_role(GetUserId(), beentry-st_userid))
PG_RETURN_NULL();
result = beentry-st_activity_start_timestamp;
*** pg_stat_get_backend_xact_start(PG_FUNCTI
*** 943,949
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid != GetUserId())
PG_RETURN_NULL();
result = beentry-st_xact_start_timestamp;
--- 944,950
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!has_privs_of_role(GetUserId(), beentry-st_userid))
PG_RETURN_NULL();
result = beentry-st_xact_start_timestamp;
*** pg_stat_get_backend_start(PG_FUNCTION_AR
*** 965,971
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid != GetUserId())
PG_RETURN_NULL();
result = beentry-st_proc_start_timestamp;
--- 966,972
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!has_privs_of_role(GetUserId(), beentry-st_userid))
PG_RETURN_NULL();
result = beentry-st_proc_start_timestamp;
*** pg_stat_get_backend_client_addr(PG_FUNCT
*** 989,995
if ((beentry = pgstat_fetch_stat_beentry(beid)) == NULL)
PG_RETURN_NULL();
! if (!superuser() beentry-st_userid !=
Re: [HACKERS] Review of GetUserId() Usage
I'll break them into three pieces- superuser() cleanup, GetUserId() - has_privs_of_role(), and the additional-role-attributes patch will just depend on the others. The superuser() cleanup has already been submitted to the current commitfest. https://commitfest.postgresql.org/action/patch_view?id=1587 -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
[HACKERS] Directory/File Access Permissions for COPY and Generic File Access Functions
All,
The attached patch for review implements a directory permission system that
allows for providing a directory read/write capability to directories for
COPY TO/FROM and Generic File Access Functions to non-superusers. This is
not a complete solution as it does not currently contain documentation or
regression tests. Though it is my hopes to get some feedback as I am sure
there are some aspects of it that need to be reworked. So I thought I'd
put it out there for comments/review.
The approach taken is to create directory aliases that have a unique name
and path, as well as an associated ACL list. A superuser can create a new
alias to any directory on the system and then provide READ or WRITE
permissions to any non-superuser. When a non-superuser then attempts to
execute a COPY TO/FROM or any one of the generic file access functions, a
permission check is performed against the aliases for the user and target
directory. Superusers are allowed to bypass all of these checks. All
alias paths must be an absolute path in order to avoid potential risks.
However, in the generic file access functions superusers are still allowed
to execute the functions with a relative path where non-superusers are
required to provide an absolute path.
- Implementation Details -
System Catalog:
pg_diralias
- dirname - the name of the directory alias
- dirpath - the directory path - must be absolute
- diracl - the ACL for the directory
Syntax:
CREATE DIRALIAS name AS 'path'
ALTER DIRALIAS name AS 'path'
ALTER DIRALIAS name RENAME TO new_name
DROP DIRALIAS name
This is probably the area that I would really appreciate your thoughts and
recommendations. To GRANT permissions to a directory alias, I had to create
a special variant of GRANT since READ and WRITE are not reserved keywords
and causes grammar issues. Therefore, I chose to start with the following
syntax:
GRANT ON DIRALIAS name permissions TO roles
where permissions is either READ, WRITE or ALL.
Any comments, suggestions or feedback would be greatly appreciated.
Thanks,
Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index b257b02..8cdc5cb 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -41,7 +41,7 @@ POSTGRES_BKI_SRCS = $(addprefix $(top_srcdir)/src/include/catalog/,\
pg_foreign_data_wrapper.h pg_foreign_server.h pg_user_mapping.h \
pg_foreign_table.h pg_rowsecurity.h \
pg_default_acl.h pg_seclabel.h pg_shseclabel.h pg_collation.h pg_range.h \
- toasting.h indexing.h \
+ pg_diralias.h toasting.h indexing.h \
)
# location of Catalog.pm
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index d30612c..3717bf5 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -30,6 +30,7 @@
#include catalog/pg_collation.h
#include catalog/pg_conversion.h
#include catalog/pg_database.h
+#include catalog/pg_diralias.h
#include catalog/pg_default_acl.h
#include catalog/pg_event_trigger.h
#include catalog/pg_extension.h
@@ -48,6 +49,7 @@
#include catalog/pg_ts_config.h
#include catalog/pg_ts_dict.h
#include commands/dbcommands.h
+#include commands/diralias.h
#include commands/proclang.h
#include commands/tablespace.h
#include foreign/foreign.h
@@ -3183,6 +3185,190 @@ ExecGrant_Type(InternalGrant *istmt)
heap_close(relation, RowExclusiveLock);
}
+/*
+ * ExecuteGrantDirAliasStmt
+ * handles the execution of the GRANT/REVOKE ON DIRALIAS command.
+ *
+ * stmt - the GrantDirAliasStmt that describes the directory aliases and
+ *permissions to be granted/revoked.
+ */
+void
+ExecuteGrantDirAliasStmt(GrantDirAliasStmt *stmt)
+{
+ Relation pg_diralias_rel;
+ Oidgrantor;
+ List *grantee_ids = NIL;
+ AclMode permissions;
+ ListCell *item;
+
+ /* Must be superuser to grant directory alias permissions */
+ if (!superuser())
+ ereport(ERROR,
+(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg(must be superuser to grant directory alias permissions)));
+
+ /*
+ * Grantor is optional. If it is not provided then set it to the current
+ * user.
+ */
+ if (stmt-grantor)
+ grantor = get_role_oid(stmt-grantor, false);
+ else
+ grantor = GetUserId();
+
+ /* Convert grantee names to oids */
+ foreach(item, stmt-grantees)
+ {
+ PrivGrantee *grantee = (PrivGrantee *) lfirst(item);
+
+ if (grantee-rolname == NULL)
+ grantee_ids = lappend_oid(grantee_ids, ACL_ID_PUBLIC);
+ else
+ {
+ Oid roleid = get_role_oid(grantee-rolname, false);
+ grantee_ids = lappend_oid(grantee_ids, roleid);
+ }
+ }
+
+ permissions = ACL_NO_RIGHTS;
+
+ /* If ALL was provided then set permissions to ACL_ALL_RIGHTS_DIRALIAS */
+ if (stmt-permissions == NIL)
+ permissions = ACL_ALL_RIGHTS_DIRALIAS;
+ else
+ {
+ /* Condense all permissions */
+ foreach(item, stmt-permissions)
+ {
+ AccessPriv *priv = (AccessPriv *)
Re: [HACKERS] superuser() shortcuts
All,
Thanks! Please add it to the next commitfest.
Sounds good. I'll update the patch and add accordingly.
Attached is an updated patch.
-Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
new file mode 100644
index 133143d..695a13c
*** a/src/backend/access/transam/xlogfuncs.c
--- b/src/backend/access/transam/xlogfuncs.c
***
*** 27,32
--- 27,33
#include miscadmin.h
#include replication/walreceiver.h
#include storage/smgr.h
+ #include utils/acl.h
#include utils/builtins.h
#include utils/numeric.h
#include utils/guc.h
*** pg_start_backup(PG_FUNCTION_ARGS)
*** 54,60
backupidstr = text_to_cstring(backupid);
! if (!superuser() !has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser or replication role to run a backup)));
--- 55,61
backupidstr = text_to_cstring(backupid);
! if (!has_replication_privilege(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser or replication role to run a backup)));
*** pg_stop_backup(PG_FUNCTION_ARGS)
*** 82,88
{
XLogRecPtr stoppoint;
! if (!superuser() !has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg(must be superuser or replication role to run a backup;
--- 83,89
{
XLogRecPtr stoppoint;
! if (!has_replication_privilege(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg(must be superuser or replication role to run a backup;
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
new file mode 100644
index d30612c..4d26b02
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*** static AclMode restrict_and_check_grant(
*** 143,148
--- 143,149
AttrNumber att_number, const char *colname);
static AclMode pg_aclmask(AclObjectKind objkind, Oid table_oid, AttrNumber attnum,
Oid roleid, AclMode mask, AclMaskHow how);
+ static bool has_catupdate_privilege(Oid roleid);
#ifdef ACLDEBUG
*** aclcheck_error_type(AclResult aclerr, Oi
*** 3425,3431
/* Check if given user has rolcatupdate privilege according to pg_authid */
static bool
! has_rolcatupdate(Oid roleid)
{
bool rolcatupdate;
HeapTuple tuple;
--- 3426,3432
/* Check if given user has rolcatupdate privilege according to pg_authid */
static bool
! has_catupdate_privilege(Oid roleid)
{
bool rolcatupdate;
HeapTuple tuple;
*** 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
--- 3631,3637
if ((mask (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE))
IsSystemClass(table_oid, classForm)
classForm-relkind != RELKIND_VIEW
! !has_catupdate_privilege(roleid)
!allowSystemTableMods)
{
#ifdef ACLDEBUG
*** has_createrole_privilege(Oid roleid)
*** 5078,5083
--- 5079,5106
ReleaseSysCache(utup);
}
return result;
+ }
+
+ /*
+ * Check whether specified role has REPLICATION priviledge (or is a superuser)
+ */
+ bool
+ has_replication_privilege(Oid roleid)
+ {
+ bool result = false;
+ HeapTuple utup;
+
+ /* Superusers bypass all permission checking. */
+ if (superuser_arg(roleid))
+ return true;
+
+ utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
+ if (HeapTupleIsValid(utup))
+ {
+ result = ((Form_pg_authid) GETSTRUCT(utup))-rolreplication;
+ ReleaseSysCache(utup);
+ }
+ return result;
}
bool
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
new file mode 100644
index c9a9baf..ed89b23
*** a/src/backend/commands/alter.c
--- b/src/backend/commands/alter.c
*** AlterObjectOwner_internal(Relation rel,
*** 807,852
bool *nulls;
bool *replaces;
! /* Superusers can bypass permission checks */
! if (!superuser())
{
! AclObjectKind aclkind = get_object_aclkind(classId);
! /* must be owner */
! if (!has_privs_of_role(GetUserId(), old_ownerId))
{
! char *objname;
! char namebuf[NAMEDATALEN];
!
! if (Anum_name != InvalidAttrNumber)
! {
! datum = heap_getattr(oldtup, Anum_name,
! RelationGetDescr(rel), isnull);
! Assert(!isnull);
! objname = NameStr(*DatumGetName(datum));
! }
! else
! {
! snprintf(namebuf, sizeof(namebuf), %u,
! HeapTupleGetOid(oldtup));
! objname = namebuf;
Re: [HACKERS] RLS Design
Thom,
Also, I seem to get an error message with the following:
# create policy nice_colours ON colours for all to joe using (visible =
true) with check (name in ('blue','green','yellow'));
CREATE POLICY
\c - joe
insert into colours (name, visible) values ('blue',false);
ERROR: function with OID 0 does not exist
And if this did work, but I only violated the USING clause, would this
still say the WITH CHECK clause was the cause?
Since RLS is built on top of the same mechanisms used for Security Barrier
Views, I figured I would check this case against that and, for the heck of
it, regular VIEWs as well. The result is the same error in both cases
(below and attached). I also verified that this issue exists for 9.4beta2
and the current REL9_4_STABLE branch. If this isn't the expected behavior
(I can't imagine that it is), I am certainly willing to dive into it
further and see what I can determine for a solution/recommendation. At any
rate, this appears to be a previously existing issue with WITH CHECK
OPTION. Thoughts?
postgres=# DROP TABLE IF EXISTS colors CASCADE;
NOTICE: table colors does not exist, skipping
DROP TABLE
postgres=# DROP ROLE IF EXISTS joe;
DROP ROLE
postgres=# CREATE ROLE joe LOGIN;
CREATE ROLE
postgres=# CREATE TABLE colors (name text, visible bool);
CREATE TABLE
postgres=# CREATE OR REPLACE VIEW v_colors_1 WITH (security_barrier) AS
postgres-# SELECT * FROM colors WHERE (name in ('blue', 'green',
'yellow'))
postgres-# WITH CHECK OPTION;
CREATE VIEW
postgres=# CREATE OR REPLACE VIEW v_colors_2 AS
postgres-# SELECT * FROM colors WHERE (name in ('blue', 'green',
'yellow'))
postgres-# WITH CHECK OPTION;
CREATE VIEW
postgres=# GRANT ALL ON v_colors_1, v_colors_2 TO joe;
GRANT
postgres=# \c - joe
You are now connected to database postgres as user joe.
postgres= INSERT INTO v_colors_1 (name, visible) VALUES ('blue', false);
ERROR: function with OID 0 does not exist
postgres= INSERT INTO v_colors_2 (name, visible) VALUES ('blue', false);
ERROR: function with OID 0 does not exist
Thanks,
Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
with_check_error.sql
Description: application/sql
--
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] Selectivity estimation for inet operators
New version with semi join estimation function attached. I have performed the following initial review: - Patch format. -- submitted as unified, but not sure it makes it any easier to read than context format. - Apply to current master (77e65bf). -- success (though, I do get Stripping trailing CR's from patch; notification) - check-world -- success - Whitespace - were the whitespace changes in pg_operator.h necessary? As for implementation, I'll leave that to those with a better understanding of the purpose/expectations of the modified functions. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] replicating DROP commands across servers
I think there's been some changes to this patch since july, care to resend a new version? Sure, here it is. The only difference with the previous version is that it now also supports column defaults. This was found to be a problem when you drop a sequence that some column default depends on -- for example a column declared SERIAL, or a sequence marked with ALTER SEQUENCE OWNED BY. The new code is able to drop both the sequence and the default value (leaving, of course, the rest of the column intact.) This required adding support for such objects in get_object_address. I have given this patch the following review: - Apply to current master (77e65bf). -- success - check-world. --success - multiple FIXME statements still exist -- are there plans to fix these items? Can the duplicated code be extracted to a static function? -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
[HACKERS] New Model For Role Attributes and Fine Grained Permssions
Hi All,
This is a proof-of-concept patch for a new model around role attributes
and fine grained permissions meant to alleviate the current over dependence
on superuser.
This is not yet complete and only serves as a proof-of-concept at this
point, but I wanted to share it in the hopes of receiving comments,
suggestions and general feedback.
The general gist of this patch is as follows:
* New system catalog pg_permission that relates role id's to permissions.
* New syntax.
- GRANT permission TO role
- REVOKE permission FROM role
where, permission is one of an enumerated value, such as CREATE ROLE or
CREATE DATABASE.
* Refactor CREATEDB and NOCREATEDB role attribute to CREATE DATABASE
permission set by GRANT or REVOKE.
* Refactor CREATEROLE and NOCREATEROLE role attribute to CREATE ROLE
permission set by GRANT or REVOKE.
Again, this is meant to serve as a proof-of-concept. It is not
comprehensive and only demonstrates how this might work with a few already
defined permissions.
I have attached the current patch based on master.
Any comments or feedback would be greatly appreciated.
Thanks,
Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
new file mode 100644
index a974bd5..8e4ad25
*** a/src/backend/catalog/Makefile
--- b/src/backend/catalog/Makefile
*** POSTGRES_BKI_SRCS = $(addprefix $(top_sr
*** 39,45
pg_ts_config.h pg_ts_config_map.h pg_ts_dict.h \
pg_ts_parser.h pg_ts_template.h pg_extension.h \
pg_foreign_data_wrapper.h pg_foreign_server.h pg_user_mapping.h \
! pg_foreign_table.h \
pg_default_acl.h pg_seclabel.h pg_shseclabel.h pg_collation.h pg_range.h \
toasting.h indexing.h \
)
--- 39,45
pg_ts_config.h pg_ts_config_map.h pg_ts_dict.h \
pg_ts_parser.h pg_ts_template.h pg_extension.h \
pg_foreign_data_wrapper.h pg_foreign_server.h pg_user_mapping.h \
! pg_foreign_table.h pg_permission.h \
pg_default_acl.h pg_seclabel.h pg_shseclabel.h pg_collation.h pg_range.h \
toasting.h indexing.h \
)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
new file mode 100644
index d9745ca..9f4b5df
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
***
*** 42,53
--- 42,55
#include catalog/pg_opclass.h
#include catalog/pg_operator.h
#include catalog/pg_opfamily.h
+ #include catalog/pg_permission.h
#include catalog/pg_proc.h
#include catalog/pg_tablespace.h
#include catalog/pg_type.h
#include catalog/pg_ts_config.h
#include catalog/pg_ts_dict.h
#include commands/dbcommands.h
+ #include commands/permission.h
#include commands/proclang.h
#include commands/tablespace.h
#include foreign/foreign.h
*** bool
*** 5065,5082
has_createrole_privilege(Oid roleid)
{
bool result = false;
- HeapTuple utup;
/* Superusers bypass all permission checking. */
if (superuser_arg(roleid))
return true;
! utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
! if (HeapTupleIsValid(utup))
! {
! result = ((Form_pg_authid) GETSTRUCT(utup))-rolcreaterole;
! ReleaseSysCache(utup);
! }
return result;
}
--- 5067,5079
has_createrole_privilege(Oid roleid)
{
bool result = false;
/* Superusers bypass all permission checking. */
if (superuser_arg(roleid))
return true;
! result = HasPermission(roleid, PERM_CREATE_ROLE);
!
return result;
}
diff --git a/src/backend/commands/Makefile b/src/backend/commands/Makefile
new file mode 100644
index 22f116b..b98212a
*** a/src/backend/commands/Makefile
--- b/src/backend/commands/Makefile
*** OBJS = aggregatecmds.o alter.o analyze.o
*** 17,23
dbcommands.o define.o discard.o dropcmds.o \
event_trigger.o explain.o extension.o foreigncmds.o functioncmds.o \
indexcmds.o lockcmds.o matview.o operatorcmds.o opclasscmds.o \
! portalcmds.o prepare.o proclang.o \
schemacmds.o seclabel.o sequence.o tablecmds.o tablespace.o trigger.o \
tsearchcmds.o typecmds.o user.o vacuum.o vacuumlazy.o \
variable.o view.o
--- 17,23
dbcommands.o define.o discard.o dropcmds.o \
event_trigger.o explain.o extension.o foreigncmds.o functioncmds.o \
indexcmds.o lockcmds.o matview.o operatorcmds.o opclasscmds.o \
! permission.o portalcmds.o prepare.o proclang.o \
schemacmds.o seclabel.o sequence.o tablecmds.o tablespace.o trigger.o \
tsearchcmds.o typecmds.o user.o vacuum.o vacuumlazy.o \
variable.o view.o
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
new file mode 100644
index f480be8..ecba8a5
*** a/src/backend/commands/dbcommands.c
--- b/src/backend/commands/dbcommands.c
***
*** 36,45
--- 36,47
#include catalog/pg_authid.h
#include catalog/pg_database.h
#include catalog/pg_db_role_setting.h
+ #include
Re: [HACKERS] RLS Design
I think we do want a way to modify policies. However, we tend to avoid syntax that involves unnatural word order, as this certainly does. Maybe it's better to follow the example of CREATE RULE and CREATE TRIGGER and do something this instead: CREATE POLICY policy_name ON table_name USING quals; ALTER POLICY policy_name ON table_name USING quals; DROP POLICY policy_name ON table_name; The advantage of this is that you can regard policy_name ON table_name as the identifier for the policy throughout the system. You need some kind of identifier of that sort anyway to support COMMENT ON, SECURITY LABEL, and ALTER EXTENSION ADD/DROP for policies. Sounds good. I certainly think it makes a lot of sense to include the ALTER functionality, if for no other reason than ease of use. Another item to consider, though I believe it can come later, is per-action policies. Following the above suggested syntax, perhaps that might look like the following? CREATE POLICY policy_name ON table_name FOR action USING quals; ALTER POLICY policy_name ON table_name FOR action USING quals; DROP POLICY policy_name ON table_name FOR action; I was also giving some thought to the use of POLICY, perhaps I am wrong, but it does seem it could be at risk of becoming ambiguous down the road. I can't think of any specific examples at the moment, but my concern is what happens if we wanted to add another type of policy, whatever that might be, later? Would it make more sense to go ahead and qualify this a little more with ROW SECURITY POLICY? Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] RLS Design
Tom, Thanks for the feedback. 20MB messages to the list aren't that friendly. Please don't do that again, unless asked to. Apologies, I didn't realize it was so large until after it was sent. At any rate, it won't happen again. FWIW, the above syntax is a nonstarter, at least unless we're willing to make POLICY a reserved word (hint: we're not). The reason is that the ADD/DROP COLUMN forms consider COLUMN to be optional, meaning that the column name could directly follow ADD; and the column type name, which could also be just a plain identifier, would directly follow that. So there's no way to resolve the ambiguity with one token of lookahead. This actually isn't just bison being stupid: in fact, you simply cannot tell whether ALTER TABLE tab ADD POLICY varchar(42); is an attempt to add a column named policy of type varchar(42), or an attempt to add a policy named varchar with quals 42. Ok. Make sense and I was afraid that was the case. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] RLS Design
Stephen, Yeah, now that we're trying to bake this into ALTER TABLE we need to be a bit more cautious. I'd think: ALTER TABLE tab POLICY ADD ... Would work though? (note: haven't looked/tested myself) Yes, I just tested it and the following would work from a grammar perspective: ALTER TABLE table_name POLICY ADD policy_name (policy_quals) ALTER TABLE table_name POLICY DROP policy_name Though, it would obviously require the addition of POLICY to the list of unreserved keywords. I don't suspect that would be a concern, as it is not reserved, but thought I would point it out just in case. Another thought I had was, would we also want the following, so that policies could be modified? ALTER TABLE table_name POLICY ALTER policy_name (policy_quals) Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?
Robert, However, I believe that there has been a lack of focus in the development of the patch thus far in a couple of key areas - first in terms of articulating how it is different from and better than a writeable security barrier view, and second on how to manage the security and operational aspects of having a feature like this. I think that the discussion subsequent to my June 10th email has let to some good discussion on both points, which was my intent, but I still think much more time and thought needs to be spent on those issues if we are to have a feature which is up to our usual standards. I do apologize to anyone who interpreted that initial as a pure rant, because it really wasn't intended that way. Contrariwise, I hope that the people defending this patch will admit that the issues I am raising are real and focus on whether and how those concerns can be addressed. I absolutely appreciate all of the feedback that has been provided. It has been educational. To your point above, I started putting together a wiki page, as Stephen has spoken to, that is meant to capture these concerns and considerations as well as to capture ideas around solutions. https://wiki.postgresql.org/wiki/Row_Security_Considerations This page is obviously not complete, but I think it is a good start. Hopefully this document will help to continue the conversation and assist in addressing all the concerns that have been brought to the table. As well, I hope that this document serves to demonstrate our intent and that we *are* taking these concerns seriously. I assure you that as one of the individuals who is working towards the acceptance of this feature/patch, I am very much concerned about meeting the expected standards of quality and security. Thanks, Adam
Re: [HACKERS] API change advice: Passing plan invalidation info from the rewriter into the planner?
Hey Tom, Hm ... I'm not following why we'd need a special case for superusers and not anyone else? Seems like any useful RLS scheme is going to require more privilege levels than just superuser and not-superuser. As it stands right now, superuser is the only case where RLS policies should not be applied/completely ignored. I suppose it is possible to create RLS policies that are related to other privilege levels, but those would still need to be applied despite user id, excepting superuser. I'll defer to Stephen or Craig on the usefulness of this scheme. Could we put the if superuser then ok test into the RLS condition test and thereby not need more than one plan at all? As I understand it, the application of RLS policies occurs in the rewriter. Therefore, when switching back and forth between superuser and not-superuser the query must be rewritten, which would ultimately result in the need for a new plan correct? If that is the case, then I am not sure how one plan is possible. However, again, I'll have to defer to Stephen or Craig on this one. Thanks, Adam
