set_user.dest_role_whitelist for set_user('rolename') #18

scrummyin · 2018-06-25T17:23:13Z

I'm working on a project that could benefit from one user being able to become another. However, I want to limit the list of roles that anyone who has access to set_user can become. I added the guc value set_user.dest_role_whitelist to handle this and thought since it is a non-breaking change that I would create a pull request.

Please let me know if it is acceptable, and if you would like more work done on it. Also, I included changes to the readme that I thought would be helpful.

mpalmi · 2018-06-26T21:04:24Z

README.md

+  listed (e.g. `'+admin'`) are allowed
+* if the whitelist is set to the empty set `''` all roles are denied
+* the default is `'*'` which means all roles are allowed
+


I like this in theory, but the whitelists have different meanings in this particular instance, since set_user.superuser_whitelist is for allowed sources and set_user.dest_role_whitelist is for allowed targets. Because of this, "allowed" and "denied" are sort of ambiguous.

We may also want to consider a different name for the whitelists, to distinguish between them. Maybe something like set_user.superuser_source_whitelist and set_user.nonsuperuser_target_whitelist?

I don't think the whitelist have different meanings in these cases, and I think being able to re-use the check_user_whitelist function illistrates that. The difference is in the target of the whitelist current_user vs the rolename argument and maybe wording/naming can fix that.

That said, I completely agree with you on a name change. My original hesitation on it was that I did not want to rename set_user.superuser_whitelist as I thought it make reading GUC variables confusing. I was unsure if the setting should reading from set_user.superuser_source_whitelist and then set_user.superuser_whitelist, in order to keep compatiable with current setups.

In summary, I will rename set_user.dest_role_whitelist to set_user.nonsuperuser_target_whitelist and *Dest_role_whitelist to *Nonsuperuser_Target_Whitelist, as those names have better meaning and no one is using them yet. However, I'll wait for you feedback on renaming set_user.superuser_whitelist and how to handle reading from the guc for it's value. The other question is while I'm working on it would you like the C variables of *SU_Whitelist renamed, and if so what to?

The re-use of check_user_whitelist is an implementation detail. Though it's nice that we can do that, it really has no bearing on the end-user that this doc is geared toward, IMO.

Good call re: superuser_whitelist. Maybe we leave that the same for now and move forward with the dest_role renaming. We may be able to do better than nonsuperuser_target_whitelist. Perhaps we should discuss the name out-of-band?

mpalmi · 2018-06-26T21:04:55Z

set_user.c

@@ -348,7 +349,11 @@ set_user(PG_FUNCTION_ARGS)
 						(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 						 errmsg("switching to superuser not allowed"),
 						 errhint("Add current user to set_user.superuser_whitelist.")));
-		}
+		} else if(!check_user_whitelist(NewUserId, Dest_role_whitelist))


style nit: else if should be on the next line:

} else if ... {

Will do thanks for the catch on that one.

mpalmi · 2018-06-27T01:57:48Z

README.md

+User's calling `set_user('rolename')` can only transition to roles listed or
+included in `set_user.dest_role_whitelist` (defaults to all roles). Additionally
+the [Whitelist logic](#whitelist-rules-and-logic) applies to `current_user` when
+`set_user()` is invoked.



Since we're going to revise this paragraph, maybe something like this instead:

The following rules govern escalation to superuser via the set_user_u('rolename') function:

current_user must be GRANTed EXECUTE ON FUNCTION set_user_u('rolename') OR current_user must be the OWNER of the set_user_u function OR current_user must be a superuser.

current_user must be listed in set_user.superuser_whitelist OR current_user must belong to a group that is listed in set_user.superuser_whitelist (e.g. '+admin')

If set_user.superuser_whitelist is the empty set , '', superuser escalation is blocked for all users.

If set_user.superuser_whitelist is the wildcard character, '*', all users with EXECUTE permission on set_user_u('rolename') can escalate to superuser.

If set_user.superuser_whitelist is not specified, the value defaults to the wildcard character, '*'.

The following rules govern non-superuser role transitions through use of the set_user('rolename') function:

current_user must be GRANTed EXECUTE ON FUNCTION set_user('rolename') OR current_user must be the OWNER of the set_user function OR current_user must be a superuser.

The destination rolename must be listed in set_user.dest_role_whitelist OR the destination rolename must belong to a group that is listed in set_user.dest_role_whitelist (e.g. '+client')

If set_user.dest_role_whitelist is the empty set , '', set_user transitioning to non-superusers is blocked for all users.

If set_user.dest_role_whitelist is the wildcard character, '*', all users with EXECUTE permission on set_user('rolename') can transition to any other non-superuser role.

If set_user.dest_user_whitelist is not specified, the value defaults to the wildcard character, '*'.

Alright, I'll make these changes, but I'm going to wait on the feedback from above since if we are going to change the names, I'd rather only have to edit this section once.

The differences between these two paragraphs illustrate the ambiguity I referenced above. I think I'd prefer to leave the logic explanations split out, since we can be more specific about what the whitelist does for each feature.

Oh sorry misunderstanding here, I will keep it split. I meant if I re-write it now I would have to edit it again later if we rename dest_user_whitelist and superuser_whitelist. So, I want to add that whole section pretty much as is and make the variable name changes all in one go.

as is == as you wrote it above

scrummyin · 2018-06-27T15:15:52Z

Alright, I think I have a more updated set of changes. Let me know if there is anything more I can do.

The guc variable `set_user.nosuperuser_target_whitelist can be used to limit the roles that any user can become when given access to execute set_user(). Previously, anyone with access to call set_user() could become any user that was not a superuser. This allowed for bypassing some security features by becoming the owner of an object. With the addition of a whitelist around the allowed target roles to set_user() the scope of set_user can be limited to an approved list. With the default of set_user.nosuperuser_target_whitelist being '*' this change is backwards compatible and can be installed on existing configurations, without reconfiguring. This has no effect on the current superuser_whitelist. As it is only called on when the target role is not a superuser. Discussion in: pgaudit#18

The GUC variable `set_user.nosuperuser_target_whitelist` can be used to limit the roles that any user can become when given access to execute set_user(). Previously, anyone with access to call set_user() could become any user that was not a superuser. This allowed for bypassing some security features by becoming the owner of an object. With the addition of a whitelist around the allowed target roles to set_user() the scope of set_user can be limited to an approved list. With the default of set_user.nosuperuser_target_whitelist being '*' this change is backwards compatible and can be installed on existing configurations, without reconfiguring. This has no effect on the current superuser_whitelist. As it is only called on when the target role is not a superuser. Discussion in: pgaudit#18

The GUC variable `set_user.nosuperuser_target_whitelist` can be used to limit the roles that any user can become when given access to execute set_user(). Previously, anyone with access to call set_user() could become any user that was not a superuser. This allowed for bypassing some security features by becoming the owner of an object. With the addition of a whitelist around the allowed target roles to set_user() the scope of set_user can be limited to an approved list. With the default of `set_user.nosuperuser_target_whitelist` being '*' this change is backwards compatible and can be installed on existing configurations, without reconfiguring. This has no effect on the current `superuser_whitelist`. As it is only called on when the target role is not a superuser. Discussion in: pgaudit#18

mpalmi · 2018-06-27T19:24:28Z

README.md

+**Note:** See rules in [Superuser Whitelist](#set_usersuperuser_whitelist-rules-and-logic)
+for logic around calling set_user_u. See
+[Nosuperuser Whitelist](#set_usernosuperuser_target_whitelist-rules-and-logic) for
+reference logic around calling set_user.


mpalmi · 2018-06-27T19:26:06Z

README.md

+
+The following rules govern non-superuser role transitions through use of
+  `set_user(text)` or `set_user(text, text)` function (for simplicity
+  only `set_user(text)` is used):


for simplicity, only set_user(text) is referenced below

mpalmi · 2018-06-27T19:26:47Z

README.md

+* The destination rolename must be listed in `set_user.nosuperuser_target_whitelist`
+  OR the destination rolename must belong to a group that is listed in
+  `set_user.nosuperuser_target_whitelist` (e.g. `'+client'`)
+* If `set_user.nosuperuser_target_whitelist` is the empty set , `''`, set_user


how about set_user(text)?

mpalmi · 2018-06-27T19:36:11Z

README.md

+  OR the destination rolename must belong to a group that is listed in
+  `set_user.nosuperuser_target_whitelist` (e.g. `'+client'`)
+* If `set_user.nosuperuser_target_whitelist` is the empty set , `''`, set_user
+  transitioning to non-superusers is blocked for all users.


s/transitioning/transitions/

mpalmi · 2018-06-27T19:36:42Z

README.md

+* `current_user` must be `GRANT`ed `EXECUTE ON FUNCTION set_user(text)` OR
+  `current_user` must be the `OWNER` of the `set_user(text)` function OR
+  `current_user` must be a superuser.
+* The destination rolename must be listed in `set_user.nosuperuser_target_whitelist`


s/destination/target/g

The GUC variable `set_user.nosuperuser_target_whitelist` can be used to limit the roles that any user can become when given access to execute set_user(). Previously, anyone with access to call set_user() could become any user that was not a superuser. This allowed for bypassing some security features by becoming the owner of an object. With the addition of a whitelist around the allowed target roles to set_user() the scope of set_user can be limited to an approved list. With the default of `set_user.nosuperuser_target_whitelist` being '*' this change is backwards compatible and can be installed on existing configurations, without reconfiguring. This has no effect on the current `superuser_whitelist`. As it is only called on when the target role is not a superuser. Discussion in: pgaudit#18

References to `set_user(text)` and `set_user_u(text)` have been changed from `set_user('rolename')` and `set_user_u('rolename') to `set_user(text)` and `set_user_u(text)`, in contexts where the functions are being referenced and not called. This makes the README easier to follow.

mpalmi · 2018-06-28T20:55:34Z

Looks good. Tested it out on my dev box for pg10/9.6.

The GUC variable `set_user.nosuperuser_target_whitelist` can be used to limit the roles that any user can become when given access to execute set_user(). Previously, anyone with access to call set_user() could become any user that was not a superuser. This allowed for bypassing some security features by becoming the owner of an object. With the addition of a whitelist around the allowed target roles to set_user() the scope of set_user can be limited to an approved list. With the default of `set_user.nosuperuser_target_whitelist` being '*' this change is backwards compatible and can be installed on existing configurations, without reconfiguring. This has no effect on the current `superuser_whitelist`. As it is only called on when the target role is not a superuser. Discussion in: #18

mpalmi requested changes Jun 27, 2018

View reviewed changes

scrummyin force-pushed the feature/dest_user_whitelist branch from a291533 to ca1be8a Compare June 27, 2018 15:51

scrummyin force-pushed the feature/dest_user_whitelist branch from ca1be8a to be3b6da Compare June 27, 2018 17:02

scrummyin force-pushed the feature/dest_user_whitelist branch from be3b6da to 92a5ad9 Compare June 27, 2018 17:15

scrummyin force-pushed the feature/dest_user_whitelist branch from 92a5ad9 to 78e41f6 Compare June 27, 2018 17:22

scrummyin force-pushed the feature/dest_user_whitelist branch from 78e41f6 to 49acf12 Compare June 27, 2018 17:24

scrummyin force-pushed the feature/dest_user_whitelist branch from 49acf12 to c4fac76 Compare June 27, 2018 18:35

scrummyin force-pushed the feature/dest_user_whitelist branch from c4fac76 to ee20d06 Compare June 27, 2018 18:36

scrummyin force-pushed the feature/dest_user_whitelist branch from ee20d06 to 7712138 Compare June 27, 2018 19:15

mpalmi reviewed Jun 27, 2018

View reviewed changes

scrummyin force-pushed the feature/dest_user_whitelist branch from 7712138 to 0836625 Compare June 27, 2018 19:42

scrummyin added 2 commits June 27, 2018 16:21

scrummyin force-pushed the feature/dest_user_whitelist branch from 8458d49 to 4fee11a Compare June 27, 2018 20:22

mpalmi merged commit 0534fb5 into pgaudit:master Jun 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set_user.dest_role_whitelist for set_user('rolename') #18

set_user.dest_role_whitelist for set_user('rolename') #18

scrummyin commented Jun 25, 2018

mpalmi Jun 26, 2018

scrummyin Jun 27, 2018

mpalmi Jun 27, 2018

mpalmi Jun 26, 2018 •

edited

Loading

scrummyin Jun 27, 2018

mpalmi Jun 27, 2018 •

edited

Loading

scrummyin Jun 27, 2018

mpalmi Jun 27, 2018

scrummyin Jun 27, 2018

scrummyin Jun 27, 2018

scrummyin commented Jun 27, 2018

mpalmi Jun 27, 2018

mpalmi Jun 27, 2018

mpalmi Jun 27, 2018

scrummyin Jun 27, 2018

mpalmi Jun 27, 2018

mpalmi Jun 27, 2018

mpalmi Jun 27, 2018

mpalmi commented Jun 28, 2018

set_user.dest_role_whitelist for set_user('rolename') #18

set_user.dest_role_whitelist for set_user('rolename') #18

Conversation

scrummyin commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpalmi Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpalmi Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scrummyin commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpalmi commented Jun 28, 2018

mpalmi Jun 26, 2018 •

edited

Loading

mpalmi Jun 27, 2018 •

edited

Loading