Re: [HACKERS] RADIUS fallback servers
On Mon, Mar 6, 2017 at 10:24 AM, Adam Brightwell wrote: >>> I wonder if removing the complexity of maintaining two separate lists >>> for the server and port would be a better/less complex approach. For >>> instance, why not go with a list of typical 'host:port' strings for >>> 'radiusservers'? If no port is specified, then simply use the default >>> for that specific host. Therefore, we would not have to worry about >>> keeping the two lists in sync. Thoughts? >> >> >> If we do that we should do it for all the parameters, no? So not just >> host:port, but something like host:port:secret:identifier? Mixing the two >> ways of doing it would be quite confusing I think. >> >> And I wonder if that format wouldn't get even more confusing if you for >> example want to use default ports, but non-default secrets. > > Yes, I agree. Such a format would be more confusing and I certainly > wouldn't be in favor of it. > >> I can see how it would probably be easier in some of the simple cases, but I >> wonder if it wouldn't make it worse in a lot of other cases. > > Ultimately, I think that it would be better off in a separate > configuration file. Something to the effect of each line representing > a server, something like: > > ' ' > > With 'radiusservers' simply being the path to that file and > 'radiusserver', etc. would remain as is. Where only one or the other > could be provided, but not both. Though, that's perhaps would be > beyond the scope of this patch. > > At any rate, I'm going to continue moving forward with testing this patch as > is. I have run through testing this patch against a small set of RADIUS servers. This testing included both single server and multiple server configurations. All seems to work as expected. -Adam -- 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] RADIUS fallback servers
>> I wonder if removing the complexity of maintaining two separate lists >> for the server and port would be a better/less complex approach. For >> instance, why not go with a list of typical 'host:port' strings for >> 'radiusservers'? If no port is specified, then simply use the default >> for that specific host. Therefore, we would not have to worry about >> keeping the two lists in sync. Thoughts? > > > If we do that we should do it for all the parameters, no? So not just > host:port, but something like host:port:secret:identifier? Mixing the two > ways of doing it would be quite confusing I think. > > And I wonder if that format wouldn't get even more confusing if you for > example want to use default ports, but non-default secrets. Yes, I agree. Such a format would be more confusing and I certainly wouldn't be in favor of it. > I can see how it would probably be easier in some of the simple cases, but I > wonder if it wouldn't make it worse in a lot of other cases. Ultimately, I think that it would be better off in a separate configuration file. Something to the effect of each line representing a server, something like: ' ' With 'radiusservers' simply being the path to that file and 'radiusserver', etc. would remain as is. Where only one or the other could be provided, but not both. Though, that's perhaps would be beyond the scope of this patch. At any rate, I'm going to continue moving forward with testing this patch as is. -Adam -- 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] RADIUS fallback servers
I've given an initial review of this patch. It applies cleanly and compiles without issue as of 6da9759. I'm going to continue with testing it against a set of RADIUS servers in the next few days. But in the meantime, I have a few questions (below). > It supports multiple RADIUS servers. For all other parameters (secret, port, > identifier) one can specify either the exact same number of entries, in > which case each server gets it's own, or exactly one entry in which case > that entry will apply to all servers. (Or zero entries for everything except > secret, which will make it the default). I wonder if removing the complexity of maintaining two separate lists for the server and port would be a better/less complex approach. For instance, why not go with a list of typical 'host:port' strings for 'radiusservers'? If no port is specified, then simply use the default for that specific host. Therefore, we would not have to worry about keeping the two lists in sync. Thoughts? > Each server is tried in order. If it responds positive, auth is OK. If it > responds negative, auth is rejected. If it does not respond at all, we move > on to the next one. > > I'm wondering if in doing this we should also make the RADIUS timeout a > configurable as HBA option, since it might become more important now? Yes, I think this would make sense and would be a good idea. -Adam -- 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] COPY command with RLS bug
> Looking for and improving test coverage for RLS is a good suggestion, > but let's not link the fate of the issue reported here with this > requirement. I have spent some time looking at this patch and this > looks in rather good shape to me (you even remembered to use the > prefix regress_* for the role name that you are adding!). So I have > marked this bug fix as ready for committer. Excellent, thanks for the review and feedback. I appreciate it. -Adam -- 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] COPY command with RLS bug
> Thanks for the report and the fix. Yup. I have added it to the 2016-11 commitfest: https://commitfest.postgresql.org/11/794/ > This seems a rather basic error to occur a year after release. > > Is this a problem with the testing of RLS? What other RLS related > failures exist in other commands? I think was simply due to a small gap in the test suite. > Perhaps we should extend rowsecurity test with a more comprehensive > set of tests rather than just fix the COPY one? I think more tests that provide value are always a *good* thing, however, would argue that other tests 'unrelated' to this fix are more of a TODO item than something to include with this fix. Though, I am certainly willing to attempt to find/add more test cases around this specific functionality if that is desired. -Adam -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] COPY command with RLS bug
All,
I have discovered a bug with the COPY command, specifically related to RLS.
The issue:
When running COPY as superuser on a table that has RLS enabled, RLS is
bypassed and therefore no issue exists. However, when performing a
COPY as a non-privileged user on the same table causes issues when
more than one column is specified as part of the command.
Assuming:
CREATE TABLE foo (a int, b int, c int);
... set up RLS
Connecting as a non-privileged user provides the following results:
COPY foo TO stdout; -- pass
COPY foo (a) TO stdout; -- pass
COPY foo (a, b, c) TO stdout; -- fail
The error related to the failure is the following:
ERROR: missing FROM-clause entry for table "b"
LINE 1: copy foo (a, b, c) to stdout;
I don't believe this to be a vulnerability simply because it doesn't
'leak' any data to a non-privileged user, it will just throw an error.
As well, this is only an issue when more than one column is specified
and '*' isn't implied. I have attached a 'test' file that can be used
to observe this behavior.
I have verified that this is an issue on REL9_5_STABLE, REL9_6_STABLE
and master.
Solution:
The issue seems to be that the target list for the resulting SELECT
statement is not being built correctly. I have attached a proposed
patch to fix this issue. As well, I have added a few regression tests
for this case.
If deemed appropriate, then I'll add this to the currently open Commitfest.
Please let me know if there are any questions.
Thanks,
Adam
test-copy-rls.sql
Description: application/sql
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 432b0ca..a3777d9 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -871,6 +871,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, uint64 *processed)
ColumnRef *cr;
ResTarget *target;
RangeVar *from;
+ List *targetList = NIL;
if (is_from)
ereport(ERROR,
@@ -878,21 +879,62 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, uint64 *processed)
errmsg("COPY FROM not supported with row-level security"),
errhint("Use INSERT statements instead.")));
- /* Build target list */
- cr = makeNode(ColumnRef);
-
+ /*
+ * Build target list
+ *
+ * If no columns are specified in the attribute list of the COPY
+ * command, then the target list is 'all' columns. Therefore, '*'
+ * should be used as the target list for the resulting SELECT
+ * statement.
+ *
+ * In the case that columns are specified in the attribute list,
+ * create a ColumnRef and ResTarget for each column and add them to
+ * the target list for the resulting SELECT statement.
+ */
if (!stmt->attlist)
+ {
+cr = makeNode(ColumnRef);
cr->fields = list_make1(makeNode(A_Star));
+cr->location = 1;
+
+target = makeNode(ResTarget);
+target->name = NULL;
+target->indirection = NIL;
+target->val = (Node *) cr;
+target->location = 1;
+
+targetList = list_make1(target);
+ }
else
-cr->fields = stmt->attlist;
+ {
+ListCell *lc;
+int location = 1;
+
+foreach(lc, stmt->attlist)
+{
+ /*
+ * Build the ColumnRef for each column. The ColumnRef
+ * 'fields' property is a String 'Value' node (see
+ * nodes/value.h) that correspond to the column name
+ * respectively.
+ */
+ cr = makeNode(ColumnRef);
+ cr->fields = list_make1(lfirst(lc));
+ cr->location = location;
+
+ /* Build the ResTarget and add the ColumnRef to it. */
+ target = makeNode(ResTarget);
+ target->name = NULL;
+ target->indirection = NIL;
+ target->val = (Node *) cr;
+ target->location = location;
- cr->location = 1;
+ /* Add each column to the SELECT statements target list */
+ targetList = lappend(targetList, target);
- target = makeNode(ResTarget);
- target->name = NULL;
- target->indirection = NIL;
- target->val = (Node *) cr;
- target->location = 1;
+ location += 1;
+}
+ }
/*
* Build RangeVar for from clause, fully qualified based on the
@@ -903,7 +945,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, uint64 *processed)
/* Build query */
select = makeNode(SelectStmt);
- select->targetList = list_make1(target);
+ select->targetList = targetList;
select->fromClause = list_make1(from);
query = (Node *) select;
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 5f6260a..13251c6 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -460,9 +460,57 @@ select * from check_con_tbl;
(2 rows)
+-- test with RLS enabled.
+CREATE USER regress_rls_copy_user;
+CREATE TABLE rls_t1 (a int, b int, c int);
+COPY rls_t1 (a, b, c) from stdin;
+CREATE POLICY p1 ON rls_t1 FOR SELECT USING (a % 2 = 0);
+ALTER TABLE rls_t1 ENABLE ROW LEVEL SECURITY;
+ALTER TABLE rls_t1 FORCE ROW LEVEL SECURITY;
+GRANT SELECT ON TABLE rls_t1 TO
Re: [HACKERS] bootstrap pg_shseclabel in relcache initialization
> Pushed, with one cosmetic change (update comment on formrdesc). I also > bumped the catversion, but didn't verify whether this is critical. Excellent! Thanks! -Adam -- 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] bootstrap pg_shseclabel in relcache initialization
> So this looks like a bugfix that we should backpatch, but on closer > inspection it turns out that we need the rowtype OID to be fixed, which > it isn't unless this: > >> -CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS >> +CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_ROWTYPE_OID(4066) >> BKI_WITHOUT_OIDS BKI_SCHEMA_MACRO > > so I'm afraid this cannot be backpatched at all; if we did, the rowtype > wouldn't match for already-initdb'd installations. > > I'm gonna push this to master only, which means you won't be able to > rely on pg_shseclabel until 9.6. I thinks that's fair. -Adam -- 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] bootstrap pg_shseclabel in relcache initialization
> In commit 5d1ff6bd559ea8df I'd expected that the > WARNINGs would certainly show up in regression test output, and I thought > I'd verified that that was the case --- did that not happen for you? I just doubled checked with both 'check' and 'check-world' and neither seemed to have an issue with it. Though, I do see the warning show up in 'regress/log/postmaster.log'. -Adam -- 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] bootstrap pg_shseclabel in relcache initialization
On Tue, Nov 10, 2015 at 9:18 AM, Adam Brightwell wrote: >> I'm with Alvaro: the most interesting question here is why that mistake >> did not blow up on you immediately. I thought we had enough safeguards >> in place to catch this type of error. > > Ok, I'll explore that a bit further as I was able to build and use > with my hook without any issue. :-/ Ok, I have verified that it does indeed eventually raise a warning about this. It took a few for it to occur/appear in my logs. I was expecting it to be more "immediate". At any rate, I don't believe there are any issues with the safeguards in place. -Adam -- 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] bootstrap pg_shseclabel in relcache initialization
>> +1 for adding to the next commitfest. >> > Me also. Done. -Adam -- 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] bootstrap pg_shseclabel in relcache initialization
> I'm with Alvaro: the most interesting question here is why that mistake > did not blow up on you immediately. I thought we had enough safeguards > in place to catch this type of error. Ok, I'll explore that a bit further as I was able to build and use with my hook without any issue. :-/ -Adam -- 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] bootstrap pg_shseclabel in relcache initialization
>> >> #define NUM_CRITICAL_SHARED_INDEXES 5/* fix if you change list
>> >> above */
>> >>
>> >
>> > Need to bump this #define? If you didn't get the error that this is
>> > supposed to throw, perhaps there's a bug somewhere worth investigating.
>>
>> Hmm... I thought that I had, are you not seeing the following change?
>>
>> -#define NUM_CRITICAL_SHARED_RELS3/* fix if you change list above */
>> +#define NUM_CRITICAL_SHARED_RELS4/* fix if you change list above */
>
> NUM_CRITICAL_SHARED_RELS != NUM_CRITICAL_SHARED_INDEXES
Whoops! It must be getting late... updated patch attached.
-Adam
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 9c3d096..b701d82 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -51,6 +51,7 @@
#include "catalog/pg_opclass.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_rewrite.h"
+#include "catalog/pg_shseclabel.h"
#include "catalog/pg_tablespace.h"
#include "catalog/pg_trigger.h"
#include "catalog/pg_type.h"
@@ -98,6 +99,7 @@ static const FormData_pg_attribute Desc_pg_database[Natts_pg_database] = {Schema
static const FormData_pg_attribute Desc_pg_authid[Natts_pg_authid] = {Schema_pg_authid};
static const FormData_pg_attribute Desc_pg_auth_members[Natts_pg_auth_members] = {Schema_pg_auth_members};
static const FormData_pg_attribute Desc_pg_index[Natts_pg_index] = {Schema_pg_index};
+static const FormData_pg_attribute Desc_pg_shseclabel[Natts_pg_shseclabel] = {Schema_pg_shseclabel};
/*
* Hash tables that index the relation cache
@@ -3187,13 +3189,14 @@ RelationCacheInitialize(void)
/*
* RelationCacheInitializePhase2
*
- * This is called to prepare for access to shared catalogs during startup.
- * We must at least set up nailed reldescs for pg_database, pg_authid,
- * and pg_auth_members. Ideally we'd like to have reldescs for their
- * indexes, too. We attempt to load this information from the shared
- * relcache init file. If that's missing or broken, just make phony
- * entries for the catalogs themselves. RelationCacheInitializePhase3
- * will clean up as needed.
+ * This is called to prepare for access to shared catalogs during
+ * startup. We must at least set up nailed reldescs for
+ * pg_database, pg_authid, pg_auth_members, and pg_shseclabel.
+ * Ideally we'd like to have reldescs for their indexes, too. We
+ * attempt to load this information from the shared relcache init
+ * file. If that's missing or broken, just make phony entries for
+ * the catalogs themselves. RelationCacheInitializePhase3 will
+ * clean up as needed.
*/
void
RelationCacheInitializePhase2(void)
@@ -3229,8 +3232,10 @@ RelationCacheInitializePhase2(void)
true, Natts_pg_authid, Desc_pg_authid);
formrdesc("pg_auth_members", AuthMemRelation_Rowtype_Id, true,
false, Natts_pg_auth_members, Desc_pg_auth_members);
+ formrdesc("pg_shseclabel", SharedSecLabelRelation_Rowtype_Id, true,
+ false, Natts_pg_shseclabel, Desc_pg_shseclabel);
-#define NUM_CRITICAL_SHARED_RELS 3 /* fix if you change list above */
+#define NUM_CRITICAL_SHARED_RELS 4 /* fix if you change list above */
}
MemoryContextSwitchTo(oldcxt);
@@ -3365,8 +3370,10 @@ RelationCacheInitializePhase3(void)
AuthIdRelationId);
load_critical_index(AuthMemMemRoleIndexId,
AuthMemRelationId);
+ load_critical_index(SharedSecLabelObjectIndexId,
+ SharedSecLabelRelationId);
-#define NUM_CRITICAL_SHARED_INDEXES 5 /* fix if you change list above */
+#define NUM_CRITICAL_SHARED_INDEXES 6 /* fix if you change list above */
criticalSharedRelcachesBuilt = true;
}
diff --git a/src/include/catalog/pg_shseclabel.h b/src/include/catalog/pg_shseclabel.h
index 0ff41f3..d8334bf 100644
--- a/src/include/catalog/pg_shseclabel.h
+++ b/src/include/catalog/pg_shseclabel.h
@@ -18,9 +18,10 @@
* typedef struct FormData_pg_shseclabel
*
*/
-#define SharedSecLabelRelationId 3592
+#define SharedSecLabelRelationId 3592
+#define SharedSecLabelRelation_Rowtype_Id 4066
-CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
+CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_ROWTYPE_OID(4066) BKI_WITHOUT_OIDS BKI_SCHEMA_MACRO
{
Oid objoid; /* OID of the shared object itself */
Oid classoid; /* OID of table containing the shared object */
@@ -31,6 +32,8 @@ CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
#endif
} FormData_pg_shseclabel;
+typedef FormData_pg_shseclabel *Form_pg_shseclabel;
+
/*
* compiler constants for pg_shseclabel
*
--
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] bootstrap pg_shseclabel in relcache initialization
>> @@ -3365,6 +3370,8 @@ RelationCacheInitializePhase3(void) >> AuthIdRelationId); >> load_critical_index(AuthMemMemRoleIndexId, >> AuthMemRelationId); >> + load_critical_index(SharedSecLabelObjectIndexId, >> + >> SharedSecLabelRelationId); >> >> #define NUM_CRITICAL_SHARED_INDEXES 5/* fix if you change list >> above */ >> > > Need to bump this #define? If you didn't get the error that this is > supposed to throw, perhaps there's a bug somewhere worth investigating. Hmm... I thought that I had, are you not seeing the following change? -#define NUM_CRITICAL_SHARED_RELS3/* fix if you change list above */ +#define NUM_CRITICAL_SHARED_RELS4/* fix if you change list above */ -Adam -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] bootstrap pg_shseclabel in relcache initialization
Hi All,
While working on an auth hook, I found that I was unable to access the
pg_shseclabel system table while processing the hook. I discovered
that the only tables that were bootstrapped and made available at this
stage of the the auth process were pg_database, pg_authid and
pg_auth_members. Unfortunately, this is problematic if you have
security labels that are associated with a role which are needed to
determine auth decisions/actions.
Given that the shared relations currently exposed can also have
security labels that can be used for auth purposes, I believe it makes
sense to make those available as well. I have attached a patch that
adds this functionality for review/discussion. If this functionality
makes sense I'll add it to the commitfest.
Thanks,
Adam
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 9c3d096..c38a8ac 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -51,6 +51,7 @@
#include "catalog/pg_opclass.h"
#include "catalog/pg_proc.h"
#include "catalog/pg_rewrite.h"
+#include "catalog/pg_shseclabel.h"
#include "catalog/pg_tablespace.h"
#include "catalog/pg_trigger.h"
#include "catalog/pg_type.h"
@@ -98,6 +99,7 @@ static const FormData_pg_attribute Desc_pg_database[Natts_pg_database] = {Schema
static const FormData_pg_attribute Desc_pg_authid[Natts_pg_authid] = {Schema_pg_authid};
static const FormData_pg_attribute Desc_pg_auth_members[Natts_pg_auth_members] = {Schema_pg_auth_members};
static const FormData_pg_attribute Desc_pg_index[Natts_pg_index] = {Schema_pg_index};
+static const FormData_pg_attribute Desc_pg_shseclabel[Natts_pg_shseclabel] = {Schema_pg_shseclabel};
/*
* Hash tables that index the relation cache
@@ -3187,13 +3189,14 @@ RelationCacheInitialize(void)
/*
* RelationCacheInitializePhase2
*
- * This is called to prepare for access to shared catalogs during startup.
- * We must at least set up nailed reldescs for pg_database, pg_authid,
- * and pg_auth_members. Ideally we'd like to have reldescs for their
- * indexes, too. We attempt to load this information from the shared
- * relcache init file. If that's missing or broken, just make phony
- * entries for the catalogs themselves. RelationCacheInitializePhase3
- * will clean up as needed.
+ * This is called to prepare for access to shared catalogs during
+ * startup. We must at least set up nailed reldescs for
+ * pg_database, pg_authid, pg_auth_members, and pg_shseclabel.
+ * Ideally we'd like to have reldescs for their indexes, too. We
+ * attempt to load this information from the shared relcache init
+ * file. If that's missing or broken, just make phony entries for
+ * the catalogs themselves. RelationCacheInitializePhase3 will
+ * clean up as needed.
*/
void
RelationCacheInitializePhase2(void)
@@ -3229,8 +3232,10 @@ RelationCacheInitializePhase2(void)
true, Natts_pg_authid, Desc_pg_authid);
formrdesc("pg_auth_members", AuthMemRelation_Rowtype_Id, true,
false, Natts_pg_auth_members, Desc_pg_auth_members);
+ formrdesc("pg_shseclabel", SharedSecLabelRelation_Rowtype_Id, true,
+ false, Natts_pg_shseclabel, Desc_pg_shseclabel);
-#define NUM_CRITICAL_SHARED_RELS 3 /* fix if you change list above */
+#define NUM_CRITICAL_SHARED_RELS 4 /* fix if you change list above */
}
MemoryContextSwitchTo(oldcxt);
@@ -3365,6 +3370,8 @@ RelationCacheInitializePhase3(void)
AuthIdRelationId);
load_critical_index(AuthMemMemRoleIndexId,
AuthMemRelationId);
+ load_critical_index(SharedSecLabelObjectIndexId,
+ SharedSecLabelRelationId);
#define NUM_CRITICAL_SHARED_INDEXES 5 /* fix if you change list above */
diff --git a/src/include/catalog/pg_shseclabel.h b/src/include/catalog/pg_shseclabel.h
index 0ff41f3..d8334bf 100644
--- a/src/include/catalog/pg_shseclabel.h
+++ b/src/include/catalog/pg_shseclabel.h
@@ -18,9 +18,10 @@
* typedef struct FormData_pg_shseclabel
*
*/
-#define SharedSecLabelRelationId 3592
+#define SharedSecLabelRelationId 3592
+#define SharedSecLabelRelation_Rowtype_Id 4066
-CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
+CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_ROWTYPE_OID(4066) BKI_WITHOUT_OIDS BKI_SCHEMA_MACRO
{
Oid objoid; /* OID of the shared object itself */
Oid classoid; /* OID of table containing the shared object */
@@ -31,6 +32,8 @@ CATALOG(pg_shseclabel,3592) BKI_SHARED_RELATION BKI_WITHOUT_OIDS
#endif
} FormData_pg_shseclabel;
+typedef FormData_pg_shseclabel *Form_pg_shseclabel;
+
/*
* compiler constants for pg_shseclabel
*
--
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] Arguable RLS security bug, EvalPlanQual() paranoia
> I'm not convinced this is the right place, but at a minimum it should be > referenced from the RLS documentation. Further, it should be noted that > users who have direct SQL access can control what the isolation level > is for their transaction. I agree that it should be referenced by the RLS docs. However, I'm not sure I can think of a better place for it. My reasons for choosing this location was that the behavior seems specific to the READ COMMITTED isolation level and was an accepted MVCC anomaly; not necessarily specific only to RLS and SBV. But, again, I'd agree that referencing it in the other locations would be desired. Of course, I'm willing to accept that I may be making the wrong assumptions. > Also, isn't it possible to avoid this by locking the records? If the > locking fails or blocks then you know another user has those records > locked and you don't update or you wait until you hold the lock. > Assuming that works (I don't immediately see why it wouldn't..), we > should provide an example. I haven't found that to work, at least not with the specific case presented by Peter. Based on the following (output from Peter's isolation test), I would understand that the 'malicious' UPDATE is waiting for the previous update to be committed before it continues, even without the FOR UPDATE lock on the rows. step no_trust_mallory: update users set group_id = 1 where user_name = 'mallory'; step update_hc: update information set info = 'secret' where group_id = 2; step updatem: update information set info = info where group_id = 2 returning 'mallory update: ' m, *; step commit_hc: commit; step updatem: <... completed> As well, due to this, as described by the READ COMMITTED documentation: "it is possible for an updating command to see an inconsistent snapshot: it can see the effects of concurrent updating commands on the same rows it is trying to update" I'm not convinced that this is something that FOR UPDATE can address for this specific case. If inconsistencies in the 'snapshot' can be expected and are accepted at this isolation level, then I'm not sure how we can reasonably expect locking the rows to have any affect. Though, again, I'm willing to accept that I am not fully understanding this behavior and that I am completely wrong. -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] Arguable RLS security bug, EvalPlanQual() paranoia
On Mon, Aug 3, 2015 at 6:21 PM, Peter Geoghegan wrote: > On Mon, Aug 3, 2015 at 3:07 PM, Stephen Frost wrote: >> Thoughts? Trying to keep it straight-forward and provide a simple >> solution for users to be able to address the issue, if they're worried >> about it. Perhaps this, plus an additional paragraph which goes into >> more detail about exactly what's going on? > > I'm still thinking about it, but I think you have the right idea here. I have attached a patch for review that I believe addresses the documentation side of this issue. Thoughts or comments? Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com transaction-isolation-rls-docs.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] unclear about row-level security USING vs. CHECK
> My vote would be to keep it as-is. Same for me. > It feels perfectly natural to me. USING clauses add to the query's > WHERE clause controlling which existing rows you can SELECT, UPDATE or > DELETE. WITH CHECK clauses control what new data you can add via > INSERT or UPDATE. UPDATE allows both, but most of the time I expect > you'll want them to be the same. I agree. In the current uses cases I have been experimenting with, this approach has made the most sense. > So having the WITH CHECK clause default to being the same as the USING > clause for UPDATE matches what I expect to be the most common usage. I agree. > Users granted permission to update a subset of the table's rows > probably don't want to give those rows away. More advanced use-cases > are still supported, but the simplest/most common case is the default, > which means that you don't have to supply the same expression twice. Yes, I agree. IMO, having to supply the same expression twice just seems cumbersome and unnecessary. While I'd certainly agree that documentation could always be improved, I have found the current behavior to be fairly intuitive and easily understood by most (if not all) DBA's I have spoken with about it. -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] row_security GUC, BYPASSRLS
> I've attached a patch to implement it. It's not fully polished but it's
> sufficient for comment, I believe. Additional comments, documentation
> and regression tests are to be added, if we have agreement on the
> grammer and implementation approach.
I have given the first patch a quick review. I think I agree with the
grammar, it makes sense to me. At first I didn't really like NO
FORCE, but I couldn't think of anything better.
One comment:
if (pg_class_ownercheck(relid, user_id))
- return RLS_NONE_ENV;
+ {
+ if (relforcerowsecurity)
+ return RLS_ENABLED;
+ else
+ return RLS_NONE_ENV;
+ }
Is the 'else' even necessary in this block?
Other than that, the approach looks good to me.
The patch applied cleanly against master (758fcfd). As well
'check-world' was successful (obviously understanding that more
related tests are necessary).
> I have a hard time with this. We're not talking about the application
> logic in this case, we're talking about the guarantees which the
> database is making to the user, be it an application or an individual.
>
> I've included a patch (the second in the set attached) which adds a
> SECURITY_NOFORCE_RLS bit which has a much tighter scope- it only applies
> after the regular owner check is done. This reduces the risk of it
> being set mistakenly dramatically, I believe. Further, the cascaded
> case is correctly handled. This also needs more polish and regression
> tests, but I wanted to solicit for comment before moving forward with
> that since we don't have a consensus about if this should be done.
I'm not sure that I understand the previous concerns around this bit
well enough to speak intelligently on this point. However, with that
said, I believe the changes made by this patch make sense.
-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] No Issue Tracker - Say it Ain't So!
>> Personally I'd also change sending patches in emails to github pull >> requests :). > > That won't happen, at least not this decade. FWIW, a year ago I might have agreed that a github pull-request would be preferable. However, since, I have grown to really like the patch via email approach. I can see a lot of value in keeping patch submission decoupled from a specific service/technology/workflow in this way. >> ... or maybe the difference is more in the data structure, the email >> discussion is a tree (with a horrible interface to the archive) while in a >> bug tracker, the discussion is linear, and easier to follow. > > FWIW in my opinion our mailing list archives interface is the best there > is --- and I disagree that the linear discussion is easy to follow, > except for trivial discussions. In my experience, following other mailing lists, I really appreciate our interface. I'm not sure that I'd call it the best, but I've certainly seen far worse and I have no real complaints about it. What I think I like best about it is that it has an community "official" status, meaning we don't depend on some other mirror/archive site to support it, like gmane or spinics. This is just my opinion though. -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] [COMMITTERS] pgsql: Use gender-neutral language in documentation
> I think conversations like this are a part of why we have trouble attracting > new contributors (of any gender) to the community. +1 -- 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] proposal: multiple psql option -c
Pavel, > with -1 option support FWIW, I have tried to apply this patch against master (7f11724) and there is a minor error, see below. >From patch: patching file src/bin/psql/settings.h Hunk #2 FAILED at 135. 1 out of 2 hunks FAILED -- saving rejects to file src/bin/psql/settings.h.rej >From settings.h.rej: --- src/bin/psql/settings.h +++ src/bin/psql/settings.h @@ -135,6 +141,7 @@ const char *prompt2; const char *prompt3; PGVerbosity verbosity; /* current error verbosity level */ + GroupCommand *group_commands; } PsqlSettings; extern PsqlSettings pset; -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] row_security GUC, BYPASSRLS
>> 1. remove row_security=force >> 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to policies >> 3. add DDL-controlled, per-table policy forcing >> >> They ought to land in that order. PostgreSQL 9.5 would need at least (1) and >> (2); would RLS experts find it beneficial for me to take care of those? > > That would be awesome, but I would say that if we do #1 & 2 for 9.5, we > also need #3. Agreed. Please let me know if there is anything I can do to help. -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] row_security GUC, BYPASSRLS
> 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to policies I believe this one has already been addressed by Stephen (20150910192313.gt3...@tamriel.snowman.net)? Are there further considerations for his proposed changes? -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] row_security GUC, BYPASSRLS
> I could also see a potential gap in such approach. Specifically, I > could see a case were there are two separate roles, one that is > entrusted with defining the policies but not able to create/modify > tables, and one with the opposite capability (I understand this to be > a fairly common use-case, at least at a system level). Since you > can't GRANT 'alter' rights to the table, then obviously the policy > definer would have to either be the owner of the table or a member of > the role that owns it, right? Given that, if by definition the policy > definer is not allowed to do anything other than define policies, then > obviously putting such a role in the table owners group would allow it > to do much more, correct? Actually, disregard, I forgot about "You must be the owner of a table to create or change policies for it." So that would obviously negate my concern. -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] row_security GUC, BYPASSRLS
On Tue, Sep 15, 2015 at 2:26 PM, Joe Conway wrote: > On 09/15/2015 12:53 PM, Tom Lane wrote: >> Joe Conway writes: >>> There are use cases where row_security=force will be set in production >>> environments, not only in testing. >> >> What cases, exactly, and is there another way to solve those problems? >> I concur with Noah's feeling that allowing security behavior to depend on >> a GUC is risky. > > There are environments where there is a strong desire to use RLS to > control access in production, even for table owners and superusers. > Obviously there are more steps needed to completely achieve this level > of control, but removing the ability to force row security is going in > the wrong direction. Noah's suggestion of using a per table attribute > would work -- in fact I like the idea of that better than using the > current GUC. FWIW, I also concur with a per table attribute for this purpose. In fact, I think I really like the per-table flexibility over an 'all-or-nothing' approach better too. I do, however, think that it makes it a bit more difficult for testing, specifically, I'm not sure how much I like the idea of 'altering' a table so that it can be tested, but that might a price worth paying for security sake. I could also see a potential gap in such approach. Specifically, I could see a case were there are two separate roles, one that is entrusted with defining the policies but not able to create/modify tables, and one with the opposite capability (I understand this to be a fairly common use-case, at least at a system level). Since you can't GRANT 'alter' rights to the table, then obviously the policy definer would have to either be the owner of the table or a member of the role that owns it, right? Given that, if by definition the policy definer is not allowed to do anything other than define policies, then obviously putting such a role in the table owners group would allow it to do much more, correct? -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] One question about security label command
> * It is really the version of libselinux.so that matters here. RHEL > 7.x has libselinux 2.2.x whereas RHEL 6.x has 2.0.x. The latter lacks > functionality required by sepgsql starting with PG 9.2. Yes, that has been my observation as well. > So given all that, here is what I propose we do: > > 1.) Commit Kouhei's patch against HEAD and 9.5 (Joe) > 2.) Commit my modified patch against 9.4 and 9.3 (Joe) > 3.) Rework patch for 9.2 (Kouhei) > 4.) Finish standing up the RHEL/CentOS 7.x buildfarm member to > test sepgsql on 9.2 and up. The animal (rhinoceros) is running > already, but still needs some custom scripting. (Joe, Andrew) > 5.) Additionally stand up a RHEL/CentOS 6.x buildfarm member to test > sepgsql on 9.1 (no changes) (Joe). > > Sound like a plan? I think this makes sense. Getting buildfarm coverage on any level is better than nothing, IMHO. Kind of a bummer that 9.1 is the only version that will work as-is on EL6 but it is what it is for now, I suppose. -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] One question about security label command
> So what about the buildfarm animal that was offered for this? We still > have this module completely uncovered in the buildfarm ... I believe that is in the works and should be made available soon. -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] One question about security label command
All, > The second approach above works. > I defined a own privileged domain (sepgsql_regtest_superuser_t) > instead of system's unconfined_t domain. > The reason why regression test gets failed was, definition of > unconfined_t in the system default policy was changed to bypass > multi-category rules; which our regression test depends on. > So, the new sepgsql_regtest_superuser_t domain performs almost > like as unconfined_t, but restricted by multi-category policy as > traditional unconfined_t did. > It is self defined domain, so will not affected by system policy > change. > Even though the sepgsql-regtest.te still uses unconfined_u and > unconfined_r pair for selinux-user and role, it requires users to > define additional selinux-user by hand if we try to define own one. > In addition, its definition has not been changed for several years. > So, I thought it has less risk to rely on unconfined_u/unconfined_r > field unlike unconfined_t domain. I have reviewed and tested this patch against 'master' at 781ed2b. The patch applies without issue and all tests pass on EL7. -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
[HACKERS] ALTER SYSTEM and GUC_LIST_QUOTE
All, While testing some behaviors with ALTER SYSTEM I discovered that GUC parameters with the GUC_LIST_QUOTE flag have a potential issue. As an example, ALTER SYSTEM SET shared_preload_libraries = ''; Results in the following output in postgresql.auto.conf: shared_preload_libraries = '""'; Therefore, when attempting to restart postgres the following error is encountered: FATAL: could not access file "": No such file or directory As well, specifying multiple items: ALTER SYSTEM SET shared_preload_libraries = 'foo,bar'; Results in: shared_preload_libraries = '"foo,bar"'; Which doesn't parse out into separate list items and therefore obviously fails. I think from an ALTER SYSTEM perspective this is an issue, as I would expect to be able to set these types of parameters to any valid value, including an empty list. I'm willing to accept that there might be something here that I am missing or not understanding about how this should work, but this doesn't seem right. Thoughts? -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] security labels on databases are bad for dump & restore
>> 1. "pg_dumpall -g" >> 2. "pg_dump --create" per database > > Gah, OK, I see your point. But we better document this, because if > you need a PhD in PostgreSQL-ology to take a backup, we're not in a > good place. Agreed. Though, honestly, I find this to be a cumbersome approach. I think it just makes things more confusing, even if it is well documented. Perhaps it might be necessary as a bridge to get to a better place. But my first question as an end user would be, 'why can't one tool do this?'. Also, by using 'pg_dumpall -g' aren't you potentially getting things that you don't want/need/care about? For instance, if database 'foo' is owned by 'user1' and database 'bar' is owned by 'user2' and neither have any knowledge/relation of/to the other, then when I dump 'foo', in this manner, wouldn't I also be including 'user2'? Said differently, a restore of a 'foo'-only dump would also include a 'bar' related role. That seems like a bad idea, IMHO. Maybe it can't be avoided, but I'd expect that only relevant information for the database being dumped would be included. -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] security labels on databases are bad for dump & restore
> While I'd favor optional --no-create if we were designing fresh, it's not > worth breaking user scripts by changing that now. Agreed. So, --create would not be enabled by default. >> How would this handle related global objects? It seems like this part >> could get a little tricky. > > Like roles and tablespaces? No need to change their treatment. Yes, those. Ok. -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] security labels on databases are bad for dump & restore
> I don't think there's any line near pg_dumpall. That tool seems to > have grown out of desperation without much actual design. I think it > makes more sense to plan around that's the best pg_dump behavior for the > various use cases. Ok. > I like Noah's proposal of having pg_dump --create reproduce all > database-level state. Should it be enabled by default? If so, then wouldn't it make more sense to call it --no-create and do the opposite? So, --no-create would exclude rather than include database-level information? Would enabling it by default cause issues with the current expected use of the tool by end users? How would this handle related global objects? It seems like this part could get a little tricky. Taking it one step further, would a --all option that dumps all databases make sense as well? Of course I know that's probably a considerable undertaking and certainly beyond the current scope. Though, I thought I'd throw it out there. Also, I think this would potentially conflict with what FabrÃzio is doing with CURRENT_DATABASE on COMMENT, though, I think it might be a preferable solution. https://commitfest.postgresql.org/5/229/ -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] Unnecessary #include in objectaddress.h?
> I wondered whether to bother about this kind of thing for a while. It > doesn't have any practical impact immediately, because obviously > pg_list.h is still included indirectly by objectaddress.h (via lock.h in > this case IIRC). If we made some restructuring that caused the other > header not to include pg_list.h anymore, that would make objectaddress.h > broken -- unless objectaddress.h itself no longer needed pg_list.h. > > We've had in previous rounds whole iterations on a "pgrminclude" script > that does this kind of thing, but the breakage after each such run is > large. > > All in all, I wouldn't bother unless there is an actual change. Understood. Thanks. -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] security labels on databases are bad for dump & restore
> Consistency with existing practice would indeed have pg_dump ignore > pg_shseclabel and have pg_dumpall reproduce its entries. I think that makes sense, but what about other DATABASE level info such as COMMENT? Should that also be ignored by pg_dump as well? I'm specifically thinking of the discussion from the following thread: http://www.postgresql.org/message-id/20150317172459.gm3...@alvh.no-ip.org If COMMENT is included then why not SECURITY LABEL or others? > In a greenfield, I would make "pg_dump --create" reproduce pertinent entries > from datacl, pg_db_role_setting, pg_shseclabel and pg_shdescription. I would > make non-creating pg_dump reproduce none of those. I think the bigger question is "Where is the line drawn between pg_dump and pg_dumpall?". At what point does one tool become the other? -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
[HACKERS] Unnecessary #include in objectaddress.h?
All, While looking at the include dependency graph for objectaddress.h: http://doxygen.postgresql.org/objectaddress_8h.html I saw that pg_list.h is both included and inherited (through multiple paths) by objectaddress.h. Perhaps this doesn't matter, but I thought I would at least bring it up and propose removing this redundant #include from objectaddress.h. If it makes sense to do so, I have attached a patch that removes it. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com diff --git a/src/include/catalog/objectaddress.h b/src/include/catalog/objectaddress.h new file mode 100644 index 37808c0..432cbe8 *** a/src/include/catalog/objectaddress.h --- b/src/include/catalog/objectaddress.h *** *** 13,19 #ifndef OBJECTADDRESS_H #define OBJECTADDRESS_H - #include "nodes/pg_list.h" #include "storage/lock.h" #include "utils/acl.h" #include "utils/relcache.h" --- 13,18 -- 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] security labels on databases are bad for dump & restore
All, >> I won't have time to do anything about this anytime soon, but I think we >> should fix that at some point. Shall I put this on the todo? Or do we >> want to create an 'open items' page that's not major version specific? > > I think adding it to the TODO would be great. I'd be willing to look/dive into this one further. -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] One question about security label command
Stephen, > Stephen, would you have the time to review this patch, and commit if > appropriate, please? And if you could set up the buildfarm animal to run > this, even better. I gave this a quick review/test against master (0a0fe2f). Everything builds and installs as would be expected. All of the context changes make sense to me. However, I am still seeing some errors in the regression tests. The errors look like they are solely based around log messages and not 'functionality'. I have attached the 'regression.diffs' for reference. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com regression.diffs 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] [RFC] sepgsql: prohibit users to relabel objects
> > Really? Why? I would think it's the policy's job to restrict relabel > operations. > I agree. This seems like an unnecessary change. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Bug fix for missing years in make_date()
> > Good point. Next patch attached. /* - * Note: we'll reject zero or negative year values. Perhaps negatives - * should be allowed to represent BC years? + * Note: Non-positive years are taken to be BCE. */ Previously, zero was rejected, what does it do now? I'm sure it represents 0 AD/CE, however, is that important enough to note given that it was not allowed previously? -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] One question about security label command
>
> The attached patch fixes the policy module of regression test.
> However, I also think we may stop to rely permission set of pre-defined
> selinux domains. Instead of pre-defined one, sepgsql-regtest.te may be
> ought to define own domain with appropriate permission set independent
> from the base selinux-policy version.
>
I have applied this patch and ran the tests. All seems to work except that
I have a minor error in the 'label' regression tests. It is simply a
result order issue, modifying the expected order in my environment resolves
the issue. I have attached the 'regression.diffs' for reference as well,
FWIW, I have also attached a patch that corrects this issue for me,
hopefully it is useful.
-Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/contrib/sepgsql/expected/label.out b/contrib/sepgsql/expected/label.out
new file mode 100644
index 9d1f904..d41eb48
*** a/contrib/sepgsql/expected/label.out
--- b/contrib/sepgsql/expected/label.out
*** SELECT objtype, objname, label FROM pg_s
*** 82,106
(3 rows)
SELECT objtype, objname, label FROM pg_seclabels
! WHERE provider = 'selinux' AND objtype = 'column' AND (objname like 't3.%' OR objname like 't4.%');
objtype | objname | label
-+-+---
- column | t3.t| unconfined_u:object_r:user_sepgsql_table_t:s0
- column | t3.s| unconfined_u:object_r:user_sepgsql_table_t:s0
- column | t3.ctid | unconfined_u:object_r:user_sepgsql_table_t:s0
- column | t3.xmin | unconfined_u:object_r:user_sepgsql_table_t:s0
- column | t3.cmin | unconfined_u:object_r:user_sepgsql_table_t:s0
- column | t3.xmax | unconfined_u:object_r:user_sepgsql_table_t:s0
column | t3.cmax | unconfined_u:object_r:user_sepgsql_table_t:s0
column | t3.tableoid | unconfined_u:object_r:user_sepgsql_table_t:s0
! column | t4.n| unconfined_u:object_r:sepgsql_table_t:s0
! column | t4.m| unconfined_u:object_r:sepgsql_table_t:s0
! column | t4.ctid | unconfined_u:object_r:sepgsql_sysobj_t:s0
! column | t4.xmin | unconfined_u:object_r:sepgsql_sysobj_t:s0
! column | t4.cmin | unconfined_u:object_r:sepgsql_sysobj_t:s0
! column | t4.xmax | unconfined_u:object_r:sepgsql_sysobj_t:s0
column | t4.cmax | unconfined_u:object_r:sepgsql_sysobj_t:s0
column | t4.tableoid | unconfined_u:object_r:sepgsql_sysobj_t:s0
(16 rows)
--
--- 82,107
(3 rows)
SELECT objtype, objname, label FROM pg_seclabels
! WHERE provider = 'selinux' AND objtype = 'column' AND (objname like 't3.%' OR objname like 't4.%')
! ORDER BY objname ASC;
objtype | objname | label
-+-+---
column | t3.cmax | unconfined_u:object_r:user_sepgsql_table_t:s0
+ column | t3.cmin | unconfined_u:object_r:user_sepgsql_table_t:s0
+ column | t3.ctid | unconfined_u:object_r:user_sepgsql_table_t:s0
+ column | t3.s| unconfined_u:object_r:user_sepgsql_table_t:s0
+ column | t3.t| unconfined_u:object_r:user_sepgsql_table_t:s0
column | t3.tableoid | unconfined_u:object_r:user_sepgsql_table_t:s0
! column | t3.xmax | unconfined_u:object_r:user_sepgsql_table_t:s0
! column | t3.xmin | unconfined_u:object_r:user_sepgsql_table_t:s0
column | t4.cmax | unconfined_u:object_r:sepgsql_sysobj_t:s0
+ column | t4.cmin | unconfined_u:object_r:sepgsql_sysobj_t:s0
+ column | t4.ctid | unconfined_u:object_r:sepgsql_sysobj_t:s0
+ column | t4.m| unconfined_u:object_r:sepgsql_table_t:s0
+ column | t4.n| unconfined_u:object_r:sepgsql_table_t:s0
column | t4.tableoid | unconfined_u:object_r:sepgsql_sysobj_t:s0
+ column | t4.xmax | unconfined_u:object_r:sepgsql_sysobj_t:s0
+ column | t4.xmin | unconfined_u:object_r:sepgsql_sysobj_t:s0
(16 rows)
--
diff --git a/contrib/sepgsql/sql/label.sql b/contrib/sepgsql/sql/label.sql
new file mode 100644
index 7a05c24..f4d50c3
*** a/contrib/sepgsql/sql/label.sql
--- b/contrib/sepgsql/sql/label.sql
*** INSERT INTO t4 VALUES (1,'mmm'), (2,'nnn
*** 78,84
SELECT objtype, objname, label FROM pg_seclabels
WHERE provider = 'selinux' AND objtype = 'table' AND objname in ('t1', 't2', 't3');
SELECT objtype, objname, label FROM pg_seclabels
! WHERE provider = 'selinux' AND objtype = 'column' AND (objname like 't3.%' OR objname like 't4.%');
--
-- Tests for SECURITY LABEL
--- 78,85
SELECT objtype, objname, label FROM pg_se
Re: [HACKERS] CATUPDATE confusion?
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?
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?
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] Additional role attributes && superuser review
Alvaro, I thought I saw a comment about using underscore to separate words in > privilege names, such as EXCLUSIVE_BACKUP rather than running it all > together. Was that idea discarded? > I'm not sure there was an actual discussion on the topic. Though, at one point I had proposed it as one of the forms of this attribute. Personally, I think it is easier to read with the underscore. But, ultimately, I defaulted to no underscore to remain consistent with the other attributes, such as CREATEDB and CREATEROLE. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Raspberry PI vs Raspberry PI 2: time to compile backend code
Michael, > This is a purely informational email... > I have put my hands on a Raspberry PI 2, and I have found that it takes 6 > minutes to compile the backend code using the 4 cores of the ARMv7 > processor, and close to 20 minutes on a single core (without ccache). The > test has been done using ArchLinux ARM with a micro SD card of class 10 > (said to be able to manage 50MB/s in write, 60MB/s in read). > In comparison, the buildfarm machine hamster, which is a Rasp PI 1, takes > close to 2 hours to do the same operation, using the same OS and a similar > class 10 card (30MB/s but the I/O is not the bottleneck). > Thanks for sharing this info. I'm looking forward to getting my hands on an rpi2 soon for some of my other projects. Having an idea of the increased performance, especially related to compilation, was certainly something I was curious about. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] CATUPDATE confusion?
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] Additional role attributes && superuser review
All,
I have attached and updated patch for review.
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 62305d2..fd4b9ab
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
***
*** 1445,1450
--- 1445,1486
+ rolbypassrls
+ bool
+ Role can bypass row level security
+
+
+
+ rolexclbackup
+ bool
+ Role can perform on-line exclusive backup operations
+
+
+
+ rolxlogreplay
+ bool
+ Role can control xlog recovery replay operations
+
+
+
+ rollogfile
+ bool
+ Role can rotate log files
+
+
+
+ rolmonitor
+ bool
+ Role can view pg_stat_* details
+
+
+
+ rolsignal
+ bool
+ Role can signal backend processes
+
+
+
rolconnlimit
int4
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
new file mode 100644
index d57243a..096c770
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT set_config('log_statement_stats',
*** 16425,16438
pg_start_backup(label text , fast boolean )
pg_lsn
!Prepare for performing on-line backup (restricted to superusers or replication roles)
pg_stop_backup()
pg_lsn
!Finish performing on-line backup (restricted to superusers or replication roles)
--- 16425,16438
pg_start_backup(label text , fast boolean )
pg_lsn
!Prepare for performing on-line exclusive backup (restricted to superusers or replication roles)
pg_stop_backup()
pg_lsn
!Finish performing on-line exclusive backup (restricted to superusers or replication roles)
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
new file mode 100644
index ea26027..0fc3b42
*** a/doc/src/sgml/ref/create_role.sgml
--- b/doc/src/sgml/ref/create_role.sgml
*** CREATE ROLE connlimit
| [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password'
| VALID UNTIL 'timestamp'
*** CREATE ROLE connlimit
diff --git a/doc/src/sgml/ref/create_user.sgml b/doc/src/sgml/ref/create_user.sgml
new file mode 100644
index 065999c..f7f10c7
*** a/doc/src/sgml/ref/create_user.sgml
--- b/doc/src/sgml/ref/create_user.sgml
*** CREATE USER connlimit
| [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password'
| VALID UNTIL 'timestamp'
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
new file mode 100644
index 2179bf7..12b8a17
*** 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,63
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")));
startpoint = do_pg_start_backup(backupidstr, fast, NULL, NULL);
--- 55,65
backupidstr = text_to_cstring(backupid);
! if (!has_replication_privilege(GetUserId())
! && !has_exclbackup_privilege(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("must be superuser, replication role or exclusive backup role to run a backup")));
startpoint = do_pg_start_backup(backupidstr, fast, NULL, NULL);
*** pg_stop_backup(PG_FUNCTION_ARGS)
*** 82,91
{
XLogRecPtr stoppoint;
! if (!superuser() && !has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! (errmsg("must be superuser or replication role to run a backup";
stoppoint = do_pg_stop_backup(NULL, true, NULL);
--- 84,94
{
XLogRecPtr stoppoint;
! if (!has_replication_privilege(GetUserId())
! && !has_exclbackup_privilege(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("must be superuser, replication role or exclusive backup role to run a backup")));
stoppoint = do_pg_stop_backup(NULL, true, NULL);
*** pg_switch_xlog(
Re: [HACKERS] Additional role attributes && superuser review
All, > > I'm slightly mystified as to how including the word "online" helps > here. It's unlikely that there will be an offline_backup permission, > because if the system is off-line, SQL-level permissions are > irrelevant. After re-reading through this thread is seems like EXCLUSIVEBACKUP (proposed by Magnus) seemed to be a potentially acceptable alternative. Relevant post: http://www.postgresql.org/message-id/cabuevez7bz0r85vut4rvxx0jkpih8hp8toqzgvpufl0kvcv...@mail.gmail.com We need to separate the logical backups (pg_dump) from the physical ones (start/stop+filesystem and pg_basebackup). We might also need to separate the two different ways of doing physical backups. Personalyl I think using the DUMP name makes that a lot more clear. Maybe we need to avoid using BACKUP alone as well, to make sure it doesn't go the other way - using BASEBACKUP and EXCLUSIVEBACKUP for those two different ones perhaps? Relevant post: http://www.postgresql.org/message-id/20141231144610.gs3...@tamriel.snowman.net I think I'm coming around to the EXCLUSIVEBACKUP option, per the discussion with Magnus. I don't particularly like LOGBACKUP and don't think it really makes sense, while PHYSBACKUP seems like it'd cover what REPLICATION already does. Thoughts? -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] CATUPDATE confusion?
> > * 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] Additional role attributes && superuser review
Robert, Thanks for the feedback. I'm slightly mystified as to how including the word "online" helps > here. It's unlikely that there will be an offline_backup permission, > because if the system is off-line, SQL-level permissions are > irrelevant. I'm certainly open to recommendations on this one. Initially, BACKUP was proposed, but based on the discussion, it is unacceptable. As mentioned, the documentation for the affected functions refer to starting/stopping an 'on-line backup', hence the current proposal. I feel like it is obviously more in line with the documentation and removes the ambiguity in what 'type' of backup it allows, as that seemed to be one of the major concerns of just using BACKUP. However, I could certainly understand if there was a confusion on the terminology of 'online' vs 'offline' if those are not regularly used terms or concepts. At any rate, I'll certainly continue to give this one thought, but I wouldn't mind any recommendations/suggestions anyone was willing to throw my way. > * LOG - allows role to rotate log files - remains broad enough to consider > > future log related operations > > Maybe LOGFILE? Only because some confusion with the LOG message level > seems possible; or confusion about whether this is a permission that > lets you log things. That's a good point. I'll change this one up. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Additional role attributes && superuser review
All,
Attached is a patch that proposes the following additional role attributes
for review:
* ONLINE_BACKUP - allows role to perform backup operations
- originally proposed as BACKUP - due to concern for the use of that term
in relation to other potential backup related permissions this form is in
line with the documentation as it describes the affected backup operations
as being 'online backups'.
- applies only to the originally proposed backup functions.
* XLOG_REPLAY - allows role to perform pause and resume on xlog_replay
operations ('pg_xlog_replay_pause' and 'pg_xlog_replay_resume')
- following the recommendation from Stephen and Magnus.
* LOG - allows role to rotate log files - remains broad enough to consider
future log related operations
* MONITOR - allows role to view pg_stat_* details (as originally proposed)
* SIGNAL - allows role to signal backend processes (as originally proposed)
The documentation still needs to be updated. If this these attributes and
the capabilities they provide are acceptable, then I'll begin moving
forward on making those updates as well.
Regarding the discussion on a DUMP/READONLY permission. I believe that
topic needs much further discussion and decided it is probably best to keep
it as a separate patch/effort. I'd certainly be willing to continue that
discussion and assist in moving any related effort forward, therefore,
please let me know if there is anything I can do to help.
Thanks,
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 2179bf7..aaf13c1
*** 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,63
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")));
startpoint = do_pg_start_backup(backupidstr, fast, NULL, NULL);
--- 55,65
backupidstr = text_to_cstring(backupid);
! if (!has_replication_privilege(GetUserId())
! && !has_online_backup_privilege(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("must be superuser, replication role or online backup role to run a backup")));
startpoint = do_pg_start_backup(backupidstr, fast, NULL, NULL);
*** pg_stop_backup(PG_FUNCTION_ARGS)
*** 82,91
{
XLogRecPtr stoppoint;
! if (!superuser() && !has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! (errmsg("must be superuser or replication role to run a backup";
stoppoint = do_pg_stop_backup(NULL, true, NULL);
--- 84,94
{
XLogRecPtr stoppoint;
! if (!has_replication_privilege(GetUserId())
! && !has_online_backup_privilege(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("must be superuser, replication role or online backup role to run a backup")));
stoppoint = do_pg_stop_backup(NULL, true, NULL);
*** pg_switch_xlog(PG_FUNCTION_ARGS)
*** 100,109
{
XLogRecPtr switchpoint;
! if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! (errmsg("must be superuser to switch transaction log files";
if (RecoveryInProgress())
ereport(ERROR,
--- 103,112
{
XLogRecPtr switchpoint;
! if (!has_online_backup_privilege(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("must be superuser or online backup role to switch transaction log files")));
if (RecoveryInProgress())
ereport(ERROR,
*** pg_create_restore_point(PG_FUNCTION_ARGS
*** 129,138
char *restore_name_str;
XLogRecPtr restorepoint;
! if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! (errmsg("must be superuser to create a restore point";
if (RecoveryInProgress())
ereport(ERROR,
--- 132,141
char *restore_name_str;
XLogRecPtr restorepoint;
! if (!has_online_backup_privilege(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! (errmsg("must be superuser or online backup role to c
Re: [HACKERS] Additional role attributes && superuser review
On Mon, Jan 5, 2015 at 11:49 AM, Robert Haas wrote: > On Wed, Dec 24, 2014 at 12:48 PM, Adam Brightwell > wrote: > > * BACKUP - allows role to perform backup operations > > * LOGROTATE - allows role to rotate log files > > * MONITOR - allows role to view pg_stat_* details > > * PROCSIGNAL - allows role to signal backend processes > > How about just "SIGNAL" instead of "PROCSIGNAL"? > Sure. > Generally, I think we'll be happier if these capabilities have names > that are actual words - or combinations of words - rather than partial > words, so I'd suggest avoiding things like PROC for PROCESS and AUTH > for AUTHORIZATION. > Agreed. > In this particular case, it seems like the name of the capability is > based off the name of an internal system data structure. That's the > sort of thing that we do not want to expose to users. As far as we > can, we should try to describe what the capability allows, not the > details of how that is (currently) implemented. Agreed. If others are also in agreement on this point then I'll update the patch accordingly. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] CATUPDATE confusion?
All,
On Sat, Dec 27, 2014 at 6:31 PM, Noah Misch wrote:
> On Sat, Dec 27, 2014 at 06:26:02PM -0500, Tom Lane wrote:
> > Stephen Frost 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
- rolcatupdate
- bool
-
-Role can update system catalogs directly. (Even a superuser cannot do
-this unless this column is true)
-
-
-
-
rolcanlogin
bool
--- 1416,1421
*** SELECT * FROM pg_locks pl LEFT JOIN pg_p
*** 8479,8494
- rolcatupdate
- bool
-
-
-Role can update system catalogs directly. (Even a superuser cannot do
-this unless this column is true)
-
-
-
-
rolcanlogin
bool
--- 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] Additional role attributes && superuser review
Magnus, Thanks for the feedback. >> BACKUP - allows role to perform pg_dump* backups of whole database. >> > > I'd suggest it's called DUMP if that's what it allows, to keep it separate > from the backup parts. > Makes sense to me. That seems really bad names, IMHO. Why? Because we use WAL and XLOG > throughout documentation and parameters and code to mean *the same thing*. > And here they'd suddenly mean different things. If we need them as separate > privileges, I think we need much better names. (And a better description - > what is "xlog operations" really?) > Fair enough, ultimately what I was trying to address is the following concern raised by Alvaro: "To me, what this repeated discussion on this particular BACKUP point says, is that the ability to run pg_start/stop_backend and the xlog related functions should be a different privilege, i.e. something other than BACKUP; because later we will want the ability to grant someone the ability to run pg_dump on the whole database without being superuser, and we will want to use the name BACKUP for that. So I'm inclined to propose something more specific for this like WAL_CONTROL or XLOG_OPERATOR, say." Upon re-reading it (and other discussions around it) I believe that I must have misinterpreted. Initially, I read it to mean that we needed the following separate permissions. 1) ability to pg_dump 2) ability to start/stop backups 3) ability to execute xlog related functions When indeed, what it meant was to have the following separate (effectively merging #2 and #3): 1) ability to pg_dump 2) ability to start/stop backups *and* ability to execute xlog related functions. My apologies on that confusion. Given this clarification: I think #1 could certainly be answered by using DUMP. I have no strong opinion in either direction, though I do think that BACKUP does make the most sense for #2. Previously, Stephen had mentioned a READONLY capability that could effectively work for pg_dump, though, Jim's suggestion of keeping 'read-all' separate from 'ability to pg_dump' seems logical. In either case, I certainly wouldn't mind having a wider agreement/consensus on this approach. So, here is a revised list of proposed attributes: * BACKUP - allows role to perform backup operations (as originally proposed) * LOG - allows role to rotate log files - remains broad enough to consider future log related operations * DUMP - allows role to perform pg_dump* backups of whole database * MONITOR - allows role to view pg_stat_* details (as originally proposed) * PROCSIGNAL - allows role to signal backend processes (as originally proposed)well Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Additional role attributes && superuser review
All, I want to revive this thread and continue to move these new role attributes forward. In summary, the ultimate goal is to include new role attributes for common operations which currently require superuser privileges. Initially proposed were the following attributes: * BACKUP - allows role to perform backup operations * LOGROTATE - allows role to rotate log files * MONITOR - allows role to view pg_stat_* details * PROCSIGNAL - allows role to signal backend processes It seems that PROCSIGNAL and MONITOR were generally well received and probably don't warrant much more discussion at this point. However, based on previous discussions, there seemed to be some uncertainty on how to handle BACKUP and LOGROTATE. Concerns: * LOGROTATE - only associated with one function/operation. * BACKUP - perceived to be too broad of a permission as it it would provide the ability to run pg_start/stop_backend and the xlog related functions. It is general sentiment is that these should be handled as separate privileges. * BACKUP - preferred usage is with pg_dump to giving a user the ability to run pg_dump on the whole database without being superuser. Previous Recommendations: * LOGROTATE - Use OPERATOR - concern was expressed that this might be too general of an attribute for this purpose. Also, concern for privilege 'upgrades' as it includes more capabilities in later releases. * LOGROTATE - Use LOG_OPERATOR - generally accepted, but concern was raise for using extraneous descriptors such as '_OPERATOR' and '_ADMIN', etc. * BACKUP - Use WAL_CONTROL for pg_start/stop_backup - no major disagreement, though same concern regarding extraneous descriptors. * BACKUP - Use XLOG_OPERATOR for xlog operations - no major disagreement, though same concern regarding extraneous descriptors. * BACKUP - Use BACKUP for granting non-superuser ability to run pg_dump on whole database. Given the above and previous discussions: I'd like to propose the following new role attributes: BACKUP - allows role to perform pg_dump* backups of whole database. WAL - allows role to execute pg_start_backup/pg_stop_backup functions. XLOG - allows role to execute xlog operations. LOG - allows role to rotate log files - remains broad enough to consider future log related operations. MONITOR - allows role to view pg_stat_* details. PROCSIGNAL - allows role to signal backend processes. If these seem reasonable, then I'll begin updating the initial/current patch submitted. But in either case, feedback and suggestions are certainly welcome and appreciated. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Alvarao, > Pushed with a couple of small changes (Catalog.pm complained about the > lack of a CATALOG() line in the new acldefs.h file; you had > pg_role_all_attributes as returning "bool" in the table, but it returns > text[]; and I added index entries for the new functions.) That's fantastic! Thanks, I appreciate it! -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Alvaro and Stephen, I propose this patch on top of Adam's v5. Also included is a full patch > against master. > I have attached an updated patch for review (role-attribute-bitmask-v7.patch). This patch incorporates the 'v5a' patch proposed by Alvaro, input validation (Assert) check in 'check_role_attribute' and the documentation updates requested by Stephen. 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..9470916 *** a/doc/src/sgml/catalogs.sgml --- b/doc/src/sgml/catalogs.sgml *** *** 1391,1479 ! rolsuper ! bool Role has superuser privileges ! rolinherit ! bool ! Role automatically inherits privileges of roles it is a !member of ! rolcreaterole ! bool Role can create more roles ! rolcreatedb ! bool Role can create databases ! rolcatupdate ! bool Role can update system catalogs directly. (Even a superuser cannot do this unless this column is true) ! rolcanlogin ! bool Role can log in. That is, this role can be given as the initial session authorization identifier ! rolreplication ! bool Role is a replication role. That is, this role can initiate streaming replication (see ) and set/unset the system backup mode using pg_start_backup and pg_stop_backup ! rolconnlimit ! int4 ! !For roles that can log in, this sets maximum number of concurrent !connections this role can make. -1 means no limit. ! ! ! ! ! rolpassword ! text !Password (possibly encrypted); null if none. If the password !is encrypted, this column will begin with the string md5 !followed by a 32-character hexadecimal MD5 hash. The MD5 hash !will be of the user's password concatenated to their user name. !For example, if user joe has password xyzzy, !PostgreSQL will store the md5 hash of !xyzzyjoe. A password that does not follow that !format is assumed to be unencrypted. - - rolvaliduntil - timestamptz - Password expiry time (only used for password authentication); -null if no expiration - --- 1391,1524 ! rolattr ! bigint ! !Role attributes; see and ! for details ! ! ! ! ! rolconnlimit ! int4 ! !For roles that can log in, this sets maximum number of concurrent !connections this role can make. -1 means no limit. ! ! ! ! ! rolpassword ! text ! !Password (possibly encrypted); null if none. If the password !is encrypted, this column will begin with the string md5 !followed by a 32-character hexadecimal MD5 hash. The MD5 hash !will be of the user's password concatenated to their user name. !For example, if user joe has password xyzzy, !PostgreSQL will store the md5 hash of !xyzzyjoe. A password that does not follow that !format is assumed to be unencrypted. ! ! ! ! ! rolvaliduntil ! timestamptz ! Password expiry time (only used for password authentication); !null if no expiration ! ! ! ! ! ! !Attributes in rolattr ! ! ! ! ! Attribute ! CREATE ROLE Option ! Description ! Position ! ! ! ! ! ! Superuser ! SUPERUSER Role has superuser privileges + 0 ! Inherit ! INHERIT ! !Role automatically inherits privileges of roles it is a member of ! ! 1 ! Create Role ! CREATEROLE Role can create more roles + 2 ! Create DB ! CREATEDB Role can create databases + 3 ! Catalog Update ! CATUPDATE Role can update system catalogs directly. (Even a superuser cannot do this unless this column is true) + 4 ! Can Login ! LOGIN Role can log in. That is, this role can be given as the initial session authorization identifier + 5 ! Replication ! REPLICATION Role is a replication role. That is, this role can initiate
Re: [HACKERS] postgres messages error
Martin,
#: libpq/auth.c:1593
> #, fuzzy, c-format
> msgid "could not to look up local user ID %ld: %s"
>
> It looks like there is an extra *to* there , so the string should be:
>
> "could not look up local user ID %ld: %s"
>
I think you are right. FWIW, I have attached a patch that fixes it for
consideration if others concur.
-Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
new file mode 100644
index 9ad99ce..2b2dbb3
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
*** auth_peer(hbaPort *port)
*** 1590,1596
if (!pw)
{
ereport(LOG,
! (errmsg("could not to look up local user ID %ld: %s",
(long) uid, errno ? strerror(errno) : _("user does not exist";
return STATUS_ERROR;
}
--- 1590,1596
if (!pw)
{
ereport(LOG,
! (errmsg("could not look up local user ID %ld: %s",
(long) uid, errno ? strerror(errno) : _("user does not exist";
return STATUS_ERROR;
}
--
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] Role Attribute Bitmask Catalog Representation
All, Overall, I'm pretty happy with the patch and would suggest moving on to > writing up the documentation changes to go along with the code changes. > I'll continue to play around with it but it all seems pretty clean to > me and will allow us to easily add the additiaonl role attributes being > discussed. > I have attached an updated patch with initial documentation changes for review. 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..b0b4fca *** a/doc/src/sgml/catalogs.sgml --- b/doc/src/sgml/catalogs.sgml *** *** 1391,1441 ! rolsuper ! bool Role has superuser privileges ! rolinherit ! bool ! Role automatically inherits privileges of roles it is a !member of ! rolcreaterole ! bool Role can create more roles ! rolcreatedb ! bool Role can create databases ! rolcatupdate ! bool !Role can update system catalogs directly. (Even a superuser cannot do !this unless this column is true) ! rolcanlogin ! bool !Role can log in. That is, this role can be given as the initial !session authorization identifier ! rolreplication ! bool Role is a replication role. That is, this role can initiate streaming replication (see ) and set/unset --- 1391,1493 ! rolattr ! bigint ! !Role attributes; see for details ! ! ! ! ! rolconnlimit ! int4 ! !For roles that can log in, this sets maximum number of concurrent !connections this role can make. -1 means no limit. ! ! ! ! ! rolpassword ! text ! !Password (possibly encrypted); null if none. If the password !is encrypted, this column will begin with the string md5 !followed by a 32-character hexadecimal MD5 hash. The MD5 hash !will be of the user's password concatenated to their user name. !For example, if user joe has password xyzzy, !PostgreSQL will store the md5 hash of !xyzzyjoe. A password that does not follow that !format is assumed to be unencrypted. ! ! ! ! ! rolvaliduntil ! timestamptz ! Password expiry time (only used for password authentication); !null if no expiration ! ! ! ! ! ! !rolattr bitmap positions ! ! ! ! ! Position ! Attribute ! Description ! ! ! ! ! ! 0 ! Superuser Role has superuser privileges ! 1 ! Inherit ! Role automatically inherits privileges of roles it is a member of ! 2 ! Create Role Role can create more roles ! 3 ! Create DB Role can create databases ! 4 ! Catalog Update !Role can update system catalogs directly. (Even a superuser cannot do this unless this column is true) ! 5 ! Can Login !Role can log in. That is, this role can be given as the initial session authorization identifier ! 6 ! Replication Role is a replication role. That is, this role can initiate streaming replication (see ) and set/unset *** *** 1445,1479 ! rolconnlimit ! int4 ! !For roles that can log in, this sets maximum number of concurrent !connections this role can make. -1 means no limit. ! ! ! ! ! rolpassword ! text !Password (possibly encrypted); null if none. If the password !is encrypted, this column will begin with the string md5 !followed by a 32-character hexadecimal MD5 hash. The MD5 hash !will be of the user's password concatenated to their user name. !For example, if user joe has password xyzzy, !PostgreSQL will store the md5 hash of !xyzzyjoe. A password that does not follow that !format is assumed to be unencrypted. - - rolvaliduntil - timestamptz - Password expiry time (only used for password authentication); -null if no expiration - --- 1497,1509 ! 7 ! By-pass Row Level Security !Role can by-pass row level security po
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Michael, > > This work will certainly continue to be pursued. If a simple move is > > possible/acceptable, then I think that would be the best option. I can > > handle that as it would appear that I am capable of moving it, if that is > > acceptable. > Yes please. Actually I could have done it, just found the option to do > so. Let's see what shows up with your work. > I have moved it to commitfest 2014-12 and marked as "Waiting on Author" if that is acceptable. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Michael, This patch (https://commitfest.postgresql.org/action/patch_view?id=1616) > has not been updated in the commitfest app for two months, making its > progress hard to track. I believe that the mentioned patch should be considered 'on hold' or 'dependent' upon the acceptance of the work that is being done in this thread. Also, the changes proposed by this thread have already been added to the next commitfest (https://commitfest.postgresql.org/action/patch_view?id=1651 ). However, even if some progress has been made > things are still not complete (documentation, etc.). Opinions about > marking that as returned with feedback for the current commit fest and > create a new entry for the next CF if this work is going to be > pursued? I guess that it would be fine simply move it to the next CF, but it > seems I cannot do that myself in the CF app. > This work will certainly continue to be pursued. If a simple move is possible/acceptable, then I think that would be the best option. I can handle that as it would appear that I am capable of moving it, if that is acceptable. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Stephen, The comment above is pretty big and I don't think we want to completely > remove it. Can you add it as another 'Note' in the 'has_role_attribute' > comment and reword it accordingly? > Ok, agreed. Will address. Whitespace issue that should be fixed- attributes should line up with > roleid. > Ok. Will address. It occurs to me that in this case (and a few others..), we're doing a > lot of extra work- each call to pg_check_role_attribute() is doing a > lookup on the oid just to get back to what the rolattr value on this row > is. How about a function which takes rolattr and the text > representation of the attribute and returns if the bit is set for that > rolattr value? Then you'd pass pg_authid.rolattr into the function > calls above instead of the role's oid. > Makes sense, I'll put that together. > I don't see any changes to the regression test files, were they > forgotten in the patch? I would think that at least the view definition > changes would require updates to the regression tests, though perhaps > nothing else. > Hmmm... :-/ The regression tests that changed were in 'src/test/regress/expected/rules.out' and should be near the bottom of the patch. > Overall, I'm pretty happy with the patch and would suggest moving on to > writing up the documentation changes to go along with the code changes. > I'll continue to play around with it but it all seems pretty clean to > me and will allow us to easily add the additiaonl role attributes being > discussed. Sounds good. I'll start on those changes next. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Stephen,
Thanks for the feedback.
> > diff --git a/src/backend/access/transam/xlogfuncs.c
> b/src/backend/access/transam/xlogfuncs.c
> > --- 56,62
> >
> > backupidstr = text_to_cstring(backupid);
> >
> > ! if (!superuser() && !check_role_attribute(GetUserId(),
> ROLE_ATTR_REPLICATION))
> > ereport(ERROR,
> > (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> > errmsg("must be superuser or replication role to run a
> backup")));
>
> The point of has_role_attribute() was to avoid the need to explicitly
> say "!superuser()" everywhere. The idea with check_role_attribute() is
> that we want to present the user (in places like pg_roles) with the
> values that are *actually* set.
>
> In other words, the above should just be:
>
> if (!has_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))
>
Yes, I understand. My original thought here was that I was replacing
'has_rolreplication' which didn't perform any superuser check and that I
was trying to adhere to minimal changes. But I agree this would be the
appropriate solution. Fixed.
>
> > + /*
> > + * check_role_attribute
> > + * Check if the role with the specified id has been assigned a
> specific
> > + * role attribute. This function does not allow any superuser
> bypass.
>
> I don't think we need to say that it doesn't "allow" superuser bypass.
> Instead, I'd comment that has_role_attribute() should be used for
> permissions checks while check_role_attribute is for checking what the
> value in the rolattr bitmap is and isn't for doing permissions checks
> directly.
>
Ok. Understood. Fixed.
> diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
> > *** pg_role_aclcheck(Oid role_oid, Oid rolei
> > *** 4602,4607
> > --- 4603,4773
> > return ACLCHECK_NO_PRIV;
> > }
> >
> > + /*
> > + * pg_has_role_attribute_id_attr
>
> I'm trying to figure out what the point of the trailing "_attr" in the
> function name is..? Doesn't seem necessary to have that for these
> functions and it'd look a bit cleaner without it, imv.
>
So, I was trying to follow what I perceived as the following convention for
these functions: pg. So, what "_attr"
represents is the second argument of the function which is the attribute to
check. I could agree that might be unnecessary, but that was my thought
process on it. At any rate, I've removed it.
> ! #define ROLE_ATTR_ALL 255 /* or (1 << N_ROLE_ATTRIBUTES) - 1 */
>
> I'd say "equals" or something rather than "or" since that kind of
> implies that it's an laternative, but we can't do that as discussed in
> the comment (which I like).
>
Agreed. Fixed.
> > ! /* role attribute check routines */
> > ! extern bool has_role_attribute(Oid roleid, RoleAttr attribute);
> > ! extern bool check_role_attribute(Oid roleid, RoleAttr attribute);
>
> With all the 'has_role_attribute(GetUserId(), ROLEATTR_BLAH)' cases, I'd
> suggest doing the same as 'superuser()' and also provide a simpler
> version: 'have_role_attribute(ROLEATTR_BLAH)' which handles doing the
> GetUserId() itself. That'd simplify quite a few of the above calls.
>
Good point. Added.
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..8475985
*** a/src/backend/access/transam/xlogfuncs.c
--- b/src/backend/access/transam/xlogfuncs.c
***
*** 22,32
--- 22,34
#include "access/xlog_internal.h"
#include "access/xlogutils.h"
#include "catalog/catalog.h"
+ #include "catalog/pg_authid.h"
#include "catalog/pg_type.h"
#include "funcapi.h"
#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")));
--- 56,62
backupidstr = text_to_cstring(ba
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
All,
I have attached an updated patch that incorporates the feedback and
recommendations provided.
Thanks,
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..ccdff09
*** a/src/backend/access/transam/xlogfuncs.c
--- b/src/backend/access/transam/xlogfuncs.c
***
*** 22,32
--- 22,34
#include "access/xlog_internal.h"
#include "access/xlogutils.h"
#include "catalog/catalog.h"
+ #include "catalog/pg_authid.h"
#include "catalog/pg_type.h"
#include "funcapi.h"
#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")));
--- 56,62
backupidstr = text_to_cstring(backupid);
! if (!superuser() && !check_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))
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";
--- 84,90
{
XLogRecPtr stoppoint;
! if (!superuser() && !check_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))
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..0b23ba0
*** 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
*** 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
--- 3610,3616
if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE)) &&
IsSystemClass(table_oid, classForm) &&
classForm->relkind != RELKIND_VIEW &&
! !has_role_attribute(roleid, ROLE_ATTR_CATUPDATE) &&
!allowSystemTableMods)
{
#ifdef ACLDEBUG
*** pg_extension_ownercheck(Oid ext_oid, Oid
*** 5051,5102
}
/*
! * Check whether specified role has CREATEROLE privilege (or is a superuser)
*
! * Note: roles do not have owners per se; instead we use this test in
! * places where an ownership-like permissions test is needed for a role.
! * Be sure to apply it to the role trying to do the operation, not the
! * role being operated on! Also note that this generally should not be
! * considered enough privilege if the target role is a superuser.
! * (We don't handle that consideration here because we want to give a
! * separate error message for such cases, so the caller has to deal with it.)
*/
bool
! 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;
}
boo
[HACKERS] check-world failure: dummy_seclabel
All, I've noticed that 'check-world' fails for dummy_seclabel after a 'clean'. I believe that in commit da34731, the EXTRA_CLEAN statement should have been removed from 'src/test/modules/dummy_seclabel/Makefile' as well. Attached is a proposed patch that addresses this issue. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com diff --git a/src/test/modules/dummy_seclabel/Makefile b/src/test/modules/dummy_seclabel/Makefile new file mode 100644 index 72049d4..d93c964 *** a/src/test/modules/dummy_seclabel/Makefile --- b/src/test/modules/dummy_seclabel/Makefile *** EXTENSION = dummy_seclabel *** 7,13 DATA = dummy_seclabel--1.0.sql REGRESS = dummy_seclabel - EXTRA_CLEAN = sql/dummy_seclabel.sql expected/dummy_seclabel.out ifdef USE_PGXS PG_CONFIG = pg_config --- 7,12 -- 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] Role Attribute Bitmask Catalog Representation
Stephen,
Thanks for this. Found time to do more of a review and have a few
> comments:
>
Thanks for taking a look at this and for the feedback.
I'd put the superuser_arg() check in role_has_attribute.
>
Ok. Though, this would affect how CATUPDATE is handled. Peter Eisentraut
previously raised a question about whether superuser checks should be
included with catupdate which led me to create the following post.
http://www.postgresql.org/message-id/cakrt6cqovt2kiykg2gff7h9k8+jvu1149zlb0extkkk7taq...@mail.gmail.com
Certainly, we could keep has_rolcatupdate for this case and put the
superuser check in role_has_attribute, but it seems like it might be worth
taking a look at whether a superuser can bypass catupdate or not. Just a
thought.
And then ditch the individual has_X_privilege() calls (I note that you
> didn't appear to add back the has_rolcatupdate() function..).
>
Ok. I had originally thought for this patch that I would try to minimize
these types of changes, though perhaps this is that opportunity previously
mentioned in refactoring those. However, the catupdate question still
remains.
> + static RoleAttr
> > + set_role_attribute(RoleAttr attributes, RoleAttr attribute)
> > + {
> > + return ((attributes & ~(0x)) | attribute);
> > + }
>
> I don't think this is doing what you think it is..? It certainly isn't
> working as expected by AlterRole(). Rather than using a function for
> these relatively simple operations, why not just have AlterRole() do:
>
> if (isX >= 0)
> attributes = (isX > 0) ? attributes | ROLE_ATTR_X
>: attributes &
> ~(ROLE_ATTR_X);
>
Yes, this was originally my first approach. I'm not recollecting why I
decided on the other route, but agree that is preferable and simpler.
> Otherwise, you'd probably need to pass a flag into set_role_attribute()
> to indicate if you're setting or unsetting the bit, or have an
> 'unset_role_attribute()' function, but all of that seems like overkill..
>
Agreed.
We don't bother with the '> 0' in any of the existing bit testing that
> goes on in the backend, so I don't think it makes sense to start now.
> Just do
>
> if (attributes & ROLE_ATTR_SUPERUSER) ... etc
>
> and be done with it.
>
Ok, easy enough.
Why not have this as 'pg_has_role_id_attribute' and then have a
> 'pg_has_role_name_attribute' also? Seems like going with the pg_
> namespace is the direction we want to go in, though I'm not inclined to
> argue about it if folks prefer has_X.
>
I have no reason for one over the other, though I did ask myself that
question. I did find it curious that in some cases there is "has_X" and
then in others "pg_has_X". Perhaps I'm not looking in the right places,
but I haven't found anything that helps to distinguish when one vs the
other is appropriate (even if it is a general rule of thumb).
Seems like you could just make temp_array be N_ROLE_ATTRIBUTES in
> size, instead of building a list, counting it, and then building the
> array from that..?
>
Yes, this is true.
Do we really need to keep has_rolinherit for any reason..? Seems
> unnecessary given it's only got one call site and it's nearly the same
> as a call to role_has_attribute() anyway. Ditto with
> has_rolreplication().
>
I really don't see any reason and as above, I can certainly do those
refactors now if that is what is desired.
Thought we were getting rid of this..?
>
> > ! #define N_ROLE_ATTRIBUTES 8 /* 1 plus the last
> 1< > ! #define ROLE_ATTR_NONE 0
> > ! #define ROLE_ATTR_ALL 255 /* All
> currently available attributes. */
>
> Or:
>
> #define ROLE_ATTR_ALL (1<
Yes, we were, however the latter causes a syntax error with initdb. :-/
> ! DATA(insert OID = 10 ( "POSTGRES" PGROLATTRALL -1 _null_ _null_));
> >
> > #define BOOTSTRAP_SUPERUSERID 10
>
> Is it actually necessary to do this substitution when the value is
> #define'd in the same .h file...? Might be something to check, if you
> haven't already.
>
Yes, I had previously checked this, I get the following error from initdb.
FATAL: invalid input syntax for integer: "ROLE_ATTR_ALL"
> + #define ROLE_ATTR_SUPERUSER (1<<0)
> > + #define ROLE_ATTR_INHERIT (1<<1)
> > + #define ROLE_ATTR_CREATEROLE(1<<2)
> > + #define ROLE_ATTR_CREATEDB (1<<3)
> > + #define ROLE_ATTR_CATUPDATE (1<<4)
> > + #define ROLE_ATTR_CANLOGIN (1<<5)
> > + #define ROLE_ATTR_REPLICATION (1<<6)
> > + #define ROLE_ATTR_BYPASSRLS (1<<7)
> > + #define N_ROLE_ATTRIBUTES 8
> > + #define ROLE_ATTR_NONE 0
>
> These shouldn't need to be here any more..?
>
No they shouldn't, not sure how I missed that.
Thanks,
Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
All,
I have attached a patch that addresses the current suggestions and
recommendations:
* Add 'get_all_role_attributes' SQL function - returns a text array
representation of the attributes from a value passed to it.
Example:
postgres=# SELECT rolname, get_all_role_attributes(rolattr) AS rolattr FROM
pg_authid;
rolname |rolattr
--+---
postgres | {Superuser,Inherit,"Create Role","Create DB","Catalog
Update",Login,Replication,"Bypass RLS"}
(1 row)
* Refactor #define's from 'parsenodes.h' to 'acl.h'
* Added #define ROLE_ATTR_ALL to represent all currently available
attributes.
* Added genbki.pl substitution for PGROLEATTRALL constant.
Please let me know what you think, all feedback is greatly appreciated.
Thanks,
Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
new file mode 100644
index d30612c..93eb2e6
*** 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
*** 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
--- 3610,3616
if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE)) &&
IsSystemClass(table_oid, classForm) &&
classForm->relkind != RELKIND_VIEW &&
! !role_has_attribute(roleid, ROLE_ATTR_CATUPDATE) &&
!allowSystemTableMods)
{
#ifdef ACLDEBUG
*** pg_extension_ownercheck(Oid ext_oid, Oid
*** 5051,5056
--- 5031,5058
}
/*
+ * Check whether the specified role has a specific role attribute.
+ */
+ bool
+ role_has_attribute(Oid roleid, RoleAttr attribute)
+ {
+ RoleAttr attributes;
+ 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)));
+
+ attributes = ((Form_pg_authid) GETSTRUCT(tuple))->rolattr;
+ ReleaseSysCache(tuple);
+
+ return ((attributes & attribute) > 0);
+ }
+
+ /*
* Check whether specified role has CREATEROLE privilege (or is a superuser)
*
* Note: roles do not have owners per se; instead we use this test in
*** pg_extension_ownercheck(Oid ext_oid, Oid
*** 5064,5102
bool
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;
}
bool
has_bypassrls_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))->rolbypassrls;
! ReleaseSysCache(utup);
! }
! return result;
}
/*
--- 5066,5089
bool
has_createrole_privilege(Oid roleid)
{
/* Superusers bypass all permission checking. */
if (superuser_arg(roleid))
return true;
! return role_has_attribute(roleid, ROLE_ATTR_CREATEROLE);
}
+ /*
+ * Check whether specified role has BYPASSRLS privilege.
+ */
bool
has_bypassrls_privilege(Oid roleid)
{
/* Superusers bypass all permission checking. */
if (superuser_arg(roleid))
return true;
! return role_has_attribute(roleid, ROLE_ATTR_BYPASSRLS);
}
/*
diff --git a/src/
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Stephen, Having an array sounds pretty reasonable to me. > Ok, sounds good, I think so too. Users interested in having a string instead could use array_to_string(). > Having to go the other way isn't as nice, imo. > My thoughts exactly, but wanted to at least put it out there. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
All,
> I think if we're going to do this - and I'm not yet convinced that
>> that's the best route, we should add returns all permissions a user
>> has. Right now that's quite easily queryable, but it won't be after
>> moving everything into one column. You'd need to manually use all has_*_
>> functions... Yes, you've added them already to pg_roles, but there's
>> sometimes good reasons to go to pg_authid instead.
>>
>
> This is a good point. I'll start looking at this and see what I can come
> up with.
>
Giving this some thought, I'm curious what would be acceptable as an end
result, specifically related to how a query on pg_authid might look/work.
I was able to preserve the structure of results from pg_roles, however,
that same approach is obviously not possible with pg_authid. So, I'm
curious what the thoughts might be on how to best solve this while
minimizing impact (perhaps not possible) on users. Currently, my thought
is to have a builtin function called 'get_all_role_attributes' or similar,
that returns an array of each attribute in string form. My thoughts are
that it might look something like the following:
SELECT rolname, get_all_role_attributes(rolattr) AS attributes FROM
pg_authid;
| rolname | attributes |
+-+-+
| user1 | {Superuser, Create Role, Create DB} |
Another approach might be that the above function return a string of comma
separated attributes, similar to what \du in psql does. IMO, I think the
array approach would be more appropriate than a string but I'm willing to
accept that neither is acceptable and would certainly be interested in
opinions.
Thanks,
Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
David, > A few related threads/discussions/posts: > > > > * http://www.postgresql.org/message-id/ > > > 20141016115914.GQ28859@.snowman > > > * > > > http://www.postgresql.org/message-id/CA+TgmobkYXNOWKEKzX2qGPSr_nvacFGueV= > > > orxND-xmZvOVYvg@.gmail > > > * http://www.postgresql.org/message-id/ > > > 20141016115914.GQ28859@.snowman > > FYI: the first and third links are the same...was there another one you > meant to provide instead? > Whoops. Yes there was, but if I remember correctly it was part of that same overarching thread. The two provided, I believe are sufficient to lead to any prior relevant discussions that influenced/motivated this patch. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Andres, Thanks for the feedback. > * int64 (C) to int8 (SQL) mapping for genbki. > > That definitely should be a separate patch. Which can be committed much > earlier than the rest - even if we don't actually end up needing it for > this feature, it's still good to have it. Agreed. I had previously submitted this as a separate patch, but I think it got lost in the weeds. At any rate, here is the relevant post: http://www.postgresql.org/message-id/CAKRt6CTgJdeGFqXevrp-DizaeHmg8gNVqu8n5T=ix3javpw...@mail.gmail.com > > * replace all role attributes columns in pg_authid with single int64 > column > > named rolattr. > > * update CreateRole and AlterRole to use rolattr. > > * update all has_*_privilege functions to check rolattr. > > * builtin SQL function 'has_role_attribute' that takes a role oid and > text > > name of the attribute as input and returns a boolean. > > I think if we're going to do this - and I'm not yet convinced that > that's the best route, we should add returns all permissions a user > has. Right now that's quite easily queryable, but it won't be after > moving everything into one column. You'd need to manually use all has_*_ > functions... Yes, you've added them already to pg_roles, but there's > sometimes good reasons to go to pg_authid instead. > This is a good point. I'll start looking at this and see what I can come up with. An array representation was also suggested by Simon ( http://www.postgresql.org/message-id/ca+u5nmjgvdz6jx_ybjk99nj7mwfgfvemxtdc44lvhq64gkn...@mail.gmail.com). Obviously there are pro's and con's to either approach. I'm not married to it, but felt that a bitmask was certainly more efficient. However, I know that an array would be more extensible given that we could envision more than 64 role attributes. I'm uncertain if that is a potential reality or not, but I believe it is certainly worth considering. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
[HACKERS] CATUPDATE confusion?
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
[HACKERS] Role Attribute Bitmask Catalog Representation
All,
I am simply breaking this out into its own thread from the discussion on
additional role attributes (
http://www.postgresql.org/message-id/20141015052259.gg28...@tamriel.snowman.net
).
A few related threads/discussions/posts:
*
http://www.postgresql.org/message-id/20141016115914.gq28...@tamriel.snowman.net
*
http://www.postgresql.org/message-id/CA+TgmobkYXNOWKEKzX2qGPSr_nvacFGueV=orxnd-xmzvov...@mail.gmail.com
*
http://www.postgresql.org/message-id/20141016115914.gq28...@tamriel.snowman.net
Based on these above I have attached an initial WIP patch for review and
discussion that takes a swing at changing the catalog representation.
This patch includes:
* int64 (C) to int8 (SQL) mapping for genbki.
* replace all role attributes columns in pg_authid with single int64 column
named rolattr.
* update CreateRole and AlterRole to use rolattr.
* update all has_*_privilege functions to check rolattr.
* builtin SQL function 'has_role_attribute' that takes a role oid and text
name of the attribute as input and returns a boolean.
Items not currently addressed:
* New syntax - previous discussion indicated a potential desire for this,
but I feel more discussion needs to occur around these before proposing as
part of a patch. Specifically, how would CREATE USER/ROLE be affected? I
suppose it is OK to keep it as WITH , though if
ALTER ROLE is modified to have ADD | DROP CAPABILITY for consistency would
WITH CAPABILITY , make more sense for CREATE? I also felt these
were mutually exclusive from an implementation perspective and therefore
thought it would be best to keep them separate.
* Documentation - want to gain feedback on implementation prior to making
changes.
* Update regression tests, rules test for system_views - want to gain
feedback on approach to handling pg_roles, etc. before updating.
Thanks,
Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
new file mode 100644
index eb91c53..523b379
*** a/src/backend/catalog/Catalog.pm
--- b/src/backend/catalog/Catalog.pm
*** sub Catalogs
*** 33,38
--- 33,39
my %RENAME_ATTTYPE = (
'int16' => 'int2',
'int32' => 'int4',
+ 'int64' => 'int8',
'Oid' => 'oid',
'NameData' => 'name',
'TransactionId' => 'xid');
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
new file mode 100644
index d30612c..93eb2e6
*** 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
*** 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
--- 3610,3616
if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE)) &&
IsSystemClass(table_oid, classForm) &&
classForm->relkind != RELKIND_VIEW &&
! !role_has_attribute(roleid, ROLE_ATTR_CATUPDATE) &&
!allowSystemTableMods)
{
#ifdef ACLDEBUG
*** pg_extension_ownercheck(Oid ext_oid, Oid
*** 5051,5056
--- 5031,5058
}
/*
+ * Check whether the specified role has a specific role attribute.
+ */
+ bool
+ role_has_attribute(Oid roleid, RoleAttr attribute)
+ {
+ RoleAttr attributes;
+ 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)));
+
+ attributes = ((Form_pg_authid) GETSTRUCT(tuple))->rolattr;
+ ReleaseSysCache(tuple);
+
+ return ((attributes & attribute) > 0);
+ }
+
+ /*
* Check whether specified role has CREATEROLE privilege (or is a superuser)
*
* Note: roles do not have owners per se;
Re: [HACKERS] superuser() shortcuts
All,
> It was brought up for discussion- see
>
> http://www.postgresql.org/message-id/20141015052259.gg28...@tamriel.snowman.net
>
> Specifically:
> ---
> One curious item to note is that the
> current if(!superuser()) {} block approach has masked an inconsistency
> in at least the FDW code which required a change to the regression
> tests- namely, we normally force FDW owners to have USAGE rights on
> the FDW, but that was being bypassed when a superuser makes the call.
> I seriously doubt that was intentional. I won't push back if someone
> really wants it to stay that way though.
> ---
>
> No one mentioned any concerns with it (and three people replied), so I'm
> inclined to think it's an agreeable change. Adam probably didn't
> mention it with this patch simply because it had already been brought
> up.
Yes, this is correct, I was under the impression that this has already been
addressed. Also, I thought it is a relatively benign change and perhaps
even one for the better. With that said, I'll certainly leave it as is if
that is the consensus.
> > Which makes me wonder whether the other changes are indeed without
> > effect or just not covered by tests.
> >
> > > * has_privilege-cleanup_11-5-2014.patch
> >
> > I don't really see the merit of this patch. A "cleanup" patch that adds
> > more code than it removes isn't really a cleanup. If you want to make
> > the APIs consistent, then let's make a patch for that. If you want to
> > make the error messages consistent, then let's make a patch for that.
>
Fair enough. I think we all agree that fixing the superuser shortcuts are
a relevant and welcome change (at least that is the sense I get). So, how
about for the time being, we table the "consistent API and error messages"
and focus solely on the shortcuts? If that is favorable, then I have
attached a patch to address those changes.
> There is other work going on replacing these role attributes with
> > something more general, so maybe this cleanup should be done as part of
> > that other work.
>
I agree and given the work that has already been done toward that effort I
think that would perhaps be the better approach to addressing them.
Perhaps 'cleanup' is just an overloaded term. The patch is to make the
> APIs and the error messages consistent. I'm amazed at how much
> discussion and work is going into these exceptionally minor changes
> which have been already broken out of the larger and far more
> interesting work (imv anyway). Having two patches, one to simply move
> the checks around and then another to make the error messages in those
> checks consistent, which will naturally end up depending on each other,
> strikes me as overkill, but we can certainly do it.
>
Agreed, but will certainly do whatever is necessary to keep these changes
moving forward. Though, I think the attached patch mitigates any need to
break these changes out further.
Andres raised a concern about the specific error message wording (which
> was intended to just make it more consistent with the rest of our
> permission-check error messages..), are there any other opinions on the
> wording? Would be great to get feedback on that..
>
Agreed, I would certainly be willing to move these changes forward as a
separate effort, but without more specific recommendations beyond what has
already been discussed and proposed I'm at a bit of a loss on where to take
it. I'm all ears on this one and would certainly appreciate any feedback
Thanks,
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..510caf6
*** a/src/backend/access/transam/xlogfuncs.c
--- b/src/backend/access/transam/xlogfuncs.c
*** 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")));
--- 54,60
backupidstr = text_to_cstring(backupid);
! if (!has_rolreplication(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 sup
Re: [HACKERS] Additional role attributes && superuser review
All,
> If we're going to change the catalog representation for the existing
> capabilities, I'd recommend that the first patch change the catalog
> representation and add the new syntax without adding any more
> capabilities; and then the second and subsequent patches can add
> additional capabilities.
Attached is an initial patch for review and discussion that takes a swing
at changing the catalog representation.
This patch includes:
* int64 (C) to int8 (SQL) mapping for genbki.
* replace all role attributes columns in pg_authid with single int64 column
named rolattr.
* update CreateRole and AlterRole to use rolattr.
* update all has_*_privilege functions to check rolattr.
* builtin SQL function 'has_role_attribute' that takes a role oid and text
name of the attribute as input and returns a boolean.
Items not currently addressed:
* New syntax - more discussion needs to occur around these potential
changes. Specifically, how would CREATE USER/ROLE be affected? I suppose
it is OK to keep it as WITH , though if ALTER ROLE
is modified to have ADD | DROP CAPABILITY for consistency would WITH
CAPABILITY , make more sense for CREATE? At any rate, I think
there is obviously much more discussion that needs to be had.
* Documentation - want to gain feedback on implementation prior to making
changes.
* Update regression tests, rules test for system_views - want to gain
feedback on approach to handling pg_roles, etc. before updating.
Also, what would be the community preference on tracking these
attached/proposed changes? Should I create a separate entry in the next CF
for this item (seems prudent) or would it be preferred to keep it attached
to the current 'new attributes' CF entry?
Thanks,
Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
new file mode 100644
index eb91c53..523b379
*** a/src/backend/catalog/Catalog.pm
--- b/src/backend/catalog/Catalog.pm
*** sub Catalogs
*** 33,38
--- 33,39
my %RENAME_ATTTYPE = (
'int16' => 'int2',
'int32' => 'int4',
+ 'int64' => 'int8',
'Oid' => 'oid',
'NameData' => 'name',
'TransactionId' => 'xid');
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
new file mode 100644
index d30612c..93eb2e6
*** 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
*** 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
--- 3610,3616
if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE)) &&
IsSystemClass(table_oid, classForm) &&
classForm->relkind != RELKIND_VIEW &&
! !role_has_attribute(roleid, ROLE_ATTR_CATUPDATE) &&
!allowSystemTableMods)
{
#ifdef ACLDEBUG
*** pg_extension_ownercheck(Oid ext_oid, Oid
*** 5051,5056
--- 5031,5058
}
/*
+ * Check whether the specified role has a specific role attribute.
+ */
+ bool
+ role_has_attribute(Oid roleid, RoleAttr attribute)
+ {
+ RoleAttr attributes;
+ 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)));
+
+ attributes = ((Form_pg_authid) GETSTRUCT(tuple))->rolattr;
+ ReleaseSysCache(tuple);
+
+ return ((attributes & attribute) > 0);
+ }
+
+ /*
* Check whether specified role has CREATEROLE privilege (or is a superuser)
*
* Note: roles do not have owners per se; instead we use this test in
*** pg_extension_ownercheck(Oid ext_
Re: [HACKERS] Additional role attributes && superuser review
All, > Currently, I am using int32 simply because int64 is causing some issues. > The issue is that genbki.pl is not able to associate it with the int8 > type as defined in pg_type.h. Therefore Schema_pg_authid in schemapg.h > isn't defined correctly. I've been digging and scratching my head on this > one but I have reached a point where I think it would be better just to ask. > Attached is a quite trivial patch that addresses the int64 (C) to int8 (SQL) mapping issue. Further digging revealed that Catalog.pm wasn't accounting for int64 (thanks Stephen). Would it be better to include this change as a separate patch (as attached) or would it be preferable to include with a larger role attribute bitmask patch? Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm new file mode 100644 index eb91c53..523b379 *** a/src/backend/catalog/Catalog.pm --- b/src/backend/catalog/Catalog.pm *** sub Catalogs *** 33,38 --- 33,39 my %RENAME_ATTTYPE = ( 'int16' => 'int2', 'int32' => 'int4', + 'int64' => 'int8', 'Oid' => 'oid', 'NameData' => 'name', 'TransactionId' => 'xid'); -- 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] Additional role attributes && superuser review
All, > 2. Catalog Representation: > > Condense all attributes in pg_authid to single int64 column and create > bitmasks accordingly. > I have been working on these changes and I was hoping for some assistance/recommendations. Currently, I am using int32 simply because int64 is causing some issues. The issue is that genbki.pl is not able to associate it with the int8 type as defined in pg_type.h. Therefore Schema_pg_authid in schemapg.h isn't defined correctly. I've been digging and scratching my head on this one but I have reached a point where I think it would be better just to ask. Any thoughts or recommendations on how I should approach this? -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] superuser() shortcuts
Attached is two separate patches to address previous
comments/recommendations:
* superuser-cleanup-shortcuts_11-5-2014.patch
* has_privilege-cleanup_11-5-2014.patch
-Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/contrib/test_decoding/expected/permissions.out b/contrib/test_decoding/expected/permissions.out
new file mode 100644
index 212fd1d..f011955
*** a/contrib/test_decoding/expected/permissions.out
--- b/contrib/test_decoding/expected/permissions.out
*** RESET ROLE;
*** 54,66
-- plain user *can't* can control replication
SET ROLE lr_normal;
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
! ERROR: must be superuser or replication role to use replication slots
INSERT INTO lr_test VALUES('lr_superuser_init');
ERROR: permission denied for relation lr_test
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
! ERROR: must be superuser or replication role to use replication slots
SELECT pg_drop_replication_slot('regression_slot');
! ERROR: must be superuser or replication role to use replication slots
RESET ROLE;
-- replication users can drop superuser created slots
SET ROLE lr_superuser;
--- 54,69
-- plain user *can't* can control replication
SET ROLE lr_normal;
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
! ERROR: permission denied to use replication slots
! HINT: You must be superuser or replication role to use replication slots.
INSERT INTO lr_test VALUES('lr_superuser_init');
ERROR: permission denied for relation lr_test
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
! ERROR: permission denied to use replication slots
! HINT: You must be superuser or replication role to use replication slots.
SELECT pg_drop_replication_slot('regression_slot');
! ERROR: permission denied to use replication slots
! HINT: You must be superuser or replication role to use replication slots.
RESET ROLE;
-- replication users can drop superuser created slots
SET ROLE lr_superuser;
*** SELECT 'init' FROM pg_create_logical_rep
*** 90,96
RESET ROLE;
SET ROLE lr_normal;
SELECT pg_drop_replication_slot('regression_slot');
! ERROR: must be superuser or replication role to use replication slots
RESET ROLE;
-- all users can see existing slots
SET ROLE lr_superuser;
--- 93,100
RESET ROLE;
SET ROLE lr_normal;
SELECT pg_drop_replication_slot('regression_slot');
! ERROR: permission denied to use replication slots
! HINT: You must be superuser or replication role to use replication slots.
RESET ROLE;
-- all users can see existing slots
SET ROLE lr_superuser;
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
new file mode 100644
index 133143d..52d8208
*** 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,63
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")));
startpoint = do_pg_start_backup(backupidstr, fast, NULL, NULL);
--- 55,65
backupidstr = text_to_cstring(backupid);
! if (!has_replication_privilege(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("permission denied to run a backup"),
! errhint("You must be superuser or replication role to run a backup.")));
startpoint = do_pg_start_backup(backupidstr, fast, NULL, NULL);
*** pg_stop_backup(PG_FUNCTION_ARGS)
*** 82,91
{
XLogRecPtr stoppoint;
! if (!superuser() && !has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! (errmsg("must be superuser or replication role to run a backup";
stoppoint = do_pg_stop_backup(NULL, true, NULL);
--- 84,94
{
XLogRecPtr stoppoint;
! if (!has_replication_privilege(GetUserId()))
ereport(ERROR,
Re: [HACKERS] superuser() shortcuts
Thanks for looking at this patch. > I suggest moving the rest of the changes into separate patches. > Hmmm... perhaps the following? * superuser-cleanup - contains above mentioned superuser shortcuts only. * has_privilege-cleanup - contains has_*_priviledge cleanup only. Would that also require a separate commitfest entry? The ha*_something_privilege() changes are also not very consistent. > > We already have have_createrole_privilege(), which does include a > superuser check, and you add has_replication_privilege() with a > superuser check, but has_catupdate_privilege() and > has_inherit_privilege() don't include a superuser check. That's clearly > a mess. > Good catch. Though, according to the documentation, not even superuser is allowed to bypass CATUPDATE. http://www.postgresql.org/docs/9.4/static/catalog-pg-authid.html. However, I can't think of a reason why "inherit" wouldn't need the superuser check. Obviously superuser is considered a member of every role, but is there a reason that a superuser would not be allowed to bypass this? I only ask because it did not have a check previously, so I figure there might have been a good reason for it? Btw., why rename have_createrole_privilege()? > Well, actually it wasn't necessarily a rename. It was a removal of that function all together as all it did was simply return the result of "has_createrole_privilege". That seemed rather redundant and unnecessary, IMO. Also, your patch has spaces between tabs. Check for whitespace errors > with git. > Yikes. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Additional role attributes && superuser review
All,
> That said, I don't feel very strongly about that position, so if you and
> Robert (and others on the thread) agree that's the right approach then
> I'll see about getting it done.
We haven't reached consensus on this one yet and I didn't want it to fall
too far off the radar.
Here is what I summarize as the current state of the discussion:
1. Syntax:
ALTER ROLE { ADD | DROP } CAPABILITY
* I think this is the most straight forward approach as it still close to
the current syntax.
* Perhaps keeping the current syntax around as deprecated to be removed in
a scheduled future version. (provide a "deprecated" message to the user?)
or
GRANT EXECUTE PRIVILEGES ON TO
* Though, this will be tricky since EXECUTE is not reserved and the
currently state of GRANT in the grammar would require either refactoring or
making it RESERVED... neither I think are acceptable at this point in time
for obvious reasons.
2. Catalog Representation:
Condense all attributes in pg_authid to single int64 column and create
bitmasks accordingly.
Obviously there is some concern for upgrading and whether to do both at
once or to do them incrementally. IMHO, I think if the changes are going
to be made, then we should go ahead and do them at the same time. Though,
would it be beneficial to separate in to two distinct patches?
-Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] alter user/role CURRENT_USER
All,
> I agree that we should probably seperate the concerns here. Personally,
> I like the idea of being able to say "CURRENT_USER" in utility commands
> to refer to the current user where a role would normally be expected, as
> I could see it simplifying things for some applications, but that's a
> new feature and independent of making role-vs-user cases more
> consistent.
>
So, I've been doing a little digging and it would appear that the ALTER
ROLE/USER consistency was brought up earlier in the year.
http://www.postgresql.org/message-id/cadyruxmv-tvsbv7mttcs+qedny6xj1+krtzfowvuhdjc5mg...@mail.gmail.com
It was returned with feedback in Commitfest 2014-06 and apparently lost
steam:
https://commitfest.postgresql.org/action/patch_view?id=1408
Tom put forward a suggestion for how to fix it:
http://www.postgresql.org/message-id/21570.1408832...@sss.pgh.pa.us
I have taken that patch and updated the documentation (attached) and ran it
through some cursory testing.
At any rate, this is probably a good starting point for those changes.
-Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/doc/src/sgml/ref/alter_user.sgml b/doc/src/sgml/ref/alter_user.sgml
new file mode 100644
index 58ae1da..ac05682
*** a/doc/src/sgml/ref/alter_user.sgml
--- b/doc/src/sgml/ref/alter_user.sgml
*** ALTER USER name RENAME TO new_name
! ALTER USER name SET configuration_parameter { TO | = } { value | DEFAULT }
! ALTER USER name SET configuration_parameter FROM CURRENT
! ALTER USER name RESET configuration_parameter
! ALTER USER name RESET ALL
--- 38,47
ALTER USER name RENAME TO new_name
! ALTER USER name [ IN DATABASE database_name ] SET configuration_parameter { TO | = } { value | DEFAULT }
! ALTER USER { name | ALL } [ IN DATABASE database_name ] SET configuration_parameter FROM CURRENT
! ALTER USER { name | ALL } [ IN DATABASE database_name ] RESET configuration_parameter
! ALTER USER { name | ALL } [ IN DATABASE database_name ] RESET ALL
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
new file mode 100644
index 0de9584..d7c0586
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
*** static Node *makeRecursiveViewSelect(cha
*** 230,236
AlterFdwStmt AlterForeignServerStmt AlterGroupStmt
AlterObjectSchemaStmt AlterOwnerStmt AlterSeqStmt AlterSystemStmt AlterTableStmt
AlterTblSpcStmt AlterExtensionStmt AlterExtensionContentsStmt AlterForeignTableStmt
! AlterCompositeTypeStmt AlterUserStmt AlterUserMappingStmt AlterUserSetStmt
AlterRoleStmt AlterRoleSetStmt AlterPolicyStmt
AlterDefaultPrivilegesStmt DefACLAction
AnalyzeStmt ClosePortalStmt ClusterStmt CommentStmt
--- 230,236
AlterFdwStmt AlterForeignServerStmt AlterGroupStmt
AlterObjectSchemaStmt AlterOwnerStmt AlterSeqStmt AlterSystemStmt AlterTableStmt
AlterTblSpcStmt AlterExtensionStmt AlterExtensionContentsStmt AlterForeignTableStmt
! AlterCompositeTypeStmt AlterUserMappingStmt
AlterRoleStmt AlterRoleSetStmt AlterPolicyStmt
AlterDefaultPrivilegesStmt DefACLAction
AnalyzeStmt ClosePortalStmt ClusterStmt CommentStmt
*** static Node *makeRecursiveViewSelect(cha
*** 520,525
--- 520,527
%type opt_existing_window_name
%type opt_if_not_exists
+ %type role_or_user
+
/*
* Non-keyword token types. These are hard-wired into the "flex" lexer.
* They must be listed first so that their numeric codes do not depend on
*** stmt :
*** 756,763
| AlterTSConfigurationStmt
| AlterTSDictionaryStmt
| AlterUserMappingStmt
- | AlterUserSetStmt
- | AlterUserStmt
| AnalyzeStmt
| CheckPointStmt
| ClosePortalStmt
--- 758,763
*** CreateUserStmt:
*** 1033,1042
*
* Alter a postgresql DBMS role
*
*/
AlterRoleStmt:
! ALTER ROLE RoleId opt_with AlterOptRoleList
{
AlterRoleStmt *n = makeNode(AlterRoleStmt);
n->role = $3;
--- 1033,1044
*
* Alter a postgresql DBMS role
*
+ * ALTER USER is accepted interchangeably with ALTER ROLE.
+ *
*/
AlterRoleStmt:
! ALTER role_or_user RoleId opt_with AlterOptRoleList
{
AlterRoleStmt *n = makeNode(AlterRoleStmt);
n->role = $3;
*** AlterRoleStmt:
*** 1046,1058
}
;
opt_in_database:
/* EMPTY */ { $$ = NULL; }
| IN_P DATABASE database_name { $$ = $3; }
;
AlterRoleSetStmt:
! ALTER ROLE RoleId opt_in_database SetResetClause
{
AlterRoleSetStmt *n = makeNode(AlterRoleSetStmt);
n->role = $3;
--- 1048,1065
}
;
+ role_or_user:
Re: [HACKERS] CREATE IF NOT EXISTS INDEX
All, FWIW, I've cleanly applied v8 of this patch to master (252e652) and check-world was successful. I also successfully ran through a few manual test cases. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] alter user/role CURRENT_USER
Kyotaro,
Zero-length identifiers are rejected in scanner so RoleId cannot
> be a valid pointer to a zero-length string. (NULL is used as
> PUBLIC in auth_ident)
>
> | postgres=# create role "";
> | ERROR: zero-length delimited identifier at or near """"
> | postgres=# create role U&"\00";
> | ERROR: invalid Unicode escape value at or near "\00""
>
Err... what? I'm not sure what you are getting at here? Why would we ever
have/want a zero-length identifier for a RoleId? Seems to me that anything
requiring a RoleId should be explicit.
As a dirty and quick hack we can use some integer values prfixed
> by single zero byte to represent special roles such as
> CURRENT_USER.
>
> | RoleId_or_curruser: RoleId{ $$ = $1; }
> | | CURRENT_USER { $$ = "\x00\x01";};
>
> | Oid ResolveRoleId(const char *name, bool missing_ok)
> | {
> | Oid roleid;
> |
> | if (name[0] == 0 && name[1] == 1)
> | roleid = GetUserId();
>
> This is ugly but needs no additional struct member or special
> logics. (Macros could make them look better.)
Yeah, that's pretty ugly. I think Alvaro's recommendation of having the
production return a node with a name or flag is the better approach.
| /* This hack lets us avoid reserving PUBLIC as a keyword*/
> | if (strcmp($1, "public") == 0)
>
> Please let me know the reason to avoid registering new keyword
> making the word unusable as an literal identifier, if any?
>
FWIW, I found the following in the archives:
http://www.postgresql.org/message-id/15516.1038718...@sss.pgh.pa.us
Now this is from 2002 and it appears it wasn't necessary to change at the
time, but I haven't yet found anything else related (it's a big archive).
Though, as I understand it, PUBLIC is now non-reserved as of SQL:2011 which
might make a compelling argument to leave it as is?
-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
Alvaro, I think it would make more sense if the file-accessing command specified > the DIRALIAS (or DIRECTORY, whatever we end up calling this) and a > pathname relative to the base one. Something like > > postgres=# CREATE DIRECTORY logdir ALIAS FOR '/pgsql/data/pg_log'; Following this, what do you think about simply expanding DIRALIAS out into to DIRECTORY ALIAS? So instead: CREATE DIRECTORY ALIAS AS '' or... CREATE DIRECTORY ALIAS FOR '' My thought on this is towards the natural word order of the command. Also, I think having it as CREATE DIRECTORY ALIAS minimizes confusion, as I think Stephen mentioned, that we are creating an alias, not an actual directory. Thoughts? postgres=# GRANT READ ON DIRECTORY logdir TO logscanner; > I personally like this form the most, however, I think the greatest hurdle with it is that it would require making READ (and WRITE) reserved keywords. Obviously, I think that is a non-starter. > logscanner=> COPY logtable FROM 'postgresql-2014-10-28.csv' IN DIRECTORY > logdir; > That's an interesting thought. Would 'IN DIRECTORY' be restricted to just the alias name? I'm not sure it would make sense to allow a directory path there, as what would be the point? At any rate, just food for thought. The ALTER ROLE GRANT READ idea proposed downthread is nice also, Agreed and probably the most logical option at this point? but one > advantage of this is not having absolute path names in the COPY command. Pardon my ignorance, but can you help me understand the advantage of not having absolute path names in the COPY command? -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
Robert, > To articular my own concerns perhaps a bit better, there are two major > things I don't like about the whole DIRALIAS proposal. Number one, > you're creating this SQL object whose name is not actually used for > anything other than manipulating the alias you created. The users are > still operating on pathnames. That's awfully strange. > That's an interesting point and I don't disagree that it seems a little strange. However, isn't this approach similar if not the same (other than operating on path names) as with some other objects, specifically rules and policies? -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
Robert, > Given that no fewer than four people - all committers - have expressed > doubts about the design of this patch, I wonder why you're bothering > to post a new version. I understand and my intent was in no way to disregard those concerns. The only reason that I have posted a new version was simply to address some minor issues that I noticed when responding to Peter's earlier comment about missing files. It seems to me that you should be discussing > the fundamental design, not making minor updates to the code. Ok. I'm certainly looking at the other options proposed and will work with Stephen to put together an appropriate design for discussion here. I really hope this is not moving in the direction of another "surprise > commit" like we had with RLS. There is absolutely NOT consensus on > this design or anything close to it. Certainly not and I am in no way confused that consensus has not been reached. -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] alter user/role CURRENT_USER
>
> +RoleId:CURRENT_USER{ $$ =
> "current_user";}
> + | USER { $$ = "current_user";}
> + | CURRENT_ROLE { $$ = "current_user";}
> + | SESSION_USER { $$ = "session_user";}
>
> This is kind of ugly, and it means you can't distinguish between a
> CURRENT_USER keyword and a quoted user name "current_user".
You are right. I'm not sure I have an opinion on how clean it is, but
FWIW, it is similar to the way that the 'auth_ident' type in the grammar is
defined (though, not to imply that it makes it right). As well, the
originally proposed "RoleId_or_curruser" suffers from the same issue. I'm
going to go out on a limb here, but is it not possible to consider
"current_user", etc. reserved in the same sense that we do with PUBLIC and
NONE and disallow users/roles from being created as them? I mean, after
all, they *are* already reserved keywords. Perhaps there is a very good
reason why we wouldn't want to do that and I am sure there is since they
have not been treated this way thus far. If anyone would like to share
why, then I'd very much appreciate the "lesson".
It's a legitimate user name, so the behavior of the following now changes:
>
> CREATE ROLE "current_user";
>
Well, it does allow this one.
> ALTER ROLE "current_user" SET work_mem='10MB';
>
However, you are right, it does interfere with this command (and pretty
much ALTER ROLE in general). :-/ Not sure what to offer there.
> ALTER USER USER does not seem like sane syntax that should be
> accepted.
>
I think that I agree, I can't imagine this being acceptable.
Taking a step back, I'm still not sure I understand the need for this
feature or the use case. It seems to have started as a potential fix to an
inconsistency between ALTER USER and ALTER ROLE syntax (which I think I
could see some value in). However, I think it has been taken beyond just
resolving the inconsistency and started to cross over into feature creep.
Is the intent simply to resolve inconsistencies between what is now an
alias of another command? Or is it to add new functionality? I think the
original proposal needs to be revisited and more time needs to be spent
defining the scope and purpose of this patch.
-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
All,
Attached is a patch with minor updates/corrections.
-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 b257b02..8cdc5cb
*** a/src/backend/catalog/Makefile
--- b/src/backend/catalog/Makefile
*** POSTGRES_BKI_SRCS = $(addprefix $(top_sr
*** 41,47
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 \
)
# location of Catalog.pm
--- 41,47
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 \
! 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
new file mode 100644
index d30612c..3717bf5
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
***
*** 30,35
--- 30,36
#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,53
--- 49,55
#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"
*** ExecGrant_Type(InternalGrant *istmt)
*** 3183,3188
--- 3185,3374
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,
+ BTEqualStrategyNu
Re: [HACKERS] alter user/role CURRENT_USER
All,
I just ran through the patch giving it a good once over, some items to
address/consider/discuss:
* Trailing whitespace.
* Why are you making changes in foreigncmds.c? These seem like unnecessary
changes. I see that you are trying to consolidate but this refactor seems
potentially out of scope.
* To the above point, is ResolveRoleId really necessary? Also, I'm not
sure I agree with passing in the tuple, I don't see any reason to pull that
look up into this function. Also, couldn't you simply just add a short
circuit in "get_role_oid" to check for "current_user" and return
GetUserId() and similar for "session_user"?
* In gram.y, is there really a need to have a separate RoleId_or_curruser?
Why not:
-RoleId:NonReservedWord { $$ = $1; };
+RoleId:CURRENT_USER{ $$ =
"current_user";}
+ | USER { $$ = "current_user";}
+ | CURRENT_ROLE { $$ = "current_user";}
+ | SESSION_USER { $$ = "session_user";}
+ | NonReservedWord { $$ = $1; }
+ ;
This would certainly reduce the number of changes to the grammar but might
also help with covering the cases mentioned by Rushabh? Also are there
cases when we don't want CURRENT_USER to be considered a RoleId?
* The following seems like an unnecessary change:
- | RoleId { $$ = (strcmp($1, "public") == 0) ? NULL : $1;
}
+ RoleId { $$ = (strcmp($1, "public") == 0) ?
+ NULL : $1; }
* Why is htup.h included in dbcommands.h?
* What's up with the following in user.c, did you replace tab with spaces?
- HeapTuple roletuple;
+ HeapTuple roletuple;
-- Not working
> alter default privileges for role current_user grant SELECT ON TABLES TO
> current_user ;
>
-- Not working
> grant user1 TO current_user;
>
Agreed. The latter of the two seems like an interesting case to me
though. But they both kind of jump out at me as potential for security
concerns (but perhaps not given the role id checks, etc). At any rate, I'd
still expect these to behave accordingly with notification/error messages
when appropriate.
Their might few more syntax like this.
>
I think this can be "covered" inherently by the grammar changes recommended
above (if such changes are appropriate). Though, I'd also recommend
investigating which commands are affected via the grammar (or RoleId) and
then making sure to update the regression tests and the documentation
accordingly.
> I understand that patch is sightly getting bigger and complex then what
> it was
> originally proposed.
>
IMHO, I think this patch has become more complex than is required.
> Before going back to more review on latest patch I would
> like to understand the requirement of this new feature. Also would like
> others
> to comment on where/how we should restrict this feature ?
>
I think this is a fair request. I believe I can see the potential
convenience of these changes, however, I'm uncertain of the necessity of
them and whether or not it opens any security concerns (again, perhaps not,
but I think it is worth asking). Also, how would this affect items like
auditing?
-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?
Hi all, This is my first post to the mailing list and I am looking forward to working with everyone in the community. With that said... I'll take a look at changing the cache key to include user ID and > ripping out the plan invalidation logic from the current patch tomorrow > but I seriously doubt I'll be able to get all of that done in the next > day or two. If anyone else is able to help out, it'd certainly be > appreciated; I really think that's the main hurdle to address at this > point with this patch- without the plan invalidation complexity, the > the patch is really just building out the catalog, the SQL-level > statements for managing it, and the bit of code required to add the > conditional to statements involving RLS-enabled tables. > I have been collaborating with Stephen on addressing this particular item with RLS. As a basis, I have been working with Craig's 'rls-9.4-upd-sb-views' branch rebased against master around 9.4beta1. Through this effort, we have concluded that for RLS the case of invalidating a plan is only necessary when switching between a superuser and a non-superuser. Obviously, re-planning on every role change would be too costly, but this approach should help minimize that cost. As well, there were not any cases outside of this one that were immediately apparent with respect to RLS that would require re-planning on a per userid basis. I have tested this approach with the following patch. https://github.com/abrightwell/postgres/commit/4c959e63f7a89b24ebbd46575a31a629d24efa75 Does this sound like a sane approach? Thoughts? Recommendations? Thanks, Adam
