Re: [HACKERS] Additional role attributes && superuser review
On Wed, Feb 03, 2016 at 01:44:28PM -0500, Robert Haas wrote: > On Thu, Jan 28, 2016 at 4:37 PM, Stephen Frost wrote: > > pg_monitor > > > > Allows roles granted more information from pg_stat_activity. Can't be > > just a regular non-default-role right as we don't, currently, have a > > way to say "filter out the values of certain columns on certain rows, > > but not on others." > > > > (There's a question here though- for the privileges which will be > > directly GRANT'able, should we GRANT those to pg_monitor, or have > > pg_monitor only provide unfiltered access to pg_stat_activity, or..? > > Further, if it's only for pg_stat_activity, should we name it > > something else?) > > I endorse this proposed role. I'd have it just grant access to > pg_stat_activity but keep the name pg_monitor so that it could apply > to other similar things in the future, but there might be other good > alternatives too. -1 for adding any pg_monitor role. That name belongs to a role allowing, from inception, more than just full pg_stat_activity access. The last, vigorous effort to define its scope failed; there may not exist one notion of "monitoring" operations qualified to claim this name. At least, the community lacks the ability to design it in the foreseeable future. > > pg_signal_backend > > > > Allows roles to signal other backend processes beyond those backends > > which are owned by a user they are a role member of. Can't be a > > regular non-default-role right as we don't, currently, have any way to > > GRANT rights to send signals only to backends you own or are a member > > of. > > I also endorse this. +1. This role has a clear scope and a name denoting that scope, so it's a sound way to introduce built-in role infrastructure. I like the concept of built-in roles. One can inherit them via user-defined roles and use NOINHERIT to regulate that. One can grant WITH ADMIN OPTION, something role options like REPLICATION don't offer. It feels more scalable than defining role options, and it's a technique non-core code can imitate. I haven't decided whether it's a problem that members of built-in roles can create objects owned by those roles. -- 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
On Thu, Jan 28, 2016 at 4:37 PM, Stephen Frost wrote: > pg_monitor > > Allows roles granted more information from pg_stat_activity. Can't be > just a regular non-default-role right as we don't, currently, have a > way to say "filter out the values of certain columns on certain rows, > but not on others." > > (There's a question here though- for the privileges which will be > directly GRANT'able, should we GRANT those to pg_monitor, or have > pg_monitor only provide unfiltered access to pg_stat_activity, or..? > Further, if it's only for pg_stat_activity, should we name it > something else?) I endorse this proposed role. I'd have it just grant access to pg_stat_activity but keep the name pg_monitor so that it could apply to other similar things in the future, but there might be other good alternatives too. > pg_signal_backend > > Allows roles to signal other backend processes beyond those backends > which are owned by a user they are a role member of. Can't be a > regular non-default-role right as we don't, currently, have any way to > GRANT rights to send signals only to backends you own or are a member > of. I also endorse this. > pg_replication > > Allows roles to use the various replication functions. Can't be a > regular non-default-role right as the REPLICATION role attribute > allows access to those functions and the GRANT system has no way of > saying "allow access to these functions if they have role attribute > X." > > Make sense, as these are cases where we can't simply write GRANT > statements and get the same result, but we don't need (or at least, > shouldn't have without supporting GRANT on catalog objects and agreement > on what they're intended for): This seems like it could be reshuffled so that it can be done with GRANT. Therefore, I don't endorse this. > pg_backup > > pg_start_backup(), pg_stop_backup(), pg_switch_xlog(), and > pg_create_restore_point() will all be managed by the normal GRANT > system and therefore we don't need a default role for those use-cases. Agreed. > pg_file_settings > > pg_file_settings() function and pg_file_settings view will be managed > by the normal GRANT system and therefore we don't need a default role > for those use-cases. Agreed. > pg_replay > > pg_xlog_replay_pause(), and pg_xlog_replay_resume() will be managed > by the normal GRANT system and therefore we don't need a default role > for those use-cases. Agreed. > pg_rotate_logfile > > pg_rotate_logfile() will be managed by the normal GRANT system and > therefore we don't need a default role for that use-cases. Agreed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
On Sun, Jan 31, 2016 at 7:55 AM, Michael Paquier wrote: > On Sun, Jan 31, 2016 at 5:32 AM, Craig Ringer wrote: >> On 29 January 2016 at 22:41, Stephen Frost wrote: >>> >>> Michael, >>> >>> * Michael Paquier (michael.paqu...@gmail.com) wrote: >>> > On Fri, Jan 29, 2016 at 6:37 AM, Stephen Frost >>> > wrote: >>> > > * Robert Haas (robertmh...@gmail.com) wrote: >>> > >> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost >>> > wrote: >>> > >> > Personally, I don't have any particular issue having both, but the >>> > >> > desire was stated that it would be better to have the regular >>> > >> > GRANT EXECUTE ON catalog_func() working before we consider having >>> > >> > default roles for same. That moves the goal posts awful far >>> > >> > though, if >>> > >> > we're to stick with that and consider how we might extend the GRANT >>> > >> > system in the future. >>> > >> >>> > >> I don't think it moves the goal posts all that far. Convincing >>> > >> pg_dump to dump grants on system functions shouldn't be a crazy large >>> > >> patch. >>> > > >>> > > I wasn't clear as to what I was referring to here. I've already >>> > > written >>> > > a patch to pg_dump to support grants on system objects and agree that >>> > > it's at least reasonable. >>> > >>> > Is it already posted somewhere? I don't recall seeing it. Robert and >>> > Noah >>> > have a point that this would be useful for users who would like to dump >>> > GRANT/REVOKE rights on system functions & all, using a new option in >>> > pg_dumpall, say --with-system-acl or --with-system-privileges. >>> >>> Multiple versions were posted on this thread. I don't fault you for not >>> finding it, this thread is a bit long in the tooth. The one I'm >>> currently working from is >>> >> >> It strikes me that this thread would possibly benefit from a wiki page >> outlining the permissions, overall concepts, etc, as it's getting awfully >> hard to follow. > > +1. This has proved to be very beneficial for UPSERT. I am marking this patch as returned with feedback per the current status of this thread. -- Michael -- 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
On Sun, Jan 31, 2016 at 5:32 AM, Craig Ringer wrote: > On 29 January 2016 at 22:41, Stephen Frost wrote: >> >> Michael, >> >> * Michael Paquier (michael.paqu...@gmail.com) wrote: >> > On Fri, Jan 29, 2016 at 6:37 AM, Stephen Frost >> > wrote: >> > > * Robert Haas (robertmh...@gmail.com) wrote: >> > >> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost >> > wrote: >> > >> > Personally, I don't have any particular issue having both, but the >> > >> > desire was stated that it would be better to have the regular >> > >> > GRANT EXECUTE ON catalog_func() working before we consider having >> > >> > default roles for same. That moves the goal posts awful far >> > >> > though, if >> > >> > we're to stick with that and consider how we might extend the GRANT >> > >> > system in the future. >> > >> >> > >> I don't think it moves the goal posts all that far. Convincing >> > >> pg_dump to dump grants on system functions shouldn't be a crazy large >> > >> patch. >> > > >> > > I wasn't clear as to what I was referring to here. I've already >> > > written >> > > a patch to pg_dump to support grants on system objects and agree that >> > > it's at least reasonable. >> > >> > Is it already posted somewhere? I don't recall seeing it. Robert and >> > Noah >> > have a point that this would be useful for users who would like to dump >> > GRANT/REVOKE rights on system functions & all, using a new option in >> > pg_dumpall, say --with-system-acl or --with-system-privileges. >> >> Multiple versions were posted on this thread. I don't fault you for not >> finding it, this thread is a bit long in the tooth. The one I'm >> currently working from is >> > > It strikes me that this thread would possibly benefit from a wiki page > outlining the permissions, overall concepts, etc, as it's getting awfully > hard to follow. +1. This has proved to be very beneficial for UPSERT. -- Michael -- 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
On 29 January 2016 at 22:41, Stephen Frost wrote: > Michael, > > * Michael Paquier (michael.paqu...@gmail.com) wrote: > > On Fri, Jan 29, 2016 at 6:37 AM, Stephen Frost > wrote: > > > * Robert Haas (robertmh...@gmail.com) wrote: > > >> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost > > wrote: > > >> > Personally, I don't have any particular issue having both, but the > > >> > desire was stated that it would be better to have the regular > > >> > GRANT EXECUTE ON catalog_func() working before we consider having > > >> > default roles for same. That moves the goal posts awful far > though, if > > >> > we're to stick with that and consider how we might extend the GRANT > > >> > system in the future. > > >> > > >> I don't think it moves the goal posts all that far. Convincing > > >> pg_dump to dump grants on system functions shouldn't be a crazy large > > >> patch. > > > > > > I wasn't clear as to what I was referring to here. I've already > written > > > a patch to pg_dump to support grants on system objects and agree that > > > it's at least reasonable. > > > > Is it already posted somewhere? I don't recall seeing it. Robert and Noah > > have a point that this would be useful for users who would like to dump > > GRANT/REVOKE rights on system functions & all, using a new option in > > pg_dumpall, say --with-system-acl or --with-system-privileges. > > Multiple versions were posted on this thread. I don't fault you for not > finding it, this thread is a bit long in the tooth. The one I'm > currently working from is > > It strikes me that this thread would possibly benefit from a wiki page outlining the permissions, overall concepts, etc, as it's getting awfully hard to follow. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] Additional role attributes && superuser review
On Fri, Jan 29, 2016 at 11:41 PM, Stephen Frost wrote: > Michael, > > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> On Fri, Jan 29, 2016 at 6:37 AM, Stephen Frost wrote: >> > * Robert Haas (robertmh...@gmail.com) wrote: >> >> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost >> wrote: >> >> > Personally, I don't have any particular issue having both, but the >> >> > desire was stated that it would be better to have the regular >> >> > GRANT EXECUTE ON catalog_func() working before we consider having >> >> > default roles for same. That moves the goal posts awful far though, if >> >> > we're to stick with that and consider how we might extend the GRANT >> >> > system in the future. >> >> >> >> I don't think it moves the goal posts all that far. Convincing >> >> pg_dump to dump grants on system functions shouldn't be a crazy large >> >> patch. >> > >> > I wasn't clear as to what I was referring to here. I've already written >> > a patch to pg_dump to support grants on system objects and agree that >> > it's at least reasonable. >> >> Is it already posted somewhere? I don't recall seeing it. Robert and Noah >> have a point that this would be useful for users who would like to dump >> GRANT/REVOKE rights on system functions & all, using a new option in >> pg_dumpall, say --with-system-acl or --with-system-privileges. > > Multiple versions were posted on this thread. I don't fault you for not > finding it, this thread is a bit long in the tooth. The one I'm > currently working from is: > > http://www.postgresql.org/message-id/attachment/38049/catalog_function_acls_v4.patch > > I'm going to split up the pg_dump changes and the backend changes, as > they can logically go in independently (though without both, we're not > moving the needle very far with regards to what administrators can do). OK. Looks like a good way to move forward to me. >> If at least >> the three of you are agreeing here I think that we should try to move at >> least toward this goal first. That seems a largely doable goal for 9.6. For >> the set of default roles, there is clearly no clear consensus regarding >> what each role should do or not, and under which limitation it should >> operate. > > I'm trying to work towards a consensus on the default roles, hence the > questions and discussion posed in the email you replied to. So the current CF entry should be marked as returned with feedback perhaps? What do you think? Once patches are ready for the default roles in backend and for pg_dump, we could then work on reviewing them separately. -- Michael -- 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
Michael, * Michael Paquier (michael.paqu...@gmail.com) wrote: > On Fri, Jan 29, 2016 at 6:37 AM, Stephen Frost wrote: > > * Robert Haas (robertmh...@gmail.com) wrote: > >> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost > wrote: > >> > Personally, I don't have any particular issue having both, but the > >> > desire was stated that it would be better to have the regular > >> > GRANT EXECUTE ON catalog_func() working before we consider having > >> > default roles for same. That moves the goal posts awful far though, if > >> > we're to stick with that and consider how we might extend the GRANT > >> > system in the future. > >> > >> I don't think it moves the goal posts all that far. Convincing > >> pg_dump to dump grants on system functions shouldn't be a crazy large > >> patch. > > > > I wasn't clear as to what I was referring to here. I've already written > > a patch to pg_dump to support grants on system objects and agree that > > it's at least reasonable. > > Is it already posted somewhere? I don't recall seeing it. Robert and Noah > have a point that this would be useful for users who would like to dump > GRANT/REVOKE rights on system functions & all, using a new option in > pg_dumpall, say --with-system-acl or --with-system-privileges. Multiple versions were posted on this thread. I don't fault you for not finding it, this thread is a bit long in the tooth. The one I'm currently working from is: http://www.postgresql.org/message-id/attachment/38049/catalog_function_acls_v4.patch I'm going to split up the pg_dump changes and the backend changes, as they can logically go in independently (though without both, we're not moving the needle very far with regards to what administrators can do). > If at least > the three of you are agreeing here I think that we should try to move at > least toward this goal first. That seems a largely doable goal for 9.6. For > the set of default roles, there is clearly no clear consensus regarding > what each role should do or not, and under which limitation it should > operate. I'm trying to work towards a consensus on the default roles, hence the questions and discussion posed in the email you replied to. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Fri, Jan 29, 2016 at 6:37 AM, Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: >> On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost wrote: >> > Personally, I don't have any particular issue having both, but the >> > desire was stated that it would be better to have the regular >> > GRANT EXECUTE ON catalog_func() working before we consider having >> > default roles for same. That moves the goal posts awful far though, if >> > we're to stick with that and consider how we might extend the GRANT >> > system in the future. >> >> I don't think it moves the goal posts all that far. Convincing >> pg_dump to dump grants on system functions shouldn't be a crazy large >> patch. > > I wasn't clear as to what I was referring to here. I've already written > a patch to pg_dump to support grants on system objects and agree that > it's at least reasonable. Is it already posted somewhere? I don't recall seeing it. Robert and Noah have a point that this would be useful for users who would like to dump GRANT/REVOKE rights on system functions & all, using a new option in pg_dumpall, say --with-system-acl or --with-system-privileges. If at least the three of you are agreeing here I think that we should try to move at least toward this goal first. That seems a largely doable goal for 9.6. For the set of default roles, there is clearly no clear consensus regarding what each role should do or not, and under which limitation it should operate. -- Michael
Re: [HACKERS] Additional role attributes && superuser review
* Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost wrote: > >> So, this seems like a case where a built-in role would be > >> well-justified. I don't really believe in built-in roles as a way of > >> bundling related permissions; I know you do, but I don't. I'd rather > >> see the individual function permissions granted individually. But > >> here you are talking about a variable level of access to the same > >> function, depending on role. And for that it seems to me that a > >> built-in role has a lot more to recommend it in that case than it does > >> in the other. If you have been granted pg_whack, you can signal any > >> process on the system; otherwise just your own. Those checks are > >> internal to pg_terminate_backend/pg_cancel_backend so GRANT is not a > >> substitute. > > > > If we extend this into the future then it seems to potentially fall > > afoul of Noah's concern that we're going to end up with two different > > ways of saying GRANT EXECUTE X ON Y. Consider the more complicated case > > of pg_stat_activity, where values are shown or hidden based on who the > > current role is. The policy system only supports whole-row, today, but > > the question has already come up, both on the lists and off, of support > > for hiding individual column values for certain rows, exactly as we do > > today for pg_stat_activity. Once we reach that point, we'll have a way > > to GRANT out these rights and a default role which just has them. > > Well, I'm not saying that predefined rolls are the *only* way to solve > a problem like this, but I think they're one way and I don't clearly > see that something else is better. However, my precise point is that > we should *not* have predefined rolls that precisely duplicate the > result of GRANT EXECUTE X ON Y. Having predefined rolls that change > the behavior of the system in other ways is a different thing. So I > don't see how this falls afoul of Noah's concern. (If it does, > perhaps he can clarify.) Apologies if it seems like what I'm getting at is obtuse but I'm trying to generalize this, to provide guidance on how to handle the larger set of privileges. If I'm following correctly, having default roles for cases where the role is specifically for something more than 'GRANT EXECUTE X ON Y' (or a set of such commands..?) makes sense. Going back to the list of roles, that would mean that default roles: pg_monitor Allows roles granted more information from pg_stat_activity. Can't be just a regular non-default-role right as we don't, currently, have a way to say "filter out the values of certain columns on certain rows, but not on others." (There's a question here though- for the privileges which will be directly GRANT'able, should we GRANT those to pg_monitor, or have pg_monitor only provide unfiltered access to pg_stat_activity, or..? Further, if it's only for pg_stat_activity, should we name it something else?) pg_signal_backend Allows roles to signal other backend processes beyond those backends which are owned by a user they are a role member of. Can't be a regular non-default-role right as we don't, currently, have any way to GRANT rights to send signals only to backends you own or are a member of. pg_replication Allows roles to use the various replication functions. Can't be a regular non-default-role right as the REPLICATION role attribute allows access to those functions and the GRANT system has no way of saying "allow access to these functions if they have role attribute X." Make sense, as these are cases where we can't simply write GRANT statements and get the same result, but we don't need (or at least, shouldn't have without supporting GRANT on catalog objects and agreement on what they're intended for): pg_backup pg_start_backup(), pg_stop_backup(), pg_switch_xlog(), and pg_create_restore_point() will all be managed by the normal GRANT system and therefore we don't need a default role for those use-cases. pg_file_settings pg_file_settings() function and pg_file_settings view will be managed by the normal GRANT system and therefore we don't need a default role for those use-cases. pg_replay pg_xlog_replay_pause(), and pg_xlog_replay_resume() will be managed by the normal GRANT system and therefore we don't need a default role for those use-cases. pg_rotate_logfile pg_rotate_logfile() will be managed by the normal GRANT system and therefore we don't need a default role for that use-cases. > > Personally, I don't have any particular issue having both, but the > > desire was stated that it would be better to have the regular > > GRANT EXECUTE ON catalog_func() working before we consider having > > default roles for same. That moves the goal posts awful far though, if > > we're to stick with that and consider how we might extend the GRANT > > system in the future. > > I don't think it moves the goal posts all that
Re: [HACKERS] Additional role attributes && superuser review
On Thu, Jan 28, 2016 at 11:04 AM, Stephen Frost wrote: >> So, this seems like a case where a built-in role would be >> well-justified. I don't really believe in built-in roles as a way of >> bundling related permissions; I know you do, but I don't. I'd rather >> see the individual function permissions granted individually. But >> here you are talking about a variable level of access to the same >> function, depending on role. And for that it seems to me that a >> built-in role has a lot more to recommend it in that case than it does >> in the other. If you have been granted pg_whack, you can signal any >> process on the system; otherwise just your own. Those checks are >> internal to pg_terminate_backend/pg_cancel_backend so GRANT is not a >> substitute. > > If we extend this into the future then it seems to potentially fall > afoul of Noah's concern that we're going to end up with two different > ways of saying GRANT EXECUTE X ON Y. Consider the more complicated case > of pg_stat_activity, where values are shown or hidden based on who the > current role is. The policy system only supports whole-row, today, but > the question has already come up, both on the lists and off, of support > for hiding individual column values for certain rows, exactly as we do > today for pg_stat_activity. Once we reach that point, we'll have a way > to GRANT out these rights and a default role which just has them. Well, I'm not saying that predefined rolls are the *only* way to solve a problem like this, but I think they're one way and I don't clearly see that something else is better. However, my precise point is that we should *not* have predefined rolls that precisely duplicate the result of GRANT EXECUTE X ON Y. Having predefined rolls that change the behavior of the system in other ways is a different thing. So I don't see how this falls afoul of Noah's concern. (If it does, perhaps he can clarify.) > Personally, I don't have any particular issue having both, but the > desire was stated that it would be better to have the regular > GRANT EXECUTE ON catalog_func() working before we consider having > default roles for same. That moves the goal posts awful far though, if > we're to stick with that and consider how we might extend the GRANT > system in the future. I don't think it moves the goal posts all that far. Convincing pg_dump to dump grants on system functions shouldn't be a crazy large patch. > What got me thinking along these lines was a question posed by Bruce > (Bruce, feel free to chime in if I've misunderstood) to me at SCALE > where we were chatting a bit about this, which was- how could we extend > GRANT to support the permissions that we actually wish > pg_terminate_backend() and pg_cancel_backend() to have, instead of using > a default role? I didn't think too much on it at the time as it strikes > me as a pretty narrow use-case and requiring quite a bit of complexity > to get right, but there again, I'd certainly view a system where the > user is allowed to have pg_start_backup() rights but not > pg_stop_backup() as being quite a small use-case also, yet that's the > direction we're largely going in with this discussion. Well, sure, in that largely artificial example it's not hard to say ha, ha, silly, but the actual examples we looked at upthread were much less obviously silly. There was plenty of room for argument about the precise contours of each predefined role. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
* Robert Haas (robertmh...@gmail.com) wrote: > On Sun, Jan 17, 2016 at 6:58 PM, Stephen Frost wrote: > > I'm not against that idea, though I continue to feel that there are > > common sets of privileges which backup tools could leverage. > > > > The other issue that I'm running into, again, while considering how to > > move back to ACL-based permissions for these objects is that we can't > > grant out the actual permissions which currently exist. That means we > > either need to break backwards compatibility, which would be pretty > > ugly, in my view, or come up with new functions and then users will have > > to know which functions to use when. > > > > As I don't think we really want to break backwards compatibility or > > remove existing functionality, the only approach which is going to make > > sense is to add additional functions in some cases. In particular, we > > will need alternate versions of pg_terminate_backend and > > pg_cancel_backend. One thought I had was to make that > > 'pg_signal_backend', but that sounds like we'd allow any signal sent by > > a user with that right, which seems a bit much to me... > > So, this seems like a case where a built-in role would be > well-justified. I don't really believe in built-in roles as a way of > bundling related permissions; I know you do, but I don't. I'd rather > see the individual function permissions granted individually. But > here you are talking about a variable level of access to the same > function, depending on role. And for that it seems to me that a > built-in role has a lot more to recommend it in that case than it does > in the other. If you have been granted pg_whack, you can signal any > process on the system; otherwise just your own. Those checks are > internal to pg_terminate_backend/pg_cancel_backend so GRANT is not a > substitute. If we extend this into the future then it seems to potentially fall afoul of Noah's concern that we're going to end up with two different ways of saying GRANT EXECUTE X ON Y. Consider the more complicated case of pg_stat_activity, where values are shown or hidden based on who the current role is. The policy system only supports whole-row, today, but the question has already come up, both on the lists and off, of support for hiding individual column values for certain rows, exactly as we do today for pg_stat_activity. Once we reach that point, we'll have a way to GRANT out these rights and a default role which just has them. Personally, I don't have any particular issue having both, but the desire was stated that it would be better to have the regular GRANT EXECUTE ON catalog_func() working before we consider having default roles for same. That moves the goal posts awful far though, if we're to stick with that and consider how we might extend the GRANT system in the future. What got me thinking along these lines was a question posed by Bruce (Bruce, feel free to chime in if I've misunderstood) to me at SCALE where we were chatting a bit about this, which was- how could we extend GRANT to support the permissions that we actually wish pg_terminate_backend() and pg_cancel_backend() to have, instead of using a default role? I didn't think too much on it at the time as it strikes me as a pretty narrow use-case and requiring quite a bit of complexity to get right, but there again, I'd certainly view a system where the user is allowed to have pg_start_backup() rights but not pg_stop_backup() as being quite a small use-case also, yet that's the direction we're largely going in with this discussion. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On 1/17/16 9:10 PM, Stephen Frost wrote: > but if it's possible to do a backup without > being a superuser and with only read access to the data directory, I > would expect every backup soltuion to view that as a feature which they > want to support, as there are environments which will find it desirable, > at a minimum, and even some which will require it. This would certainly be a desirable feature for pgBackRest. The fewer processes running with the ability to alter the cluster the better. Even if they are well-tested it's still one less thing to worry about. -- -David da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Sun, Jan 17, 2016 at 6:58 PM, Stephen Frost wrote: > I'm not against that idea, though I continue to feel that there are > common sets of privileges which backup tools could leverage. > > The other issue that I'm running into, again, while considering how to > move back to ACL-based permissions for these objects is that we can't > grant out the actual permissions which currently exist. That means we > either need to break backwards compatibility, which would be pretty > ugly, in my view, or come up with new functions and then users will have > to know which functions to use when. > > As I don't think we really want to break backwards compatibility or > remove existing functionality, the only approach which is going to make > sense is to add additional functions in some cases. In particular, we > will need alternate versions of pg_terminate_backend and > pg_cancel_backend. One thought I had was to make that > 'pg_signal_backend', but that sounds like we'd allow any signal sent by > a user with that right, which seems a bit much to me... So, this seems like a case where a built-in role would be well-justified. I don't really believe in built-in roles as a way of bundling related permissions; I know you do, but I don't. I'd rather see the individual function permissions granted individually. But here you are talking about a variable level of access to the same function, depending on role. And for that it seems to me that a built-in role has a lot more to recommend it in that case than it does in the other. If you have been granted pg_whack, you can signal any process on the system; otherwise just your own. Those checks are internal to pg_terminate_backend/pg_cancel_backend so GRANT is not a substitute. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
* Bruce Momjian (br...@momjian.us) wrote: > On Sun, Jan 17, 2016 at 09:23:14PM -0500, Stephen Frost wrote: > > > > Group ownership and permissions aren't a backup-method-specific > > > > requirement either, in my view. I'm happy to chat with Marco (who has > > > > said he would be weighing in on this thread when he is able to) > > > > regarding barman, and whomever would be appropriate for BART (perhaps > > > > you could let me know..?), but if it's possible to do a backup without > > > > being a superuser and with only read access to the data directory, I > > > > would expect every backup soltuion to view that as a feature which they > > > > want to support, as there are environments which will find it desirable, > > > > at a minimum, and even some which will require it. > > > > > > pg_dump doesn't need to read the PGDATA directory, and I thought this > > > permission was to be used by pg_dump users as well. > > > > No. That has been a source of confusion, though I'm not quite sure how > > or why, beyond the general assumption that anything 'backup' must > > include 'pg_dump' (I don't generally consider that to be the case, > > myself, but it seems others do...). > > I think the source of that is that many people have asked for > backup-only uses, and I thought running pg_dump or pg_dumpall was one of > those cases. I'd want that to be a seperate permission which would really be something along the lines of "allowed to read everything through a DB connection", which is what pg_dump/pg_dumpall need. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Sun, Jan 17, 2016 at 09:23:14PM -0500, Stephen Frost wrote: > > > Group ownership and permissions aren't a backup-method-specific > > > requirement either, in my view. I'm happy to chat with Marco (who has > > > said he would be weighing in on this thread when he is able to) > > > regarding barman, and whomever would be appropriate for BART (perhaps > > > you could let me know..?), but if it's possible to do a backup without > > > being a superuser and with only read access to the data directory, I > > > would expect every backup soltuion to view that as a feature which they > > > want to support, as there are environments which will find it desirable, > > > at a minimum, and even some which will require it. > > > > pg_dump doesn't need to read the PGDATA directory, and I thought this > > permission was to be used by pg_dump users as well. > > No. That has been a source of confusion, though I'm not quite sure how > or why, beyond the general assumption that anything 'backup' must > include 'pg_dump' (I don't generally consider that to be the case, > myself, but it seems others do...). I think the source of that is that many people have asked for backup-only uses, and I thought running pg_dump or pg_dumpall was one of those cases. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription + -- 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
* Bruce Momjian (br...@momjian.us) wrote: > On Sun, Jan 17, 2016 at 09:10:23PM -0500, Stephen Frost wrote: > > > While the group owner of the directory is a distributions question, the > > > permissions are usually a backup-method-specific requirement. I can see > > > us creating an SQL function that opens up group permissions on the data > > > directory for specific backup tools that need it, then granting > > > permissions on that function to the backup role. This is another > > > example where different backup tools need different permissions. > > > > I don't believe we can really consider group ownership and group > > permissions independently. They really go hand-in-hand. On > > RedHat-based system, where the group is set as 'staff', you probably > > don't want group permissions to be allowed. On Debian-based systems, > > where there is a dedicated 'postgres' group, group permissions are fine > > to allow. > > Yes, I can see that as problematic. Seems it would have to be something > done by the administrator from the command-line. initdb on both RedHat and Debian-based systems is run, generally speaking, from the packaging scripts. They would be able to pass the correct options to initdb (or PG itself, if we decide that's necessary..). > > Group ownership and permissions aren't a backup-method-specific > > requirement either, in my view. I'm happy to chat with Marco (who has > > said he would be weighing in on this thread when he is able to) > > regarding barman, and whomever would be appropriate for BART (perhaps > > you could let me know..?), but if it's possible to do a backup without > > being a superuser and with only read access to the data directory, I > > would expect every backup soltuion to view that as a feature which they > > want to support, as there are environments which will find it desirable, > > at a minimum, and even some which will require it. > > pg_dump doesn't need to read the PGDATA directory, and I thought this > permission was to be used by pg_dump users as well. No. That has been a source of confusion, though I'm not quite sure how or why, beyond the general assumption that anything 'backup' must include 'pg_dump' (I don't generally consider that to be the case, myself, but it seems others do...). This is only for file-based backups. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Sun, Jan 17, 2016 at 09:10:23PM -0500, Stephen Frost wrote: > > While the group owner of the directory is a distributions question, the > > permissions are usually a backup-method-specific requirement. I can see > > us creating an SQL function that opens up group permissions on the data > > directory for specific backup tools that need it, then granting > > permissions on that function to the backup role. This is another > > example where different backup tools need different permissions. > > I don't believe we can really consider group ownership and group > permissions independently. They really go hand-in-hand. On > RedHat-based system, where the group is set as 'staff', you probably > don't want group permissions to be allowed. On Debian-based systems, > where there is a dedicated 'postgres' group, group permissions are fine > to allow. Yes, I can see that as problematic. Seems it would have to be something done by the administrator from the command-line. > Group ownership and permissions aren't a backup-method-specific > requirement either, in my view. I'm happy to chat with Marco (who has > said he would be weighing in on this thread when he is able to) > regarding barman, and whomever would be appropriate for BART (perhaps > you could let me know..?), but if it's possible to do a backup without > being a superuser and with only read access to the data directory, I > would expect every backup soltuion to view that as a feature which they > want to support, as there are environments which will find it desirable, > at a minimum, and even some which will require it. pg_dump doesn't need to read the PGDATA directory, and I thought this permission was to be used by pg_dump users as well. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription + -- 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
* Bruce Momjian (br...@momjian.us) wrote: > On Sun, Jan 17, 2016 at 06:58:25PM -0500, Stephen Frost wrote: > > I'm not against that idea, though I continue to feel that there are > > common sets of privileges which backup tools could leverage. > > > > The other issue that I'm running into, again, while considering how to > > move back to ACL-based permissions for these objects is that we can't > > grant out the actual permissions which currently exist. That means we > > Is that because many of them are complex, e.g. you can kill only your > own sessions? Right. > > either need to break backwards compatibility, which would be pretty > > ugly, in my view, or come up with new functions and then users will have > > to know which functions to use when. > > > > As I don't think we really want to break backwards compatibility or > > remove existing functionality, the only approach which is going to make > > sense is to add additional functions in some cases. In particular, we > > will need alternate versions of pg_terminate_backend and > > pg_cancel_backend. One thought I had was to make that > > Like these? Could we define own-user-type rights? Interesting idea but I don't really see that being general enough that we would want to burn a GRANT bit for it... Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Bruce, * Bruce Momjian (br...@momjian.us) wrote: > On Sun, Jan 17, 2016 at 01:57:22PM -0500, Stephen Frost wrote: > > Right, we also check in the backend on startup for certain permissions. > > I don't recall offhand if that's forced to 700 or if we allow 750. > > > > > > I don't recall offhand if that means we'd have to make changes to allow > > > > that, but, for my 2c, I don't see why we wouldn't allow it to be an > > > > option. > > > > > > OK, that would be an initdb change then. > > > > It would need to be optional, so distributions and users could choose > > which makes sense for their systems. > > While the group owner of the directory is a distributions question, the > permissions are usually a backup-method-specific requirement. I can see > us creating an SQL function that opens up group permissions on the data > directory for specific backup tools that need it, then granting > permissions on that function to the backup role. This is another > example where different backup tools need different permissions. I don't believe we can really consider group ownership and group permissions independently. They really go hand-in-hand. On RedHat-based system, where the group is set as 'staff', you probably don't want group permissions to be allowed. On Debian-based systems, where there is a dedicated 'postgres' group, group permissions are fine to allow. Group ownership and permissions aren't a backup-method-specific requirement either, in my view. I'm happy to chat with Marco (who has said he would be weighing in on this thread when he is able to) regarding barman, and whomever would be appropriate for BART (perhaps you could let me know..?), but if it's possible to do a backup without being a superuser and with only read access to the data directory, I would expect every backup soltuion to view that as a feature which they want to support, as there are environments which will find it desirable, at a minimum, and even some which will require it. Lastly, I'm pretty sure this would have to be a postgresql.conf option as we check the permissions on the data directory on startup. I don't see how having an SQL function would work there as I certainly wouldn't want to have the permissions changing on a running system. That strikes me as being risky without any real benefit. Either it's safe and acceptable to allow those rights to the group, or it isn't. A temproary grant really isn't helping with anything that I can see, surely there are numerous ways to exploit such a time-based grant. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Sun, Jan 17, 2016 at 06:58:25PM -0500, Stephen Frost wrote: > I'm not against that idea, though I continue to feel that there are > common sets of privileges which backup tools could leverage. > > The other issue that I'm running into, again, while considering how to > move back to ACL-based permissions for these objects is that we can't > grant out the actual permissions which currently exist. That means we Is that because many of them are complex, e.g. you can kill only your own sessions? > either need to break backwards compatibility, which would be pretty > ugly, in my view, or come up with new functions and then users will have > to know which functions to use when. > > As I don't think we really want to break backwards compatibility or > remove existing functionality, the only approach which is going to make > sense is to add additional functions in some cases. In particular, we > will need alternate versions of pg_terminate_backend and > pg_cancel_backend. One thought I had was to make that Like these? Could we define own-user-type rights? -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription + -- 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
On Sun, Jan 17, 2016 at 01:57:22PM -0500, Stephen Frost wrote: > Right, we also check in the backend on startup for certain permissions. > I don't recall offhand if that's forced to 700 or if we allow 750. > > > > I don't recall offhand if that means we'd have to make changes to allow > > > that, but, for my 2c, I don't see why we wouldn't allow it to be an > > > option. > > > > OK, that would be an initdb change then. > > It would need to be optional, so distributions and users could choose > which makes sense for their systems. While the group owner of the directory is a distributions question, the permissions are usually a backup-method-specific requirement. I can see us creating an SQL function that opens up group permissions on the data directory for specific backup tools that need it, then granting permissions on that function to the backup role. This is another example where different backup tools need different permissions. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription + -- 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
* Bruce Momjian (br...@momjian.us) wrote: > On Wed, Jan 6, 2016 at 12:29:14PM -0500, Robert Haas wrote: > > The point is that with the GRANT EXECUTE ON FUNCTION proposal, authors > > of monitoring tools enjoy various really noteworthy advantages. They > > can have monitoring roles which have *exactly* the privileges that > > their tool needs, not whatever set of permissions (larger or smaller) > > the core project has decide the pg_monitor role should have. They can > > have optional features requiring extra permissions and those extra > > permissions can be granted in precisely those shops where those extra > > features are in use. They can deploy a new versions of their > > monitoring tool that requires an extra privilege on an existing > > PostgreSQL release without requiring any core modifications, which > > shaves years of time off the deployment schedule and avoids > > contentious arguments with the lovable folks who populate this mailing > > list. That sounds *terrific* to me compared to the alternative you > > are proposing. > > I assume backup tools would either document the functions they want > access to via SQL commands, or supply a script. I assume they would > create a non-login role (group) with the desired permissions, and then > have users inherit from that. They would also need to be able to allow > upgrades where they would (conditionally?) add the role and then > add/revoke permissions as needed, e.g. they might not need all > permissions they needed in a previous release, or they might need new > ones. > > That all seems very straight-forward to me. I'm not against that idea, though I continue to feel that there are common sets of privileges which backup tools could leverage. The other issue that I'm running into, again, while considering how to move back to ACL-based permissions for these objects is that we can't grant out the actual permissions which currently exist. That means we either need to break backwards compatibility, which would be pretty ugly, in my view, or come up with new functions and then users will have to know which functions to use when. As I don't think we really want to break backwards compatibility or remove existing functionality, the only approach which is going to make sense is to add additional functions in some cases. In particular, we will need alternate versions of pg_terminate_backend and pg_cancel_backend. One thought I had was to make that 'pg_signal_backend', but that sounds like we'd allow any signal sent by a user with that right, which seems a bit much to me... Perhaps that's what I'll do though, barring other suggestions. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Wed, Jan 6, 2016 at 12:29:14PM -0500, Robert Haas wrote: > The point is that with the GRANT EXECUTE ON FUNCTION proposal, authors > of monitoring tools enjoy various really noteworthy advantages. They > can have monitoring roles which have *exactly* the privileges that > their tool needs, not whatever set of permissions (larger or smaller) > the core project has decide the pg_monitor role should have. They can > have optional features requiring extra permissions and those extra > permissions can be granted in precisely those shops where those extra > features are in use. They can deploy a new versions of their > monitoring tool that requires an extra privilege on an existing > PostgreSQL release without requiring any core modifications, which > shaves years of time off the deployment schedule and avoids > contentious arguments with the lovable folks who populate this mailing > list. That sounds *terrific* to me compared to the alternative you > are proposing. I assume backup tools would either document the functions they want access to via SQL commands, or supply a script. I assume they would create a non-login role (group) with the desired permissions, and then have users inherit from that. They would also need to be able to allow upgrades where they would (conditionally?) add the role and then add/revoke permissions as needed, e.g. they might not need all permissions they needed in a previous release, or they might need new ones. That all seems very straight-forward to me. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription + -- 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
* Bruce Momjian (br...@momjian.us) wrote: > On Sun, Jan 17, 2016 at 01:49:19PM -0500, Stephen Frost wrote: > > * Bruce Momjian (br...@momjian.us) wrote: > > > > pgbackrest: > > > > > > > > To run pgbackrest as a non-superuser and not the 'postgres' system > > > > user, grant the pg_backup role to the backrest user and ensure the > > > > backrest system user has read access to the database files (eg: by > > > > having the system user be a member of the 'postgres' group): > > > -- > > > > > > Just to clarify, the 'postgres' OS user group cannot read the data > > > directory, e.g. > > > > > > drwx-- 19 postgres staff 4096 Jan 17 12:19 data/ > > > ^^^group > > > > > > I assume we don't want to change that. > > > > This is going to be distribution dependent, unfortunately. On > > Debian-based distributions, the group is 'postgres' and it'd be > > perfectly reasonable to allow that group to read the data directory. > > Well, while the group name would be OS-dependent, the lack of any group > permisions in not OS-dependent and is forced by initdb: > > umask(S_IRWXG | S_IRWXO); > > create_data_directory(); Right, we also check in the backend on startup for certain permissions. I don't recall offhand if that's forced to 700 or if we allow 750. > > I don't recall offhand if that means we'd have to make changes to allow > > that, but, for my 2c, I don't see why we wouldn't allow it to be an > > option. > > OK, that would be an initdb change then. It would need to be optional, so distributions and users could choose which makes sense for their systems. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Sun, Jan 17, 2016 at 01:49:19PM -0500, Stephen Frost wrote: > * Bruce Momjian (br...@momjian.us) wrote: > > > pgbackrest: > > > > > > To run pgbackrest as a non-superuser and not the 'postgres' system > > > user, grant the pg_backup role to the backrest user and ensure the > > > backrest system user has read access to the database files (eg: by > > > having the system user be a member of the 'postgres' group): > > -- > > > > Just to clarify, the 'postgres' OS user group cannot read the data > > directory, e.g. > > > > drwx-- 19 postgres staff 4096 Jan 17 12:19 data/ > > ^^^group > > > > I assume we don't want to change that. > > This is going to be distribution dependent, unfortunately. On > Debian-based distributions, the group is 'postgres' and it'd be > perfectly reasonable to allow that group to read the data directory. Well, while the group name would be OS-dependent, the lack of any group permisions in not OS-dependent and is forced by initdb: umask(S_IRWXG | S_IRWXO); create_data_directory(); > I don't recall offhand if that means we'd have to make changes to allow > that, but, for my 2c, I don't see why we wouldn't allow it to be an > option. OK, that would be an initdb change then. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription + -- 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
* Bruce Momjian (br...@momjian.us) wrote: > On Mon, Jan 4, 2016 at 12:55:16PM -0500, Stephen Frost wrote: > > I'd like to be able to include, in both of those, a simple set of > > instructions for granting the necessary rights to the user who is > > running those processes. A set of rights which an administrator can go > > look up and easily read and understand the result of those grants. For > > example: > > > ... > > pgbackrest: > > > > To run pgbackrest as a non-superuser and not the 'postgres' system > > user, grant the pg_backup role to the backrest user and ensure the > > backrest system user has read access to the database files (eg: by > > having the system user be a member of the 'postgres' group): > -- > > Just to clarify, the 'postgres' OS user group cannot read the data > directory, e.g. > > drwx-- 19 postgres staff 4096 Jan 17 12:19 data/ > ^^^group > > I assume we don't want to change that. This is going to be distribution dependent, unfortunately. On Debian-based distributions, the group is 'postgres' and it'd be perfectly reasonable to allow that group to read the data directory. I don't recall offhand if that means we'd have to make changes to allow that, but, for my 2c, I don't see why we wouldn't allow it to be an option. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Mon, Jan 4, 2016 at 12:55:16PM -0500, Stephen Frost wrote: > I'd like to be able to include, in both of those, a simple set of > instructions for granting the necessary rights to the user who is > running those processes. A set of rights which an administrator can go > look up and easily read and understand the result of those grants. For > example: > ... > pgbackrest: > > To run pgbackrest as a non-superuser and not the 'postgres' system > user, grant the pg_backup role to the backrest user and ensure the > backrest system user has read access to the database files (eg: by > having the system user be a member of the 'postgres' group): -- Just to clarify, the 'postgres' OS user group cannot read the data directory, e.g. drwx-- 19 postgres staff 4096 Jan 17 12:19 data/ ^^^group I assume we don't want to change that. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription + -- 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
On Wed, Jan 6, 2016 at 11:13 AM, Stephen Frost wrote: > I just wanted to start off by saying thank you for taking the time read > and comment with your thoughts on this concept. I was a bit frustrated > about it feeling rather late, but appreciate the comments which have > been made as they've certainly been constructive. Thanks. >> Well, there's certainly some set of privileges that will make them all >> work. But it might include more than some of them want. If you done >> any analysis of this, I have not seen it posted to this thread. > > I can certainly work up a formal analysis and submit it for > consideration. I would be in favor of that. >> 1. This doesn't really seem like the same thing. You're introducing >> something quite new here: these are not role attributes that apply >> only to the role itself, but inheritable role attributes. > > This approach started out by adding role attributes to handle these > kinds of rights, but in discussion with Tom and Magnus, iirc (no, I > don't have the specific links or threads, though I have asked Magnus to > take a look at this thread, as he has time), the idea of default roles > seemed better specifically because they would then be inheritable and > granting access could also be delegated. I agree that predefined roles are better than additional role attributes. I don't agree that predefined roles are better than GRANT EXECUTE ON FUNCTION pg_catalog.blah(). I think full support for GRANT EXECUTE ON FUNCTION pg_catalog.blah() is undeniably useful and will be a very clear improvement over what we have now. I think predefined roles are a reasonable thing for somebody to want, but I don't think it's nearly as clear-cut, and I'm very much unconvinced that we know that the ones you're proposing are the right ones. >> 3. It was clear from the outset that the replication role's basic >> purpose was to be sufficient privilege for a streaming standby and no >> more. The remit of these roles is a lot less clear, at least to me. > > I've certainly intended the intention of these roles to be clear and > documented. The point I was trying to address above is that we > certainly appear fine to add additional privileges as new capabilities > are added to existing role attributes (the entire logical replication > system was added after the replication role attribute). Mmmph. I think we got lucky there as much as anything. replication already allows sufficient privilege to extract all data in the database, so allowing it to also request a logical change stream isn't really a privilege escalation. That's not going to be true for what you are proposing here. >> > Adding pg_dump dump and restore support for catalog ACLs implies that >> > we're supporting ACLs on all catalog objects- including tables. >> >> Not to me it doesn't. I think we could support it just for functions, >> and have it continue to be as weird as it is currently for other types >> of objects until somebody gets around to straightening that out. If >> we want to support it for more object types, great, but I don't think >> that's a hard requirement. > > Alright, that would certainly simplify things if we're talking about > only functions. The only concern which I have there is if there are any > non-function cases that we'll end up coming across, and I'm still a bit > nervous about the "old pg_dump / new database" restore concern, but > perhaps that's an acceptable issue. I wouldn't worry about that a bit. We recommend that users always pg_dump using the newest version of pg_dump, even if the dumped server is older. That advice will be entirely satisfactory in this case also. > Based on Noah's response (which I respond to below), we seem to still > be debating the whole concept of default roles. I'm happy to provide > detailed analysis if we're able to agree on the concept. I think you need to provide detailed analysis SO THAT we can agree on the concept. >> Oh, sure: they are not backup tools specifically. But anything that >> might need elevated privileges deserves consideration here: what sort >> of subdivision of the superuser role would make that need go away? > > The general approach which I've been using for the default roles is that > they should grant rights which aren't able to be used to trivially make > oneself a superuser. That's a good principle, but not a sufficient guide. >> Stop. Even if PostgreSQL introduces pg_monitor, check_postgres will do >> itself >> a favor by not using it. The moment the two projects' sense of monitoring >> privileges falls out of lockstep, benefits from using pg_monitor go negative. >> check_postgres should instead furnish a script that creates a role holding >> required privileges and/or SECURITY DEFINER helpers. If check_postgres >> starts >> using an object, say pgstattuple, it will wish to use it in all versions. >> Since PostgreSQL will change pg_monitor privileges in major releases only, >> check_postgres would wait 6-18
Re: [HACKERS] Additional role attributes && superuser review
Robert, Noah, I just wanted to start off by saying thank you for taking the time read and comment with your thoughts on this concept. I was a bit frustrated about it feeling rather late, but appreciate the comments which have been made as they've certainly been constructive. * Robert Haas (robertmh...@gmail.com) wrote: > On Mon, Jan 4, 2016 at 4:56 PM, Stephen Frost wrote: > >> First, it's not really going to matter to users very much whether the > >> command to enable one of these features is a single GRANT command or a > >> short sequence of GRANT commands executed one after another. So even > >> if we don't have roles that bundle together multiple permissions, you > >> will still be able to do this; you just might need more than one > >> command. Probably, I'm guessing, not very many commands, but more > >> than one. > > > > I disagree. I find that it does make a difference to users to be using > > a well documented and clear approach over one which involves fiddling > > with the individual pieces to get everything to work, and if a new > > function comes along that is useful for backup users, that would have to > > also be granted, even if it would clearly be useful to a backup role. > > How is that a fair way to characterize the discussion here? Just > because you have to execute several SQL commands instead of one > doesn't turn a "well-documented and clear approach" into something > that involves "fiddling with individual pieces". Cutting and pasting > a sequence of 3 or 4 SQL commands into a psql window is not a lot > harder than copying and pasting a single one, and does not turn a good > approach into a shambles. I was looking at it from a perspective of what we have currently vs. what the future state with default roles would be. That's not an entirely fair characterization. If we supported ACLs on catalog objects via pg_dump, then we could add documentation along the lines of "backups generally need access to functions X, Y, Z, here's an example of how to create such a role: blah, blah." Of course, that documentation would likely have to be repeated in the various backup tools, though it's possible they could tailor those, if there was something different about their particular tool. > >> Second, I think it's really unlikely that you're going to maintain > >> perfect alignment between your predefined roles and the permissions > >> that third-party tools need over the course of multiple releases. I > >> think the chances of that working out are just about zero. Sure, you > >> can make the initial set of permissions granted to pg_backup match > >> exactly what pgbackrest needs, but it's a good bet that one of the six > >> or eight widely-used backup tools uses something that's not included > >> in that set. > > > > There may be something I've missed with the proposed pg_backup role, but > > I don't believe you're correct that there isn't a set of privileges > > which all of those backup tools need and which could be provided through > > the pg_backup role. > > Well, there's certainly some set of privileges that will make them all > work. But it might include more than some of them want. If you done > any analysis of this, I have not seen it posted to this thread. I can certainly work up a formal analysis and submit it for consideration. > >> And even if not, it doesn't require very much > >> imagination to suppose that some tool, maybe pgbackrest or maybe > >> something else, that comes along over the next few releases will > >> require some extra permission. When that happens, will we include add > >> that permission to the pg_backup role, or not? > > > > As I pointed out previously, we've already been doing this with the > > replication role attribute and I don't recall any complaining about it. > > 1. This doesn't really seem like the same thing. You're introducing > something quite new here: these are not role attributes that apply > only to the role itself, but inheritable role attributes. This approach started out by adding role attributes to handle these kinds of rights, but in discussion with Tom and Magnus, iirc (no, I don't have the specific links or threads, though I have asked Magnus to take a look at this thread, as he has time), the idea of default roles seemed better specifically because they would then be inheritable and granting access could also be delegated. > 2. I believe that the discussion about what the replication role > should and should not allow involved considerably more people than > have discussed any of the specific roles you propose to add here. I didn't intend to dispute that, but... > 3. It was clear from the outset that the replication role's basic > purpose was to be sufficient privilege for a streaming standby and no > more. The remit of these roles is a lot less clear, at least to me. I've certainly intended the intention of these roles to be clear and documented. The point I was trying to address above is that we certainly app
Re: [HACKERS] Additional role attributes && superuser review
On Mon, Jan 04, 2016 at 12:55:16PM -0500, Stephen Frost wrote: > * Noah Misch (n...@leadboat.com) wrote: > > On Tue, Dec 29, 2015 at 08:35:50AM -0500, Stephen Frost wrote: > I'm approaching this largely from a 3rd-party application perspective. > There are two examples off-hand which I'm considering: > > check_postgres > pgbackrest > > I'd like to be able to include, in both of those, a simple set of > instructions for granting the necessary rights to the user who is > running those processes. A set of rights which an administrator can go > look up and easily read and understand the result of those grants. For > example: > > check_postgres: > > Most check_postgres sub-commands can be run without superuser > privileges by granting the pg_monitor role to the monitoring user: > > GRANT pg_monitor TO monitor; > > For information regarding the pg_monitor role, see: > http://www.postgresql.org/docs/current/static/roles/database-roles.html Stop. Even if PostgreSQL introduces pg_monitor, check_postgres will do itself a favor by not using it. The moment the two projects' sense of monitoring privileges falls out of lockstep, benefits from using pg_monitor go negative. check_postgres should instead furnish a script that creates a role holding required privileges and/or SECURITY DEFINER helpers. If check_postgres starts using an object, say pgstattuple, it will wish to use it in all versions. Since PostgreSQL will change pg_monitor privileges in major releases only, check_postgres would wait 6-18 months to use a privilege in just one of five supported versions. If PostgreSQL hackers ever disagree with check_postgres hackers about whether a privilege belongs in the pg_monitor role, then check_postgres will wish it had never used pg_monitor. For a sample controversy, some monitoring tool may well call pg_read_file, but that's arguably too much power to be giving _every_ monitoring tool. By the way, the pg_monitor role you were ready to commit does not cover today's check_postgres needs. Among restricted objects, check_postgres uses at least pg_ls_dir, pg_stats, pg_settings, and pg_stat_activity. Having said that, it remains premature to design predefined roles. It would be a waste to mobilize such a design effort with GRANT's limitation clouding the issue. > and, to reiterate what I said above, I'd rather have one abstraction for > these kinds of permissions instead of a mish-mash of instructions. The > difference I can imagine being between: > > For backups and monitoring, you can use default roles: > > GRANT pg_backup,pg_monitor to new_admin; > > but for other regular privileges such as rotating logfiles, or sending > signals to other processes, you have to explicitly GRANT permissions: > > GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO new_admin; > GRANT EXECUTE ON FUNCTION pg_signal_backend() TO new_admin; I don't mind having those two ways to transfer privilege. If I have to settle for one or the other, I pick the latter. > > > Further, I'm not convinced that adding support for dumping ACLs or, in > > > general, encouraging users to define their own ACLs on catalog objects > > > is a good idea. We certainly have no mechanism in place today for those > > > ACLs to be respected by SysCache and encouraging their use when we won't > > > actually respect them is likely to be confusing. > > > > What's this problem with syscache? It sounds important. > > CREATE TABLE and DROP TABLE aren't going to care one bit if you have > access to pg_class or not. The same goes for basically everything else. > > If we really want to support ACLs on the catalog, we'd have to either > caveat that none of the internal lookups will respect them or revamp > SysCache and any other direct catalog access to do permission checks > first, which I don't think we really want to do. Oh, that. Having internal lookups ignore the ACLs is more good than bad, and users have little cause to expect something different. You don't need INSERT on pg_attribute to add a column, so why expect lack of SELECT on pg_attribute to prevent dropping one? I might document it like so: While GRANT can modify the privileges of a system catalog table, that affects only queries that address the catalog as an SQL table. Internal, system access to the same underlying data will proceed normally. For example, "REVOKE SELECT ON pg_proc FROM PUBLIC" does not preclude calling functions or even preclude passing them to pg_get_functiondef. It does block queries that name pg_proc in a FROM clause. > This entire discussion of privileges-on-catalog-objects should really > also consider the ongoing discussion about providing policies for the > catalog via RLS. If we start pg_dump'ing the ACLs of catalog objects > then we'd, presumably, also want to pg_dump out any policies defined > against catalog objects. I would have no qualms supporting ACL changes while not supporting added policies, indexes, triggers,
Re: [HACKERS] Additional role attributes && superuser review
On Mon, Jan 4, 2016 at 5:22 PM, Stephen Frost wrote: >> So, is this another case where the support is all in off-list fora and >> thus invisible, or can you point to specific on-list discussions where >> it was supported, and to the opinions offered in support? I don't >> really remember many opinions that were any more positive than "I >> wouldn't be strongly opposed to this" or "If we're going to do this >> then we ought to do it in X way". I'm happy to be corrected if I'm >> misrepresenting the record, but I'd characterize the overall reaction >> to this proposal as tepid: nobody hated it, but nobody really loved it >> either, and a bunch of mild concerns were offered. > > I agree that this has largely been the on-list reaction. To be fair, > it's been largely the off-list reaction also, which I've expressly > tried to seek out, as mentioned above. I'm not asking anyone to love > it, I'm not entirely convinced it's lovable myself, but I do feel it's > useful and worth making an effort for. I think the question of whether the specific proposals on the table are in fact useful is one that deserves more study. I am not convinced of that. I believe something like this could be useful, but I don't see a lot of evidence that the particular roles you're arguing for actually are. > I'd love to have folks from other companies involved in these > discussions. I'll even reach out explicitly to seek their comment, as > I've done with other hackers at conferences, and try to get them to > voice their opinions here. Great, thanks. >> What really bothers me about this thread is that these predefined >> roles are intended to be useful for third-party tools, but the people >> who maintain those third-party tools have said basically nothing. > > For my 2c, I believe that to be, by-and-large, because they don't want > to get their hopes up until they see something actually get committed. > Following long and deep threads such as these are quite a committment. Yep. >> I >> don't recall, for example, Dave Page weighing in on what pgAdmin >> needs, or anybody commenting on to what degree any of these proposals >> would meet the needs of Slony or pgBouncer or pgPool or any backup >> tool (other than perhaps pgbackrest, which I assume your proposals >> cater to) or any monitoring tool. Like, we've heard zip. Either >> those people don't know this thread exists, or they can't understand >> it, or they think it's so boring that they can't be bothered to write >> in and say whether this is useful or not. I'd have a lot more >> confidence that we are making a good decision if some of those people >> would show up and say "I have reviewed this proposal and it looks { >> great | terrible | mediocre } for $TOOL because $REASON". > > We *have* heard complaints from people, multiple times on various lists, > that they'd like to set up check_postgres, Nagios, $MONITORINGTOOL, with > a role that *isn't* a superuser. True. But we should verify that this proposal actually meets those needs, not just assume it does. > I'll ask Greg S-M if he would have > time to weigh in on this though, check_postgres was specifically one of > the tools which I was looking at when considering the pg_monitor role. OK, that sounds like a good idea. > I'm not sure about the references you use above to Slony or pgBouncer or > pgPool as those aren't backup tools, to my mind.. I would expect barman > and other backup tools to also use pg_start/stop_backup and > pg_switch_xlog. I'm not sure that there's a way to cater to one backup > role when it comes to how filesystem-level backups are handled in PG, > but perhaps I've missed something there that barman uses and which isn't > included currently. Oh, sure: they are not backup tools specifically. But anything that might need elevated privileges deserves consideration here: what sort of subdivision of the superuser role would make that need go away? > Of course, my reviewing barman or other tools wouldn't have the same > support as Simon weighing in, so I'll try and pursue that avenue as > well. Cool. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
On Mon, Jan 4, 2016 at 4:56 PM, Stephen Frost wrote: >> First, it's not really going to matter to users very much whether the >> command to enable one of these features is a single GRANT command or a >> short sequence of GRANT commands executed one after another. So even >> if we don't have roles that bundle together multiple permissions, you >> will still be able to do this; you just might need more than one >> command. Probably, I'm guessing, not very many commands, but more >> than one. > > I disagree. I find that it does make a difference to users to be using > a well documented and clear approach over one which involves fiddling > with the individual pieces to get everything to work, and if a new > function comes along that is useful for backup users, that would have to > also be granted, even if it would clearly be useful to a backup role. How is that a fair way to characterize the discussion here? Just because you have to execute several SQL commands instead of one doesn't turn a "well-documented and clear approach" into something that involves "fiddling with individual pieces". Cutting and pasting a sequence of 3 or 4 SQL commands into a psql window is not a lot harder than copying and pasting a single one, and does not turn a good approach into a shambles. >> Second, I think it's really unlikely that you're going to maintain >> perfect alignment between your predefined roles and the permissions >> that third-party tools need over the course of multiple releases. I >> think the chances of that working out are just about zero. Sure, you >> can make the initial set of permissions granted to pg_backup match >> exactly what pgbackrest needs, but it's a good bet that one of the six >> or eight widely-used backup tools uses something that's not included >> in that set. > > There may be something I've missed with the proposed pg_backup role, but > I don't believe you're correct that there isn't a set of privileges > which all of those backup tools need and which could be provided through > the pg_backup role. Well, there's certainly some set of privileges that will make them all work. But it might include more than some of them want. If you done any analysis of this, I have not seen it posted to this thread. >> And even if not, it doesn't require very much >> imagination to suppose that some tool, maybe pgbackrest or maybe >> something else, that comes along over the next few releases will >> require some extra permission. When that happens, will we include add >> that permission to the pg_backup role, or not? > > As I pointed out previously, we've already been doing this with the > replication role attribute and I don't recall any complaining about it. 1. This doesn't really seem like the same thing. You're introducing something quite new here: these are not role attributes that apply only to the role itself, but inheritable role attributes. 2. I believe that the discussion about what the replication role should and should not allow involved considerably more people than have discussed any of the specific roles you propose to add here. 3. It was clear from the outset that the replication role's basic purpose was to be sufficient privilege for a streaming standby and no more. The remit of these roles is a lot less clear, at least to me. > I wasn't suggesting that we give everyone the same privileges to some > default 'pgAdmin' role but rather that providing an abstraction of the > set of privileges possible against the catalog objects, into sets that > make sense together, is a useful simplification for users and that it'd > be a better approach than asking users to figure out what sets make > sense on their own. I have no objection to that *in theory*. What's not clear to me is that the way that you have broken it up actually meets the bona fide needs of actual tools in a useful way. > Adding pg_dump dump and restore support for catalog ACLs implies that > we're supporting ACLs on all catalog objects- including tables. Not to me it doesn't. I think we could support it just for functions, and have it continue to be as weird as it is currently for other types of objects until somebody gets around to straightening that out. If we want to support it for more object types, great, but I don't think that's a hard requirement. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
* Robert Haas (robertmh...@gmail.com) wrote: > On Mon, Jan 4, 2016 at 3:07 PM, Stephen Frost wrote: > > I'm not sure it's entirely relevant now- I've outlined the reasoning in > > my email to Noah as a, hopefully, pretty comprehensive summary. If that > > doesn't sway your minds then it seems unlikely that a reference to a > > thread from 6 months or a year ago would. Further, as happens, other > > discussions were in person where I discussed the ideas with other > > hackers at conferences. I got generally positive responses to the idea > > of default roles with specific sets of rights, which is the path that > > I've been on since. As with most decisions, there was not a formal vote > > over the proposals for me to reference. I do specifically recall the > > opinion that sets of privileges probably make more sense than granting > > out individual ones, from Tom, if I'm remembering correctly. > > To be honest, that's not really my point. You have cited off-list > discussions you've had over and over as a reason why you proceeded > down some particular path. That is fine up to a point - I discuss > lots of things with people off-list, too - but a consensus that a > particular design is acceptable for commit has to mean the consensus > on this mailing list, and nothing else. In seven years of reading > this mailing list, I can't remember a single other person on this > mailing list saying "I'm going to commit this because I talked to a > bunch of unspecified people at conferences I attended and they liked > it", but you've used essentially that rationale a couple of times now. I've found consensus among folks on the lists for far more of my commits than I've cited off-list discussions for. Further, consensus on these lists is largely in the quiet, which is why I go out of my way to attempt to engage parties who may be interested when there *isn't* much discussion on the lists. I'm trying to do better than simply assuming consensus based on no feedback to the contrary. Had I assumed minimal response meant consensus for this patch, as is typically the norm, this patch would have been committed six months ago. Instead, I tried to engage people at conferences to ensure that there really was consensus on the approach. > > For my 2c, I don't see a default role or two as creating terribly much > > clutter. > > I don't believe any version of this proposal has involved only one or > two such roles. They've all had at least four or five AFAICS. Now > maybe that's still OK, but 4 or 5 > 1 or 2, and the number is only > likely to go up in the future. The 'role or two' was under the expectation that we'd still have default roles for the more complicated cases (pg_backup, et al) and that we would only be removing the default role of pg_switch_xlog and pg_file_settings (the only two which an individual GRANT could replace). > > Changing all of our code that currently has internal checks to > > rely on the external checks and adjusting the permissions on the > > individual functions will be a fair bit of churn though. > > I'm not sure it'll really be that bad, but if you have some evidence > that I'm wrong I'd like to hear it. I implemented the discussed pg_dump support for ACLs and looked at the changes required. I may not be remembering it entirely, but it's not that I've not looked at it. > >> More > >> broadly, I'm not very convinced that even the roles which allow for > >> rights on multiple objects are going to meet with general approval. > > > > There's been rather little oposition to the idea and support when I've > > discussed it. Of course, that was before it got to the point where I > > was planning to commit it. Perhaps there will be once it's actually in, > > or maybe not until it's in the wild. In any case, I continue to feel, > > as others have, that we can make adjustments moving forward. > > So, is this another case where the support is all in off-list fora and > thus invisible, or can you point to specific on-list discussions where > it was supported, and to the opinions offered in support? I don't > really remember many opinions that were any more positive than "I > wouldn't be strongly opposed to this" or "If we're going to do this > then we ought to do it in X way". I'm happy to be corrected if I'm > misrepresenting the record, but I'd characterize the overall reaction > to this proposal as tepid: nobody hated it, but nobody really loved it > either, and a bunch of mild concerns were offered. I agree that this has largely been the on-list reaction. To be fair, it's been largely the off-list reaction also, which I've expressly tried to seek out, as mentioned above. I'm not asking anyone to love it, I'm not entirely convinced it's lovable myself, but I do feel it's useful and worth making an effort for. > >> There has been enough discussion of which roles should be created and > >> which things should be included in each one, and the overall tenor of > >> that discussion
Re: [HACKERS] Additional role attributes && superuser review
* Robert Haas (robertmh...@gmail.com) wrote: > On Mon, Jan 4, 2016 at 12:55 PM, Stephen Frost wrote: > > I'd like to be able to include, in both of those, a simple set of > > instructions for granting the necessary rights to the user who is > > running those processes. A set of rights which an administrator can go > > look up and easily read and understand the result of those grants. For > > example: > > > > check_postgres: > > > > Most check_postgres sub-commands can be run without superuser > > privileges by granting the pg_monitor role to the monitoring user: > > > > GRANT pg_monitor TO monitor; > > > > For information regarding the pg_monitor role, see: > > http://www.postgresql.org/docs/current/static/roles/database-roles.html > > > > pgbackrest: > > > > To run pgbackrest as a non-superuser and not the 'postgres' system > > user, grant the pg_backup role to the backrest user and ensure the > > backrest system user has read access to the database files (eg: by > > having the system user be a member of the 'postgres' group): > > > > GRANT pg_backup to backrest; > > > > For information regarding the pg_backup role, see: > > http://www.postgresql.org/docs/current/static/roles/database-roles.html > > So I have two comments on this. > > First, it's not really going to matter to users very much whether the > command to enable one of these features is a single GRANT command or a > short sequence of GRANT commands executed one after another. So even > if we don't have roles that bundle together multiple permissions, you > will still be able to do this; you just might need more than one > command. Probably, I'm guessing, not very many commands, but more > than one. I disagree. I find that it does make a difference to users to be using a well documented and clear approach over one which involves fiddling with the individual pieces to get everything to work, and if a new function comes along that is useful for backup users, that would have to also be granted, even if it would clearly be useful to a backup role. > Second, I think it's really unlikely that you're going to maintain > perfect alignment between your predefined roles and the permissions > that third-party tools need over the course of multiple releases. I > think the chances of that working out are just about zero. Sure, you > can make the initial set of permissions granted to pg_backup match > exactly what pgbackrest needs, but it's a good bet that one of the six > or eight widely-used backup tools uses something that's not included > in that set. There may be something I've missed with the proposed pg_backup role, but I don't believe you're correct that there isn't a set of privileges which all of those backup tools need and which could be provided through the pg_backup role. > And even if not, it doesn't require very much > imagination to suppose that some tool, maybe pgbackrest or maybe > something else, that comes along over the next few releases will > require some extra permission. When that happens, will we include add > that permission to the pg_backup role, or not? If we do, then we'll > be giving excess permissions to all the other backup tools that don't > need that new right, and maybe surprising users who upgrade without > realizing that some of their roles now have new rights. If we don't, > then that tool won't be able to work without some additional > twiddling. I just find it incredibly unlikely that every monitoring > tool, or every backup tool, or every admin tool will require exactly > the same set of permissions. As I pointed out previously, we've already been doing this with the replication role attribute and I don't recall any complaining about it. This discussion started out with the idea of adding more role attributes to address this need to break out superuser rights, as we have done in the past, and then moved to discussing default roles instead because it's a better solution for abstracting permissions as roles are more easily grantable, delegation of role granting can be done, and role membership works with inheritance. The arguments you raise here would apply nearly the same to the replication role attribute, but in practice, we don't seem to have any question regarding how that's handled, nor have we gotten complaints about it. I expect the same would be true with default roles and don't see any particular reason to fear otherwise. > > I can see similar bits of documentation being included in pgAdmin or > > other tools. > > ...and pgAdmin is a particularly good example. The permissions that > pgAdmin requires depend on what you want to do with it, and it does a > lot of things, and it might do more or fewer things in the future. > You can't even fairly assume that everyone wants to give the same > permissions to pgAdmin, let alone that some competing tool will > require the same set. I wasn't suggesting that we give everyone the same privileges to some default 'pgAdmin' r
Re: [HACKERS] Additional role attributes && superuser review
On Mon, Jan 4, 2016 at 3:07 PM, Stephen Frost wrote: > I'm not sure it's entirely relevant now- I've outlined the reasoning in > my email to Noah as a, hopefully, pretty comprehensive summary. If that > doesn't sway your minds then it seems unlikely that a reference to a > thread from 6 months or a year ago would. Further, as happens, other > discussions were in person where I discussed the ideas with other > hackers at conferences. I got generally positive responses to the idea > of default roles with specific sets of rights, which is the path that > I've been on since. As with most decisions, there was not a formal vote > over the proposals for me to reference. I do specifically recall the > opinion that sets of privileges probably make more sense than granting > out individual ones, from Tom, if I'm remembering correctly. To be honest, that's not really my point. You have cited off-list discussions you've had over and over as a reason why you proceeded down some particular path. That is fine up to a point - I discuss lots of things with people off-list, too - but a consensus that a particular design is acceptable for commit has to mean the consensus on this mailing list, and nothing else. In seven years of reading this mailing list, I can't remember a single other person on this mailing list saying "I'm going to commit this because I talked to a bunch of unspecified people at conferences I attended and they liked it", but you've used essentially that rationale a couple of times now. > For my 2c, I don't see a default role or two as creating terribly much > clutter. I don't believe any version of this proposal has involved only one or two such roles. They've all had at least four or five AFAICS. Now maybe that's still OK, but 4 or 5 > 1 or 2, and the number is only likely to go up in the future. > Changing all of our code that currently has internal checks to > rely on the external checks and adjusting the permissions on the > individual functions will be a fair bit of churn though. I'm not sure it'll really be that bad, but if you have some evidence that I'm wrong I'd like to hear it. >> More >> broadly, I'm not very convinced that even the roles which allow for >> rights on multiple objects are going to meet with general approval. > > There's been rather little oposition to the idea and support when I've > discussed it. Of course, that was before it got to the point where I > was planning to commit it. Perhaps there will be once it's actually in, > or maybe not until it's in the wild. In any case, I continue to feel, > as others have, that we can make adjustments moving forward. So, is this another case where the support is all in off-list fora and thus invisible, or can you point to specific on-list discussions where it was supported, and to the opinions offered in support? I don't really remember many opinions that were any more positive than "I wouldn't be strongly opposed to this" or "If we're going to do this then we ought to do it in X way". I'm happy to be corrected if I'm misrepresenting the record, but I'd characterize the overall reaction to this proposal as tepid: nobody hated it, but nobody really loved it either, and a bunch of mild concerns were offered. >> There has been enough discussion of which roles should be created and >> which things should be included in each one, and the overall tenor of >> that discussion seems to be that different people have different >> ideas. > > Michael had a question about pg_switch_xlog, but he appeared to > reconsider that position after the subsequent discussion, which put us > back to essentially the same proposal that I started with, I believe. I > don't recall terribly much other discussion or concern about what roles > should be created or what should be included in each one, though I'd be > happy to hear your thoughts on what you'd like to see. Honestly, my vote is for creating only those predefined roles that are clearly endorsed by three people with different employers, which I currently believe to be true of none of the proposed roles. It's not even that I suspect you or anyone of ballot-stuffing; it's just that people who work at different companies are likely to work with different tools and those tools may have different requirements. I mean, people at 2ndQuadrant probably mostly use repmgr; people at Crunchy probably like pgbackrest; people at OmniTI probably use PITRtools; and EnterpriseDB employees are more likely than average to be familiar with BART. If several of those people come together and say they all agree that predefined role X is perfect for the needs of all of their respective tools, I'd consider that a really good sign that we've hit on something that is of general utility. Otherwise, I'd just the authors of each tool specify the GRANT EXECUTE ON FUNCTION lines necessary for their own tool and call it good. I think that's almost as convenient and a lot more flexible. What really bothers me abou
Re: [HACKERS] Additional role attributes && superuser review
On Mon, Jan 4, 2016 at 12:55 PM, Stephen Frost wrote: > I'd like to be able to include, in both of those, a simple set of > instructions for granting the necessary rights to the user who is > running those processes. A set of rights which an administrator can go > look up and easily read and understand the result of those grants. For > example: > > check_postgres: > > Most check_postgres sub-commands can be run without superuser > privileges by granting the pg_monitor role to the monitoring user: > > GRANT pg_monitor TO monitor; > > For information regarding the pg_monitor role, see: > http://www.postgresql.org/docs/current/static/roles/database-roles.html > > pgbackrest: > > To run pgbackrest as a non-superuser and not the 'postgres' system > user, grant the pg_backup role to the backrest user and ensure the > backrest system user has read access to the database files (eg: by > having the system user be a member of the 'postgres' group): > > GRANT pg_backup to backrest; > > For information regarding the pg_backup role, see: > http://www.postgresql.org/docs/current/static/roles/database-roles.html So I have two comments on this. First, it's not really going to matter to users very much whether the command to enable one of these features is a single GRANT command or a short sequence of GRANT commands executed one after another. So even if we don't have roles that bundle together multiple permissions, you will still be able to do this; you just might need more than one command. Probably, I'm guessing, not very many commands, but more than one. Second, I think it's really unlikely that you're going to maintain perfect alignment between your predefined roles and the permissions that third-party tools need over the course of multiple releases. I think the chances of that working out are just about zero. Sure, you can make the initial set of permissions granted to pg_backup match exactly what pgbackrest needs, but it's a good bet that one of the six or eight widely-used backup tools uses something that's not included in that set. And even if not, it doesn't require very much imagination to suppose that some tool, maybe pgbackrest or maybe something else, that comes along over the next few releases will require some extra permission. When that happens, will we include add that permission to the pg_backup role, or not? If we do, then we'll be giving excess permissions to all the other backup tools that don't need that new right, and maybe surprising users who upgrade without realizing that some of their roles now have new rights. If we don't, then that tool won't be able to work without some additional twiddling. I just find it incredibly unlikely that every monitoring tool, or every backup tool, or every admin tool will require exactly the same set of permissions. > I can see similar bits of documentation being included in pgAdmin or > other tools. ...and pgAdmin is a particularly good example. The permissions that pgAdmin requires depend on what you want to do with it, and it does a lot of things, and it might do more or fewer things in the future. You can't even fairly assume that everyone wants to give the same permissions to pgAdmin, let alone that some competing tool will require the same set. >> Being narrowly tied to a specific function, it's just a suboptimal spelling >> of >> GRANT. The gap in GRANT has distorted the design for these predefined roles. >> I do not anticipate a sound design discussion about specific predefined roles >> so long as the state of GRANT clouds the matter. > > I'm loathe to encourage any direct modification of catalog objects, > even if it's just ACLs. I've seen too many cases, as I imagine others > have, of users destroying their databases or running into unexpected > results when modifying the catalog. The catalog modifications supported > should be explicitly provided through other means rather than direct > commands against the catalog objects. I see the default roles approach > as being similar to having: > > ALTER DATABASE db WITH CONNECTION LIMIT 5; > > instead of suggesting users issue: > > UPDATE DATABASE SET datconnlimit = 5 WHERE datname = 'db'; This doesn't make any sense to me. Nobody was proposing issuing an UPDATE against pg_database directly (and it would have to be pg_database, not just database as you wrote here). We're talking about whether the user is going to GRANT pg_rotate_logfile TO ... or GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO ... which is not the same sort of thing at all. >> What's this problem with syscache? It sounds important. > > CREATE TABLE and DROP TABLE aren't going to care one bit if you have > access to pg_class or not. The same goes for basically everything else. > > If we really want to support ACLs on the catalog, we'd have to either > caveat that none of the internal lookups will respect them or revamp > SysCache and any other direct catalog access to do permission checks > f
Re: [HACKERS] Additional role attributes && superuser review
* Michael Paquier (michael.paqu...@gmail.com) wrote: > On Thu, Dec 31, 2015 at 4:26 PM, Noah Misch wrote: > > The proposed pg_replication role introduces abstraction that could, as you > > hope, spare a DBA from studying sets of functions to grant together. The > > pg_rotate_logfile role, however, does not shield the DBA from complexity. > > Being narrowly tied to a specific function, it's just a suboptimal spelling > > of > > GRANT. The gap in GRANT has distorted the design for these predefined > > roles. > > I do not anticipate a sound design discussion about specific predefined > > roles > > so long as the state of GRANT clouds the matter. > > As the patch stands, the system roles that are just close to synonyms > of GRANT/REVOKE are the following: > - pg_file_settings, which works just on the system view > pg_file_settings and the function pg_show_all_file_settings(). > - pg_rotate_logfile as mentioned already. The above are fine, I believe. > - pg_signal_backend, which is just checked once in pg_signal_backend This is a bit more complicated. pg_signal_backend() isn't directly exposed, pg_cancel_backend() and pg_termiante_backend() are. I'm not saying that doesn't mean we should, necessairly, keep the pg_signal_backend role, just that it's more than just a single function. Further, pg_terminate_backend and pg_cancel_backend are callable by anyone today. We'd have to invent new functions or change user-visible behavior in an unfortunate way- we don't want *anyone* to be able to call those functions on *anyone* unless they've been specifically granted that access. Today, you can cancel and/or terminate your own backends and superusers can cancel and/or termiante anyone's. The point of pg_signal_backend was to allow non-superusers to cancel and/or terminate any non-superuser-started backends. With the default role approach, we can provide exactly the intended semantics and not break backwards compatibility for those functions. > Based on those concerns, it looks clear that they should be shaved off > from the patch. I'd like to hear back regarding the summary email that I sent to Noah and the follow-up I sent to Robert, as they have time to reply, of course. It's certainly easy enough to remove those default roles if that's the consensus though. > >> > To summarize, I think the right next step is to resume designing pg_dump > >> > support for system object ACLs. I looked over your other two patches > >> > and will > >> > unshelve those reviews when their time comes. > >> > >> To be clear, I don't believe the two patches are particularly involved > >> with each other and don't feel that one needs to wait for the other. > > > > Patch 2/3 could stand without patch 3/3, but not vice-versa. It's patch 2/3 > > that makes pg_dumpall skip ^pg_ roles, and that must be in place no later > > than > > the first patch that adds a predefined ^pg_ role. > > I am a bit confused by this statement, and I disagree with Stephen's > point as well. It seems to me that 2/3 exists *because* 3/3 is here. > Why would we want to restrict the role names that can be used if we > are not going to introduce some system roles that are created at > bootstrap? My comment was, evidently, not anywhere near clear enough. The two patches I was referring to were the pg_dump-support-for-catalog-ACLs and the default-roles patches. Apologies for the confusion there. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
* Robert Haas (robertmh...@gmail.com) wrote: > On Tue, Dec 29, 2015 at 5:35 AM, Stephen Frost wrote: > > * Noah Misch (n...@leadboat.com) wrote: > >> > Updated patch attached. I'll give it another good look and then commit > >> > it, barring objections. > >> > >> This thread and its satellite[1] have worked their way through a few > >> designs. > >> At first, it was adding role attributes, alongside existing attributes like > >> REPLICATION and BYPASSRLS. It switched[2] to making pg_dump preserve ACLs > >> on > >> system objects. Built-in roles joined[3] the pg_dump work to offer > >> predefined > >> collections of ACL grants. Finally, it dropped[4] the pg_dump side and > >> hard-coded the roles into the features they govern. > > > > Correct, after quite a bit of discussion and the conclusion that, while > > pg_dump support for dumping ACLs might be interesting, it was quite a > > bit more complex an approach than this use-case justified. > > Hmm. I don't think I agree with that conclusion. Who were the > participants in that discussion, and how many people spoke in favor > from moving on from that proposal - which I rather liked - to what > you've got now? Do you have links to the relevant portion of the > relevant thread? I'm not sure it's entirely relevant now- I've outlined the reasoning in my email to Noah as a, hopefully, pretty comprehensive summary. If that doesn't sway your minds then it seems unlikely that a reference to a thread from 6 months or a year ago would. Further, as happens, other discussions were in person where I discussed the ideas with other hackers at conferences. I got generally positive responses to the idea of default roles with specific sets of rights, which is the path that I've been on since. As with most decisions, there was not a formal vote over the proposals for me to reference. I do specifically recall the opinion that sets of privileges probably make more sense than granting out individual ones, from Tom, if I'm remembering correctly. In any case, I can work on the pg_dump support for catalog ACLs if there is agreement now on that being the direction to go in. If there's agreement that we are happy with the idea of default roles also then I can strip out those few which are only one-permission and which therefore wouldn't be necessary, if we had ACL support on catalog objects, quite easily. > I think it's not a very good thing if we add roles that allow, say, > execution of exactly one SQL function. The > dump-grants-on-system-objects proposal would accomplish the same thing > in a much more flexible way, and with less catalog clutter. For my 2c, I don't see a default role or two as creating terribly much clutter. Changing all of our code that currently has internal checks to rely on the external checks and adjusting the permissions on the individual functions will be a fair bit of churn though. > More > broadly, I'm not very convinced that even the roles which allow for > rights on multiple objects are going to meet with general approval. There's been rather little oposition to the idea and support when I've discussed it. Of course, that was before it got to the point where I was planning to commit it. Perhaps there will be once it's actually in, or maybe not until it's in the wild. In any case, I continue to feel, as others have, that we can make adjustments moving forward. > There has been enough discussion of which roles should be created and > which things should be included in each one, and the overall tenor of > that discussion seems to be that different people have different > ideas. Michael had a question about pg_switch_xlog, but he appeared to reconsider that position after the subsequent discussion, which put us back to essentially the same proposal that I started with, I believe. I don't recall terribly much other discussion or concern about what roles should be created or what should be included in each one, though I'd be happy to hear your thoughts on what you'd like to see. I certainly don't like the idea of punting on all of this to the user to figure out, even if it does meet the specific requirements our clients have asked for, it strikes me that we can do better. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Noah, * Noah Misch (n...@leadboat.com) wrote: > On Tue, Dec 29, 2015 at 08:35:50AM -0500, Stephen Frost wrote: > > * Noah Misch (n...@leadboat.com) wrote: > > > The one argument which you've put forth for adding the complexity of > > dumping catalog ACLs is that we might reduce the number of default > > roles provided to the user. > > Right. If "GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO mydba" worked as > well as it works on user-defined functions, the community would not choose to > add a pg_rotate_logfile role holding just that one permission. I understand that's your position, but I disagree with your conclusion. If we're going to provide default roles, which I continue to feel is a good approach, then I would suggest we use them as an abstraction for the permissions which we view as senible sets of grantable rights. I dislike the idea of having default roles and then making an exception for single-permission cases. I'm approaching this largely from a 3rd-party application perspective. There are two examples off-hand which I'm considering: check_postgres pgbackrest I'd like to be able to include, in both of those, a simple set of instructions for granting the necessary rights to the user who is running those processes. A set of rights which an administrator can go look up and easily read and understand the result of those grants. For example: check_postgres: Most check_postgres sub-commands can be run without superuser privileges by granting the pg_monitor role to the monitoring user: GRANT pg_monitor TO monitor; For information regarding the pg_monitor role, see: http://www.postgresql.org/docs/current/static/roles/database-roles.html pgbackrest: To run pgbackrest as a non-superuser and not the 'postgres' system user, grant the pg_backup role to the backrest user and ensure the backrest system user has read access to the database files (eg: by having the system user be a member of the 'postgres' group): GRANT pg_backup to backrest; For information regarding the pg_backup role, see: http://www.postgresql.org/docs/current/static/roles/database-roles.html I can see similar bits of documentation being included in pgAdmin or other tools. For the pg_rotate_logfile permission, specifically, we were asked by a client about that permission with the use case being a logrotate-type of tool, which only has access to the log files but needs to be able to perform a rotation. This particular client is pretty tech savvy and I don't think they'd have a problem using GRANT EXECUTE if that was the only option, but I can see a similar use-case with logrotate or pgAdmin or even for regular non-superuser admins using psql and, to reiterate what I said above, I'd rather have one abstraction for these kinds of permissions instead of a mish-mash of instructions. The difference I can imagine being between: For backups and monitoring, you can use default roles: GRANT pg_backup,pg_monitor to new_admin; but for other regular privileges such as rotating logfiles, or sending signals to other processes, you have to explicitly GRANT permissions: GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO new_admin; GRANT EXECUTE ON FUNCTION pg_signal_backend() TO new_admin; > > I disagree that we would. Having a single > > set of default roles which provide a sensible breakdown of permissions > > is a better approach than asking every administrator and application > > developer who is building tools on top of PG to try and work through > > what makes sense themselves, even if that means we have a default role > > with a small, or even only an individual, capability. > > The proposed pg_replication role introduces abstraction that could, as you > hope, spare a DBA from studying sets of functions to grant together. The > pg_rotate_logfile role, however, does not shield the DBA from complexity. I disagree with the above statement. Having to understand only one level of abstraction (the default roles) does reduce the complexity and means that the DBA does not have to work out if the specifc GRANT requested by the end user would result in some other access or if there are any unexpected issues to encounter with issuing GRANTs directly on catalog objects- something we don't currently support, so such concern is certainly reasonable. > Being narrowly tied to a specific function, it's just a suboptimal spelling of > GRANT. The gap in GRANT has distorted the design for these predefined roles. > I do not anticipate a sound design discussion about specific predefined roles > so long as the state of GRANT clouds the matter. I'm loathe to encourage any direct modification of catalog objects, even if it's just ACLs. I've seen too many cases, as I imagine others have, of users destroying their databases or running into unexpected results when modifying the catalog. The catalog modifications supported should be explicitly provided through other means rather than direct commands against the ca
Re: [HACKERS] Additional role attributes && superuser review
Based on the feedback here, I have returned this patch to Needs Review status. (Waiting on Author would be fairer actually, since we are waiting for an updated version.) As far as I can make it from Noah and Robert's comments, what we would like to see here is a way for pg_dump to output nondefault grants to system objects; that would solve part of the need here where this patch proposes default roles for one or two system objects. Another part of this patch, which could presumably be committed independently, is the addition of default roles that are related to larger sets of system objects. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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
On Thu, Dec 31, 2015 at 4:26 PM, Noah Misch wrote: > On Tue, Dec 29, 2015 at 08:35:50AM -0500, Stephen Frost wrote: >> * Noah Misch (n...@leadboat.com) wrote: >> I disagree that we would. Having a single >> set of default roles which provide a sensible breakdown of permissions >> is a better approach than asking every administrator and application >> developer who is building tools on top of PG to try and work through >> what makes sense themselves, even if that means we have a default role >> with a small, or even only an individual, capability. > > The proposed pg_replication role introduces abstraction that could, as you > hope, spare a DBA from studying sets of functions to grant together. The > pg_rotate_logfile role, however, does not shield the DBA from complexity. > Being narrowly tied to a specific function, it's just a suboptimal spelling of > GRANT. The gap in GRANT has distorted the design for these predefined roles. > I do not anticipate a sound design discussion about specific predefined roles > so long as the state of GRANT clouds the matter. As the patch stands, the system roles that are just close to synonyms of GRANT/REVOKE are the following: - pg_file_settings, which works just on the system view pg_file_settings and the function pg_show_all_file_settings(). - pg_rotate_logfile as mentioned already. - pg_signal_backend, which is just checked once in pg_signal_backend Based on those concerns, it looks clear that they should be shaved off from the patch. >> > To summarize, I think the right next step is to resume designing pg_dump >> > support for system object ACLs. I looked over your other two patches and >> > will >> > unshelve those reviews when their time comes. >> >> To be clear, I don't believe the two patches are particularly involved >> with each other and don't feel that one needs to wait for the other. > > Patch 2/3 could stand without patch 3/3, but not vice-versa. It's patch 2/3 > that makes pg_dumpall skip ^pg_ roles, and that must be in place no later than > the first patch that adds a predefined ^pg_ role. I am a bit confused by this statement, and I disagree with Stephen's point as well. It seems to me that 2/3 exists *because* 3/3 is here. Why would we want to restrict the role names that can be used if we are not going to introduce some system roles that are created at bootstrap? -- Michael -- 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
* Michael Paquier (michael.paqu...@gmail.com) wrote: > On Tue, Dec 29, 2015 at 11:55 PM, Stephen Frost wrote: > > I could go either way on that, really. I don't find namespace to be > > confusing when used in that way, but I'll change it since others do. > > It seems to me that the way patch does it is fine.. Alright, fair enough. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Tue, Dec 29, 2015 at 08:35:50AM -0500, Stephen Frost wrote: > * Noah Misch (n...@leadboat.com) wrote: > The one argument which you've put forth for adding the complexity of > dumping catalog ACLs is that we might reduce the number of default > roles provided to the user. Right. If "GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO mydba" worked as well as it works on user-defined functions, the community would not choose to add a pg_rotate_logfile role holding just that one permission. > I disagree that we would. Having a single > set of default roles which provide a sensible breakdown of permissions > is a better approach than asking every administrator and application > developer who is building tools on top of PG to try and work through > what makes sense themselves, even if that means we have a default role > with a small, or even only an individual, capability. The proposed pg_replication role introduces abstraction that could, as you hope, spare a DBA from studying sets of functions to grant together. The pg_rotate_logfile role, however, does not shield the DBA from complexity. Being narrowly tied to a specific function, it's just a suboptimal spelling of GRANT. The gap in GRANT has distorted the design for these predefined roles. I do not anticipate a sound design discussion about specific predefined roles so long as the state of GRANT clouds the matter. > > To summarize, I think the right next step is to resume designing pg_dump > > support for system object ACLs. I looked over your other two patches and > > will > > unshelve those reviews when their time comes. > > To be clear, I don't believe the two patches are particularly involved > with each other and don't feel that one needs to wait for the other. Patch 2/3 could stand without patch 3/3, but not vice-versa. It's patch 2/3 that makes pg_dumpall skip ^pg_ roles, and that must be in place no later than the first patch that adds a predefined ^pg_ role. > Further, I'm not convinced that adding support for dumping ACLs or, in > general, encouraging users to define their own ACLs on catalog objects > is a good idea. We certainly have no mechanism in place today for those > ACLs to be respected by SysCache and encouraging their use when we won't > actually respect them is likely to be confusing. What's this problem with syscache? It sounds important. nm -- 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
On Thu, Dec 31, 2015 at 1:50 AM, Robert Haas wrote: > Under those circumstances, it seems very dubious to proceed > with this. Michael seems to think that we can go ahead and start > changing things and sort out whatever is broken later, but that > doesn't sound like a very good plan to me. I meant [snip]tuning those roles during this development cycle.[/snip] But I'll just withdraw as there are enough concerns, from two committers on top of it. My point was just to move on with this patch in the direction where the overall consensus seemed to be at the point I begun participating in this thread. -- Michael -- 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
On Tue, Dec 29, 2015 at 5:35 AM, Stephen Frost wrote: > * Noah Misch (n...@leadboat.com) wrote: >> > Updated patch attached. I'll give it another good look and then commit >> > it, barring objections. >> >> This thread and its satellite[1] have worked their way through a few designs. >> At first, it was adding role attributes, alongside existing attributes like >> REPLICATION and BYPASSRLS. It switched[2] to making pg_dump preserve ACLs on >> system objects. Built-in roles joined[3] the pg_dump work to offer >> predefined >> collections of ACL grants. Finally, it dropped[4] the pg_dump side and >> hard-coded the roles into the features they govern. > > Correct, after quite a bit of discussion and the conclusion that, while > pg_dump support for dumping ACLs might be interesting, it was quite a > bit more complex an approach than this use-case justified. Hmm. I don't think I agree with that conclusion. Who were the participants in that discussion, and how many people spoke in favor from moving on from that proposal - which I rather liked - to what you've got now? Do you have links to the relevant portion of the relevant thread? I think it's not a very good thing if we add roles that allow, say, execution of exactly one SQL function. The dump-grants-on-system-objects proposal would accomplish the same thing in a much more flexible way, and with less catalog clutter. More broadly, I'm not very convinced that even the roles which allow for rights on multiple objects are going to meet with general approval. There has been enough discussion of which roles should be created and which things should be included in each one, and the overall tenor of that discussion seems to be that different people have different ideas. Under those circumstances, it seems very dubious to proceed with this. Michael seems to think that we can go ahead and start changing things and sort out whatever is broken later, but that doesn't sound like a very good plan to me. We can change the internals of a bad implementation later and replace it with a good implementation, but changing user-visible behavior once people have started relying on it is a lot harder. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
On Tue, Dec 29, 2015 at 11:55 PM, Stephen Frost wrote: > I could go either way on that, really. I don't find namespace to be > confusing when used in that way, but I'll change it since others do. It seems to me that the way patch does it is fine.. -- Michael -- 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
Amit, * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote: > On 2015/12/23 7:23, Stephen Frost wrote: > > Updated patch attached. I'll give it another good look and then commit > > it, barring objections. > > Just a minor nitpick about a code comment - > > /* > + * Check that the user is not trying to create a role in the reserved > + * "pg_" namespace. > + */ > +if (IsReservedName(stmt->role)) > > The wording may be slightly confusing, especially saying "... in ... > namespace". ISTM, "namespace" is fairly extensively used around the code > to mean something like "a schema's namespace". > > Could perhaps be reworded as: > > /* > + * Check that the user is not trying to create a role with reserved > + * name, ie, one starting with "pg_". > > If OK, there seems to be one more place further down in the patch with > similar wording. I could go either way on that, really. I don't find namespace to be confusing when used in that way, but I'll change it since others do. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Noah, * Noah Misch (n...@leadboat.com) wrote: > > Updated patch attached. I'll give it another good look and then commit > > it, barring objections. > > This thread and its satellite[1] have worked their way through a few designs. > At first, it was adding role attributes, alongside existing attributes like > REPLICATION and BYPASSRLS. It switched[2] to making pg_dump preserve ACLs on > system objects. Built-in roles joined[3] the pg_dump work to offer predefined > collections of ACL grants. Finally, it dropped[4] the pg_dump side and > hard-coded the roles into the features they govern. Correct, after quite a bit of discussion and the conclusion that, while pg_dump support for dumping ACLs might be interesting, it was quite a bit more complex an approach than this use-case justified. Further, adding support to pg_dump for dumping ACLs could be done independently of default roles. The one argument which you've put forth for adding the complexity of dumping catalog ACLs is that we might reduce the number of default roles provided to the user. I disagree that we would. Having a single set of default roles which provide a sensible breakdown of permissions is a better approach than asking every administrator and application developer who is building tools on top of PG to try and work through what makes sense themselves, even if that means we have a default role with a small, or even only an individual, capability. I also disagree that we won't be able to adjust the privileges granted to a role in the future. We have certainly made adjustments to what a 'replication' role is able to do, which has largely been in the 'more capabilities' direction that you opine concern over. > To summarize, I think the right next step is to resume designing pg_dump > support for system object ACLs. I looked over your other two patches and will > unshelve those reviews when their time comes. To be clear, I don't believe the two patches are particularly involved with each other and don't feel that one needs to wait for the other. Further, I'm not convinced that adding support for dumping ACLs or, in general, encouraging users to define their own ACLs on catalog objects is a good idea. We certainly have no mechanism in place today for those ACLs to be respected by SysCache and encouraging their use when we won't actually respect them is likely to be confusing. I had thought differently at one point but my position changed during the discussion when I realized the complexity and potential confusion it could cause and considered that against the simplicity and relatively low cost of having default roles. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Hi, On 2015/12/23 7:23, Stephen Frost wrote: > Updated patch attached. I'll give it another good look and then commit > it, barring objections. Just a minor nitpick about a code comment - /* + * Check that the user is not trying to create a role in the reserved + * "pg_" namespace. + */ +if (IsReservedName(stmt->role)) The wording may be slightly confusing, especially saying "... in ... namespace". ISTM, "namespace" is fairly extensively used around the code to mean something like "a schema's namespace". Could perhaps be reworded as: /* + * Check that the user is not trying to create a role with reserved + * name, ie, one starting with "pg_". If OK, there seems to be one more place further down in the patch with similar wording. Thanks, Amit -- 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
On Tue, Dec 22, 2015 at 05:23:47PM -0500, Stephen Frost wrote: > > >> On Tue, Dec 22, 2015 at 1:41 AM, Stephen Frost > > >> wrote: > > >>> Updated and rebased patch attached which takes the 'pg_switch_xlog' > > >>> default role back out, leaving us with: > > >>> > > >>> pg_monitor - View privileged info > > >>> pg_backup - start/stop backups, switch xlog, create restore points > > >>> pg_replay - Pause/resume xlog replay on replicas > > >>> pg_replication - Create/destroy/etc replication slots > > >>> pg_rotate_logfile - Request logfile rotation > > >>> pg_signal_backend - Signal other backends (cancel query/terminate) > > >>> pg_file_settings - View configuration settings in all config files > Updated patch attached. I'll give it another good look and then commit > it, barring objections. This thread and its satellite[1] have worked their way through a few designs. At first, it was adding role attributes, alongside existing attributes like REPLICATION and BYPASSRLS. It switched[2] to making pg_dump preserve ACLs on system objects. Built-in roles joined[3] the pg_dump work to offer predefined collections of ACL grants. Finally, it dropped[4] the pg_dump side and hard-coded the roles into the features they govern. I find pg_dump support for system object ACLs to be the best of the goals pursued here. [5] proposed a good division of pg_dump+roles into multiple patches, and the pg_dump support for system object ACLs belongs as the first of the series. There's nothing to like about today's behavior of allowing the GRANT but omitting it from dumps, and attacking the problem at that layer will provide considerable admin freedom. Starting there is important, because it will change the roles design. I doubt we would add role pg_rotate_logfile if "GRANT EXECUTE ON FUNCTION pg_rotate_logfile() TO mydba" were fully usable; likewise for other roles that would carry a single ACL. Contrary to comments upthread, we won't be particularly free to redefine the scope of built-in roles later. Removing privileges from a role will be an ordinary compatibility break, and adding privileges will be quite sensitive. Your first patch, to catalogs.sgml, stands alone. May as well get that out of the way. To answer one question you asked a few times, reserving the pg_ role prefix is fine. (Whether any particular pg_foo role proposal sinks or floats is a separate question.) To summarize, I think the right next step is to resume designing pg_dump support for system object ACLs. I looked over your other two patches and will unshelve those reviews when their time comes. Thanks, nm [1] http://www.postgresql.org/message-id/flat/20150508042928.gp30...@tamriel.snowman.net [2] http://www.postgresql.org/message-id/20150402045311.gw3...@tamriel.snowman.net [3] http://www.postgresql.org/message-id/20150429144722.gy30...@tamriel.snowman.net [4] http://www.postgresql.org/message-id/20150508042928.gp30...@tamriel.snowman.net [5] http://www.postgresql.org/message-id/CA+TgmobH4tdccajn7VmPT-1RqBdzLYcAz5jUz4bJ=rkqs_g...@mail.gmail.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] Additional role attributes && superuser review
* Michael Paquier (michael.paqu...@gmail.com) wrote: > On Tue, Dec 22, 2015 at 2:54 PM, Amit Langote > wrote: > > On 2015/12/22 14:05, Michael Paquier wrote: > >> On Tue, Dec 22, 2015 at 1:41 AM, Stephen Frost wrote: > >>> Updated and rebased patch attached which takes the 'pg_switch_xlog' > >>> default role back out, leaving us with: > >>> > >>> pg_monitor - View privileged info > >>> pg_backup - start/stop backups, switch xlog, create restore points > >>> pg_replay - Pause/resume xlog replay on replicas > >>> pg_replication - Create/destroy/etc replication slots > >>> pg_rotate_logfile - Request logfile rotation > >>> pg_signal_backend - Signal other backends (cancel query/terminate) > >>> pg_file_settings - View configuration settings in all config files > >> > >> Thanks. This looks fine to me. I just have one single comment: > >> > >> + Request logfile rotation. > >> s/logfile/transaction log file/ > > > > Looks correct as is. Or maybe "server's log file" as in: > > > > 9.26.2. Server Signaling Functions > > > > pg_rotate_logfile(): Rotate server's log file > > You're right, this is not a WAL segment, just a normal log file. Your > phrasing is better. Works for me. Updated patch attached. I'll give it another good look and then commit it, barring objections. Thanks! Stephen From f493051be96514c0f0e178ef74d6d824f702a7c2 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Wed, 18 Nov 2015 11:50:57 -0500 Subject: [PATCH 1/3] Add note regarding permissions in pg_catalog Add a note to the system catalog section pointing out that while modifying the permissions on catalog tables is possible, it's unlikely to have the desired effect. --- doc/src/sgml/catalogs.sgml | 11 +++ 1 file changed, 11 insertions(+) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 97ef618..3b7768c 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -21,6 +21,17 @@ particularly esoteric operations, such as adding index access methods. + + +Changing the permissions on objects in the system catalogs, while +possible, is unlikely to have the desired effect as the internal +lookup functions use a cache and do not check the permissions nor +policies of tables in the system catalog. Further, permission +changes to objects in the system catalogs are not preserved by +pg_dump or across upgrades. + + + Overview -- 2.5.0 From dece838e3ad74549c6f07dc878c8637dc2db674d Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Wed, 30 Sep 2015 07:04:55 -0400 Subject: [PATCH 2/3] Reserve the "pg_" namespace for roles This will prevent users from creating roles which begin with "pg_" and will check for those roles before allowing an upgrade using pg_upgrade. This will allow for default roles to be provided at initdb time. --- doc/src/sgml/ref/psql-ref.sgml | 8 +-- src/backend/catalog/catalog.c | 5 ++-- src/backend/commands/user.c | 40 src/backend/utils/adt/acl.c | 41 + src/bin/pg_dump/pg_dumpall.c| 2 ++ src/bin/pg_upgrade/check.c | 40 ++-- src/bin/psql/command.c | 4 ++-- src/bin/psql/describe.c | 5 +++- src/bin/psql/describe.h | 2 +- src/bin/psql/help.c | 4 ++-- src/include/utils/acl.h | 1 + src/test/regress/expected/rolenames.out | 18 +++ src/test/regress/sql/rolenames.sql | 8 +++ 13 files changed, 166 insertions(+), 12 deletions(-) diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 6d0cb3d..76bb642 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -1365,13 +1365,15 @@ testdb=> -\dg[+] [ pattern ] +\dg[S+] [ pattern ] Lists database roles. (Since the concepts of users and groups have been unified into roles, this command is now equivalent to \du.) +By default, only user-created roles are shown; supply the +S modifier to include system roles. If pattern is specified, only those roles whose names match the pattern are listed. If the form \dg+ is used, additional information @@ -1525,13 +1527,15 @@ testdb=> -\du[+] [ pattern ] +\du[S+] [ pattern ] Lists database roles. (Since the concepts of users and groups have been unified into roles, this command is now equivalent to \dg.) +By default, only user-created roles are shown; supply the +S modifier to include system roles. If pattern is specified, only those roles whose names match the pattern are listed. If the form \du+ is used, additional information diff --git a/src/ba
Re: [HACKERS] Additional role attributes && superuser review
On Tue, Dec 22, 2015 at 2:54 PM, Amit Langote wrote: > On 2015/12/22 14:05, Michael Paquier wrote: >> On Tue, Dec 22, 2015 at 1:41 AM, Stephen Frost wrote: >>> Updated and rebased patch attached which takes the 'pg_switch_xlog' >>> default role back out, leaving us with: >>> >>> pg_monitor - View privileged info >>> pg_backup - start/stop backups, switch xlog, create restore points >>> pg_replay - Pause/resume xlog replay on replicas >>> pg_replication - Create/destroy/etc replication slots >>> pg_rotate_logfile - Request logfile rotation >>> pg_signal_backend - Signal other backends (cancel query/terminate) >>> pg_file_settings - View configuration settings in all config files >> >> Thanks. This looks fine to me. I just have one single comment: >> >> + Request logfile rotation. >> s/logfile/transaction log file/ > > Looks correct as is. Or maybe "server's log file" as in: > > 9.26.2. Server Signaling Functions > > pg_rotate_logfile(): Rotate server's log file You're right, this is not a WAL segment, just a normal log file. Your phrasing is better. -- Michael -- 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
On 2015/12/22 14:05, Michael Paquier wrote: > On Tue, Dec 22, 2015 at 1:41 AM, Stephen Frost wrote: >> Updated and rebased patch attached which takes the 'pg_switch_xlog' >> default role back out, leaving us with: >> >> pg_monitor - View privileged info >> pg_backup - start/stop backups, switch xlog, create restore points >> pg_replay - Pause/resume xlog replay on replicas >> pg_replication - Create/destroy/etc replication slots >> pg_rotate_logfile - Request logfile rotation >> pg_signal_backend - Signal other backends (cancel query/terminate) >> pg_file_settings - View configuration settings in all config files > > Thanks. This looks fine to me. I just have one single comment: > > + Request logfile rotation. > s/logfile/transaction log file/ Looks correct as is. Or maybe "server's log file" as in: 9.26.2. Server Signaling Functions pg_rotate_logfile(): Rotate server's log file Thanks, Amit -- 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
On Tue, Dec 22, 2015 at 1:41 AM, Stephen Frost wrote: > Updated and rebased patch attached which takes the 'pg_switch_xlog' > default role back out, leaving us with: > > pg_monitor - View privileged info > pg_backup - start/stop backups, switch xlog, create restore points > pg_replay - Pause/resume xlog replay on replicas > pg_replication - Create/destroy/etc replication slots > pg_rotate_logfile - Request logfile rotation > pg_signal_backend - Signal other backends (cancel query/terminate) > pg_file_settings - View configuration settings in all config files > > Michael, another review would be great, if you don't mind. I'm going to > be going through it also more closely since it sounds like we've got > consensus on at least this initial set of default roles and rights. If > all looks good then I'll commit it. Thanks. This looks fine to me. I just have one single comment: + Request logfile rotation. s/logfile/transaction log file/ -- Michael -- 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
Michael, all, * Michael Paquier (michael.paqu...@gmail.com) wrote: > OK, let's do so then by having this one fall under pg_backup. Let's > not be my grunting concerns be an obstacle for this patch, and we > could still change it afterwards in this release beta cycle anyway > based on user feedback. Updated and rebased patch attached which takes the 'pg_switch_xlog' default role back out, leaving us with: pg_monitor - View privileged info pg_backup - start/stop backups, switch xlog, create restore points pg_replay - Pause/resume xlog replay on replicas pg_replication - Create/destroy/etc replication slots pg_rotate_logfile - Request logfile rotation pg_signal_backend - Signal other backends (cancel query/terminate) pg_file_settings - View configuration settings in all config files Michael, another review would be great, if you don't mind. I'm going to be going through it also more closely since it sounds like we've got consensus on at least this initial set of default roles and rights. If all looks good then I'll commit it. Thanks! Stephen From 59bda4266a96976547e0aa44874ad716bf3dbdc9 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Wed, 18 Nov 2015 11:50:57 -0500 Subject: [PATCH 1/3] Add note regarding permissions in pg_catalog Add a note to the system catalog section pointing out that while modifying the permissions on catalog tables is possible, it's unlikely to have the desired effect. --- doc/src/sgml/catalogs.sgml | 11 +++ 1 file changed, 11 insertions(+) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 97ef618..3b7768c 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -21,6 +21,17 @@ particularly esoteric operations, such as adding index access methods. + + +Changing the permissions on objects in the system catalogs, while +possible, is unlikely to have the desired effect as the internal +lookup functions use a cache and do not check the permissions nor +policies of tables in the system catalog. Further, permission +changes to objects in the system catalogs are not preserved by +pg_dump or across upgrades. + + + Overview -- 2.5.0 From a659b7ecd220c0671d3cc272b3774318bce97567 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Wed, 30 Sep 2015 07:04:55 -0400 Subject: [PATCH 2/3] Reserve the "pg_" namespace for roles This will prevent users from creating roles which begin with "pg_" and will check for those roles before allowing an upgrade using pg_upgrade. This will allow for default roles to be provided at initdb time. --- doc/src/sgml/ref/psql-ref.sgml | 8 +-- src/backend/catalog/catalog.c | 5 ++-- src/backend/commands/user.c | 40 src/backend/utils/adt/acl.c | 41 + src/bin/pg_dump/pg_dumpall.c| 2 ++ src/bin/pg_upgrade/check.c | 40 ++-- src/bin/psql/command.c | 4 ++-- src/bin/psql/describe.c | 5 +++- src/bin/psql/describe.h | 2 +- src/bin/psql/help.c | 4 ++-- src/include/utils/acl.h | 1 + src/test/regress/expected/rolenames.out | 18 +++ src/test/regress/sql/rolenames.sql | 8 +++ 13 files changed, 166 insertions(+), 12 deletions(-) diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 6d0cb3d..76bb642 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -1365,13 +1365,15 @@ testdb=> -\dg[+] [ pattern ] +\dg[S+] [ pattern ] Lists database roles. (Since the concepts of users and groups have been unified into roles, this command is now equivalent to \du.) +By default, only user-created roles are shown; supply the +S modifier to include system roles. If pattern is specified, only those roles whose names match the pattern are listed. If the form \dg+ is used, additional information @@ -1525,13 +1527,15 @@ testdb=> -\du[+] [ pattern ] +\du[S+] [ pattern ] Lists database roles. (Since the concepts of users and groups have been unified into roles, this command is now equivalent to \dg.) +By default, only user-created roles are shown; supply the +S modifier to include system roles. If pattern is specified, only those roles whose names match the pattern are listed. If the form \du+ is used, additional information diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c index 81ccebf..184aa7d 100644 --- a/src/backend/catalog/catalog.c +++ b/src/backend/catalog/catalog.c @@ -184,8 +184,9 @@ IsToastNamespace(Oid namespaceId) * True iff name starts with the pg_
Re: [HACKERS] Additional role attributes && superuser review
Michael, * Michael Paquier (michael.paqu...@gmail.com) wrote: > On Tue, Dec 1, 2015 at 9:18 AM, Michael Paquier > wrote: > > OK, let's do so then by having this one fall under pg_backup. Let's > > not be my grunting concerns be an obstacle for this patch, and we > > could still change it afterwards in this release beta cycle anyway > > based on user feedback. > > Three weeks later... > This thread has not moved a iota. Stephen, are you planning to work > more on this patch? It seems that we found a consensus. If nothing > happens, I am afraid that the destiny of this patch will be to be > returned with feedback, it is the 5th CF where this entry is > registered. Ok, seems you're right that we've got consensus on it. I'll post an updated patch later today which I'll plan to commit. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Tue, Dec 1, 2015 at 9:18 AM, Michael Paquier wrote: > On Tue, Dec 1, 2015 at 3:32 AM, Stephen Frost wrote: >> * Robert Haas (robertmh...@gmail.com) wrote: >>> On Fri, Nov 20, 2015 at 12:29 PM, Stephen Frost wrote: >>> > * Michael Paquier (michael.paqu...@gmail.com) wrote: >>> >> On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote: >>> >> > * Michael Paquier (michael.paqu...@gmail.com) wrote: >>> >> >> It seems weird to not have a dedicated role for pg_switch_xlog. >>> >> > >>> >> > I didn't add a pg_switch_xlog default role in this patch series, but >>> >> > would be happy to do so if that's the consensus. It's quite easy to >>> >> > do. >>> >> >>> >> Agreed. I am not actually getting why that's part of the backup >>> >> actually. That would be more related to archiving, both being >>> >> unrelated concepts. But at this point I guess that's mainly a >>> >> philosophical split. >>> > >>> > As David notes, they're actually quite related. Note that in our >>> > documentation pg_switch_xlog() is listed in the "Backup Control >>> > Functions" table. >>> > >>> > I can think of a use-case for a user who can call pg_switch_xlog, but >>> > not pg_start_backup()/pg_stop_backup(), but I have to admit that it >>> > seems rather limited and I'm on the fence about it being a worthwhile >>> > distinction. >>> >>> Sounds too narrow to me. Are we going to have a separate predefined >>> role for every security-restricted function to which someone might >>> want to grant access? That seems over the top to me. >> >> I certainly don't want to go down to that level and was, as seen above, >> unsure about having pg_switch_xlog() as a differentiated privilege. >> Michael, do you still see that as a useful independent capability? > > OK, let's do so then by having this one fall under pg_backup. Let's > not be my grunting concerns be an obstacle for this patch, and we > could still change it afterwards in this release beta cycle anyway > based on user feedback. Three weeks later... This thread has not moved a iota. Stephen, are you planning to work more on this patch? It seems that we found a consensus. If nothing happens, I am afraid that the destiny of this patch will be to be returned with feedback, it is the 5th CF where this entry is registered. -- Michael -- 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
On Tue, Dec 1, 2015 at 3:32 AM, Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: >> On Fri, Nov 20, 2015 at 12:29 PM, Stephen Frost wrote: >> > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> >> On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote: >> >> > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> >> >> It seems weird to not have a dedicated role for pg_switch_xlog. >> >> > >> >> > I didn't add a pg_switch_xlog default role in this patch series, but >> >> > would be happy to do so if that's the consensus. It's quite easy to do. >> >> >> >> Agreed. I am not actually getting why that's part of the backup >> >> actually. That would be more related to archiving, both being >> >> unrelated concepts. But at this point I guess that's mainly a >> >> philosophical split. >> > >> > As David notes, they're actually quite related. Note that in our >> > documentation pg_switch_xlog() is listed in the "Backup Control >> > Functions" table. >> > >> > I can think of a use-case for a user who can call pg_switch_xlog, but >> > not pg_start_backup()/pg_stop_backup(), but I have to admit that it >> > seems rather limited and I'm on the fence about it being a worthwhile >> > distinction. >> >> Sounds too narrow to me. Are we going to have a separate predefined >> role for every security-restricted function to which someone might >> want to grant access? That seems over the top to me. > > I certainly don't want to go down to that level and was, as seen above, > unsure about having pg_switch_xlog() as a differentiated privilege. > Michael, do you still see that as a useful independent capability? OK, let's do so then by having this one fall under pg_backup. Let's not be my grunting concerns be an obstacle for this patch, and we could still change it afterwards in this release beta cycle anyway based on user feedback. -- Michael -- 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
Stephen Frost wrote: > * Robert Haas (robertmh...@gmail.com) wrote: > > > I can think of a use-case for a user who can call pg_switch_xlog, but > > > not pg_start_backup()/pg_stop_backup(), but I have to admit that it > > > seems rather limited and I'm on the fence about it being a worthwhile > > > distinction. > > > > Sounds too narrow to me. Are we going to have a separate predefined > > role for every security-restricted function to which someone might > > want to grant access? That seems over the top to me. > > I certainly don't want to go down to that level and was, as seen above, > unsure about having pg_switch_xlog() as a differentiated privilege. > Michael, do you still see that as a useful independent capability? Hmm, Robert's argument seems reasonable -- we can continue to offer access to individual elements by granting execute on a security-definer function owned by predefined user pg_backup. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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
* Robert Haas (robertmh...@gmail.com) wrote: > On Fri, Nov 20, 2015 at 12:29 PM, Stephen Frost wrote: > > * Michael Paquier (michael.paqu...@gmail.com) wrote: > >> On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote: > >> > * Michael Paquier (michael.paqu...@gmail.com) wrote: > >> >> It seems weird to not have a dedicated role for pg_switch_xlog. > >> > > >> > I didn't add a pg_switch_xlog default role in this patch series, but > >> > would be happy to do so if that's the consensus. It's quite easy to do. > >> > >> Agreed. I am not actually getting why that's part of the backup > >> actually. That would be more related to archiving, both being > >> unrelated concepts. But at this point I guess that's mainly a > >> philosophical split. > > > > As David notes, they're actually quite related. Note that in our > > documentation pg_switch_xlog() is listed in the "Backup Control > > Functions" table. > > > > I can think of a use-case for a user who can call pg_switch_xlog, but > > not pg_start_backup()/pg_stop_backup(), but I have to admit that it > > seems rather limited and I'm on the fence about it being a worthwhile > > distinction. > > Sounds too narrow to me. Are we going to have a separate predefined > role for every security-restricted function to which someone might > want to grant access? That seems over the top to me. I certainly don't want to go down to that level and was, as seen above, unsure about having pg_switch_xlog() as a differentiated privilege. Michael, do you still see that as a useful independent capability? > I don't think we should make it our goal to completely eliminate the > use of SECURITY DEFINER functions for privilege delegation. Of > course, being able to grant privileges directly is nicer, because then > the client code doesn't have to know about it. But I think it's OK, > even good, if the predefined roles cater to the common cases, and the > less common cases aren't handled quite as elegantly. Agreed. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Fri, Nov 20, 2015 at 12:29 PM, Stephen Frost wrote: > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote: >> > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> >> It seems weird to not have a dedicated role for pg_switch_xlog. >> > >> > I didn't add a pg_switch_xlog default role in this patch series, but >> > would be happy to do so if that's the consensus. It's quite easy to do. >> >> Agreed. I am not actually getting why that's part of the backup >> actually. That would be more related to archiving, both being >> unrelated concepts. But at this point I guess that's mainly a >> philosophical split. > > As David notes, they're actually quite related. Note that in our > documentation pg_switch_xlog() is listed in the "Backup Control > Functions" table. > > I can think of a use-case for a user who can call pg_switch_xlog, but > not pg_start_backup()/pg_stop_backup(), but I have to admit that it > seems rather limited and I'm on the fence about it being a worthwhile > distinction. Sounds too narrow to me. Are we going to have a separate predefined role for every security-restricted function to which someone might want to grant access? That seems over the top to me. I don't think we should make it our goal to completely eliminate the use of SECURITY DEFINER functions for privilege delegation. Of course, being able to grant privileges directly is nicer, because then the client code doesn't have to know about it. But I think it's OK, even good, if the predefined roles cater to the common cases, and the less common cases aren't handled quite as elegantly. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
On Tuesday, November 24, 2015, Alvaro Herrera wrote: > Stephen Frost wrote: > > > Even so, in the interest of having more fine-grained permission > > controls, I've gone ahead and added a pg_switch_xlog default role. > > Note that this means that pg_switch_xlog() can be called by both > > pg_switch_xlog roles and pg_backup roles. I'd be very much against > > removing the ability to call pg_switch_xlog from the pg_backup role as > > that really is a capability which is needed by users running backups and > > it'd just add unnecessary complexity to require users setting up backup > > tools to grant two different roles to get the backup to work. > > Isn't it simpler to grant pg_switch_xlog to pg_backup in the default > config? > I'm not against it, but it would imply a set of data lines for pg_auth_members, which we don't have today. We can't easily directly GRANT the role due to the restrictions put in place to prevent regular users from changing the system roles. On the other hand, we could change the check to only apply when we aren't in bootstrap mode. Thanks! Stephen
Re: [HACKERS] Additional role attributes && superuser review
Stephen Frost wrote: > Even so, in the interest of having more fine-grained permission > controls, I've gone ahead and added a pg_switch_xlog default role. > Note that this means that pg_switch_xlog() can be called by both > pg_switch_xlog roles and pg_backup roles. I'd be very much against > removing the ability to call pg_switch_xlog from the pg_backup role as > that really is a capability which is needed by users running backups and > it'd just add unnecessary complexity to require users setting up backup > tools to grant two different roles to get the backup to work. Isn't it simpler to grant pg_switch_xlog to pg_backup in the default config? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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
Michael, * Michael Paquier (michael.paqu...@gmail.com) wrote: > On Sat, Nov 21, 2015 at 2:29 AM, Stephen Frost wrote: > > * Michael Paquier (michael.paqu...@gmail.com) wrote: > > Even so, in the interest of having more fine-grained permission > > controls, I've gone ahead and added a pg_switch_xlog default role. > > Note that this means that pg_switch_xlog() can be called by both > > pg_switch_xlog roles and pg_backup roles. I'd be very much against > > removing the ability to call pg_switch_xlog from the pg_backup role as > > that really is a capability which is needed by users running backups and > > it'd just add unnecessary complexity to require users setting up backup > > tools to grant two different roles to get the backup to work. > > There is going to be many opinions regarding the granularity of this > control, each one of us having a different opinion at the end. I don't > think this should be a stopper for this patch, hence I am fine with the > judgement you think is good. We could still more finely tune those default > roles later in the dev cycle of 9.6 (10.0?). Agreed. > Thanks, this looks good to me. Great. > I guess that's better than nothing. Agreed. > I don't think you mean to refer to the switch of segments files here. Same > comment for pg_current_xlog_insert_location, pg_last_xlog_receive_location > and pg_last_xlog_replay_location. Urgh. Got a bit ahead of myself there, apologies. I've updated all of these and a number of other minor typos and incorrect comments along the way. > + ereport(ERROR, > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > +errmsg("must be superuser or member of > pg_file_settings to see all config file settings"))); > Should avoid abbreviations => "all configuration file settings". Fixed. > > -\dg[+] [ linkend="APP-PSQL-patterns"> class="parameter">pattern ] > +\dgS[+] [ linkend="APP-PSQL-patterns"> class="parameter">pattern ] > > I'm picky here, but that should be "\dg[S+]". Same for \du[S+]. Fixed Updated patch attached. Thanks! Stephen From 37b4627352287328ad28cd167620f51c26a68f04 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Wed, 18 Nov 2015 11:50:57 -0500 Subject: [PATCH 1/3] Add note regarding permissions in pg_catalog Add a note to the system catalog section pointing out that while modifying the permissions on catalog tables is possible, it's unlikely to have the desired effect. --- doc/src/sgml/catalogs.sgml | 11 +++ 1 file changed, 11 insertions(+) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 97ef618..3b7768c 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -21,6 +21,17 @@ particularly esoteric operations, such as adding index access methods. + + +Changing the permissions on objects in the system catalogs, while +possible, is unlikely to have the desired effect as the internal +lookup functions use a cache and do not check the permissions nor +policies of tables in the system catalog. Further, permission +changes to objects in the system catalogs are not preserved by +pg_dump or across upgrades. + + + Overview -- 2.5.0 From 408a8ee807edbe875ff9fd860ebfd6c859dcffb8 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Wed, 30 Sep 2015 07:04:55 -0400 Subject: [PATCH 2/3] Reserve the "pg_" namespace for roles This will prevent users from creating roles which begin with "pg_" and will check for those roles before allowing an upgrade using pg_upgrade. This will allow for default roles to be provided at initdb time. --- doc/src/sgml/ref/psql-ref.sgml | 8 +-- src/backend/catalog/catalog.c | 5 ++-- src/backend/commands/user.c | 40 src/backend/utils/adt/acl.c | 41 + src/bin/pg_dump/pg_dumpall.c| 2 ++ src/bin/pg_upgrade/check.c | 40 ++-- src/bin/psql/command.c | 4 ++-- src/bin/psql/describe.c | 5 +++- src/bin/psql/describe.h | 2 +- src/bin/psql/help.c | 4 ++-- src/include/utils/acl.h | 1 + src/test/regress/expected/rolenames.out | 18 +++ src/test/regress/sql/rolenames.sql | 8 +++ 13 files changed, 166 insertions(+), 12 deletions(-) diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index 5899bb4..e0de107 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -1336,13 +1336,15 @@ testdb=> -\dg[+] [ pattern ] +\dg[S+] [ pattern ] Lists database roles. (Since the concepts of users and groups have been unified into roles, this command is now equivalent to \du.) +By default, onl
Re: [HACKERS] Additional role attributes && superuser review
On Sat, Nov 21, 2015 at 2:29 AM, Stephen Frost wrote: > * Michael Paquier (michael.paqu...@gmail.com) wrote: > Even so, in the interest of having more fine-grained permission > controls, I've gone ahead and added a pg_switch_xlog default role. > Note that this means that pg_switch_xlog() can be called by both > pg_switch_xlog roles and pg_backup roles. I'd be very much against > removing the ability to call pg_switch_xlog from the pg_backup role as > that really is a capability which is needed by users running backups and > it'd just add unnecessary complexity to require users setting up backup > tools to grant two different roles to get the backup to work. There is going to be many opinions regarding the granularity of this control, each one of us having a different opinion at the end. I don't think this should be a stopper for this patch, hence I am fine with the judgement you think is good. We could still more finely tune those default roles later in the dev cycle of 9.6 (10.0?). >> For the code paths where a backend would be actually running, >> you could use the following: >> SET client_min_messages TO 'error'; >> -- This should return "1" and not an ERROR nor a WARNING if PID does not exist. >> select count(pg_terminate_backend(0)); >> But that's ugly depending on your taste. > > I really dislike that. > >> Do you think that it makes sense to add tests as well in the second >> patch to check that restrictions pg_* are in place? Except that I >> don't have much more to say about patches 1 and 2 which are rather >> straight-forward. > > Ah, yes. I've now moved those hunks to the second patch where they > really should have been. > >> Regarding patch 3, the documentation of psql should mention the new >> subommands \dgS and \dgS+. That's a one-liner. > > Ah, right. I've updated the psql SGML documentation for \duS and \dgS. > The '\h' output had already been updated. Was there something else > which you meant above that I've missed? Note that these fixes went into > the second patch. Thanks, this looks good to me. >> +GRANT pg_backup TO regressuser1; >> +SET SESSION AUTHORIZATION regressuser1; >> +SELECT pg_start_backup('abc'); -- fail >> +ERROR: WAL level not sufficient for making an online backup >> +HINT: wal_level must be set to "archive", "hot_standby", or >> "logical" at server start. >> make install would wal on a server with something else than wal_level >> = minimal. Just checking that errors kick correctly looks fine to me >> here. > > I've removed those checks as they could get annoying on the buildfarm or > for people doing make installcheck, as you say, but I'm not really > thrilled that we're only testing the failure paths. I guess that's better than nothing. >> +GRANT pg_backup TO pg_monitor; -- error >> +ERROR: role "pg_monitor" is reserved >> +DETAIL: Roles starting with "pg_" are reserved. >> +GRANT testrol0 TO pg_backup; -- error >> +ERROR: role "pg_backup" is reserved >> We would gain with verbose error messages here, like, "cannot GRANT >> somerole to a system role", and "cannot GRANT system role to >> somerole". > > Alright, I've changed these to have better detail messages. @@ -183,6 +190,13 @@ pg_current_xlog_location(PG_FUNCTION_ARGS) { XLogRecPtr current_recptr; + if (!has_privs_of_role(GetUserId(), DEFAULT_ROLE_BACKUPID) && + !has_privs_of_role(GetUserId(), DEFAULT_ROLE_MONITORID) && + !has_privs_of_role(GetUserId(), DEFAULT_ROLE_SWITCH_XLOGID)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), +errmsg("must be superuser or member of pg_monitor, pg_backup, or pg_switch_xlog to switch transaction log files"))); I don't think you mean to refer to the switch of segments files here. Same comment for pg_current_xlog_insert_location, pg_last_xlog_receive_location and pg_last_xlog_replay_location. + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), +errmsg("must be superuser or member of pg_file_settings to see all config file settings"))); Should avoid abbreviations => "all configuration file settings". -\dg[+] [ pattern ] +\dgS[+] [ pattern ] I'm picky here, but that should be "\dg[S+]". Same for \du[S+]. The rest looks fine. Regards, -- Michael
Re: [HACKERS] Additional role attributes && superuser review
* Michael Paquier (michael.paqu...@gmail.com) wrote: > On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote: > > * Michael Paquier (michael.paqu...@gmail.com) wrote: > >> It seems weird to not have a dedicated role for pg_switch_xlog. > > > > I didn't add a pg_switch_xlog default role in this patch series, but > > would be happy to do so if that's the consensus. It's quite easy to do. > > Agreed. I am not actually getting why that's part of the backup > actually. That would be more related to archiving, both being > unrelated concepts. But at this point I guess that's mainly a > philosophical split. As David notes, they're actually quite related. Note that in our documentation pg_switch_xlog() is listed in the "Backup Control Functions" table. I can think of a use-case for a user who can call pg_switch_xlog, but not pg_start_backup()/pg_stop_backup(), but I have to admit that it seems rather limited and I'm on the fence about it being a worthwhile distinction. Even so, in the interest of having more fine-grained permission controls, I've gone ahead and added a pg_switch_xlog default role. Note that this means that pg_switch_xlog() can be called by both pg_switch_xlog roles and pg_backup roles. I'd be very much against removing the ability to call pg_switch_xlog from the pg_backup role as that really is a capability which is needed by users running backups and it'd just add unnecessary complexity to require users setting up backup tools to grant two different roles to get the backup to work. > > I'm a bit confused- this is a separate change. Note that the previous > > patch was actually a git patchset which had two patches- one to do the > > reservation and a second to add the default roles. The attached > > patchset is actually three patches: > > 1- Update to catalog documentation regarding permissions > > 2- Reserve 'pg_' role namespace > > 3- Add default roles > > I see, that's why I got confused. I just downloaded your file and > applied it blindly with git apply or patch (don't recall which). Other > folks tend to publish that as a separate set of files generated by > git-format-patch. The file was generated using 'git format-patch', but sent to one file instead of multiple. 'git am' understands that format and will add the independent commits to the current branch. Note that the git documentation (see 'man git-format-patch' and 'man git-apply', at least on Debian-based systems) specifically recommends using 'git am' to create commits from patches generated by 'git format-patch'. The workflow you're describing would require saving off each patch, doing a 'patch' or 'git apply' run for each, then committing the changes with your own commit message and only then would you have the entire series. That doesn't seem like a better approach. > >> Shouldn't pg_current_xlog_location, pg_current_xlog_insert_location, > >> pg_last_xlog_receive_location and pg_last_xlog_replay_location fall under a > >> restriction category like pg_monitor? I recall of course that we discussed > >> that some months ago and that a lot of people were reluctant to harden this > >> area to not break any existing monitoring tool, still this patch may be the > >> occasion to restrict a bit those functions, particularly on servers where > >> wal_compression is enabled. > > > > For my 2c, I believe they should. I've not modified them in that way in > > this patchset, but I can certainly do so if others agree. > > My vote goes into hardening them a bit this way. I've made those changes in this patch set. Note that these functions can now only be called by roles which are members of pg_monitor, pg_backup, or pg_switch_xlog (or superuser, of course). > > I've added regression tests for the default roles where it was > > relatively straight-forward to do so. I don't see a trivial way to test > > that the pg_signal_backend role works though, as an example. > > It is at least possible to check that the error code paths are > working. Perhaps, but... > For the code paths where a backend would be actually running, > you could use the following: > SET client_min_messages TO 'error'; > -- This should return "1" and not an ERROR nor a WARNING if PID does not > exist. > select count(pg_terminate_backend(0)); > But that's ugly depending on your taste. I really dislike that. > Do you think that it makes sense to add tests as well in the second > patch to check that restrictions pg_* are in place? Except that I > don't have much more to say about patches 1 and 2 which are rather > straight-forward. Ah, yes. I've now moved those hunks to the second patch where they really should have been. > Regarding patch 3, the documentation of psql should mention the new > subommands \dgS and \dgS+. That's a one-liner. Ah, right. I've updated the psql SGML documentation for \duS and \dgS. The '\h' output had already been updated. Was there something else which you meant above that I've missed? Note that these fixes went into the s
Re: [HACKERS] Additional role attributes && superuser review
On 11/19/15 2:13 AM, Michael Paquier wrote: > On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote: >> * Michael Paquier (michael.paqu...@gmail.com) wrote: >>> It seems weird to not have a dedicated role for pg_switch_xlog. >> >> I didn't add a pg_switch_xlog default role in this patch series, but >> would be happy to do so if that's the consensus. It's quite easy to do. > > Agreed. I am not actually getting why that's part of the backup > actually. That would be more related to archiving, both being > unrelated concepts. But at this point I guess that's mainly a > philosophical split. I wouldn't say that backup and archiving are unrelated since backups aren't valid without the proper set of WAL segments. When configuring archiving/backup I use pg_switch_xlog() to verify that WAL segments are getting to the archive. I also use pg_switch_xlog() after pg_create_restore_point() to force the WAL segment containing the restore point record to the archive. Otherwise it might not be possible (or at least not easy) to recover to the restore point in case of a problem. The required WAL segment is likely to get pushed to the archive before it is needed but I would rather not bet on that. -- -David da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Thu, Nov 19, 2015 at 7:10 AM, Stephen Frost wrote: > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> It seems weird to not have a dedicated role for pg_switch_xlog. > > I didn't add a pg_switch_xlog default role in this patch series, but > would be happy to do so if that's the consensus. It's quite easy to do. Agreed. I am not actually getting why that's part of the backup actually. That would be more related to archiving, both being unrelated concepts. But at this point I guess that's mainly a philosophical split. > I'm guessing you were referring to pg_admin with your comment that you > were "not convinced that we actually need it." After thinking about > it for a bit, I tend to agree. A user could certainly create their own > role which combines these all together if they wanted to (or do any > other combinations, based on their particular needs). I've gone ahead > and removed pg_admin from the set of default roles. Yeah that 's what I meant. Thanks, I should have been more precise with my words. >> + if (IsReservedName(stmt->role)) >> + ereport(ERROR, >> + (errcode(ERRCODE_RESERVED_NAME), >> +errmsg("role name \"%s\" is reserved", >> +stmt->role), >> +errdetail("Role names starting with >> \"pg_\" are reserved."))); >> Perhaps this could be a separate change? It seems that restricting roles >> with pg_ on the cluster is not a bad restriction in any case. You may want >> to add regression tests to trigger those errors as well. > > I'm a bit confused- this is a separate change. Note that the previous > patch was actually a git patchset which had two patches- one to do the > reservation and a second to add the default roles. The attached > patchset is actually three patches: > 1- Update to catalog documentation regarding permissions > 2- Reserve 'pg_' role namespace > 3- Add default roles I see, that's why I got confused. I just downloaded your file and applied it blindly with git apply or patch (don't recall which). Other folks tend to publish that as a separate set of files generated by git-format-patch. >> Shouldn't pg_current_xlog_location, pg_current_xlog_insert_location, >> pg_last_xlog_receive_location and pg_last_xlog_replay_location fall under a >> restriction category like pg_monitor? I recall of course that we discussed >> that some months ago and that a lot of people were reluctant to harden this >> area to not break any existing monitoring tool, still this patch may be the >> occasion to restrict a bit those functions, particularly on servers where >> wal_compression is enabled. > > For my 2c, I believe they should. I've not modified them in that way in > this patchset, but I can certainly do so if others agree. My vote goes into hardening them a bit this way. > I've added regression tests for the default roles where it was > relatively straight-forward to do so. I don't see a trivial way to test > that the pg_signal_backend role works though, as an example. It is at least possible to check that the error code paths are working. For the code paths where a backend would be actually running, you could use the following: SET client_min_messages TO 'error'; -- This should return "1" and not an ERROR nor a WARNING if PID does not exist. select count(pg_terminate_backend(0)); But that's ugly depending on your taste. >> Bonus: >> -static void >> -check_permissions(void) >> -{ >> - if (!superuser() && !has_rolreplication(GetUserId())) >> - ereport(ERROR, >> - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), >> -(errmsg("must be superuser or replication >> role to use replication slots"; >> -} >> I would have let check_permissions and directly done the checks on >> has_rolreplication and has_privs_of_role in it, the same code being >> duplicated three times. > > For my 2c, I dislike the notion of "check_permissions()" functions being > added throughout the codebase as I'm afraid it'd get confusing, which is > why I got rid of it. I'm much happier seeing the actual permissions > check as it should be happening early on in the primary function which > is being called into. If there is really a push to go back to having a > 'check_permissions()' like function, I'd argue that we should make the > function name more descriptive of what's actually being done and have > them be distinct from each other. As I recall, I discussed this change > with Andres and he didn't feel particularly strongly about this one way > or the other, therefore, I've not made this change for this patchset. Fine for me. Usually people point out such code duplications are bad things, and here we just have a static routine being used for a set of functions interacting with the same kind of object, here a replication slot. But I'm fine either way. Do you think that it makes
Re: [HACKERS] Additional role attributes && superuser review
Michael, Thanks for the review! * Michael Paquier (michael.paqu...@gmail.com) wrote: > Patch needs a rebase, some catalog OIDs and there was a conflict in misc.c > (see attached for the rebase. none of the comments mentioning issues are > fixed by it). Done (did it a bit differently from what you had, to hopefully avoid future OID conflicts and also to allow us a bit of room to add additional default roles later, if we choose, in nearby OID space). > =# grant pg_replay to pg_backup ; > GRANT ROLE > =# \du pg_backup > List of roles > Role name | Attributes | Member of > ---+--+- > pg_backup | Cannot login | {pg_replay} > Perhaps we should restrict granting a default role to another default role? Done (in the second patch in the series, the one reserving 'pg_'). > + > + Also note that changing the permissions on objects in the system > + catalogs, while possible, is unlikely to have the desired effect as > + the internal lookup functions use a cache and do not check the > + permissions nor policies of tables in the system catalog. Further, > + permission changes to objects in the system catalogs are not > + preserved by pg_dump or across upgrades. > + > This bit could be perhaps applied as a separate patch. Done as a separate patch (first one in the series). > + > + pg_backup > + Start and stop backups, switch xlogs, and create restore > points. > + > s/switch xlogs/switch to a new transaction log file/ Fixed. > It seems weird to not have a dedicated role for pg_switch_xlog. I didn't add a pg_switch_xlog default role in this patch series, but would be happy to do so if that's the consensus. It's quite easy to do. > In func.sgml, the descriptions of pg_switch_xlog, pg_xlog_replay_pause and > pg_xlog_replay_resume still mention that those functions are restricted > only to superusers. The documentation needs an update. It would be good to > double-check if there are similar mistakes for the other functions. Fixed. Also did a review and found a number of other places which required updating, so those have also been fixed. > + pg_montior > Typo, pg_monitor. Fixed. > + Pause and resume xlog replay on replicas. > Those descriptions should be complete sentences or not? Both styles are > mixed in user-manag.sgml. I didn't take any action on this. > + > + pg_replication > + Create, destroy, and work with replication slots. > + > pg_replication_slots would be more adapted? Perhaps... I didn't make this change, but if others agree that adding "_slots" would help, I'd be happy to do so. Personally, I'd like to keep these names shorter, if possible, but I don't want it to be confusing either. > + > + pg_file_settings > + Allowed to view config settings from all config files > + > I would say "configuration files" instead. An abbreviation is not adapted. Done. > + pg_admin > + > +Granted pg_backup, pg_monitor, pg_reply, pg_replication, > +pg_rotate_logfile, pg_signal_backend and pg_file_settings roles. > + > Typo, s/pg_reply/pg_replay and I think that there should be > markups around those role names. I am actually not convinced that we > actually need it. I added markups around the role names, where used. I'm guessing you were referring to pg_admin with your comment that you were "not convinced that we actually need it." After thinking about it for a bit, I tend to agree. A user could certainly create their own role which combines these all together if they wanted to (or do any other combinations, based on their particular needs). I've gone ahead and removed pg_admin from the set of default roles. > + if (IsReservedName(stmt->role)) > + ereport(ERROR, > + (errcode(ERRCODE_RESERVED_NAME), > +errmsg("role name \"%s\" is reserved", > +stmt->role), > +errdetail("Role names starting with > \"pg_\" are reserved."))); > Perhaps this could be a separate change? It seems that restricting roles > with pg_ on the cluster is not a bad restriction in any case. You may want > to add regression tests to trigger those errors as well. I'm a bit confused- this is a separate change. Note that the previous patch was actually a git patchset which had two patches- one to do the reservation and a second to add the default roles. The attached patchset is actually three patches: 1- Update to catalog documentation regarding permissions 2- Reserve 'pg_' role namespace 3- Add default roles > Shouldn't pg_current_xlog_location, pg_current_xlog_insert_location, > pg_last_xlog_receive_location and pg_last_xlog_replay_location fall under a > restriction category like pg_monitor? I recall of course that we discussed > that some months ago and that a lot of people were reluctant t
Re: [HACKERS] Additional role attributes && superuser review
On Wed, Nov 18, 2015 at 10:06 PM, Michael Paquier wrote: > > > On Wed, Sep 30, 2015 at 8:11 PM, Stephen Frost wrote: > > * Heikki Linnakangas (hlinn...@iki.fi) wrote: > >> I agree with Robert's earlier point that this needs to be split into > >> multiple patches, which can then be reviewed and discussed > >> separately. Pending that, I'm going to mark this as "Waiting on > >> author" in the commitfest. > > > > Attached is an initial split which divides up reserving the role names > > from actually adding the initial set of default roles. > > I have begun looking at this patch, and here are some comments after > screening it. > > Patch needs a rebase, some catalog OIDs and there was a conflict in misc.c > (see attached for the rebase. none of the comments mentioning issues are > fixed by it). > > =# grant pg_replay to pg_backup ; > GRANT ROLE > =# \du pg_backup > List of roles > Role name | Attributes | Member of > ---+--+- > pg_backup | Cannot login | {pg_replay} > Perhaps we should restrict granting a default role to another default role? > > + > + Also note that changing the permissions on objects in the system > + catalogs, while possible, is unlikely to have the desired effect as > + the internal lookup functions use a cache and do not check the > + permissions nor policies of tables in the system catalog. Further, > + permission changes to objects in the system catalogs are not > + preserved by pg_dump or across upgrades. > + > This bit could be perhaps applied as a separate patch. > > + > + pg_backup > + Start and stop backups, switch xlogs, and create restore > points. > + > s/switch xlogs/switch to a new transaction log file/ > It seems weird to not have a dedicated role for pg_switch_xlog. > > In func.sgml, the descriptions of pg_switch_xlog, pg_xlog_replay_pause and > pg_xlog_replay_resume still mention that those functions are restricted > only to superusers. The documentation needs an update. It would be good to > double-check if there are similar mistakes for the other functions. > > + pg_montior > Typo, pg_monitor. > > + Pause and resume xlog replay on replicas. > Those descriptions should be complete sentences or not? Both styles are > mixed in user-manag.sgml. > > + > + pg_replication > + Create, destroy, and work with replication slots. > + > pg_replication_slots would be more adapted? > > + > + pg_file_settings > + Allowed to view config settings from all config > files > + > I would say "configuration files" instead. An abbreviation is not adapted. > > + pg_admin > + > +Granted pg_backup, pg_monitor, pg_reply, pg_replication, > +pg_rotate_logfile, pg_signal_backend and pg_file_settings roles. > + > Typo, s/pg_reply/pg_replay and I think that there should be > markups around those role names. I am actually not convinced that we > actually need it. > > + if (IsReservedName(stmt->role)) > + ereport(ERROR, > + (errcode(ERRCODE_RESERVED_NAME), > +errmsg("role name \"%s\" is reserved", > +stmt->role), > +errdetail("Role names starting with > \"pg_\" are reserved."))); > Perhaps this could be a separate change? It seems that restricting roles > with pg_ on the cluster is not a bad restriction in any case. You may want > to add regression tests to trigger those errors as well. > > Shouldn't pg_current_xlog_location, pg_current_xlog_insert_location, > pg_last_xlog_receive_location and pg_last_xlog_replay_location fall under a > restriction category like pg_monitor? I recall of course that we discussed > that some months ago and that a lot of people were reluctant to harden this > area to not break any existing monitoring tool, still this patch may be the > occasion to restrict a bit those functions, particularly on servers where > wal_compression is enabled. > > It would be nice to have regression tests as well to check that role > restrictions are applied where they should. > Bonus: -static void -check_permissions(void) -{ - if (!superuser() && !has_rolreplication(GetUserId())) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), -(errmsg("must be superuser or replication role to use replication slots"; -} I would have let check_permissions and directly done the checks on has_rolreplication and has_privs_of_role in it, the same code being duplicated three times. -- Michael
Re: [HACKERS] Additional role attributes && superuser review
On Wed, Sep 30, 2015 at 8:11 PM, Stephen Frost wrote: > * Heikki Linnakangas (hlinn...@iki.fi) wrote: >> I agree with Robert's earlier point that this needs to be split into >> multiple patches, which can then be reviewed and discussed >> separately. Pending that, I'm going to mark this as "Waiting on >> author" in the commitfest. > > Attached is an initial split which divides up reserving the role names > from actually adding the initial set of default roles. I have begun looking at this patch, and here are some comments after screening it. Patch needs a rebase, some catalog OIDs and there was a conflict in misc.c (see attached for the rebase. none of the comments mentioning issues are fixed by it). =# grant pg_replay to pg_backup ; GRANT ROLE =# \du pg_backup List of roles Role name | Attributes | Member of ---+--+- pg_backup | Cannot login | {pg_replay} Perhaps we should restrict granting a default role to another default role? + + Also note that changing the permissions on objects in the system + catalogs, while possible, is unlikely to have the desired effect as + the internal lookup functions use a cache and do not check the + permissions nor policies of tables in the system catalog. Further, + permission changes to objects in the system catalogs are not + preserved by pg_dump or across upgrades. + This bit could be perhaps applied as a separate patch. + + pg_backup + Start and stop backups, switch xlogs, and create restore points. + s/switch xlogs/switch to a new transaction log file/ It seems weird to not have a dedicated role for pg_switch_xlog. In func.sgml, the descriptions of pg_switch_xlog, pg_xlog_replay_pause and pg_xlog_replay_resume still mention that those functions are restricted only to superusers. The documentation needs an update. It would be good to double-check if there are similar mistakes for the other functions. + pg_montior Typo, pg_monitor. + Pause and resume xlog replay on replicas. Those descriptions should be complete sentences or not? Both styles are mixed in user-manag.sgml. + + pg_replication + Create, destroy, and work with replication slots. + pg_replication_slots would be more adapted? + + pg_file_settings + Allowed to view config settings from all config files + I would say "configuration files" instead. An abbreviation is not adapted. + pg_admin + +Granted pg_backup, pg_monitor, pg_reply, pg_replication, +pg_rotate_logfile, pg_signal_backend and pg_file_settings roles. + Typo, s/pg_reply/pg_replay and I think that there should be markups around those role names. I am actually not convinced that we actually need it. + if (IsReservedName(stmt->role)) + ereport(ERROR, + (errcode(ERRCODE_RESERVED_NAME), +errmsg("role name \"%s\" is reserved", +stmt->role), +errdetail("Role names starting with \"pg_\" are reserved."))); Perhaps this could be a separate change? It seems that restricting roles with pg_ on the cluster is not a bad restriction in any case. You may want to add regression tests to trigger those errors as well. Shouldn't pg_current_xlog_location, pg_current_xlog_insert_location, pg_last_xlog_receive_location and pg_last_xlog_replay_location fall under a restriction category like pg_monitor? I recall of course that we discussed that some months ago and that a lot of people were reluctant to harden this area to not break any existing monitoring tool, still this patch may be the occasion to restrict a bit those functions, particularly on servers where wal_compression is enabled. It would be nice to have regression tests as well to check that role restrictions are applied where they should. Regards, -- Michael diff --git a/contrib/test_decoding/expected/permissions.out b/contrib/test_decoding/expected/permissions.out index 212fd1d..79a7f86 100644 --- a/contrib/test_decoding/expected/permissions.out +++ b/contrib/test_decoding/expected/permissions.out @@ -54,13 +54,13 @@ RESET ROLE; -- 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 +ERROR: must be superuser or member of pg_replication 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 +ERROR: must be superuser or member of pg_replication to use replication slots SELECT pg_drop_replication_slot('regression_slot'); -ERROR:
Re: [HACKERS] Additional role attributes && superuser review
* Heikki Linnakangas (hlinn...@iki.fi) wrote: > I agree with Robert's earlier point that this needs to be split into > multiple patches, which can then be reviewed and discussed > separately. Pending that, I'm going to mark this as "Waiting on > author" in the commitfest. Attached is an initial split which divides up reserving the role names from actually adding the initial set of default roles. Thanks! Stephen From 593ae64bf22bc343d7a5824d65c1224a77091710 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Wed, 30 Sep 2015 07:04:55 -0400 Subject: [PATCH 1/2] Reserve the "pg_" namespace for roles This will prevent users from creating roles which being with "pg_" and will check for those roles before allowing an upgrade using pg_upgrade. This will allow for default roles to be provided at initdb time. --- src/backend/catalog/catalog.c | 5 +++-- src/backend/commands/user.c | 32 src/bin/pg_dump/pg_dumpall.c | 2 ++ src/bin/pg_upgrade/check.c| 40 ++-- src/bin/psql/command.c| 4 ++-- src/bin/psql/describe.c | 5 - src/bin/psql/describe.h | 2 +- src/bin/psql/help.c | 4 ++-- 8 files changed, 84 insertions(+), 10 deletions(-) diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c index 81ccebf..184aa7d 100644 --- a/src/backend/catalog/catalog.c +++ b/src/backend/catalog/catalog.c @@ -184,8 +184,9 @@ IsToastNamespace(Oid namespaceId) * True iff name starts with the pg_ prefix. * * For some classes of objects, the prefix pg_ is reserved for - * system objects only. As of 8.0, this is only true for - * schema and tablespace names. + * system objects only. As of 8.0, this was only true for + * schema and tablespace names. With 9.6, this is also true + * for roles. */ bool IsReservedName(const char *name) diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index 295e0b0..fde8d15 100644 --- a/src/backend/commands/user.c +++ b/src/backend/commands/user.c @@ -17,6 +17,7 @@ #include "access/htup_details.h" #include "access/xact.h" #include "catalog/binary_upgrade.h" +#include "catalog/catalog.h" #include "catalog/dependency.h" #include "catalog/indexing.h" #include "catalog/objectaccess.h" @@ -312,6 +313,17 @@ CreateRole(CreateRoleStmt *stmt) } /* + * Check that the user is not trying to create a role is the reserved + * "pg_" namespace. + */ + if (IsReservedName(stmt->role)) + ereport(ERROR, +(errcode(ERRCODE_RESERVED_NAME), + errmsg("role name \"%s\" is reserved", + stmt->role), + errdetail("Role names starting with \"pg_\" are reserved."))); + + /* * Check the pg_authid relation to be certain the role doesn't already * exist. */ @@ -1117,6 +1129,7 @@ RenameRole(const char *oldname, const char *newname) int i; Oid roleid; ObjectAddress address; + Form_pg_authid authform; rel = heap_open(AuthIdRelationId, RowExclusiveLock); dsc = RelationGetDescr(rel); @@ -1136,6 +1149,7 @@ RenameRole(const char *oldname, const char *newname) */ roleid = HeapTupleGetOid(oldtuple); + authform = (Form_pg_authid) GETSTRUCT(oldtuple); if (roleid == GetSessionUserId()) ereport(ERROR, @@ -1146,6 +1160,24 @@ RenameRole(const char *oldname, const char *newname) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("current user cannot be renamed"))); + /* + * Check that the user is not trying to rename a system role and + * not trying to rename a role into the reserved "pg_" namespace. + */ + if (IsReservedName(NameStr(authform->rolname))) + ereport(ERROR, +(errcode(ERRCODE_RESERVED_NAME), + errmsg("role name \"%s\" is reserved", + NameStr(authform->rolname)), + errdetail("Role names starting with \"pg_\" are reserved."))); + + if (IsReservedName(newname)) + ereport(ERROR, +(errcode(ERRCODE_RESERVED_NAME), + errmsg("role name \"%s\" is reserved", + newname), + errdetail("Role names starting with \"pg_\" are reserved."))); + /* make sure the new name doesn't exist */ if (SearchSysCacheExists1(AUTHNAME, CStringGetDatum(newname))) ereport(ERROR, diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index 3461335..addabd0 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -673,6 +673,7 @@ dumpRoles(PGconn *conn) "pg_catalog.shobj_description(oid, 'pg_authid') as rolcomment, " "rolname = current_user AS is_current_user " "FROM pg_authid " + "WHERE rolname !~ '^pg_' " "ORDER BY 2"); else if (server_version >= 90100) printfPQExpBuffer(buf, @@ -895,6 +896,7 @@ dumpRoleMembership(PGconn *conn) "LEFT JOIN pg_authid ur on ur.oid = a.roleid " "LEFT JOIN pg_authid um on um.oid = a.member " "LEFT JOIN pg_authid ug on ug.oid = a.grantor " + "WHERE NOT (ur.rolname ~ '^pg_' AND um.rolname ~ '^pg_')" "ORDER BY 1,2,3"); if
Re: [HACKERS] Additional role attributes && superuser review
On Sat, Jul 11, 2015 at 6:06 AM, Heikki Linnakangas wrote: > On 05/08/2015 07:35 AM, Stephen Frost wrote: >> In consideration of the fact that you can't create schemas which start >> with "pg_" and therefore the default search_path wouldn't work for that >> user, and that we also reserve "pg_" for tablespaces, I'm not inclined >> to worry too much about this case. Further, if we accept this argument, >> then we simply can't ever provide additional default or system roles, >> ever. That'd be a pretty narrow corner to have painted ourselves into. > > > Well, you could still provide them through some other mechanism, like > require typing "SYSTEM ROLE pg_backup" any time you mean that magic role. > But I agree, reserving pg_* is much better. I wish we had done it when we > invented roles (6.5?), so there would be no risk that you would upgrade from > a system that already has a "pg_foo" role. But I think it'd still be OK. > > I agree with Robert's earlier point that this needs to be split into > multiple patches, which can then be reviewed and discussed separately. > Pending that, I'm going to mark this as "Waiting on author" in the > commitfest. ... And now marked as returned with feedback. -- Michael -- 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
On 05/08/2015 07:35 AM, Stephen Frost wrote: Gavin, * Gavin Flower (gavinflo...@archidevsys.co.nz) wrote: What if I had a company with several subsidiaries using the same database, and want to prefix roles and other things with the subsidiary's initials? (I am not saying this would be a good architecture!!!) If you admit that it's not a good solution then I'm not quite sure how much we really want to worry about it. :) For example if one subsidiary was called 'Perfect Gentleman', so I would want roles prefixed by 'pg_' and would be annoyed if I couldn't! You might try creating a schema for that user.. You'll hopefully find it difficult to do. :) In consideration of the fact that you can't create schemas which start with "pg_" and therefore the default search_path wouldn't work for that user, and that we also reserve "pg_" for tablespaces, I'm not inclined to worry too much about this case. Further, if we accept this argument, then we simply can't ever provide additional default or system roles, ever. That'd be a pretty narrow corner to have painted ourselves into. Well, you could still provide them through some other mechanism, like require typing "SYSTEM ROLE pg_backup" any time you mean that magic role. But I agree, reserving pg_* is much better. I wish we had done it when we invented roles (6.5?), so there would be no risk that you would upgrade from a system that already has a "pg_foo" role. But I think it'd still be OK. I agree with Robert's earlier point that this needs to be split into multiple patches, which can then be reviewed and discussed separately. Pending that, I'm going to mark this as "Waiting on author" in the commitfest. - Heikki -- 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
Gavin, * Gavin Flower (gavinflo...@archidevsys.co.nz) wrote: > What if I had a company with several subsidiaries using the same > database, and want to prefix roles and other things with the > subsidiary's initials? (I am not saying this would be a good > architecture!!!) If you admit that it's not a good solution then I'm not quite sure how much we really want to worry about it. :) > For example if one subsidiary was called 'Perfect Gentleman', so I > would want roles prefixed by 'pg_' and would be annoyed if I > couldn't! You might try creating a schema for that user.. You'll hopefully find it difficult to do. :) In consideration of the fact that you can't create schemas which start with "pg_" and therefore the default search_path wouldn't work for that user, and that we also reserve "pg_" for tablespaces, I'm not inclined to worry too much about this case. Further, if we accept this argument, then we simply can't ever provide additional default or system roles, ever. That'd be a pretty narrow corner to have painted ourselves into. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Wed, Apr 29, 2015 at 8:20 PM, Alvaro Herrera wrote: >> Finally, you've got the idea of making pg_ a reserved prefix for >> roles, adding some predefined roles, and giving them some predefined >> privileges. That should be yet another patch. > > On this part I have a bit of a problem -- the prefix is not really > reserved, is it. I mean, evidently it's still possible to create roles > with the pg_ prefix ... otherwise, how come the new lines to > system_views.sql that create the "predefined" roles work in the first > place? I think if we're going to reserve role names, we should reserve > them for real: CREATE ROLE should flat out reject creation of such > roles, and the default ones should be created during bootstrap. > > IMO anyway. This is exactly what I mean about needing separate discussion for separate parts of the patch. There's so much different stuff in there right now that objections like this won't necessarily come out until it's far too late to change things around. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
On 30/04/15 12:20, Alvaro Herrera wrote: Robert Haas wrote: I think that if you commit this the way you have it today, everybody will go, oh, look, Stephen committed something, but it looks complicated, I won't pay attention. Yeah, that sucks. Finally, you've got the idea of making pg_ a reserved prefix for roles, adding some predefined roles, and giving them some predefined privileges. That should be yet another patch. On this part I have a bit of a problem -- the prefix is not really reserved, is it. I mean, evidently it's still possible to create roles with the pg_ prefix ... otherwise, how come the new lines to system_views.sql that create the "predefined" roles work in the first place? I think if we're going to reserve role names, we should reserve them for real: CREATE ROLE should flat out reject creation of such roles, and the default ones should be created during bootstrap. IMO anyway. What if I had a company with several subsidiaries using the same database, and want to prefix roles and other things with the subsidiary's initials? (I am not saying this would be a good architecture!!!) For example if one subsidiary was called 'Perfect Gentleman', so I would want roles prefixed by 'pg_' and would be annoyed if I couldn't! Cheers, Gavin -- 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
Robert Haas wrote: > I think that if you commit this the way you have it today, everybody > will go, oh, look, Stephen committed something, but it looks > complicated, I won't pay attention. Yeah, that sucks. > Finally, you've got the idea of making pg_ a reserved prefix for > roles, adding some predefined roles, and giving them some predefined > privileges. That should be yet another patch. On this part I have a bit of a problem -- the prefix is not really reserved, is it. I mean, evidently it's still possible to create roles with the pg_ prefix ... otherwise, how come the new lines to system_views.sql that create the "predefined" roles work in the first place? I think if we're going to reserve role names, we should reserve them for real: CREATE ROLE should flat out reject creation of such roles, and the default ones should be created during bootstrap. IMO anyway. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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
On Wed, Apr 29, 2015 at 10:47 AM, Stephen Frost wrote: > Here is the latest revision of this patch. I think this patch is too big and does too many things. It should be broken up into small patches which can be discussed and validated independently. The fact that your commit message is incredibly long is a sign that there's too much going on here, and that message doesn't even cover all of it. It seems to me that the core change here is really to pg_dump. You're adding the ability for pg_dump to dump and restore privileges on objects in pg_dump. That capability is a complete - and useful - feature in its own right, with no other changes. Then the next thing you've got here is a series of patches that change the rights to execute various utility functions. Some of those, like the changes to pg_stop_backup(), are pretty much a slam-dunk, because they don't really change user-visible behavior; they just give the DBA more options. Others, like the changes to replication permissions, are likely to be more controversial. You should split the stuff that's a slam-dunk apart from the stuff that makes policy decisions, and plan to commit the former changes first, and the latter changes only if and when there is very clear agreement on the specific policies to be changed. Finally, you've got the idea of making pg_ a reserved prefix for roles, adding some predefined roles, and giving them some predefined privileges. That should be yet another patch. I think that if you commit this the way you have it today, everybody will go, oh, look, Stephen committed something, but it looks complicated, I won't pay attention. And then, six months from now when we're in beta, or maybe after final, people will start looking at this and realizing that there are parts of it they hate, but it will be hard to fix at that point. Breaking it out will hopefully allow more discussion on the individual features of in here, of which there are probably at least four. It will also make it easier to revert part of it rather than all of it if that turns out to be needed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
Robert, all, * Stephen Frost (sfr...@snowman.net) wrote: > * Stephen Frost (sfr...@snowman.net) wrote: > > * Robert Haas (robertmh...@gmail.com) wrote: > > > The tricky part of this seems to me to be the pg_dump changes. The > > > new catalog flag seems a little sketchy to me; wouldn't it be better > > > to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL, > > > DUMP_NONE? > > > > I agree that the pg_dump changes are a very big part of this change. > > I'll look at using an enum and see if that would work. > > I've implemented this approach and there are things which I like about > it and things which I don't. I'd love to hear your thoughts. As > mentioned previously, this patch does not break the pg_stat_activity or > pg_stat_replication view APIs. Similairly, the functions which directly > feed those views return the same results they did previously, there are > just additional functions now which provide everything, and view on top > of those, for users who are GRANT'd access to them. Here is the latest revision of this patch. The big change here is the addition of default roles. This approach has been brought up a few times and Magnus recently mentioned it to me again. Having the default roles greatly reduced the impact of this change on the test_deparse regression test, which was quite nice. Updates are included for pg_upgrade and pg_dumpall to handle roles which start with "pg_" specially as we are now claiming those as "System defined" roles (similar to how we claim schemas starting with pg_ are system defined, etc). These new default roles are in line with the previously discussed role attributes, but have the advantage that they act just like normal roles and work inside of the normal permissions system. They are: pg_backup - Start and stop backups, switch xlogs, create restore points. pg_monitor - View privileged system info (user activity, replica lag) pg_replay - Control XLOG replay on replica (pause, resume) pg_replication - Create, destroy, work with replication slots pg_admin - All of the above, plus rotate logfiles and signal backends Feedback on all of this would be great. One interesting idea is that, with these defined default roles, we could rip out the majority of the changes to pg_dump and declare that users have to use only the roles we provide to manage access to those functions (or risk any changes they make to the ACLs of system objects disappearing across upgrades or pg_dump/restore's, which is what happens today anyway). I'm a bit on the fence about it myself; it'd certainly reduce the risk of this change but it would also limit users to only being able to operate at the pre-defined levels we've set, but then again, the same was going to be true with the role attributes-based approach and I don't recall any complaints during that discussion. Thoughts? Feedback on this would be most welcome; it's been a long time incubating and I'd really like to get this capability in and close it out of the current commitfest. I'm certainly of the opinion that it will be a welcome step forward for quite a few of our users as the discussion about how to create non-superuser roles for certain operations (a monitor role, in particular, but also backup and replay) has come up quite a bit, both on the lists and directly from clients. Thanks! Stephen From 488df9c567fdd0a56afa084a8f22f8b8a2412bd7 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Thu, 19 Mar 2015 14:49:26 -0400 Subject: [PATCH] Use GRANT for access to privileged functions Historically, we have hard-coded the permissions for privileged functions in C code (eg: if (superuser()) then allow X, else don't). Unfortunately, that's pretty limiting and means that functions which are useful for roles that should not be superusers (eg: monitoring, backup management) require that the user calling them be a superuser. This leads to far more uses of superuser roles than is ideal. Thankfully, we have a very handy and complex privilege system for managing who has access to what already built into PG. This is the GRANT system which has existed since very near the beginning of PG. This provides a set of system functions which are not able to be executed by all users by default and allows administrators to grant access to those functions for the users (eg: monitoring or other roles) where they feel it is appropriate. Further, create a set of pre-defined roles (which start with "pg_") for administrators to use to grant bulk access with. This greatly simplifies the granting of "monitor", "backup", and similar privileges. pg_upgrade and pg_dumpall are updated to treat roles starting with "pg_" in much the same way as the default role is handled, and pg_upgrade has been taught to complain if it finds any roles starting with "pg_" in a 9.4 or older system. Having pre-defined roles also allows 3rd-party utilities (eg: check_postgres.pl) to depend on these roles and the access they provide in t
Re: [HACKERS] Additional role attributes && superuser review
Robert, * Stephen Frost (sfr...@snowman.net) wrote: > * Robert Haas (robertmh...@gmail.com) wrote: > > On Thu, Apr 2, 2015 at 12:53 AM, Stephen Frost wrote: > > > Clearly, further testing and documentation is required and I'll be > > > getting to that over the next couple of days, but it's pretty darn late > > > and I'm currently getting libpq undefined reference errors, probably > > > because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :) > > > > > > Thoughts? > > > > The tricky part of this seems to me to be the pg_dump changes. The > > new catalog flag seems a little sketchy to me; wouldn't it be better > > to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL, > > DUMP_NONE? > > I agree that the pg_dump changes are a very big part of this change. > I'll look at using an enum and see if that would work. I've implemented this approach and there are things which I like about it and things which I don't. I'd love to hear your thoughts. As mentioned previously, this patch does not break the pg_stat_activity or pg_stat_replication view APIs. Similairly, the functions which directly feed those views return the same results they did previously, there are just additional functions now which provide everything, and view on top of those, for users who are GRANT'd access to them. This does change the API of a few functions which previously allowed roles with the "replication" attribute to call them. We could provide additional functions to provide both paths but I don't believe it's really necessary. Indeed, I feel it's better for administrators to explicit grant access to those functions instead. Note that this doesn't use an enum but a bit field for which components of a given object should be dumped out. While I like that in general, it meant a lot of changes and I'll be the first to admit that I've not tested every possible pg_dump option permutation to make sure that the correct results are returned. I plan to do that in the coming weeks and will address any issues found. Is this, more-or-less, what you were thinking of? I looked at removing the relatively grotty options (dataOnly, aclsSkip, etc) and it didn't appear trivial to use the bitmask instead. I can look into that further though, as I do feel that it'd be good if we could reduce our dependence on those options in favor of the bitmask approach. Thoughts? Thanks! Stephen From dd682d4d9dc7f25ae38bb5fc5ed5082c5071 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Thu, 19 Mar 2015 14:49:26 -0400 Subject: [PATCH] Use GRANT for access to privileged functions Historically, we have hard-coded the permissions for privileged functions in C code (eg: if (superuser()) then allow X, else don't). Unfortunately, that's pretty limiting and means that functions which are useful for monitoring require that the user calling them be a superuser. That's a problem because, generally speaking, monitoring systems should not have more access than they need and certainly should not have write access to a system. Thankfully, we have a very handy and complex privilege system for managing who has access to what already built into PG. This is the GRANT system which has existed since very near the beginning of PG. Therefore, provide a set of system functions which are not able to be executed by all users by default and allow administrators to grant access to those functions for the users (eg: monitoring or other roles) where they feel it is appropriate. We avoid breaking existing APIs for the system views by providing backwards compatible functions which continue to filter in the C code based on the user's credentials, where that is possible. For a few functions (eg: pg_logical_slot_* and friends), it makes more sense to break compatibility as they are relatively new and require admins to GRANT access to those functions explicitly on upgrade (this should be noted in the release notes). Other functions, which allowed access based on role attribute 'replication', may need to have GRANTs applied to them, but note that replication connections do not go through the normal GRANT system and therefore tools such as pg_basebackup will not be impacted by this change. In general, this change requires administrators to be more explicit about which roles have access to these functions. Last, but certainly not least, this changes pg_dump to include ACLs for the pg_catalog schema. This is necessary as administrators are now expected and encouraged to set privileges on functions and perhaps views differently from their defaults and we need to preserve those settings. --- contrib/test_decoding/expected/permissions.out | 17 +- contrib/test_decoding/sql/permissions.sql |9 + src/backend/access/transam/xlogfuncs.c | 30 - src/backend/catalog/system_views.sql | 72 ++ src/backend/replication/logical/logicalfuncs.c | 11 - src/backend/replication/slotfuncs.c| 15 - src
Re: [HACKERS] Additional role attributes && superuser review
Robert, * Robert Haas (robertmh...@gmail.com) wrote: > On Thu, Apr 2, 2015 at 12:53 AM, Stephen Frost wrote: > > Clearly, further testing and documentation is required and I'll be > > getting to that over the next couple of days, but it's pretty darn late > > and I'm currently getting libpq undefined reference errors, probably > > because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :) > > > > Thoughts? > > The tricky part of this seems to me to be the pg_dump changes. The > new catalog flag seems a little sketchy to me; wouldn't it be better > to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL, > DUMP_NONE? I agree that the pg_dump changes are a very big part of this change. I'll look at using an enum and see if that would work. > Is this creating a user-visible API break, where pg_stat_activity and > pg_stat_replication now always show only the publicly-visible > information, and privileged users must explicitly decide to query the > _all versions? If so, -1 from me on that part of this. Thanks for bringing it up. No, the existing API is exactly the same for the existing views- the only difference is that now there are _all versions which provide unfiltered data (but you have to have permission to call the _all() functions underneath, or you get a permission denied error). Where this does have an API change is with the simpler functions that used to do "if (superuser() || replication_role())" and now depend on the GRANT system instead. We can't (today, at least) say: GRANT EXECUTE ON function_whatever() TO replication_roles; And have that be kept current as that role attribute is modified. I've not done it yet, but we could certainly have pg_dump dump out GRANTs for the *current* users which have that role attribute and give those users access to the functions via the normal permissions system. I'm not really sure that's a good idea though- it might be better to have a clean break here (and one which is clearly in the "more secure/explicit" direction) and tell admins in the release notes to GRANT EXECUTE on the functions for the roles they want to give permission to. We could also duplicate the functions and have the existing ones remain as-is and have the new ones just depend on the GRANT system, but I don't particularly like that since I'm afraid we'd end up in the same boat we're in now with pg_user and friends. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On Thu, Apr 2, 2015 at 12:53 AM, Stephen Frost wrote: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> Stephen Frost writes: >> > REVOKE'ing access *without* removing the permissions checks would defeat >> > the intent of these changes, which is to allow an administrator to grant >> > the ability for a certain set of users to cancel and/or terminate >> > backends started by other users, without also granting those users >> > superuser rights. >> >> I see: we have two different use-cases and no way for GRANT/REVOKE >> to manage both cases using permissions on a single object. Carry >> on then. > > Alright, after going about this three or four different ways, I've > settled on the approach proposed in the attached patch. Most of it is > pretty straight-forward: drop the hard-coded check in the function > itself and REVOKE EXECUTE on the function from PUBLIC. The interesting > bits are those pieces which are more complex than the "all-or-nothing" > cases. > > This adds a few new SQL-level functions which return unfiltered results, > if you're allowed to call them based on the GRANT system (and EXECUTE > privileges for them are REVOKE'd from PUBLIC, of course). These are: > pg_stat_get_activity_all, pg_stat_get_wal_senders_all, and > pg_signal_backend (which is similar but not the same- that allows you to > send signals to other backends as a regular user, if you are allowed to > call that function and the other backend wasn't started by a superuser). > > There are two new views added, which simply sit on top of the new > functions: pg_stat_activity_all and pg_stat_replication_all. > > Minimizing the amount of code duplication wasn't too hard with the > pg_stat_get_wal_senders case; as it was already returning a tuplestore > and so I simply moved most of the logic into a "helper" function which > handled populating the tuplestore and then each call path was relatively > small and mostly boilerplate. pg_stat_get_activity required a bit more > in the way of changes, but they were essentially to modify it to return > a tuplestore like pg_stat_get_wal_senders, and then add in the extra > function and boilerplate for the non-filtered path. > > I had considered (and spent a good bit of time implementing...) a number > of alternatives, mostly around trying to do more at the SQL level when > it came to filtering, but that got very ugly very quickly. There's no > simple way to find out "what was the effective role of the caller of > this function" from inside of a security definer function (which would > be required for the filtering, as it would need to be able to call the > function underneath). This is necessary, of course, because functions > in views run as the caller of the view, not as the view owner (that's > useful in some cases, less useful in others). I looked at exposing the > information about the effective role which was calling a function, but > that got pretty ugly too. The GUC system has a stack, but we don't > actually update the 'role' GUC when we are in security definer > functions. There's the notion of an "outer" role ID, but that doesn't > account for intermediate security definer functions and so, while it's > fine for what it does, it wasn't really helpful in this case. > > I do still think it'd be nice to provide that information and perhaps we > can do so with fmgr_security_definer(), but it's beyond what's really > needed here and I don't think it'd end up being particularly cleaner to > do it all in SQL now that I've refactored pg_stat_get_activity. > > I'd further like to see the ability to declare on either a function call > by function call basis (inside a view defintion), of even at the view > level (as that would allow me to build up multiple views with different > parameters) "who to run this function as", where the options would be > "view owner" or "view querier", very similar to our SECURITY DEFINER vs. > SECURITY INVOKER options for functions today. This is interesting > because, more and more, we're building functions which are actually > returning data sets, not individual values, and we want to filter those > sets sometimes and not other times. In any case, those are really > thoughts for the future and get away from what this is all about, which > is reducing the need for monitoring and/or "regular" admins to have the > "superuser" bit when they don't really need it. > > Clearly, further testing and documentation is required and I'll be > getting to that over the next couple of days, but it's pretty darn late > and I'm currently getting libpq undefined reference errors, probably > because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :) > > Thoughts? The tricky part of this seems to me to be the pg_dump changes. The new catalog flag seems a little sketchy to me; wouldn't it be better to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL, DUMP_NONE? Is this creating a user-visible API break, where pg_stat_activity and pg_stat_replication now
Re: [HACKERS] Additional role attributes && superuser review
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > REVOKE'ing access *without* removing the permissions checks would defeat > > the intent of these changes, which is to allow an administrator to grant > > the ability for a certain set of users to cancel and/or terminate > > backends started by other users, without also granting those users > > superuser rights. > > I see: we have two different use-cases and no way for GRANT/REVOKE > to manage both cases using permissions on a single object. Carry > on then. Alright, after going about this three or four different ways, I've settled on the approach proposed in the attached patch. Most of it is pretty straight-forward: drop the hard-coded check in the function itself and REVOKE EXECUTE on the function from PUBLIC. The interesting bits are those pieces which are more complex than the "all-or-nothing" cases. This adds a few new SQL-level functions which return unfiltered results, if you're allowed to call them based on the GRANT system (and EXECUTE privileges for them are REVOKE'd from PUBLIC, of course). These are: pg_stat_get_activity_all, pg_stat_get_wal_senders_all, and pg_signal_backend (which is similar but not the same- that allows you to send signals to other backends as a regular user, if you are allowed to call that function and the other backend wasn't started by a superuser). There are two new views added, which simply sit on top of the new functions: pg_stat_activity_all and pg_stat_replication_all. Minimizing the amount of code duplication wasn't too hard with the pg_stat_get_wal_senders case; as it was already returning a tuplestore and so I simply moved most of the logic into a "helper" function which handled populating the tuplestore and then each call path was relatively small and mostly boilerplate. pg_stat_get_activity required a bit more in the way of changes, but they were essentially to modify it to return a tuplestore like pg_stat_get_wal_senders, and then add in the extra function and boilerplate for the non-filtered path. I had considered (and spent a good bit of time implementing...) a number of alternatives, mostly around trying to do more at the SQL level when it came to filtering, but that got very ugly very quickly. There's no simple way to find out "what was the effective role of the caller of this function" from inside of a security definer function (which would be required for the filtering, as it would need to be able to call the function underneath). This is necessary, of course, because functions in views run as the caller of the view, not as the view owner (that's useful in some cases, less useful in others). I looked at exposing the information about the effective role which was calling a function, but that got pretty ugly too. The GUC system has a stack, but we don't actually update the 'role' GUC when we are in security definer functions. There's the notion of an "outer" role ID, but that doesn't account for intermediate security definer functions and so, while it's fine for what it does, it wasn't really helpful in this case. I do still think it'd be nice to provide that information and perhaps we can do so with fmgr_security_definer(), but it's beyond what's really needed here and I don't think it'd end up being particularly cleaner to do it all in SQL now that I've refactored pg_stat_get_activity. I'd further like to see the ability to declare on either a function call by function call basis (inside a view defintion), of even at the view level (as that would allow me to build up multiple views with different parameters) "who to run this function as", where the options would be "view owner" or "view querier", very similar to our SECURITY DEFINER vs. SECURITY INVOKER options for functions today. This is interesting because, more and more, we're building functions which are actually returning data sets, not individual values, and we want to filter those sets sometimes and not other times. In any case, those are really thoughts for the future and get away from what this is all about, which is reducing the need for monitoring and/or "regular" admins to have the "superuser" bit when they don't really need it. Clearly, further testing and documentation is required and I'll be getting to that over the next couple of days, but it's pretty darn late and I'm currently getting libpq undefined reference errors, probably because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :) Thoughts? Thanks! Stephen diff --git a/contrib/test_decoding/expected/permissions.out b/contrib/test_decoding/expected/permissions.out new file mode 100644 index 212fd1d..538ebdc *** a/contrib/test_decoding/expected/permissions.out --- b/contrib/test_decoding/expected/permissions.out *** SET synchronous_commit = on; *** 4,9 --- 4,16 CREATE ROLE lr_normal; CREATE ROLE lr_superuser SUPERUSER; CREATE ROLE lr_replication REPLICATION; + GRANT EXECUTE ON FUN
Re: [HACKERS] Additional role attributes && superuser review
Stephen Frost writes: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> That seems fairly ugly. Why would we need a new, duplicative function >> here? (Apologies if the reasoning was spelled out upthread, I've not >> been paying much attention.) > Currently, those functions allow users to signal backends which are > owned by them, which means they can be used by anyone. Simply > REVOKE'ing access to them would remove that capability and an admin who > then GRANT's access to the function would need to understand that > they're allowing that user the ability to cancel/terminate any backends > (except those initiated by superusers, at least if we keep that check as > discussed upthread). > If those functions just had simply superuser() checks that prevented > anyone else from using them then this wouldn't be an issue. > REVOKE'ing access *without* removing the permissions checks would defeat > the intent of these changes, which is to allow an administrator to grant > the ability for a certain set of users to cancel and/or terminate > backends started by other users, without also granting those users > superuser rights. I see: we have two different use-cases and no way for GRANT/REVOKE to manage both cases using permissions on a single object. Carry on then. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > ... Lastly, there is the question of pg_cancel_backend and > > pg_terminate_backend. My thinking on this is to create a new > > 'pg_signal_backend' which admins could grant access to and leave the > > existing functions alone (modulo the change for has_privs_of_role as > > discussed previously). We'd rename the current 'pg_signal_backend' to > > something else (maybe '_helper'); it's not exposed anywhere and > > therefore renaming it shouldn't cause any heartache. > > That seems fairly ugly. Why would we need a new, duplicative function > here? (Apologies if the reasoning was spelled out upthread, I've not > been paying much attention.) Currently, those functions allow users to signal backends which are owned by them, which means they can be used by anyone. Simply REVOKE'ing access to them would remove that capability and an admin who then GRANT's access to the function would need to understand that they're allowing that user the ability to cancel/terminate any backends (except those initiated by superusers, at least if we keep that check as discussed upthread). If those functions just had simply superuser() checks that prevented anyone else from using them then this wouldn't be an issue. REVOKE'ing access *without* removing the permissions checks would defeat the intent of these changes, which is to allow an administrator to grant the ability for a certain set of users to cancel and/or terminate backends started by other users, without also granting those users superuser rights. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Stephen Frost writes: > ... Lastly, there is the question of pg_cancel_backend and > pg_terminate_backend. My thinking on this is to create a new > 'pg_signal_backend' which admins could grant access to and leave the > existing functions alone (modulo the change for has_privs_of_role as > discussed previously). We'd rename the current 'pg_signal_backend' to > something else (maybe '_helper'); it's not exposed anywhere and > therefore renaming it shouldn't cause any heartache. That seems fairly ugly. Why would we need a new, duplicative function here? (Apologies if the reasoning was spelled out upthread, I've not been paying much attention.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
All, * Stephen Frost (sfr...@snowman.net) wrote: > Alright, I've got an initial patch to do this for pg_start/stop_backup, > pg_switch_xlog, and pg_create_restore_point. The actual backend changes > are quite small, as expected. I'll add in the changes for the other > functions being discussed and adapt the documentation changes from > the earlier patch to make sense, but what I'd really appreciate are any > comments or thoughts regarding the changes to pg_dump (which are generic > to all of the function changes, of course). So, I've tested this approach with extensions and binary upgrade and things appear to all be working correctly, but there's a few issues remaining to discuss: The functions pg_start_backup and pg_stop_backup can currently be run by replication roles but those roles won't have any permissions on those functions with the new approach unless we GRANT those rights during pg_dump and/or pg_upgrade. Note that this isn't an issue for tools which use the replication protocol (eg: pg_basebackup) since the replication protocol calls do_pg_start_bacup() directly. As such, my first question is- do we want to worry about this? We should certainly mention it in the release notes but I'm not sure if we really want to do anything else. The second question is in regards to pg_stat_activity. My first suggestion for how to address that would be to have the function return everything and then have the view perform the filtering to return only what's shown today for users. Admins could then grant access to the function for whichever users they want to have access to everything. That strikes me as the best way to avoid changing common usage while still providing the flexibility. Another option would be to still invent the MONITOR role attribute and keep the rest the same. Again, we'd want to mention something in the release notes regarding this change. Lastly, there is the question of pg_cancel_backend and pg_terminate_backend. My thinking on this is to create a new 'pg_signal_backend' which admins could grant access to and leave the existing functions alone (modulo the change for has_privs_of_role as discussed previously). We'd rename the current 'pg_signal_backend' to something else (maybe '_helper'); it's not exposed anywhere and therefore renaming it shouldn't cause any heartache. For pg_create_restore_point, pg_switch_xlog, pg_xlog_replay_pause, pg_xlog_replay_resume, pg_rotate_logfile, we can just remove the if(!superuser()) ereport() checks and REVOKE rights on them during the initdb process, since there isn't anything complicated happening for those today. One other question which I've been thinking about is if we want to try and preserve permission changes associated with other catalog objects (besides functions), or if we even want to change how functions are handled regarding permission changes. We don't currently preserve any such changes across dump/restore or even binary upgrades and this would be changing that. I don't recall any complaints about not preserving permission changes to objects in pg_catalog, but one could argue that our lack of doing so today is a bug. Thoughts? Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Peter, all, * Peter Eisentraut (pete...@gmx.net) wrote: > Why are we not using roles and function execute privileges for this? Alright, I've got an initial patch to do this for pg_start/stop_backup, pg_switch_xlog, and pg_create_restore_point. The actual backend changes are quite small, as expected. I'll add in the changes for the other functions being discussed and adapt the documentation changes from the earlier patch to make sense, but what I'd really appreciate are any comments or thoughts regarding the changes to pg_dump (which are generic to all of the function changes, of course). I've added a notion of "the catalog schema" to pg_dump's internal _namespaceinfo representation and then marked pg_catalog as being that schema, as well as being a "dumpable" schema. Throughout the selectDumpable functions, I've made changes to only mark the objects in the catalog as dumpable if they are functions. I'm planning to look into the extension and binary upgrade paths since I'm a bit worried those may not work with this approach, but I wanted to get this out there for at least an initial review as, if people feel this makes things too ugly on the pg_dump side of things then we may want to reconsider using the role attributes instead. Thanks! Stephen diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c new file mode 100644 index 2179bf7..36029d0 *** a/src/backend/access/transam/xlogfuncs.c --- b/src/backend/access/transam/xlogfuncs.c *** pg_start_backup(PG_FUNCTION_ARGS) *** 54,64 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); PG_RETURN_LSN(startpoint); --- 54,59 *** pg_stop_backup(PG_FUNCTION_ARGS) *** 82,92 { 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); PG_RETURN_LSN(stoppoint); --- 77,82 *** pg_switch_xlog(PG_FUNCTION_ARGS) *** 100,110 { XLogRecPtr switchpoint; - if (!superuser()) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - (errmsg("must be superuser to switch transaction log files"; - if (RecoveryInProgress()) ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), --- 90,95 *** pg_create_restore_point(PG_FUNCTION_ARGS *** 129,139 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, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), --- 114,119 diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql new file mode 100644 index 2800f73..38605de *** a/src/backend/catalog/system_views.sql --- b/src/backend/catalog/system_views.sql *** RETURNS interval *** 897,899 --- 897,907 LANGUAGE INTERNAL STRICT IMMUTABLE AS 'make_interval'; + + -- Revoke privileges for functions that should not be available to + -- all users. Administrators are allowed to change this later, if + -- they wish. + REVOKE EXECUTE ON FUNCTION pg_start_backup(text, boolean) FROM public; + REVOKE EXECUTE ON FUNCTION pg_stop_backup() FROM public; + REVOKE EXECUTE ON FUNCTION pg_switch_xlog() FROM public; + REVOKE EXECUTE ON FUNCTION pg_create_restore_point(text) FROM public; diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c new file mode 100644 index fdfb431..164608a *** a/src/bin/pg_dump/pg_dump.c --- b/src/bin/pg_dump/pg_dump.c *** selectDumpableNamespace(NamespaceInfo *n *** 1234,1245 --- 1234,1255 * If specific tables are being dumped, do not dump any complete * namespaces. If specific namespaces are being dumped, dump just those * namespaces. Otherwise, dump all non-system namespaces. + * + * Note that we do consider dumping ACLs of functions in pg_catalog, + * so mark that as a dumpable namespace, but further mark it as the + * catalog namespace. */ + + /* Will be set when we may be dumping catalog ACLs, see below. */ + nsinfo->catalog = false; + if (table_include_oids.head != NULL) nsinfo->dobj.dump = false; else if (schema_include_oids.head != NULL) nsinfo->dobj.dump = simple_oid_list_member(&schema_include_oids, nsinfo->dobj.catId.oid); + else if (strncmp(nsinfo->dobj.name, "pg_catalog", 10) == 0) + nsinfo->dobj.dump = nsinfo->ca
Re: [HACKERS] Additional role attributes && superuser review
* Peter Eisentraut (pete...@gmx.net) wrote: > On 2/28/15 10:10 PM, Stephen Frost wrote: > > * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: > >> I have attached and updated patch for review. > > > > Thanks! I've gone over this and made quite a few documentation and > > comment updates, but not too much else, so I'm pretty happy with how > > this is coming along. As mentioned elsewhere, this conflicts with the > > GetUserId() to has_privs_of_role() cleanup, but as I anticipate handling > > both this patch and that one, I'll find some way to manage. :) > > > > Updated patch attached. Barring objections, I'll be moving forward with > > this soonish. Would certainly appreciate any additional testing or > > review that you (or anyone!) has time to provide. > > Let's move this discussion to the right thread. Agreed. > Why are we not using roles and function execute privileges for this? There isn't a particular reason not to, except that the existing checks are in C code and those would need to be removed and the permission changes done at initdb time to revoke EXECUTE from PUBLIC for these functions. Further, as you pointed out, we'd need to dump out the permissions for the catalog tables and functions with this approach. I don't expect that to be too difficult to do though. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
On 2/28/15 10:10 PM, Stephen Frost wrote: > Adam, > > * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: >> I have attached and updated patch for review. > > Thanks! I've gone over this and made quite a few documentation and > comment updates, but not too much else, so I'm pretty happy with how > this is coming along. As mentioned elsewhere, this conflicts with the > GetUserId() to has_privs_of_role() cleanup, but as I anticipate handling > both this patch and that one, I'll find some way to manage. :) > > Updated patch attached. Barring objections, I'll be moving forward with > this soonish. Would certainly appreciate any additional testing or > review that you (or anyone!) has time to provide. Let's move this discussion to the right thread. Why are we not using roles and function execute privileges for this? -- 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
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Let's go with the "NO_" prefix then ... that seems better to me than no > separator. Works for me. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Stephen Frost wrote: > Alvaro, > > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > > That being so, I would consider the idea that the NO bit is a separate > > word rather than run together with the actual privilege name. And given > > that CREATE has all the options default to "NO", there is no need to > > have these options at all in CREATE, is there? > > That's a good point, except that INHERIT is actually on by default, and > LOGIN defaults to 'on' if you use CREATE USER, and 'off' if you use > CREATE ROLE. If they were actually all 'no' by default then this > simplication would work but it's not and therefore I don't think we want > to have some which are allowed at CREATE time with 'on' and some with > 'off' depending on whatever the default is. Today, you can write a > script to easily duplicate an existing role by just looking at what is > on and off and using X and NOX. This approach would require that script > to know what's valid at CREATE time and what isn't. All true. > > where privilege can be: > >SUPERUSER | CREATEDB | CREATEROLE > > | CREATEUSER | INHERIT | LOGIN > > | REPLICATION | EXCLUSIVE_BACKUP | ... > > and option can be: > >CONNECTION LIMIT connlimit > > | [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password' > > | VALID UNTIL 'timestamp' > > With the 'NO' distinct, as discussed above, it seems like we should be > able to support this.. I certainly like it more too. Great. > Certainly, and I like where you're going with this, just seems like > there's a couple wrinkles to deal with. Yeah. Let's go with the "NO_" prefix then ... that seems better to me than no separator. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Alvaro Herrera writes: > > That being so, I would consider the idea that the NO bit is a separate > > word rather than run together with the actual privilege name. And given > > that CREATE has all the options default to "NO", there is no need to > > have these options at all in CREATE, is there? > > FWIW, I disagree with that, mainly because I don't think we should cast in > stone the assumption that NO will always be the default for everything we > might invent in the future. Also, the precedent of the existing options > will lead people to expect that they can explicitly say NO-whatever. Right, and, in fact, not everything is 'NO' by default today anyway. Further, people might try to say 'NO CONNECTION LIMIT', and while we might want to try and support that, there might be options where 'NO' doesn't actually make sense ('NO VALID UNTIL'?). Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Alvaro Herrera writes: > That being so, I would consider the idea that the NO bit is a separate > word rather than run together with the actual privilege name. And given > that CREATE has all the options default to "NO", there is no need to > have these options at all in CREATE, is there? FWIW, I disagree with that, mainly because I don't think we should cast in stone the assumption that NO will always be the default for everything we might invent in the future. Also, the precedent of the existing options will lead people to expect that they can explicitly say NO-whatever. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Stephen Frost wrote: > > So you'd advocate EXCLUSIVE_BACKUP and NOEXCLUSIVE_BACKUP? Or > > NO_EXCLUSIVE_BACKUP? Or..? If this was a green field, I think we might > > actually use spaces instead, but I'm really not sure we want to go > > through and redo everything that way at this point.. We'd end up > > breaking a lot of scripts that currently work today and I'm really not > > convinced it's better enough to justify that. > > Well, all things considered, we actually are in a green field here, > aren't we. We don't have to break old scripts, but no existing script > is using > ALTER USER foo NOEXCLUSIVEBACKUP > because that option doesn't currently exist. Sorry for not being clear, I was talking about how we'd deal with the existing ones (to try and have a consistent approach across all of the options). > That being so, I would consider the idea that the NO bit is a separate > word rather than run together with the actual privilege name. And given > that CREATE has all the options default to "NO", there is no need to > have these options at all in CREATE, is there? That's a good point, except that INHERIT is actually on by default, and LOGIN defaults to 'on' if you use CREATE USER, and 'off' if you use CREATE ROLE. If they were actually all 'no' by default then this simplication would work but it's not and therefore I don't think we want to have some which are allowed at CREATE time with 'on' and some with 'off' depending on whatever the default is. Today, you can write a script to easily duplicate an existing role by just looking at what is on and off and using X and NOX. This approach would require that script to know what's valid at CREATE time and what isn't. > Then things such as > ALTER USER foo NOREPLICATION > become synonyms for > ALTER USER foo NO REPLICATION (or whatever) I'm not against supporting this and if it's a synonym then it won't break existing scripts. I can look into this, but it ends up being an independent discussion from actually adding these new options. > Something like that would be my choice for UI, anyway. The existing > stuff seems to clutter it overly, and while it works sorta OK for half a > dozen privs, it becomes clunkier as you have more of them. From the > perspective of docs, I think this whole thing becomes simpler if you > split out the NO from each privilege name; currently we have I agree that it does get clunkier and this is certainly an interesting discussion as we might be able to do something better, which would certainly be great. > alvherre=# \h alter user > Command: ALTER USER > Description: change a database role > Syntax: > ALTER USER name [ [ WITH ] option [ ... ] ] > > where option can be: > > SUPERUSER | NOSUPERUSER > | CREATEDB | NOCREATEDB > | CREATEROLE | NOCREATEROLE > | CREATEUSER | NOCREATEUSER > | INHERIT | NOINHERIT > | LOGIN | NOLOGIN > | REPLICATION | NOREPLICATION > | CONNECTION LIMIT connlimit > | [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password' > | VALID UNTIL 'timestamp' > > And it would become something like > > ALTER USER name [ WITH ] { [ NO ] privilege | option } [ ... ] > > where privilege can be: > SUPERUSER | CREATEDB | CREATEROLE >| CREATEUSER | INHERIT | LOGIN >| REPLICATION | EXCLUSIVE_BACKUP | ... > and option can be: > CONNECTION LIMIT connlimit >| [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password' >| VALID UNTIL 'timestamp' With the 'NO' distinct, as discussed above, it seems like we should be able to support this.. I certainly like it more too. > (the NOSUPERUSER etc forms of the old options would continue to work for > the sake of backwards compatibility, but we wouldn't provide NO- > prefixed forms of the new privileges.) I'm not sure that I like that.. My initial reaction is that it'd complicate the code and confuse people coming to this later- and to what purpose..? To force everyone to use 'NO' instead? Maybe in some far off future we could say that the old 'NOX' format was deprecated and remove it, but I have a hard time seeing the need to. > I'm not wedded to any of this, but I think it ought to be at least given > some consideration. Certainly, and I like where you're going with this, just seems like there's a couple wrinkles to deal with. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Stephen Frost wrote: > So you'd advocate EXCLUSIVE_BACKUP and NOEXCLUSIVE_BACKUP? Or > NO_EXCLUSIVE_BACKUP? Or..? If this was a green field, I think we might > actually use spaces instead, but I'm really not sure we want to go > through and redo everything that way at this point.. We'd end up > breaking a lot of scripts that currently work today and I'm really not > convinced it's better enough to justify that. Well, all things considered, we actually are in a green field here, aren't we. We don't have to break old scripts, but no existing script is using ALTER USER foo NOEXCLUSIVEBACKUP because that option doesn't currently exist. That being so, I would consider the idea that the NO bit is a separate word rather than run together with the actual privilege name. And given that CREATE has all the options default to "NO", there is no need to have these options at all in CREATE, is there? So we would have only the "positive" actions in create: CREATE USER foo REPLICATION EXCLUSIVE_BACKUP (if you want no EXCLUSIVE_BACKUP privilege, just leave it out. It isn't any more complicated than saying NOEXCLUSIVEBACKUP) You could turn these off in ALTER: ALTER USER foo NO EXCLUSIVE_BACKUP; or perhaps ALTER USER foo DROP EXCLUSIVE_BACKUP; or some such. (REVOKE would be nicer, I guess, but IIRC that conflicts with other stuff. I guess another option, in line with the current optional WITH, would be to have WITHOUT: ALTER USER foo WITHOUT EXCLUSIVE_BACKUP; But that looks a bit silly to me.) Then things such as ALTER USER foo NOREPLICATION become synonyms for ALTER USER foo NO REPLICATION (or whatever) Something like that would be my choice for UI, anyway. The existing stuff seems to clutter it overly, and while it works sorta OK for half a dozen privs, it becomes clunkier as you have more of them. From the perspective of docs, I think this whole thing becomes simpler if you split out the NO from each privilege name; currently we have alvherre=# \h alter user Command: ALTER USER Description: change a database role Syntax: ALTER USER name [ [ WITH ] option [ ... ] ] where option can be: SUPERUSER | NOSUPERUSER | CREATEDB | NOCREATEDB | CREATEROLE | NOCREATEROLE | CREATEUSER | NOCREATEUSER | INHERIT | NOINHERIT | LOGIN | NOLOGIN | REPLICATION | NOREPLICATION | CONNECTION LIMIT connlimit | [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password' | VALID UNTIL 'timestamp' And it would become something like ALTER USER name [ WITH ] { [ NO ] privilege | option } [ ... ] where privilege can be: SUPERUSER | CREATEDB | CREATEROLE | CREATEUSER | INHERIT | LOGIN | REPLICATION | EXCLUSIVE_BACKUP | ... and option can be: CONNECTION LIMIT connlimit | [ ENCRYPTED | UNENCRYPTED ] PASSWORD 'password' | VALID UNTIL 'timestamp' (the NOSUPERUSER etc forms of the old options would continue to work for the sake of backwards compatibility, but we wouldn't provide NO- prefixed forms of the new privileges.) I'm not wedded to any of this, but I think it ought to be at least given some consideration. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > >> If we were choosing those names nowadays, would we choose CREATEDB at > >> all in the first place? I think we'd go for something more verbose, > >> probably CREATE_DATABASE. (CREATEROLE is not as old as CREATEDB, but my > >> bet is that it was modelled after CREATEUSER without considering the > >> whole readability topic too much.) > >> > >> Anyway it doesn't seem to me that consistency with lack of separators in > >> those very old names should be our guiding principle here. > > > So you'd advocate EXCLUSIVE_BACKUP and NOEXCLUSIVE_BACKUP? Or > > NO_EXCLUSIVE_BACKUP? Or..? If this was a green field, I think we might > > actually use spaces instead, but I'm really not sure we want to go > > through and redo everything that way at this point.. We'd end up > > breaking a lot of scripts that currently work today and I'm really not > > convinced it's better enough to justify that. > > FWIW, I agree with Alvaro, and I'd go with e.g. NO_EXCLUSIVE_BACKUP. Ok, works for me. Will update (including tab completion) and send out for review. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Stephen Frost writes: > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: >> If we were choosing those names nowadays, would we choose CREATEDB at >> all in the first place? I think we'd go for something more verbose, >> probably CREATE_DATABASE. (CREATEROLE is not as old as CREATEDB, but my >> bet is that it was modelled after CREATEUSER without considering the >> whole readability topic too much.) >> >> Anyway it doesn't seem to me that consistency with lack of separators in >> those very old names should be our guiding principle here. > So you'd advocate EXCLUSIVE_BACKUP and NOEXCLUSIVE_BACKUP? Or > NO_EXCLUSIVE_BACKUP? Or..? If this was a green field, I think we might > actually use spaces instead, but I'm really not sure we want to go > through and redo everything that way at this point.. We'd end up > breaking a lot of scripts that currently work today and I'm really not > convinced it's better enough to justify that. FWIW, I agree with Alvaro, and I'd go with e.g. NO_EXCLUSIVE_BACKUP. I concur that multiple separate words would be a syntax mess, but I see no reason not to use underscores instead. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Adam Brightwell wrote: > > 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. > > If we were choosing those names nowadays, would we choose CREATEDB at > all in the first place? I think we'd go for something more verbose, > probably CREATE_DATABASE. (CREATEROLE is not as old as CREATEDB, but my > bet is that it was modelled after CREATEUSER without considering the > whole readability topic too much.) > > Anyway it doesn't seem to me that consistency with lack of separators in > those very old names should be our guiding principle here. So you'd advocate EXCLUSIVE_BACKUP and NOEXCLUSIVE_BACKUP? Or NO_EXCLUSIVE_BACKUP? Or..? If this was a green field, I think we might actually use spaces instead, but I'm really not sure we want to go through and redo everything that way at this point.. We'd end up breaking a lot of scripts that currently work today and I'm really not convinced it's better enough to justify that. Thanks! Stephen signature.asc Description: Digital signature