Обсуждение: OWNER TO on all objects

Hi,

This is a preview patch - DON'T COMMIT IT TO HEAD!

What I've done in this patch is add the following:

ALTER AGGREGATE / OWNER TO
ALTER CONVERSION / OWNER TO
ALTER FUNCTION / OWNER TO
ALTER OPERATOR / OWNER TO
ALTER OPERATOR CLASS / OWNER TO
ALTER SCHEMA / OWNER TO
ALTER TYPE / OWNER TO

That means we can change the owner of all objects.

Next, I modified pg_dump to remove all SET SESSION AUTHORIZATION
commands for object creation.  (I left them in on the COPY commands).

Then I made it so that pg_dump will output an OWNER TO statement after
every object creation.

This means that pg_dump can dump a restorable dump in cases where, say,
a super user created a language, and then had their superuser privs
dropped, or when a user has created a table, but has then had their
create privileges removed.

At the moment, i'm happy with how it dumps and reloads the regression
database, and i'm working on adding tests for all OWNER TO in the
regression suite.  Full doc updates are already included.

Please review and give me feedback!  The patch is large, but not at all
complex :)

Some questions:

* Do we need the set session auth for COPY commands still?

* Are there any subtle implications of changing owners that I haven't
realised?  I know that it will affect SECURITY DEFINER for functions,
but I put that in the docs.

* Is doing this ok: ObjectIdGetDatum(typTup->typrelid)

* Is there any reason there is no RENAME TO command for operators?

Chris

Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
> Then I made it so that pg_dump will output an OWNER TO statement after 
> every object creation.

Perhaps better to put these out towards the end of the dump, not right
after the creation of the object?  Or is that what you're doing?

I would envision the safest procedure as creating all objects, loading
all data, etc, then all ALTER OWNERs, then all GRANT/REVOKEs.

> * Do we need the set session auth for COPY commands still?

Not if you still own the table while loading into it (see above point).

However, this all assumes a complete dump/restore.  Consider data-only
restores.  Consider partial restores using pg_restore's options for
that.  What happens then?  It'd likely be appropriate to issue set
session auth during scenarios involving pre-existing objects.

> * Is there any reason there is no RENAME TO command for operators?

Lack of round tuits, no doubt.
        regards, tom lane

> Perhaps better to put these out towards the end of the dump, not right
> after the creation of the object?  Or is that what you're doing?

I just inserted the ALTER OWNER statement between the CREATE and the 
GRANTs.  Why do you want them at the end of the dump?

> I would envision the safest procedure as creating all objects, loading
> all data, etc, then all ALTER OWNERs, then all GRANT/REVOKEs.

I don't yet understand your reasoning for wanting this all at the end...

> Not if you still own the table while loading into it (see above point).

Can we not load as superuser?

> However, this all assumes a complete dump/restore.  Consider data-only
> restores.  Consider partial restores using pg_restore's options for
> that.  What happens then?  It'd likely be appropriate to issue set
> session auth during scenarios involving pre-existing objects.

OK, i will test all those situations...  What scenarios did you have in 
mind?

Chris

Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
> I just inserted the ALTER OWNER statement between the CREATE and the 
> GRANTs.  Why do you want them at the end of the dump?

So that the initial owner is still owner when he does COPY, ALTER TABLE
ADD PRIMARY KEY, etc etc.  Else you're gonna have problems.

The regression database is next to useless as a testbed for this,
btw, since all the objects in it are owned by the superuser anyway.
        regards, tom lane

> So that the initial owner is still owner when he does COPY, ALTER TABLE
> ADD PRIMARY KEY, etc etc.  Else you're gonna have problems.

I was thinking of doing all COPY and ALTER as superuser as well...

Or are you trying to make it work when run as non-super?  Which is won't 
since ALTER OWNER will require superuser.

ie. the entire script always runs as a single user, most usefully a 
superuser.

Chris

Christopher Kings-Lynne wrote:
> * Is there any reason there is no RENAME TO command for operators?

That might change the precedence of the operator and get you in a big 
mess with stored expressions everywhere.

Christopher Kings-Lynne wrote:
> Then I made it so that pg_dump will output an OWNER TO statement
> after every object creation.

I'd prefer it if OWNER TO were only added if it is actually necessary 
(or there be some option to turn it off).  I don't want to edit the 
entire dump file if I want to restore the database into another SQL 
database.

> That might change the precedence of the operator and get you in a big 
> mess with stored expressions everywhere.

What if you could only do it on non-system operators?

Chris

> I'd prefer it if OWNER TO were only added if it is actually necessary 
> (or there be some option to turn it off).  I don't want to edit the 
> entire dump file if I want to restore the database into another SQL 
> database.

There is the existing --no-owner option, which this patch respects, same 
as old pg_dump.

It's not possible to dump it if necessary, as it's not possible at 
restore time to know the user they are restoring as I guess.

The old pg_dump would output a session auth, and then wouldn't bother 
changing it until necessary.  However, with ALTER OWNER, I have to dump 
it for every object, even if it's the same user.

I guess I could optimise so that if the owner matches the superuser 
specified with the -S option, I wouldn't bother dumping the ALTER?

Chris

Peter Eisentraut <peter_e@gmx.net> writes:
> Christopher Kings-Lynne wrote:
>> * Is there any reason there is no RENAME TO command for operators?

> That might change the precedence of the operator

... true ...

> and get you in a big mess with stored expressions everywhere.

Not with respect to compiled expressions.  It could conceivably break
stored function source texts and application-generated queries, but
those are broken a fortiori by the new operator name.

So I don't think this objection has a lot of weight.
        regards, tom lane

Christopher Kings-Lynne wrote:
> The old pg_dump would output a session auth, and then wouldn't bother
> changing it until necessary.  However, with ALTER OWNER, I have to
> dump it for every object, even if it's the same user.

Well, the advantage of SET SESSION AUTHORIZATION is that it is SQL 
compliant, whereas ALTER OWNER is not.  So I'm in favor of changing 
nothing.  The examples you listed in you original mail, where the 
privilege to do something was later dropped so the original state is 
not reproducible, are to me examples that the privilege system is 
flawed.  You could use ALTER OWNER in those cases only, because those 
states are not SQL compliant anyway.

Tom Lane wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
> > Christopher Kings-Lynne wrote:
> >> * Is there any reason there is no RENAME TO command for operators?
> >
> > That might change the precedence of the operator
>
> ... true ...
>
> > and get you in a big mess with stored expressions everywhere.
>
> Not with respect to compiled expressions.  It could conceivably break
> stored function source texts and application-generated queries, but
> those are broken a fortiori by the new operator name.
>
> So I don't think this objection has a lot of weight.

IIRC, it was the objection that you put forth when I last attempted to 
do it...  The question is perhaps not so much whether we can get away 
with it, it's whether the behavior is reasonable and consistent for 
users that don't know implementation details.

Peter Eisentraut <peter_e@gmx.net> writes:
> Tom Lane wrote:
>> Peter Eisentraut <peter_e@gmx.net> writes:
>>> That might change the precedence of the operator
>> 
>> So I don't think this objection has a lot of weight.

> IIRC, it was the objection that you put forth when I last attempted to 
> do it...

Was it?  I vaguely remember objecting to something on the basis of
possible precedence changes, but I don't recall it being RENAME
OPERATOR.

> The question is perhaps not so much whether we can get away 
> with it, it's whether the behavior is reasonable and consistent for 
> users that don't know implementation details.

Agreed.  Probably the main point here is the inconsistency in behavior
for stored rules and constraint/default expressions (which would track
the operator rename and would stay parenthesized correctly), versus
stored function source code (which would not).  I don't think it would
surprise anyone that the query texts embedded in their app code don't
magically update ;-) but for the stuff that's "all stored in the
database" they might expect consistency.

On the other hand, the same inconsistency already exists for table and
column names, and I've not heard great squawks about it.  So I withdraw
any previous objection I might have made to RENAME OPERATOR.  It's not
obvious that it's more dangerous than any other rename.
        regards, tom lane

> Well, the advantage of SET SESSION AUTHORIZATION is that it is SQL 
> compliant, whereas ALTER OWNER is not.  So I'm in favor of changing 
> nothing.

That, however is a highly theoretical, and quite non-practical 
"solution".  It leaves many of the world's postgresql database 
non-upgradable and "fixing" postgres so that revoking someone's create 
privilege dropped all their tables is _madness_.  You can't but agree 
that the SQL spec is totally broken in that respect.  They've broken the 
underlying orthogonality of their permissions system.

I think Tom even may have mentioned that the SQL rules about that sort 
of thing only seem to apply to domains or something anyway...

I mean, if I (as a PostgreSQL developer) cannot upgrade my _own_ 
database then how does anyone else have a chance?

Chris

Peter Eisentraut <peter_e@gmx.net> writes:
> Well, the advantage of SET SESSION AUTHORIZATION is that it is SQL 
> compliant, whereas ALTER OWNER is not.  So I'm in favor of changing 
> nothing.

That's a fair point, but you have to admit that it's a bit abstract
while Chris has a real problem he needs to solve.  Our dumps are awfully
low on the SQL-compliance scale anyway :-(

> The examples you listed in you original mail, where the 
> privilege to do something was later dropped so the original state is 
> not reproducible, are to me examples that the privilege system is 
> flawed.

Sure, but we're not fixing the privilege system this time round (unless
you have work in progress you haven't mentioned ;-)).  In any case this
answer is no help for dumping existing databases with "illegal"
configurations, and newer pg_dumps will still be expected to cope with
those.

> You could use ALTER OWNER in those cases only, because those 
> states are not SQL compliant anyway.

Is it really possible for pg_dump to detect such cases and decide which
method to use?  I'd be in favor of this if it were practical to do,
but it sounds suspiciously close to AI-complete ... especially when
considering scenarios involving pg_restore into an existing database ...

This brings up a question for Chris, which is whether he's implemented
this in a way that forces the decision at pg_dump time, or whether
it is made during pg_restore.  I would definitely agree that we need
to postpone the choice of which to do till pg_restore.  In other words,
a dump archive should only show object ownerships and not prejudge
how those ownerships will get set during the restore session.
        regards, tom lane

> That's a fair point, but you have to admit that it's a bit abstract
> while Chris has a real problem he needs to solve.  Our dumps are awfully
> low on the SQL-compliance scale anyway :-(

We could keep around an option for dumping the auth statements instead 
of alter statements perhaps.

> Sure, but we're not fixing the privilege system this time round (unless
> you have work in progress you haven't mentioned ;-)). 

I don't think any fix to privs system which creates a situation where 
you cannot drop CREATE privilege from someone who owns a table is jsut 
silly.

> This brings up a question for Chris, which is whether he's implemented
> this in a way that forces the decision at pg_dump time, or whether
> it is made during pg_restore.  I would definitely agree that we need
> to postpone the choice of which to do till pg_restore.  In other words,
> a dump archive should only show object ownerships and not prejudge
> how those ownerships will get set during the restore session.

I've implented it exactly like comments are implemented.  I just created 
a dumpOwner() function that adds an archive entry to the current object.   It appears in the pg_dump.c basically
wherevera dumpComment() 
 
appears, but always before the dumpAcls() if there is one.

What we need to do is decide on the exact semantics of how this is going 
to work, and then I can make all the (hopefully small) changes required 
to make it work.

OK, are these the requirements?  Please comment.

* I fix ALTER OWNER to allow it to work if you are NOT a superuser, but 
ARE the existing owner.  This makes it work just like set session auth 
and means that if your dump includes only stuff you own, it will still work.

* Text mode dumps
 - I issue alter owner after every object creation, suppressing ALL 
session auths, including COPY
 - I keep a switch to disable alter owner and dump set session instead.  Is this really necessary?
 - The -S option only affects enabling and disabling triggers and i 
don't have to worry about it
 - The only difference is data-only dumps - we still need set session 
auth?  Actually, no - read the next point.
 - How does the above point affect full dumps that include schema and 
data?  In my proposal, the copy commands will run as the user running 
the script, not the table owner anymore.  Presumably, the user running 
the script is a superuser.  Given that it is possible for a table owner 
to revoke their own INSERT privilege on their table, the existing 
behaviour is broken anyway.
 - The --no-owner option means no alter owner or session auth 
statements are dumped.
 - pg_dump currently in the case when the owner of a table no longer 
exists, dumps SET SESSION AUTHORIZATION DEFAULT.  I will simply omit the 
ALTER OWNER command.

* Custom format dumps
 - OK, I admit I have little experience with this format.
 - The alter owner objects will be stored as toc entries just like 
comment on objects.
 - They should pop back out of the archive when creating a text dump 
from a binary one, identical to the text format.
 - With respect to Tom's question about restore-time option - how is it 
different to now?? A that moment, we have the pg_restore -O option to 
not restore the session auth commands - what needs to change?  I just 
won't output the ALTER OWNER commands so everything will be owned by 
whoever runs pg_restore.

Does that seem like the way to go?

Chris

>  - How does the above point affect full dumps that include schema and 
> data?  In my proposal, the copy commands will run as the user running 
> the script, not the table owner anymore.  Presumably, the user running 
> the script is a superuser.  Given that it is possible for a table owner 
> to revoke their own INSERT privilege on their table, the existing 
> behaviour is broken anyway.

Yes, confirmed.  Current pg_dump is quite broken in the situation where 
a table owner revokes their own INSERT privilege.  It attempts to run 
COPY as the owner of the table.  Only a superuser is guaranteed to be 
able to always insert.  We should fix that.

Chris

There is one other consideration, and that is that current pg_dump likes 
to set session_auth to user of object before outputting drop command, 
when '-c' is specificed.

I propose that we eliminate that set session_auth as well.  If the user 
running the script is the owner of that object or a superuser, then they 
will be able to drop it.  If they are neither, then the drop will fail, 
but the session_authorization would have failed anyway!

Chris

Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
>> This brings up a question for Chris, which is whether he's implemented
>> this in a way that forces the decision at pg_dump time, or whether
>> it is made during pg_restore.

> I've implented it exactly like comments are implemented.  I just created 
> a dumpOwner() function that adds an archive entry to the current object. 
>    It appears in the pg_dump.c basically wherever a dumpComment() 
> appears, but always before the dumpAcls() if there is one.

I think this is wrong, primarily because it's gonna be seriously
incompatible with existing dump files.  The existing technique is
that each TOC entry says who owns the object.  You should use that
information and not have to rely on new additions to the file format.

> * I fix ALTER OWNER to allow it to work if you are NOT a superuser, but 
> ARE the existing owner.

No, you don't.  That allows non-superusers to give away object
ownership, which is well-established as a security hole; Unix
filesystems stopped doing it years ago.

>   - How does the above point affect full dumps that include schema and 
> data?  In my proposal, the copy commands will run as the user running 
> the script, not the table owner anymore.  Presumably, the user running 
> the script is a superuser.  Given that it is possible for a table owner 
> to revoke their own INSERT privilege on their table, the existing 
> behaviour is broken anyway.

This is why GRANT/REVOKE has to be postponed to the end.  I think it
would be a lot simpler and more reliable if you also postponed ALTER
OWNER.

> * Custom format dumps
>   - OK, I admit I have little experience with this format.

Then you have fundamentally failed to grok pg_dump, and you should
rethink everything you've done to date.  The way things work is that
EVERYTHING effectively goes through a custom dump.  pg_dump in text
mode is really pg_dump followed by pg_restore with the intermediate
TOC just kept in memory temporarily.  Therefore, any time you have done
something that you don't know how to convert into pg_restore behavior,
it's because you were hacking the wrong place.  Everything you need to
know about an object *must* go through the TOC representation and then
be converted to text at the restore side.

>   - With respect to Tom's question about restore-time option - how is it 
> different to now?? A that moment, we have the pg_restore -O option to 
> not restore the session auth commands - what needs to change?  I just 
> won't output the ALTER OWNER commands so everything will be owned by 
> whoever runs pg_restore.

I think there needs to be a restore-side switch that chooses whether
to emit ALTER OWNER or SET SESSION AUTH commands.  This is probably
just for pro-forma SQL compliance, unless Peter has some brilliant
insight about how to avoid ALTER OWNER.
        regards, tom lane

Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
> There is one other consideration, and that is that current pg_dump likes 
> to set session_auth to user of object before outputting drop command, 
> when '-c' is specificed.

> I propose that we eliminate that set session_auth as well.  If the user 
> running the script is the owner of that object or a superuser, then they 
> will be able to drop it.  If they are neither, then the drop will fail, 
> but the session_authorization would have failed anyway!

Seems reasonable.  The -c option is pretty badly thought out anyway :-(
        regards, tom lane

> I think this is wrong, primarily because it's gonna be seriously
> incompatible with existing dump files.  The existing technique is
> that each TOC entry says who owns the object.  You should use that
> information and not have to rely on new additions to the file format.

Hrm.  OK, i might be able to do that, but constructing the ALTER OWNER 
commands will be difficult I think.  Each TOC entry seems to have the 
'OPERATOR (int4, none)' or 'TABLE blah' string in it from memory, so I 
assume I can pull that out.  I had failed to consider restoring from 
existing dump files actually.

>>* I fix ALTER OWNER to allow it to work if you are NOT a superuser, but 
>>ARE the existing owner.
> 
> 
> No, you don't.  That allows non-superusers to give away object
> ownership, which is well-established as a security hole; Unix
> filesystems stopped doing it years ago.

I worded that badly.  I meant "allow a user to change the owner of 
something to what it already is".  ie. Just make the no-op allowed by 
everyone.  session_auth already does this.  This means I can make text 
mode dumps that have ALTER OWNER in them and then even if you are not a 
superuser, so long as you own everything, you're ok.

>>  - How does the above point affect full dumps that include schema and 
>>data?  In my proposal, the copy commands will run as the user running 
>>the script, not the table owner anymore.  Presumably, the user running 
>>the script is a superuser.  Given that it is possible for a table owner 
>>to revoke their own INSERT privilege on their table, the existing 
>>behaviour is broken anyway.
> 
> This is why GRANT/REVOKE has to be postponed to the end.  I think it
> would be a lot simpler and more reliable if you also postponed ALTER
> OWNER.

Or we just always run the COPYs as the person executing the script, ie. 
remove session_auth from the COPY commands.

> Then you have fundamentally failed to grok pg_dump, and you should
> rethink everything you've done to date.  The way things work is that
> EVERYTHING effectively goes through a custom dump.  pg_dump in text
> mode is really pg_dump followed by pg_restore with the intermediate
> TOC just kept in memory temporarily.  Therefore, any time you have done
> something that you don't know how to convert into pg_restore behavior,
> it's because you were hacking the wrong place.  Everything you need to
> know about an object *must* go through the TOC representation and then
> be converted to text at the restore side.

I'm well aware of how it works - but compared to text format, i don't 
have as much experience.  I have done a fair bit of pg_dump hacking in 
my time...  All my changes work perfectly with pg_restore and the binary 
dump format.  I can pg_dump my production db using custom and plain text 
and there is no diff between the plain text and the plain text extracted 
from the binary dump.  I can also reload that dump and dump it again, 
and keep cycling it with no diff - without moving the grants/owners to 
the end.

I will have to spend some time investigating how to collect up the 
grants and stuff and move them to the end, if you still feel it is 
necessary.

>>  - With respect to Tom's question about restore-time option - how is it 
>>different to now?? A that moment, we have the pg_restore -O option to 
>>not restore the session auth commands - what needs to change?  I just 
>>won't output the ALTER OWNER commands so everything will be owned by 
>>whoever runs pg_restore.
> 
> 
> I think there needs to be a restore-side switch that chooses whether
> to emit ALTER OWNER or SET SESSION AUTH commands.  This is probably
> just for pro-forma SQL compliance, unless Peter has some brilliant
> insight about how to avoid ALTER OWNER.

Yes, well if I change it how you suggested in your first paragraph 
(which has to happen for backwards compatibility), then this wouldn't 
seem too hard.

Chris

Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes:
>> No, you don't.  That allows non-superusers to give away object
>> ownership, which is well-established as a security hole; Unix
>> filesystems stopped doing it years ago.

> I worded that badly.  I meant "allow a user to change the owner of 
> something to what it already is".  ie. Just make the no-op allowed by 
> everyone.  session_auth already does this.

Ah.  Okay, no objection to that.  (In fact I believe we put in the
special case for session_auth for exactly the same reason.)
        regards, tom lane

>>I worded that badly.  I meant "allow a user to change the owner of 
>>something to what it already is".  ie. Just make the no-op allowed by 
>>everyone.  session_auth already does this.
> 
> 
> Ah.  Okay, no objection to that.  (In fact I believe we put in the
> special case for session_auth for exactly the same reason.)

Actually, do I make it that anyone can do a no-op user change, or can 
only the user who is the existing owner do the no-op?  It's a very tiny 
different and probably won't make much difference but perhaps it's 
better to make it a bit tighter check?  What do you think?

Chris

>>  - How does the above point affect full dumps that include schema and 
>>data?  In my proposal, the copy commands will run as the user running 
>>the script, not the table owner anymore.  Presumably, the user running 
>>the script is a superuser.  Given that it is possible for a table owner 
>>to revoke their own INSERT privilege on their table, the existing 
>>behaviour is broken anyway.
> 
> This is why GRANT/REVOKE has to be postponed to the end.  I think it
> would be a lot simpler and more reliable if you also postponed ALTER
> OWNER.

Ok, I have thought about this a bit and I have decided that you are 
right in that grant revoke MUST be deferred to the end of the file.

The reason is that there are two conditions under which we expect a dump 
restore to work.  First is if you are restoring as a superuser second is 
if you are restoring as a user who owns ALL the objects in the file.

Restoring as superuser will work so long as we use alter owner instead 
of changing the current user.

The second is more tricky due to the fact that an object owner can 
revoke their own privileges from their objects.  If a user does this, it 
cannot be guaranteed that they will be able to restore unless all grants 
are done last and all objects are initially created as that user. 
Consider revoking your own REFERENCES privilege, as an example.

Hence, I will have to make it that all existing dumping is done, then 
all the ownership changes will be done, then all the grants will be done.

Does that seem reasonable?

Chris

> I think this is wrong, primarily because it's gonna be seriously
> incompatible with existing dump files.  The existing technique is
> that each TOC entry says who owns the object.  You should use that
> information and not have to rely on new additions to the file format.

> This is why GRANT/REVOKE has to be postponed to the end.  I think it
> would be a lot simpler and more reliable if you also postponed ALTER
> OWNER.

OK, implementing this is nasty.  How do I collect up all the ACLs from 
EXISTING custom archives and move them to the end??  This is hard 
because ACLs are just dependents on their parent object and cannot be 
sorted on their own to the end of the dump.

Since the dumping process outputs to stdout as it goes along, I'd have 
to create some big in-memory string of all acls and owners collected so 
far.  That seems bad.

The alternative is to scan the entire archive twice.  On the second scan 
I would only output owner and acl commands.

Another option is to simply not bother fixing old custom dumps.  They 
could just still restore exactly how they would have without any changes 
from me.  I would add new TOC types to the 7.5 pg_dump that could be 
sorted to the end...

What do I do?

Chris

Any change someone who knows (or who can declare that we not fix 
existing dumps) comment on this?

Chris

Christopher Kings-Lynne wrote:

>> I think this is wrong, primarily because it's gonna be seriously
>> incompatible with existing dump files.  The existing technique is
>> that each TOC entry says who owns the object.  You should use that
>> information and not have to rely on new additions to the file format.
> 
> 
>> This is why GRANT/REVOKE has to be postponed to the end.  I think it
>> would be a lot simpler and more reliable if you also postponed ALTER
>> OWNER.
> 
> 
> OK, implementing this is nasty.  How do I collect up all the ACLs from 
> EXISTING custom archives and move them to the end??  This is hard 
> because ACLs are just dependents on their parent object and cannot be 
> sorted on their own to the end of the dump.
> 
> Since the dumping process outputs to stdout as it goes along, I'd have 
> to create some big in-memory string of all acls and owners collected so 
> far.  That seems bad.
> 
> The alternative is to scan the entire archive twice.  On the second scan 
> I would only output owner and acl commands.
> 
> Another option is to simply not bother fixing old custom dumps.  They 
> could just still restore exactly how they would have without any changes 
> from me.  I would add new TOC types to the 7.5 pg_dump that could be 
> sorted to the end...
> 
> What do I do?
> 
> Chris
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
> 
>               http://www.postgresql.org/docs/faqs/FAQ.html