Re: Additional role attributes && superuser review - Mailing list pgsql-hackers

* Magnus Hagander (magnus@hagander.net) wrote:
> On Wed, Dec 31, 2014 at 3:08 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Magnus Hagander (magnus@hagander.net) wrote:
> > > that doing so would be strictly more than what pg_dump actually requires
> > > > but it's also what we actually have support for in our privilege
> > system.
> > >
> > > Yeah, but would it also be what people would actually *want*?
> >
> > I can certainly see reasons why you'd want such a role to be able to
> > execute functions- in particular, consider xlog_pause anx xlog_resume.
>
> If this role can't execute those functions then they probably can't
> > successfully run pg_dump against a replica.
>
> uh, pg_dump doesn't run those commands :P I don't see why that's a
> requirement at all.  And you can still always grant those things on top of
> whatever the privilege gives you.

Ok, so, first off, this is all about the discussion around "is this
additive or not"..  Since we just agreed that it's additive, I'm not
sure I see the need to discuss the EXECUTE privileges..

Having said that though..

If you can't pause/resume then you can end up with your pg_dump
transaction getting killed.  I'm aware of folks who already do this
today, by hand with shell scripts..  I agree that pg_dump doesn't do it,
but I do think it'd be nice to have and I can certainly see the use-case
for them..

> > I think having it do exactly what pg_dump needs, and not things like
> > > execute functions etc, would be the thing people want for a 'DUMP'
> > > privilege.
> >
> > What if we want pg_dump in 9.6 to have an option to execute xlog_pause
> > and xlog_resume for you?  You wouldn't be able to run that against a 9.5
> > database (or at least, that option wouldn't work).
>
> It would if you added an explicit grant for it, which would have to be
> documented.

Huh?  An explicit grant for xlog_pause/xlog_resume won't work as we
check role attributes rights inside the function..

> I don't see how that's different if the definition is "allows select on all
> tables". That wouldn't automatically give it the ability to do xlog_pause
> in an old version either.

Ok, that I agree with- you don't automatically get xlog_pause rights if
you have the 'select on all tables' right.

> > We've discussed having a role attribute for COPY-from-filesystem, but
> > pg_dump doesn't use that ever, it only uses COPY TO STDOUT.  I'm not
> > a fan of making a COPY_TO_STDOUT-vs-SELECT distinction just for this..
>
> Yeah, it's probably going overboard with it, since AFAICT the only thing
> that would actually be affected is RULEs on SELECT, which I bet most people
> don't use on their tables.

Well, we could make SELECT not work, but if you've got COPY then you can
still get all the data, so, yeah, not much different.  I seriously doubt
many people are using rules..

> > > One point would be that if we define it as "exactly what pg_dump needs",
> > > that definition can change in a future major version.
> >
> > Sure, but that then gets a bit ugly because we encourage running the
> > latest version of pg_dump against the prior-major-version.
>
> But the latest version of pg_dump *knows* how the prior major version
> behaved with these things, and can either adapt, or give the user a message
> about what they need to do to adapt.

Yes, that's true.

> > No, it would still be *extremely* useful.  We have an option to pg_dump
> > to say "please dump with RLS enabled".  What that means is that you'd be
> > able to dump the entire database for all data you're allowed to see
> > through RLS policies.  How is that useless?  I certainly think it'd be
> > very handy and a good way to get *segregated* dumps according to policy.
>
> Hmm. Yeah, I guess - my mindset was int he "database backup" mode, where a
> "silently partial" backup is a really scary thing rather than a feature :)

Agreed, we don't ever want that.

> I guess as long as you still dump all the parts, RLS itself ensures that
> foreign keys etc will actually be valid upon a reaload?

If you set up your policies correctly, yes.  RLS is flexible enough that
you could create policies which fail, but you have the same problem
today to some extent (consider tables you don't have SELECT rights on).

> > > We could/should also throw a WARNING if DUMP Is granted to a role without
> > > BYPASSRLS in case row level security is enabled in the system, I think.
> > But
> > > that's more of an implementation detail.
> >
> > That's a bit ugly and RLS could be added to a relation after the DUMP
> > privilege is granted.
>
> Yes, it's not going to be all-covering, but it can still be a useful
> hint/warning in the cases where it *does* that. We obviously still need
> pg_dump to give the error in both scenarios.

I'm not against doing it, personally, but I suspect others won't like it
(or at least, that's been the case in the past with other things..).

> What I think this part of the discussion is getting at is that there's a
> > lot of people who view pg_dump as explicitly for "dump the ENTIRE
> > database".  While that's one use-case it is certainly not the only one.
> > I find "pg_dump schema X" to be very common and often find that pg_dump
> > against an entire database isn't practical because of the size of the
> > database.
>
> Agreed. I was in that mindset since we were talking about other backups.
> And FWIW, one reason I like to call it "DUMP" and not anything to do with
> BACKUP is exactly that. pg_dump is often not very useful for backups
> anymore, due to the size of databases, but it's very useful for other
> things.

Ok.

> > Ok, I see the point you're making that we could make this into a
> > capability which isn't something which can be expressed through our
> > existing GRANT system.  That strikes me as a solution trying to find a
> > problem though.  There's no need to invent such an oddity for this
> > particular use-case, I don't think.
>
> Maybe not, but we should make sure we don't paint ourselves into a corner
> where we cannot do it in the future either.

Agreed.  Do you see a risk here of that?

> > For regular permissions, we could just pre-populate the system with
> > > predefined roles and use regular GRANTs to those roles, instead of
> > relying
> > > on role attributes, which might in that case make it even more clear?
> >
> > The reason for this approach is to address exactly the nightmare that is
> > trying to maintain those regular permissions across all the objects in
> > the system.  Today, people simply give the role trying to do the pg_dump
> > superuser, which is the best option we have.  Saying 'grant SELECT on
> > all the tables and USAGE on all the schemas' isn't suggested because
> > it's a huge pain.  This role attribute provides a middle ground where
> > the pg_dump'ing role isn't a superuser, but you don't have to ensure
> > usage and select are granted to it for every relation.
>
> No, what I'm saying is we could have *predefined role* that allows "select
> on all the tables and usage on all the schemas". And you are unable to
> actually remove that. It's not stored on the object, so you cannot REVOKE
> the permission on the *object*. Since it's not store on the object it will
> also automatically apply to all new objects, regardless of what you've done
> with DEFAULT PRIVILEGES etc.
>
> But you can grant users and other roles access to this role, and it's dealt
> with like other roles from the "who has it" perspective, instead of being
> special-cased.
>
> Instead of "ALTER USER foo WITH DUMP" (or whatever), you'd just do a "GRANT
> pgdump TO foo" (which would then also work through things like group
> membership as well)

There's a definite backwards-compatibility concern with this, of course,
but I see where you're coming from.  This only really applies with this
particular pg_dump-related role-attribute discussion though, right?

This role wouldn't be able to be logged in with, I presume?  Would it
show up when you run \du?  What about in pg_authid?  I feel like it
would have to and I worry that users might not care for it- and what
happens if they want to remove it?

The question about role-attribute vs. inheirited-right is one that I've
been wondering about also though.

> Another is simply that you don't want the cronjob that runs the backup
> > to be able to read any of the data directly (why would you?).  I agree
> > that the individuals who have this capability would still need to
> > coordinate or at lesat not step on each other or they could end up with
> > bad backups.
> >
> > Another way to address that would be to require that the role calling
> > stop_backup be a member of the role which called start_backup (that also
> > address the superuser case, as superusers are considered members of all
> > roles).  That seems like a pretty reasonable sanity check requirement.
>
> This diverges a bit from the actual role attribute discussion here, but I
> think a better solution to this is to actually have a separate interface
> rather than pg_start/pg_stop backup. One of the main problems with
> pg_start/pg_stop vs pg_basebackup is that you can easily leave the system
> in a broken state (for example by forgetting to pg_stop_backup, or by
> accidentally pg_stop_backup:ing in the middle of someone elses backup).

I agree.  I've had this happen to me a number of times and it really
stinks to have your *next* backup fail because the last one failed
half-way and didn't run pg_stop_backup.

> pg_basebackup gets around this by automatically executing do_pg_stop_backup
> if the client disconnects. This also lets us allow parallel base backups.
> We could have an equivalent functionality exposed through a SQL function,
> or argument to pg_start_backup - it would require the backup software to
> keep the connection running as it works and then disconnect when it's done,
> but for anything beyond the most trivial shellscript that's not exactly
> hard, and it would make the backups safer.

Agreed, I do like that idea.

> If we did that, perhaps we don't even need a separate privilege for
> pg_start_backup() as it is today, btu can leave that as superuser?

I don't see how this follows though..  We are not talking about only
pg_basebackup or only about where the user running pg_start/stop
has filesystem-level access to the database.

> > Not quite- remember that reply_pause/resume can be done on a replica, so
> > they are useful to be able to grant independent from superuser.  Perhaps
> > we call such a role attribute XLOGREPLAY ?
>
> Yes, that's what I meant - they are useful, but they're not interesting for
> the bacukp itself. So yes, something like XLOGREPLAY makes sense.

Ok, that works for me.

> > > > You mean something that restricts the user to read even *if* write
> > > > > permissions has been granted on an individual table? Yeah, that would
> > > > > actually be quite useful, I think - sort of a "reverse privilege".
> > > >
> > > > Yes.  My thinking on how to approach this was to forcibly set all of
> > > > their transactions to read-only.
> > >
> > > Agreed that this would be very useful.
> >
> > Glad we agree..  Any thoughts on implementation? :)
>
> Where's your patch? :)

Haha. :P

> In theory we'd just have to trap any attempt to set the value for
> transaction_read_only, no? Same as hot_standby does?

I like the idea..  Will have to give it a shot and see what happens. :)
Thanks!
    Stephen