Re: pg_dump roles support

Greetings,

Discussing psql options made me recall an annoying problem that we've
run into. There's no way (unless it was added to 8.3 and I missed it,
but I don't think so) to tell pg_dump 'switch to this role before
doing anything else'. That's very frustrating when you use no-inherit
roles for admins. eg:

create role admin with noinherit;
grant postgres to admin;
grant admin to joesysadmin;

pg_dump -U joesysadmin mydb;

Fails because joesysadmin hasn't got rights to everything directly.
It'd be nice if pg_dump could take a '-r postgres' to 'set role' to
a role which has the necessary permissions before locking all the
tables and whatnot. The same 'set role' would also be included at the
top of the resulting dump file. We could have a seperate flag for
that but I don't think it's necessary.

Comments?

I doubt there'd be very much code involved but I'd be willing to write
a patch if people agree with the general idea/approach.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> create role admin with noinherit;
> grant postgres to admin;
> grant admin to joesysadmin;

> pg_dump -U joesysadmin mydb;

> Fails because joesysadmin hasn't got rights to everything directly.

Seems like the correct answer to that is "use a saner role
configuration".

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > create role admin with noinherit;
> > grant postgres to admin;
> > grant admin to joesysadmin;
>
> > pg_dump -U joesysadmin mydb;
>
> > Fails because joesysadmin hasn't got rights to everything directly.
>
> Seems like the correct answer to that is "use a saner role
> configuration".

Funny, it's exactly the type of setup described here:
http://www.postgresql.org/docs/8.2/interactive/role-membership.html

Far as I can tell anyway. What would you suggest? The point here is
that joesysadmin shouldn't get full postgres privs on login since most
of the time he won't need them. When he does need them, he can do a
'set role postgres', do what he needs to do and then 'reset role' when
he's done. Minimizing the amount of time with superuser privs is a good
thing in general, I would think.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> Seems like the correct answer to that is "use a saner role
>> configuration".

> Far as I can tell anyway. What would you suggest? The point here is
> that joesysadmin shouldn't get full postgres privs on login since most
> of the time he won't need them.

It's sane to set up a manually-used admin account that way, I agree.
What doesn't follow is that an account configured for manual use should
be used for non-interactive stuff like pg_dump.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Far as I can tell anyway. What would you suggest? The point here is
> > that joesysadmin shouldn't get full postgres privs on login since most
> > of the time he won't need them.
>
> It's sane to set up a manually-used admin account that way, I agree.
> What doesn't follow is that an account configured for manual use should
> be used for non-interactive stuff like pg_dump.

I strongly disagree that pg_dump isn't to be used manually, which I
think is what you were getting at. We use it a great deal to dump
individual schemas and copy them between systems. We *don't* use it
anymore for full database dumps (something which was done in the past)
because we use PITR instead. In fact, I encourage my users to use it to
copy schema structures around when they need a seperate area for
development or testing.

What's frustrating is when an admin needs to copy a schema that he
doesn't directly have rights to (another user's schema, or a schema used
by a site or something) he has to login to the database server
(something I'd like to minimize anyway- almost everything can easily be
done from psql), su to root, su to postgres, do the pg_dump on the box,
and then work out copying it off the box.

I suppose I could write my own pg_dump that actually used psql
underneath or add it's functionality to our perl toolkit (it's damn
handy to be able to change permissions on every table in a schema with
one command, and it takes role as an argument, heh), but rewriting the
dependency handling and whatnot isn't something I'm really keen to do.

Thanks,

Stephen

Greetings,

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> Discussing psql options made me recall an annoying problem that we've
> run into. There's no way (unless it was added to 8.3 and I missed it,
> but I don't think so) to tell pg_dump 'switch to this role before
> doing anything else'. That's very frustrating when you use no-inherit
> roles for admins. eg:

I've looked into using PGOPTIONS to set the role, and it doesn't seem to
be possible because when we're processing the backend command-line
options we're not yet in a transaction state, so variable.c:assign_role
will always come back with NULL and you get:

vardamir:/home/sfrost> PGOPTIONS="-c role=postgres" psql -d networx -h vardamir
psql: FATAL: invalid value for parameter "role": "postgres"

In current CVS the relevant lines in variable.c are around 868. That's
my best guess as to what's happening anyway, I havn't had a chance to
actually hook up a debugger and trace it.

As I discuss above, it'd be really nice have a --role or similar option
to ask pg_dump to set role to a particular user before dumping the
database.

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> Discussing psql options made me recall an annoying problem that we've
>> run into. There's no way (unless it was added to 8.3 and I missed it,
>> but I don't think so) to tell pg_dump 'switch to this role before
>> doing anything else'. That's very frustrating when you use no-inherit
>> roles for admins. eg:

> I've looked into using PGOPTIONS to set the role, and it doesn't seem to
> be possible because when we're processing the backend command-line
> options we're not yet in a transaction state, so variable.c:assign_role
> will always come back with NULL and you get:

> vardamir:/home/sfrost> PGOPTIONS="-c role=postgres" psql -d networx -h vardamir
> psql: FATAL: invalid value for parameter "role": "postgres"

FWIW, I found by experimentation that ALTER USER ... SET ROLE does work
to cause a SET ROLE at login, though that might be a bit useless for
your purposes --- you'd more or less need a dedicated userid for
pg_dump.

regards, tom lane

Hello,

Stephen Frost wrote:
> As I discuss above, it'd be really nice have a --role or similar option
> to ask pg_dump to set role to a particular user before dumping the
> database.

I created a patch to set the role to a specified name just after the db connection.
Please review it for possible upstream inclusion.

Regards,
Laszlo Benedek

Hello,

daveg wrote:
>> I created a patch to set the role to a specified name just after the db
>> connection.
>>
> I was going to do this, but you have beat me to it. You will want to update
> the sgml documentation, and pg_dumpall as well.
>
> -dg
>
>
Ok, here is the next one.
pg_dumpall now just passes the --role option to pg_dump. What do you
think, is it enough
or it should issue the SET ROLE TO ... command in its own session too?

Laszlo Benedek

=?ISO-8859-1?Q?Benedek_L=E1szl=F3?= <laci(at)benedekl(dot)tvnetwork(dot)hu> writes:
> pg_dumpall now just passes the --role option to pg_dump. What do you
> think, is it enough
> or it should issue the SET ROLE TO ... command in its own session too?

I think it would have to, in the general case. Consider the possibility
that someone has restricted access to the system catalogs, for instance.

You have missed an important component of Stephen's original proposal,
which was the point that something similar is needed on the restore
side. This is a little bit tricky since the context at restore time
is not necessarily the same as the context at dump time. When using
an archive file it's not a problem: the behavior can be driven off a
--role switch to pg_restore, and this is independent of what pg_dump
did. In a dump to plain text, though, I'm not sure what to do. The
simplest design would have pg_dump's --role switch control both
what it does in its own connection to the source database, and what it
puts into the output script. I'm not sure that's adequate though.
Is it worth having two different switches for the two cases? If we
think it's a corner case to need different role IDs, we could just
leave it like that and tell anyone who needs different behaviors that
they have to go through an archive file and pg_restore. Stephen,
you were the one who wanted this in the first place, what's your
use-cases look like?

Some other review nitpicking:

The documentation part of the patch is well short of acceptable IMHO,
since it gives no hint of what this switch might be good for, and
indeed encourages the user to confuse it with the -U switch by injecting
a mention of it into the middle of a discussion about -U.

It is not normally considered appropriate for individual patches to
edit the release notes; and it's DEFINITELY not appropriate to put
a mention of a feature addition into the wrong section of the release
notes.

> + {"role", required_argument, NULL, 'r' + 0x80},

This is not a good choice of option code IMHO ... what if the value is
stored in a signed char on some machines? If you can't find a free
letter you like, use a small integer code, as you can find being done
elsewhere.

BTW, the patch fails to compile on a strict ANSI C compiler, because
you are using a C++-ism of declaring a variable mid-block.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> =?ISO-8859-1?Q?Benedek_L=E1szl=F3?= <laci(at)benedekl(dot)tvnetwork(dot)hu> writes:
> > pg_dumpall now just passes the --role option to pg_dump. What do you
> > think, is it enough
> > or it should issue the SET ROLE TO ... command in its own session too?
>
> I think it would have to, in the general case. Consider the possibility
> that someone has restricted access to the system catalogs, for instance.

I would agree with this. pg_dumpall should do the 'set role' in its
session as well.

> You have missed an important component of Stephen's original proposal,
> which was the point that something similar is needed on the restore
> side. This is a little bit tricky since the context at restore time
> is not necessarily the same as the context at dump time. When using
> an archive file it's not a problem: the behavior can be driven off a
> --role switch to pg_restore, and this is independent of what pg_dump
> did. In a dump to plain text, though, I'm not sure what to do. The
> simplest design would have pg_dump's --role switch control both
> what it does in its own connection to the source database, and what it
> puts into the output script. I'm not sure that's adequate though.

This makes sense to me and I feel it's adequate. If necessary, people
can post-process their .sql files using sed or something similar.
That's made reasonably easy by having a 'set role' in the .sql file. I
actively dislike the idea that pg_restore would modify the input stream
from a text file, even if it was passed a --role switch.

> Is it worth having two different switches for the two cases? If we
> think it's a corner case to need different role IDs, we could just
> leave it like that and tell anyone who needs different behaviors that
> they have to go through an archive file and pg_restore. Stephen,
> you were the one who wanted this in the first place, what's your
> use-cases look like?

My primary use cases are performing a pg_dump when logging in as one
user but needing the permissions of another role, followed by loading
the data into another system when logging in as one user and needing to
set role first to another. In at least 90% of those cases, that role is
postgres, and in the other 10% most, if not all, are the same role on
both sides. There are a few cases where we might change the restore-as
role away from the dumped-as role, but we're happy to use pg_restore to
handle that, or take care of changing the role in the .sql file (which
is what we tend to use, honestly) using sed or similar.

Alot of this is driven from the fact that we don't allow admins to
remotely connect directly as postgres (akin to disabling remote root
logins in sshd_config via PermitRootLogin, and for the same reasons).
They must authenticate and connect as their own user first and then use
'set role postgres;' to gain superuser rights. Not being able to have
pg_dump do that set role has been quite frustrating as we use it
extensively for transferring data between systems.

> Some other review nitpicking:

I agree with the other comments.

Thanks,

Stephen

Tom Lane wrote:
> Some other review nitpicking:
>
>
Thank you for your review. I really need all suggestions, since I never
posted any patch to the community before.

The next patch will emit the SET ROLE command in the generated dump,
as you and Stephen said. This will fit in my workflow too, since mostly I
need to restore using the same role as the dump.

Regards, Laszlo Benedek

Hello,

I modified my previous patch supporting the --role option in pg_dump and
pg_dumpall.
The attached patch contains the following things:

- pg_dump and pg_dumpall accepts the --role=rolename parameter, and
sends a SET ROLE
command on their connections
- pg_dumpall passes this option to the called pg_dump process
- pg_dump emits the SET ROLE command into the archive
- sgml documentation of this feature

Please review it

Regards, Laszlo Benedek

Hello All,

in my last mail
http://www.mail-archive.com/pgsql-hackers(at)postgresql(dot)org/msg114835.html
I have sent you a patch of pg_dump and pg_dumpall --role option support.
Nobody replied
or commented jet so now I send it again. The attached patch is the same
as the last one,
except it is against the current upstream HEAD.

The patch contains the following things:

- pg_dump and pg_dumpall accepts the --role=rolename parameter, and
sends a SET ROLE

command on their connections
- pg_dumpall passes this option to the called pg_dump process
- pg_dump emits the SET ROLE command into the archive
- sgml documentation of this feature

Summary:

doc/src/sgml/ref/pg_dump.sgml | 16 ++++++++++
doc/src/sgml/ref/pg_dumpall.sgml | 27 +++++++++++++----
src/bin/pg_dump/pg_backup.h | 2 +
src/bin/pg_dump/pg_backup_archiver.c | 38 +++++++++++++++++++++--
src/bin/pg_dump/pg_dump.c | 55
+++++++++++++++++++++++++++++++++-
src/bin/pg_dump/pg_dumpall.c | 26 +++++++++++++++-
6 files changed, 153 insertions(+), 11 deletions(-)

Thank you, best regards:
Laszlo Benedek

Benedek László wrote:

Hi,

> The patch contains the following things:
>
>
> - pg_dump and pg_dumpall accepts the --role=rolename parameter, and
> sends a SET ROLE command on their connections

Minor comment -- I think you need to quote the role name in the SET
command. Otherwise roles with funny names will fail (try a role with a
space for example)

> - sgml documentation of this feature

The SGML patch seems to contain unnecessary whitespace changes; please
clean that up.

> + /* te->defn should have the form SET role = 'foo'; */
> + char *defn = strdup(te->defn);
> + char *ptr1;
> + char *ptr2 = NULL;
> +
> + ptr1 = strchr(defn, '\'');
> + if (ptr1)
> + ptr2 = strchr(++ptr1, '\'');

Does this work if the role name contains a ' ?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Hi,

Thank you for your review.

On 2008-11-07 21:20, Alvaro Herrera wrote:
>> The patch contains the following things:
>>
>>
>> - pg_dump and pg_dumpall accepts the --role=rolename parameter, and
>> sends a SET ROLE command on their connections
>>
>
> Minor comment -- I think you need to quote the role name in the SET
> command. Otherwise roles with funny names will fail (try a role with a
> space for example)
>
>
Of course you need to quote the role names with special characters in it.
I tested it this way (from bash):
$ src/bin/pg_dump/pg_dump -h localhost -p 4003 --role "asd ' \" qwe" test
Note the bash style escaping of the string [asd ' " qwe].
It created a dump file with SET role = "asd ' "" qwe"; line in it. Seems
fine for me.
> The SGML patch seems to contain unnecessary whitespace changes; please
> clean that up.
>
Maybe you missed an updated version of the patch? Available here:
http://archives.postgresql.org/pgsql-hackers/2008-11/msg00391.php

>> + /* te->defn should have the form SET role = 'foo'; */
>> + char *defn = strdup(te->defn);
>> + char *ptr1;
>> + char *ptr2 = NULL;
>> +
>> + ptr1 = strchr(defn, '\'');
>> + if (ptr1)
>> + ptr2 = strchr(++ptr1, '\'');
>>
>
> Does this work if the role name contains a ' ?
>
Right, this one fails with ' in the role name. An update coming soon closing this issue.

Regards,

Benedek Laszlo

On 2008-11-08 09:25, Benedek László wrote:
>> Does this work if the role name contains a ' ?
>
> Right, this one fails with ' in the role name. An update coming soon
> closing this issue.
>
Here is an updated patch, which deals with 's in the rolename.
Please review.

doc/src/sgml/ref/pg_dump.sgml | 16 +++++++++
doc/src/sgml/ref/pg_dumpall.sgml | 15 ++++++++
src/bin/pg_dump/pg_backup.h | 2 +
src/bin/pg_dump/pg_backup_archiver.c | 35 ++++++++++++++++++-
src/bin/pg_dump/pg_dump.c | 60
+++++++++++++++++++++++++++++++++-
src/bin/pg_dump/pg_dumpall.c | 23 +++++++++++++
6 files changed, 148 insertions(+), 3 deletions(-)

Thank you, regards

Benedek Laszlo

=?ISO-8859-1?Q?Benedek_L=E1szl=F3?= <laci(at)benedekl(dot)tvnetwork(dot)hu> writes:
> Here is an updated patch, which deals with 's in the rolename.

Committed with revisions as per subsequent discussion: pg_restore has
its own switch and there's no change in archive contents.

regards, tom lane