Streaming replication as a separate permissions

Here's a patch that changes walsender to require a special privilege
for replication instead of relying on superuser permissions. We
discussed this back before 9.0 was finalized, but IIRC we ran out of
time. The motivation being that you really want to use superuser as
little as possible - and since being a replication slave is a read
only role, it shouldn't require the maximum permission available in
the system.

Obviously the patch needs docs and some system views updates, which I
will add later. But I wanted to post what I have so far for a quick
review to confirm whether I'm on the right track or not... How it
works should be rather obvious - adds a "WITH
REPLICATION/NOREPLICATION" to the create and alter role commands, and
then check this when a connection attempts to start the walsender.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Here's a patch that changes walsender to require a special privilege
> for replication instead of relying on superuser permissions. We
> discussed this back before 9.0 was finalized, but IIRC we ran out of
> time. The motivation being that you really want to use superuser as
> little as possible - and since being a replication slave is a read
> only role, it shouldn't require the maximum permission available in
> the system.

Maybe it needn't require "max" permissions, but one of the motivations
for requiring superusernesss was to prevent Joe User from sucking every
last byte of data out of your database (and into someplace he could
examine it at leisure). This patch opens that barn door wide, because
so far as I can see, it allows anybody at all to grant the replication
privilege ... or revoke it, thereby breaking your replication setup.
I think only superusers should be allowed to change the flag.

regards, tom lane

On Thu, Dec 23, 2010 at 10:15 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Here's a patch that changes walsender to require a special privilege
>> for replication instead of relying on superuser permissions. We
>> discussed this back before 9.0 was finalized, but IIRC we ran out of
>> time. The motivation being that you really want to use superuser as
>> little as possible - and since being a replication slave is a read
>> only role, it shouldn't require the maximum permission available in
>> the system.
>
> Maybe it needn't require "max" permissions, but one of the motivations
> for requiring superusernesss was to prevent Joe User from sucking every
> last byte of data out of your database (and into someplace he could
> examine it at leisure).  This patch opens that barn door wide, because
> so far as I can see, it allows anybody at all to grant the replication
> privilege ... or revoke it, thereby breaking your replication setup.
> I think only superusers should be allowed to change the flag.

I haven't looked at the patch yet, but I think we should continue to
allow superuser-ness to be *sufficient* for replication - i.e.
superusers will automatically have the replication privilege just as
they do any other - and merely allow this as an option for when you
want to avoid doing it that way.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I haven't looked at the patch yet, but I think we should continue to
> allow superuser-ness to be *sufficient* for replication - i.e.
> superusers will automatically have the replication privilege just as
> they do any other - and merely allow this as an option for when you
> want to avoid doing it that way.

I don't particularly mind breaking that. If we leave it as-is, we'll
be encouraging people to use superuser accounts for things that don't
need that, which can't be good from a security standpoint.

BTW, is it possible to set things up so that a REPLICATION account
can be NOLOGIN, thereby making it really hard to abuse for other
purposes? Or does the login privilege check come too soon?

regards, tom lane

On Thu, Dec 23, 2010 at 10:54 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I haven't looked at the patch yet, but I think we should continue to
>> allow superuser-ness to be *sufficient* for replication - i.e.
>> superusers will automatically have the replication privilege just as
>> they do any other - and merely allow this as an option for when you
>> want to avoid doing it that way.
>
> I don't particularly mind breaking that.  If we leave it as-is, we'll
> be encouraging people to use superuser accounts for things that don't
> need that, which can't be good from a security standpoint.

And if we break it, we'll be adding an additional, mandatory step to
make replication work that isn't required today. You might think
that's OK, but I think the majority opinion is that it's already
excessively complex.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Dec 23, 2010 at 16:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Here's a patch that changes walsender to require a special privilege
>> for replication instead of relying on superuser permissions. We
>> discussed this back before 9.0 was finalized, but IIRC we ran out of
>> time. The motivation being that you really want to use superuser as
>> little as possible - and since being a replication slave is a read
>> only role, it shouldn't require the maximum permission available in
>> the system.
>
> Maybe it needn't require "max" permissions, but one of the motivations
> for requiring superusernesss was to prevent Joe User from sucking every
> last byte of data out of your database (and into someplace he could
> examine it at leisure).  This patch opens that barn door wide, because
> so far as I can see, it allows anybody at all to grant the replication
> privilege ... or revoke it, thereby breaking your replication setup.
> I think only superusers should be allowed to change the flag.

That was certainly not intentional - and doesn't work that way for me
at least, unless I broke it right before I submitted it.

oh hang on.. Yeah, it's allowing anybody *that has CREATE ROLE*
privilege to do it, I think. And I agree that's wrong and should be
fixed. But I can't see it allowing anybody at all to do it - am I
misreading the code?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Thu, Dec 23, 2010 at 16:57, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Dec 23, 2010 at 10:54 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> I haven't looked at the patch yet, but I think we should continue to
>>> allow superuser-ness to be *sufficient* for replication - i.e.
>>> superusers will automatically have the replication privilege just as
>>> they do any other - and merely allow this as an option for when you
>>> want to avoid doing it that way.
>>
>> I don't particularly mind breaking that.  If we leave it as-is, we'll
>> be encouraging people to use superuser accounts for things that don't
>> need that, which can't be good from a security standpoint.
>
> And if we break it, we'll be adding an additional, mandatory step to
> make replication work that isn't required today.  You might think
> that's OK, but I think the majority opinion is that it's already
> excessively complex.

Most of the people I run across in the real world are rather surprised
how *easy* it is to set up, and not how complex. And tbh, the only
complexity complaints I've heard there are about the requirement to
start/backup/stop to get it up and running. I've always told everybody
to create a separate account to do it, and not heard a single comment
about that.

That said, how about a compromise in that we add the replication flag
by default to the initial superuser when it's created? That way, it's
at least possible to remove it if you want to. Would that address your
complexity concern?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Thu, Dec 23, 2010 at 16:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think only superusers should be allowed to change the flag.

> That was certainly not intentional - and doesn't work that way for me
> at least, unless I broke it right before I submitted it.

> oh hang on.. Yeah, it's allowing anybody *that has CREATE ROLE*
> privilege to do it, I think. And I agree that's wrong and should be
> fixed. But I can't see it allowing anybody at all to do it - am I
> misreading the code?

Ah, sorry, yeah there are probably CREATE ROLE privilege checks
somewhere upstream of here. I was expecting to see a privilege check
added by the patch itself, and did not, so I complained.

regards, tom lane

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Thu, Dec 23, 2010 at 16:57, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Dec 23, 2010 at 10:54 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I don't particularly mind breaking that. If we leave it as-is, we'll
>>> be encouraging people to use superuser accounts for things that don't
>>> need that, which can't be good from a security standpoint.

>> And if we break it, we'll be adding an additional, mandatory step to
>> make replication work that isn't required today. You might think
>> that's OK, but I think the majority opinion is that it's already
>> excessively complex.

> Most of the people I run across in the real world are rather surprised
> how *easy* it is to set up, and not how complex. And tbh, the only
> complexity complaints I've heard there are about the requirement to
> start/backup/stop to get it up and running. I've always told everybody
> to create a separate account to do it, and not heard a single comment
> about that.

FWIW, it seems unreasonable to me to expect that we will not be breaking
any part of a 9.0 replication configuration over the next release or
two. We *knew* we were shipping a rough version that would require
refinements, and this is one of the planned refinements.

> That said, how about a compromise in that we add the replication flag
> by default to the initial superuser when it's created? That way, it's
> at least possible to remove it if you want to. Would that address your
> complexity concern?

It does nothing to address my security concern. I want to discourage
people from using superuser accounts for this, full stop.

regards, tom lane

On 12/23/2010 08:59 PM, Magnus Hagander wrote:
> On Thu, Dec 23, 2010 at 16:57, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Dec 23, 2010 at 10:54 AM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas<robertmhaas(at)gmail(dot)com> writes:
>>>> I haven't looked at the patch yet, but I think we should continue to
>>>> allow superuser-ness to be *sufficient* for replication - i.e.
>>>> superusers will automatically have the replication privilege just as
>>>> they do any other - and merely allow this as an option for when you
>>>> want to avoid doing it that way.
>>>
>>> I don't particularly mind breaking that. If we leave it as-is, we'll
>>> be encouraging people to use superuser accounts for things that don't
>>> need that, which can't be good from a security standpoint.
>>
>> And if we break it, we'll be adding an additional, mandatory step to
>> make replication work that isn't required today. You might think
>> that's OK, but I think the majority opinion is that it's already
>> excessively complex.
>
> Most of the people I run across in the real world are rather surprised
> how *easy* it is to set up, and not how complex. And tbh, the only
> complexity complaints I've heard there are about the requirement to
> start/backup/stop to get it up and running. I've always told everybody
> to create a separate account to do it, and not heard a single comment
> about that.

I agree - people I talked to are fairly surprised on us not using a
dedicated replication role but are surprised at the complexity of
actually initializing the replication (mostly the "we cannot do a base
backup over the replication connection" missfeature)

Stefan

On 12/23/10 7:49 AM, Robert Haas wrote:
> I haven't looked at the patch yet, but I think we should continue to
> allow superuser-ness to be *sufficient* for replication - i.e.
> superusers will automatically have the replication privilege just as
> they do any other - and merely allow this as an option for when you
> want to avoid doing it that way.

Yes. Currently I already create a separate "replicator" superuser just
so that I can simply track which connections belong to replication. It
would be great if it could make the "replicator" user less than a superuser.

If we still make it possible for "postgres" to replicate, then we don't
add any complexity to the simplest setup.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> If we still make it possible for "postgres" to replicate, then we don't
> add any complexity to the simplest setup.

Well, that's one laudable goal here, but "secure by default" is another
one that ought to be taken into consideration.

regards, tom lane

On 12/23/10 2:21 PM, Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> If we still make it possible for "postgres" to replicate, then we don't
>> add any complexity to the simplest setup.
>
> Well, that's one laudable goal here, but "secure by default" is another
> one that ought to be taken into consideration.

I don't see how *not* granting the superuser replication permissions
makes things more secure. The superuser can grant replication
permissions to itself, so why is suspending them by default beneficial?
I'm not following your logic here.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> On 12/23/10 2:21 PM, Tom Lane wrote:
>> Well, that's one laudable goal here, but "secure by default" is another
>> one that ought to be taken into consideration.

> I don't see how *not* granting the superuser replication permissions
> makes things more secure. The superuser can grant replication
> permissions to itself, so why is suspending them by default beneficial?
> I'm not following your logic here.

Well, the reverse of that is just as true: if we ship it without
replication permissions on the postgres user, people can change that if
they'd rather not create a separate role for replication. But I think
we should encourage people to NOT do it that way. Setting it up that
way by default hardly encourages use of a more secure arrangement.

regards, tom lane

* Josh Berkus (josh(at)agliodbs(dot)com) wrote:
> On 12/23/10 2:21 PM, Tom Lane wrote:
> > Well, that's one laudable goal here, but "secure by default" is another
> > one that ought to be taken into consideration.
>
> I don't see how *not* granting the superuser replication permissions
> makes things more secure. The superuser can grant replication
> permissions to itself, so why is suspending them by default beneficial?
> I'm not following your logic here.

The point is that the *replication* role can't grant itself superuser
privs. Having the replication role compromised isn't great, but if that
role is *also* a superuser, then the whole database server could be
compromised. Encouraging users to continue to configure remote systems
with the ability to connect as a superuser when it's not necessary is a
bad idea.

One compromise would be to:

a) let superusers be granted the replication permission
b) have pg_dump assume that superusers have that permission when dumping
from a version which pre-dates the replication grant
c) have pg_upgrade assume the superuser has that permission when
upgrading
d) *not* grant replication to the default superuser

A better alternative, imv, would be to just have a & d, and mention in
the release notes that users *should* create a dedicated replication
role which is *not* a superuser but *does* have the replication grant,
but if they don't want to change their existing configurations, they can
just grant the replication privilege to whatever role they're currently
using.

Thanks,

Stephen

On 12/23/10 2:33 PM, Stephen Frost wrote:
> A better alternative, imv, would be to just have a & d, and mention in
> the release notes that users *should* create a dedicated replication
> role which is *not* a superuser but *does* have the replication grant,
> but if they don't want to change their existing configurations, they can
> just grant the replication privilege to whatever role they're currently
> using.

Well, if we really want people to change their behavior then we need to
make it easy for them:

1) have a replication permission
2) *by default* create a replication user with the replication
permission when we initdb.
3) have an example line for a replication connection by the replication
user in the default pg_hba.conf (commented out).
4) change all our docs and examples to use that replication user.

If using the replication user is easier than any other path, people
will. If it's harder, they won't.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> On 12/23/10 2:33 PM, Stephen Frost wrote:
>> A better alternative, imv, would be to just have a & d, and mention in
>> the release notes that users *should* create a dedicated replication
>> role which is *not* a superuser but *does* have the replication grant,
>> but if they don't want to change their existing configurations, they can
>> just grant the replication privilege to whatever role they're currently
>> using.

> Well, if we really want people to change their behavior then we need to
> make it easy for them:

> 1) have a replication permission
> 2) *by default* create a replication user with the replication
> permission when we initdb.

Yeah, I could see doing that ... the entry would be wasted if you're not
doing any replication, but one wasted catalog entry isn't much.

However, it'd be a real good idea for that role to be NOLOGIN if it's
there by default.

regards, tom lane

* Josh Berkus (josh(at)agliodbs(dot)com) wrote:
> 1) have a replication permission

Right, that's what this patch is about.

> 2) *by default* create a replication user with the replication
> permission when we initdb.

I'm not entirely sure about this one.. I'm not against it but I'm also
not really 'for' it. :)

> 3) have an example line for a replication connection by the replication
> user in the default pg_hba.conf (commented out).

Sure.

> 4) change all our docs and examples to use that replication user.

I don't have a problem with that.

Thanks,

Stephen

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> However, it'd be a real good idea for that role to be NOLOGIN if it's
> there by default.

Definitely. I'd also suggest we WARN if someone creates a situation
where a role has both replication and login, and maybe prevent it
altogether, it's just a bad idea..

Thanks,

Stephen

On Thu, Dec 23, 2010 at 23:44, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> On 12/23/10 2:33 PM, Stephen Frost wrote:
>>> A better alternative, imv, would be to just have a & d, and mention in
>>> the release notes that users *should* create a dedicated replication
>>> role which is *not* a superuser but *does* have the replication grant,
>>> but if they don't want to change their existing configurations, they can
>>> just grant the replication privilege to whatever role they're currently
>>> using.
>
>> Well, if we really want people to change their behavior then we need to
>> make it easy for them:
>
>> 1) have a replication permission
>> 2) *by default* create a replication user with the replication
>> permission when we initdb.
>
> Yeah, I could see doing that ... the entry would be wasted if you're not
> doing any replication, but one wasted catalog entry isn't much.
>
> However, it'd be a real good idea for that role to be NOLOGIN if it's
> there by default.

That shouldn't be too hard - I'll look at making the patch do that to
make sure it *isn't* that hard ;)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

> I'm not entirely sure about this one.. I'm not against it but I'm also
> not really 'for' it. :)

2 reasons: (a) if users need to CREATE USER, that *does* add an extra
step and they're more likely to just choose to grant to postgres
instead, (b) having a standard installed user (just like the user
"postgres" is standard) would make our docs, examples and tutorials much
easier to use.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

On Thu, Dec 23, 2010 at 5:59 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>
>> I'm not entirely sure about this one..  I'm not against it but I'm also
>> not really 'for' it. :)
>
> 2 reasons: (a) if users need to CREATE USER, that *does* add an extra
> step and they're more likely to just choose to grant to postgres
> instead, (b) having a standard installed user (just like the user
> "postgres" is standard) would make our docs, examples and tutorials much
> easier to use.

If the user exists but is disabled by default, I'm not sure that
really provides any advantage over not having it at all. And it
certainly can't be enabled by default.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> If the user exists but is disabled by default, I'm not sure that
> really provides any advantage over not having it at all. And it
> certainly can't be enabled by default.

I was presuming that NOLOGIN wouldn't prevent a replication connections
via the replication user. So the user would be "enabled" as far as
replication was concerned.

And currently, there is no replication connection in the pg_hba.conf.
So that's not a change; in fact, having a sample one would be an
improvement.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

On Dec23, 2010, at 16:54 , Tom Lane wrote:
> BTW, is it possible to set things up so that a REPLICATION account
> can be NOLOGIN, thereby making it really hard to abuse for other
> purposes? Or does the login privilege check come too soon?

Please don't. This violates the principle of least surprise big time!
And, whats worse, violate it in such a way that people *underestimate*
the permissions a particular role has, which from a security POV is
the worst than can happen. You pointed out yourself that REPLICATION
is a powerful permission to have because it essentially grants you read
access to the whole cluster. By allowing roles to connect a WAL receivers
even if they have NOLOGIN set, you're giving these powerful permission
to a role a DBA might think is disabled!

Now, I *can* see that having roles which may only connect as WAL receivers,
not to issue queries, is worthwhile. However, please either

A) Invert a new flag for that, for example SQL/NOSQL. A pure replication
role would have WITH REPLICATION NOSQL, while most other would have
WITH NOREPLICATION SQL. It's debatable wether postgres should have
WITH REPLICATION SQL or WITH NOREPLICATION SQL by default.

B) Forbid REPLICATION roles from connecting as anything else than WAL
receivers altogether. It'd then probably be a good idea to prevent
such roles from having SUPERUSER, CREATEDB, CREATEROLE and INHERIT
set as these flag wouldn't then make any sense.

The only downside of (B) that I can see is that it'd break setups that
work with 9.0.

best regards,
Florian Pflug

Florian Pflug <fgp(at)phlo(dot)org> writes:
> On Dec23, 2010, at 16:54 , Tom Lane wrote:
>> BTW, is it possible to set things up so that a REPLICATION account
>> can be NOLOGIN, thereby making it really hard to abuse for other
>> purposes? Or does the login privilege check come too soon?

> Please don't. This violates the principle of least surprise big time!

How so? (Please note I said *can be*, not *has to be*.)

The point of this is to ensure that if someone does illicitly come by
the replication role's password, they can't use it to log in. They can
still steal all your data, but they can't actually get into the
database. I don't see why it's a bad idea to configure things that way.

regards, tom lane

On Dec24, 2010, at 04:16 , Tom Lane wrote:
> Florian Pflug <fgp(at)phlo(dot)org> writes:
>> On Dec23, 2010, at 16:54 , Tom Lane wrote:
>>> BTW, is it possible to set things up so that a REPLICATION account
>>> can be NOLOGIN, thereby making it really hard to abuse for other
>>> purposes? Or does the login privilege check come too soon?
>
>> Please don't. This violates the principle of least surprise big time!
>
> How so? (Please note I said *can be*, not *has to be*.)

Because a DBA might "ALTER ROLE replication WITH NOLOGIN", thinking he has
just disabled that role. Only to find out weeks later than he hasn't
and that someone has been using that role to stream weeks worth of
confidential data to who knows where.

The problem here is that you suggest NOLOGIN should mean "Not allowed
to issue SQL commands", which really isn't what the name "NOLOGIN"
conveys. The concept itself is perfectly fine, but the name is dangerously
confusing.

> The point of this is to ensure that if someone does illicitly come by
> the replication role's password, they can't use it to log in. They can
> still steal all your data, but they can't actually get into the
> database. I don't see why it's a bad idea to configure things that way.

It's perfectly fine to configure things that way, and is in fact what I would
do. I'd just prefer the name for that setting to convey it's actual meaning
which is why I suggested adding a SQL/NOSQL flag. (Or SESSION/NOSESSION,
or whatever). Or, much simpler, to prevent WITH REPLICATION roles from issuing
SQL commands altogether. That'd achieve your goal just as well and is way less
confusing.

best regards,
Florian Pflug

Florian Pflug <fgp(at)phlo(dot)org> writes:
> The problem here is that you suggest NOLOGIN should mean "Not allowed
> to issue SQL commands", which really isn't what the name "NOLOGIN"
> conveys.

No, it means "not allowed to connect". It's possible now to issue
commands as a NOLOGIN user, you just have to use SET ROLE to become the
user. I think you're arguing about a design choice that was already
made some time ago.

regards, tom lane

On Thu, Dec 23, 2010 at 11:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Florian Pflug <fgp(at)phlo(dot)org> writes:
>> The problem here is that you suggest NOLOGIN should mean "Not allowed
>> to issue SQL commands", which really isn't what the name "NOLOGIN"
>> conveys.
>
> No, it means "not allowed to connect".  It's possible now to issue
> commands as a NOLOGIN user, you just have to use SET ROLE to become the
> user.  I think you're arguing about a design choice that was already
> made some time ago.

I think I agree with Florian about the confusing-ness of the proposed
semantics. Aren't you saying you want NOLOGIN mean "not allowed to
log in for the purposes of issuing SQL commands, but allowed to log in
for replication"? Uggh.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> I think I agree with Florian about the confusing-ness of the proposed
> semantics. Aren't you saying you want NOLOGIN mean "not allowed to
> log in for the purposes of issuing SQL commands, but allowed to log in
> for replication"? Uggh.

I like the general idea of a replication-only "role" or "login". Maybe
implementing that as a role w/ all the things that come along with it
being a role isn't right, but we don't want to have to reinvent all the
supported auth mechanisms (and please don't propose limiting the auth
options for the replication login!). Is there a way we can leverage the
auth mechanisms, etc, while forcing the 'replication role' to only be
able to do what a 'replication role' should do?

Thanks,

Stephen

On Dec24, 2010, at 05:00 , Tom Lane wrote:
> Florian Pflug <fgp(at)phlo(dot)org> writes:
>> The problem here is that you suggest NOLOGIN should mean "Not allowed
>> to issue SQL commands", which really isn't what the name "NOLOGIN"
>> conveys.
>
> No, it means "not allowed to connect".

Exactly. Which proves my point, unless you're ready to argue that
replication connections somehow don't count as "connections".

> It's possible now to issue
> commands as a NOLOGIN user, you just have to use SET ROLE to become the
> user. I think you're arguing about a design choice that was already
> made some time ago.

You've lost me, how is that an argument in your favour? I *wasn't* arguing
that NOLOGIN ought to mean "No allowed to issue SQL commands". It was what
*your* proposal of letting a role connect for replication purposes despite
a NOLOGIN flag would *make* NOLOGIN mean.

best regards,
Florian Pflug

On Thu, 2010-12-23 at 10:53 +0100, Magnus Hagander wrote:

> Here's a patch that changes walsender to require a special privilege
> for replication instead of relying on superuser permissions. We
> discussed this back before 9.0 was finalized, but IIRC we ran out of
> time. The motivation being that you really want to use superuser as
> little as possible - and since being a replication slave is a read
> only role, it shouldn't require the maximum permission available in
> the system.

Is backup part of this new privilege, or not?

I think if we're going to introduce a new level of privilege, then we
should introduce all delegatable privs in one software release. Much
better than having someone think up a new delegatable priv each release
for next 5 years.

Other possible ones include unsafe PL creation, seeing logged SQL etc..

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Dec 27, 2010 at 09:32, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Thu, 2010-12-23 at 10:53 +0100, Magnus Hagander wrote:
>
>> Here's a patch that changes walsender to require a special privilege
>> for replication instead of relying on superuser permissions. We
>> discussed this back before 9.0 was finalized, but IIRC we ran out of
>> time. The motivation being that you really want to use superuser as
>> little as possible - and since being a replication slave is a read
>> only role, it shouldn't require the maximum permission available in
>> the system.
>
> Is backup part of this new privilege, or not?

The "integrated base backup", once we have that, that's based on the
walsender protocol? yes.
pg_dump style backups? No.

> I think if we're going to introduce a new level of privilege, then we
> should introduce all delegatable privs in one software release. Much
> better than having someone think up a new delegatable priv each release
> for next 5 years.
>
> Other possible ones include unsafe PL creation, seeing logged SQL etc..

That's certainly an option, but that means someone would have to come
up with a list ;) And one that's reasonable - for example, "unsafe pl
creation" is from a security perspective (which is the only thing
that's really intersting here) the same as superuser.

Seeing logged SQL isn't - but being able to filter the logfiles on
that requires a *lot* more than just defining a security privilege. If
we mean "arbitrary log file reading", the easiest way to fix that
would be to stop checking for superuser permissions in the
read-file-function, and instead use the permissions *on the function*
to control it. In fact, that is something that we could (should?) do
for a bunch of other functions as well, so that we can in that way
provide much more granular permissions level than just blanked
assigning of privileges.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, Dec 27, 2010 at 9:36 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Seeing logged SQL isn't - but being able to filter the logfiles on
> that requires a *lot* more than just defining a security privilege. If
> we mean "arbitrary log file reading", the easiest way to fix that
> would be to stop checking for superuser permissions in the
> read-file-function, and instead use the permissions *on the function*
> to control it. In fact, that is something that we could (should?) do
> for a bunch of other functions as well, so that we can in that way
> provide much more granular permissions level than just blanked
> assigning of privileges.

That would require having users change the permissions on system
objects, which seems, icky (would they even be dumped?). Given that
the superuser could already create a security definer wrapper function
with the privileges required, I don't think this is needed.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Dec 24, 2010 at 05:46, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> I think I agree with Florian about the confusing-ness of the proposed
>> semantics.  Aren't you saying you want NOLOGIN mean "not allowed to
>> log in for the purposes of issuing SQL commands, but allowed to log in
>> for replication"?  Uggh.
>
> I like the general idea of a replication-only "role" or "login".  Maybe
> implementing that as a role w/ all the things that come along with it
> being a role isn't right, but we don't want to have to reinvent all the
> supported auth mechanisms (and please don't propose limiting the auth
> options for the replication login!).  Is there a way we can leverage the
> auth mechanisms, etc, while forcing the 'replication role' to only be
> able to do what a 'replication role' should do?

We could quite easily make a replication role *never* be able to
connect to a non-walsender backend. That would mean that if you set
your role to WITH REPLICATION, it can *only* be used for replication
and nothing else (well, you could still SET ROLE to it, but given that
it's not a superuser (anymore), that doesn't have any security
implications. Though we could perhaps restrict that as well in the SET
ROLE processing code).

This requires there to be a separate user for replication in all cases
- which isn't a bad thing IMHO. And it also doesn't "abuse"/confuse
the NOLOGIN attribute.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, 2010-12-27 at 10:36 +0100, Magnus Hagander wrote:
> > Is backup part of this new privilege, or not?
>
> The "integrated base backup", once we have that, that's based on the
> walsender protocol? yes.
> pg_dump style backups? No.

Where does pg_start_backup()/stop fit?

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Dec 27, 2010 at 11:34, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2010-12-27 at 10:36 +0100, Magnus Hagander wrote:
>> > Is backup part of this new privilege, or not?
>>
>> The "integrated base backup", once we have that, that's based on the
>> walsender protocol? yes.
>> pg_dump style backups? No.
>
> Where does pg_start_backup()/stop fit?

Good question :)

Given that the integrated-base-backup would call it for you, that one
would definitely get it automatically.

Given that the latest discissions seem to have most people wanting the
replication role *not* to be allowed to log in and run general SQL, we
should not drive the start/stop backup permissions from that
privilege.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, Dec 27, 2010 at 10:53, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Fri, Dec 24, 2010 at 05:46, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>>> I think I agree with Florian about the confusing-ness of the proposed
>>> semantics.  Aren't you saying you want NOLOGIN mean "not allowed to
>>> log in for the purposes of issuing SQL commands, but allowed to log in
>>> for replication"?  Uggh.
>>
>> I like the general idea of a replication-only "role" or "login".  Maybe
>> implementing that as a role w/ all the things that come along with it
>> being a role isn't right, but we don't want to have to reinvent all the
>> supported auth mechanisms (and please don't propose limiting the auth
>> options for the replication login!).  Is there a way we can leverage the
>> auth mechanisms, etc, while forcing the 'replication role' to only be
>> able to do what a 'replication role' should do?
>
> We could quite easily make a replication role *never* be able to
> connect to a non-walsender backend. That would mean that if you set
> your role to WITH REPLICATION, it can *only* be used for replication
> and nothing else (well, you could still SET ROLE to it, but given that
> it's not a superuser (anymore), that doesn't have any security
> implications. Though we could perhaps restrict that as well in the SET
> ROLE processing code).
>
> This requires there to be a separate user for replication in all cases
> - which isn't a bad thing IMHO. And it also doesn't "abuse"/confuse
> the NOLOGIN attribute.

Actually, having implemented that and tested it, I realize that's a
pretty bad idea. For one thing, it broke my own pg_streamrecv program,
since it requires the ability to connect to the master and select a
pg_current_xlog_location().

We could allow NOLOGIN connections to do replication, but I agree with
those saying that's pretty confusing - to the point of making it very
easy for a DBA to think he's secure when he's not. And having the
*ability* to use the same role as replication and superuser isn't a
bad thing - it's either *requiring* it or *having it so by default*
that's the problem, IMHO.

I think it's Ok to ship with replication off and require people to
either create a replication role or assign the permissions to their
superuser - as long as this is well documented in the manual.

The compromise I think would be to ship with a replication role
installed and *enabled*. There's no win at all in shipping with a
disabled replication role - it would still require the "extra step"
that people want to avoid.

What I'd rather do is make the "all" keyword for databases in
pg_hba.conf actually include replication connections - now that we
have a separate permissions - thus removing the need to configure a
specific row in pg_hba.conf for those users who just put a "0.0.0.0/0
md5" row in there to dumb it down. That still let's those who care
about higher security lock it down if they want to, while still
removing a step for those users who don't care.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Dec27, 2010, at 12:15 , Magnus Hagander wrote:
> Actually, having implemented that and tested it, I realize that's a
> pretty bad idea. For one thing, it broke my own pg_streamrecv program,
> since it requires the ability to connect to the master and select a
> pg_current_xlog_location().

I'm starting to think what we really want here is a kind of read-only
superuser. WITH REPLICATION already essentially gives you read-only
access to the whole database. Thus, allowing WITH REPLICATION roles
read-only access to everything on the SQL level also doesn't really
extend their abilities, it merely makes getting some information faster
and more convenient. It'd also make WITH REPLICATION the perfect fit
for pg_dump-style backups, if you're uneasy about using a superuser
for that.

best regards,
Florian Pflug

On Mon, 2010-12-27 at 12:00 +0100, Magnus Hagander wrote:
> On Mon, Dec 27, 2010 at 11:34, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On Mon, 2010-12-27 at 10:36 +0100, Magnus Hagander wrote:
> >> > Is backup part of this new privilege, or not?
> >>
> >> The "integrated base backup", once we have that, that's based on the
> >> walsender protocol? yes.
> >> pg_dump style backups? No.
> >
> > Where does pg_start_backup()/stop fit?
>
> Good question :)
>
> Given that the integrated-base-backup would call it for you, that one
> would definitely get it automatically.
>
> Given that the latest discissions seem to have most people wanting the
> replication role *not* to be allowed to log in and run general SQL, we
> should not drive the start/stop backup permissions from that
> privilege.

So what your suggesting would actually defeat the purpose of having the
new privilege. Unless we trust in a new, untried method. Hmmm.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Dec 27, 2010 at 14:25, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2010-12-27 at 12:00 +0100, Magnus Hagander wrote:
>> On Mon, Dec 27, 2010 at 11:34, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> > On Mon, 2010-12-27 at 10:36 +0100, Magnus Hagander wrote:
>> >> > Is backup part of this new privilege, or not?
>> >>
>> >> The "integrated base backup", once we have that, that's based on the
>> >> walsender protocol? yes.
>> >> pg_dump style backups? No.
>> >
>> > Where does pg_start_backup()/stop fit?
>>
>> Good question :)
>>
>> Given that the integrated-base-backup would call it for you, that one
>> would definitely get it automatically.
>>
>> Given that the latest discissions seem to have most people wanting the
>> replication role *not* to be allowed to log in and run general SQL, we
>> should not drive the start/stop backup permissions from that
>> privilege.
>
> So what your suggesting would actually defeat the purpose of having the
> new privilege. Unless we trust in a new, untried method. Hmmm.

No, it doesn't.

In my experience, most DBAs will connect with their DBA account
(usually the superuser, yes..) to run pg_start_backup() and
pg_stop_backup(). That's no reason to let the slave sever run with
superuser privileges all the time...

That said, I agree that the we shouldn't *prevent* the DBA from
setting up an account that is both superuser and replicator - just
that we shouldn't do it by default.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, 2010-12-27 at 14:41 +0100, Magnus Hagander wrote:
> >> >
> >> > Where does pg_start_backup()/stop fit?
> >>
> >> Good question :)
> >>
> >> Given that the integrated-base-backup would call it for you, that one
> >> would definitely get it automatically.
> >>
> >> Given that the latest discissions seem to have most people wanting the
> >> replication role *not* to be allowed to log in and run general SQL, we
> >> should not drive the start/stop backup permissions from that
> >> privilege.
> >
> > So what your suggesting would actually defeat the purpose of having the
> > new privilege. Unless we trust in a new, untried method. Hmmm.
>
> No, it doesn't.
>
> In my experience, most DBAs will connect with their DBA account
> (usually the superuser, yes..) to run pg_start_backup() and
> pg_stop_backup(). That's no reason to let the slave sever run with
> superuser privileges all the time...

Remember the standby's superuser id is exactly the same as the main
server's superuserid. So unless you are going to stop the standby from
knowing its own superusers there's no huge benefit there. Is that what
you mean to do?

> That said, I agree that the we shouldn't *prevent* the DBA from
> setting up an account that is both superuser and replicator - just
> that we shouldn't do it by default.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Dec 27, 2010 at 14:51, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2010-12-27 at 14:41 +0100, Magnus Hagander wrote:
>> >> >
>> >> > Where does pg_start_backup()/stop fit?
>> >>
>> >> Good question :)
>> >>
>> >> Given that the integrated-base-backup would call it for you, that one
>> >> would definitely get it automatically.
>> >>
>> >> Given that the latest discissions seem to have most people wanting the
>> >> replication role *not* to be allowed to log in and run general SQL, we
>> >> should not drive the start/stop backup permissions from that
>> >> privilege.
>> >
>> > So what your suggesting would actually defeat the purpose of having the
>> > new privilege. Unless we trust in a new, untried method. Hmmm.
>>
>> No, it doesn't.
>>
>> In my experience, most DBAs will connect with their DBA account
>> (usually the superuser, yes..) to run pg_start_backup() and
>> pg_stop_backup(). That's no reason to let the slave sever run with
>> superuser privileges all the time...
>
> Remember the standby's superuser id is exactly the same as the main
> server's superuserid. So unless you are going to stop the standby from
> knowing its own superusers there's no huge benefit there. Is that what
> you mean to do?

I'm sorry, I have no idea what you mean by that.

You will certainly be able to log into the standby with a superuser
account, nobody is preventing that. This is about protecting the
*master*. For example, from modifications made by a user who hacked
the standby.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Mon, Dec 27, 2010 at 10:53, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> We could quite easily make a replication role *never* be able to
>> connect to a non-walsender backend. That would mean that if you set
>> your role to WITH REPLICATION, it can *only* be used for replication
>> and nothing else (well, you could still SET ROLE to it, but given that
>> it's not a superuser (anymore), that doesn't have any security
>> implications.

> Actually, having implemented that and tested it, I realize that's a
> pretty bad idea.

OK, so if we're not going to recommend that REPLICATION roles be
NOLOGIN, we're back to the original question: should the REPLICATION
bit give any other special privileges? I can see the point of allowing
such a user to issue pg_start_backup and pg_stop_backup.

regards, tom lane

On Mon, Dec 27, 2010 at 16:33, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> On Mon, Dec 27, 2010 at 10:53, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>> We could quite easily make a replication role *never* be able to
>>> connect to a non-walsender backend. That would mean that if you set
>>> your role to WITH REPLICATION, it can *only* be used for replication
>>> and nothing else (well, you could still SET ROLE to it, but given that
>>> it's not a superuser (anymore), that doesn't have any security
>>> implications.
>
>> Actually, having implemented that and tested it, I realize that's a
>> pretty bad idea.
>
> OK, so if we're not going to recommend that REPLICATION roles be
> NOLOGIN, we're back to the original question: should the REPLICATION
> bit give any other special privileges?  I can see the point of allowing
> such a user to issue pg_start_backup and pg_stop_backup.

Yes, those would definitely be useful.

We are, basically, talking about where we'll relax the "only
superuser" one, right? Since they are normal roles, the DBA can always
GRANT permissions on *objects* to them, but there are superuser-only
things taht you can't GRANT away...

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, 2010-12-27 at 14:54 +0100, Magnus Hagander wrote:

> You will certainly be able to log into the standby with a superuser
> account, nobody is preventing that. This is about protecting the
> *master*. For example, from modifications made by a user who hacked
> the standby.

The users for master and standby are identical, so if they have access
to the standby, they have access to the master. That's why we allow
replication to be specifically excluded by the pg_hba.conf.

So I don't see how this helps.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Dec 27, 2010 at 16:45, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2010-12-27 at 14:54 +0100, Magnus Hagander wrote:
>
>> You will certainly be able to log into the standby with a superuser
>> account, nobody is preventing that. This is about protecting the
>> *master*. For example, from modifications made by a user who hacked
>> the standby.
>
> The users for master and standby are identical, so if they have access
> to the standby, they have access to the master. That's why we allow
> replication to be specifically excluded by the pg_hba.conf.

You are assuming there *is* a standby.

This is a defence against someone connecting with psql (or whatever)
directly to the master, *pretending to be* the standby (same
username/password, possibly even the same server ip).

Currently, this user gets the key to the kingdom and can modify things
freely on the master. With the patch, this user cannot. He can still
initiate streaming and eventually capture all your data, but he can't
modify it.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, Dec 27, 2010 at 16:40, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Mon, Dec 27, 2010 at 16:33, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>> On Mon, Dec 27, 2010 at 10:53, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>>> We could quite easily make a replication role *never* be able to
>>>> connect to a non-walsender backend. That would mean that if you set
>>>> your role to WITH REPLICATION, it can *only* be used for replication
>>>> and nothing else (well, you could still SET ROLE to it, but given that
>>>> it's not a superuser (anymore), that doesn't have any security
>>>> implications.
>>
>>> Actually, having implemented that and tested it, I realize that's a
>>> pretty bad idea.
>>
>> OK, so if we're not going to recommend that REPLICATION roles be
>> NOLOGIN, we're back to the original question: should the REPLICATION
>> bit give any other special privileges?  I can see the point of allowing
>> such a user to issue pg_start_backup and pg_stop_backup.
>
> Yes, those would definitely be useful.

Updated patch, still pending docs, but otherwise updated: allow
start/stop backup, make sure only superuser can turn on/off the flag,
include in system views, show properly in psql.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Updated patch, still pending docs, but otherwise updated: allow
> start/stop backup, make sure only superuser can turn on/off the flag,
> include in system views, show properly in psql.

I'd suggest avoiding creating the static cache variable
AuthenticatedUserIsReplicationRole. This can't possibly be sufficiently
interesting from a performance point of view to justify the risks
associated with stale cache values. Just look up the pg_authid syscache
entry when needed, ie, treat it more like rolcreaterole than rolsuper.

BTW, you forgot pg_dumpall support.

regards, tom lane

On Mon, Dec 27, 2010 at 22:42, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Updated patch, still pending docs, but otherwise updated: allow
>> start/stop backup, make sure only superuser can turn on/off the flag,
>> include in system views, show properly in psql.
>
> I'd suggest avoiding creating the static cache variable
> AuthenticatedUserIsReplicationRole.  This can't possibly be sufficiently
> interesting from a performance point of view to justify the risks
> associated with stale cache values.  Just look up the pg_authid syscache
> entry when needed, ie, treat it more like rolcreaterole than rolsuper.

Sure, I catually had it that way first. But doing it this way was less
code. But I realize I should've revisited that decision when I made
the change to pg_start_backup and pg_stop_backup - before that the
checks would only happen during a very short window of time at the
start of the connection, but now it can happen later..

> BTW, you forgot pg_dumpall support.

Gah. I knew that, but somehow dropped it from my TODO. Thanks for the reminder!

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, Dec 27, 2010 at 22:53, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Mon, Dec 27, 2010 at 22:42, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>> Updated patch, still pending docs, but otherwise updated: allow
>>> start/stop backup, make sure only superuser can turn on/off the flag,
>>> include in system views, show properly in psql.
>>
>> I'd suggest avoiding creating the static cache variable
>> AuthenticatedUserIsReplicationRole.  This can't possibly be sufficiently
>> interesting from a performance point of view to justify the risks
>> associated with stale cache values.  Just look up the pg_authid syscache
>> entry when needed, ie, treat it more like rolcreaterole than rolsuper.
>
> Sure, I catually had it that way first. But doing it this way was less
> code. But I realize I should've revisited that decision when I made
> the change to pg_start_backup and pg_stop_backup - before that the
> checks would only happen during a very short window of time at the
> start of the connection, but now it can happen later..
>
>
>> BTW, you forgot pg_dumpall support.
>
> Gah. I knew that, but somehow dropped it from my TODO. Thanks for the reminder!

Ok, here's an updated patch that does both these and includes
documentation and regression test changes. With that, I think we're
good to go.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Tue, Dec 28, 2010 at 13:05, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Mon, Dec 27, 2010 at 22:53, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> On Mon, Dec 27, 2010 at 22:42, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>>> Updated patch, still pending docs, but otherwise updated: allow
>>>> start/stop backup, make sure only superuser can turn on/off the flag,
>>>> include in system views, show properly in psql.
>>>
>>> I'd suggest avoiding creating the static cache variable
>>> AuthenticatedUserIsReplicationRole.  This can't possibly be sufficiently
>>> interesting from a performance point of view to justify the risks
>>> associated with stale cache values.  Just look up the pg_authid syscache
>>> entry when needed, ie, treat it more like rolcreaterole than rolsuper.
>>
>> Sure, I catually had it that way first. But doing it this way was less
>> code. But I realize I should've revisited that decision when I made
>> the change to pg_start_backup and pg_stop_backup - before that the
>> checks would only happen during a very short window of time at the
>> start of the connection, but now it can happen later..
>>
>>
>>> BTW, you forgot pg_dumpall support.
>>
>> Gah. I knew that, but somehow dropped it from my TODO. Thanks for the reminder!
>
> Ok, here's an updated patch that does both these and includes
> documentation and regression test changes. With that, I think we're
> good to go.

I've applied this version (with some minor typo-fixes).

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Wed, Dec 29, 2010 at 5:09 AM, Magnus Hagander <magnus(at)hagander(dot)net>wrote:

> > Ok, here's an updated patch that does both these and includes
> > documentation and regression test changes. With that, I think we're
> > good to go.
>
> I've applied this version (with some minor typo-fixes).
>
>
Do you think we could have worded these a bit better

<entry>Prepare for performing on-line backup (restricted to superusers or
replication roles)</entry>

to say 'restricted to superusers _and_ replication roles'.

Saying 'restricted to superusers _or_ replication roles' may mean that at
any time we allow one or the other, but not both (reader might assume that
decision is based on some other GUC).

Using 'and' would mean that we allow it for both of those roles.

Any specific reason NOREPLICATION_P and REPLICATION_P use the _P suffix?
AIUI, that suffix is used in gram.y to tag a token to mean it belongs to
Parser, and to avoid conflict with the same token elsewhere; NULL_P is a
good example.

In pg_authid.h, 8 spaces used between 'bool' and 'rolreplication', instead
tabs should have been used as the surrounding code.

Regards,
--
gurjeet.singh
@ EnterpriseDB - The Enterprise Postgres Company
http://www.EnterpriseDB.com

singh(dot)gurjeet(at){ gmail | yahoo }.com
Twitter/Skype: singh_gurjeet

Mail sent from my BlackLaptop device

On Wed, Dec 29, 2010 at 15:05, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> wrote:
> On Wed, Dec 29, 2010 at 5:09 AM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
>>
>> > Ok, here's an updated patch that does both these and includes
>> > documentation and regression test changes. With that, I think we're
>> > good to go.
>>
>> I've applied this version (with some minor typo-fixes).
>>
>
> Do you think we could have worded these a bit better
>
> <entry>Prepare for performing on-line backup (restricted to superusers or
> replication roles)</entry>
>
> to say 'restricted to superusers _and_ replication roles'.
>
> Saying 'restricted to superusers _or_ replication roles' may mean that at
> any time we allow one or the other, but not both (reader might assume that
> decision is based on some other GUC).

Uh, not sure, actually. I would read the "and" as meaning you needed
*both*, which isn't true. We do allow, at any time, one or the other -
*or* both.

> Any specific reason NOREPLICATION_P and REPLICATION_P use the _P suffix?

Um, I just copied it off a similar entry elsewhere. I saw no comment
about what _P actually means, and I can't say I know. I know very
little about the bison files :-)

> AIUI, that suffix is used in gram.y to tag a token to mean it belongs to
> Parser, and to avoid conflict with the same token elsewhere; NULL_P is a
> good example.
>
> In pg_authid.h, 8 spaces used between 'bool' and 'rolreplication', instead
> tabs should have been used as the surrounding code.

Bleh. Well, pgindent will fix that.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Excerpts from Magnus Hagander's message of mié dic 29 11:40:34 -0300 2010:
> On Wed, Dec 29, 2010 at 15:05, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> wrote:

> > Any specific reason NOREPLICATION_P and REPLICATION_P use the _P suffix?
>
> Um, I just copied it off a similar entry elsewhere. I saw no comment
> about what _P actually means, and I can't say I know. I know very
> little about the bison files :-)

Some lexer keywords have a _P prefix because otherwise they'd collide
with some symbol in Windows header files or something like that. It's
old stuff, but I think you, Magnus, were around at that time.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Wed, Dec 29, 2010 at 20:12, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Magnus Hagander's message of mié dic 29 11:40:34 -0300 2010:
>> On Wed, Dec 29, 2010 at 15:05, Gurjeet Singh <singh(dot)gurjeet(at)gmail(dot)com> wrote:
>
>> > Any specific reason NOREPLICATION_P and REPLICATION_P use the _P suffix?
>>
>> Um, I just copied it off a similar entry elsewhere. I saw no comment
>> about what _P actually means, and I can't say I know. I know very
>> little about the bison files :-)
>
> Some lexer keywords have a _P prefix because otherwise they'd collide
> with some symbol in Windows header files or something like that.  It's
> old stuff, but I think you, Magnus, were around at that time.

Heh. That doesn't mean I *remember* it :-)

But yes, I see in commit 12c942383296bd626131241c012c2ab81b081738 the
comment "convert some keywords.c symbols to KEYWORD_P to prevent
conflict".

Based on that, I should probably change it back, right? I just tried a
patch for it and it compiles and checks just fine with the _P parts
removed.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Excerpts from Magnus Hagander's message of jue dic 30 08:57:09 -0300 2010:
> On Wed, Dec 29, 2010 at 20:12, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:

> > Some lexer keywords have a _P prefix because otherwise they'd collide
> > with some symbol in Windows header files or something like that.  It's
> > old stuff, but I think you, Magnus, were around at that time.
>
> Heh. That doesn't mean I *remember* it :-)

:-)

> But yes, I see in commit 12c942383296bd626131241c012c2ab81b081738 the
> comment "convert some keywords.c symbols to KEYWORD_P to prevent
> conflict".

Wow, what a mess of a patch ... nowadays this would be like 10 commits
(or so I hope) ... hey, did Bruce sabotage the qnx4 port surreptitiously?

> Based on that, I should probably change it back, right? I just tried a
> patch for it and it compiles and checks just fine with the _P parts
> removed.

Hmm, I wouldn't bother really. It's not that important anyway, IMHO.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On tor, 2010-12-23 at 17:29 -0500, Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
> > On 12/23/10 2:21 PM, Tom Lane wrote:
> >> Well, that's one laudable goal here, but "secure by default" is another
> >> one that ought to be taken into consideration.
>
> > I don't see how *not* granting the superuser replication permissions
> > makes things more secure. The superuser can grant replication
> > permissions to itself, so why is suspending them by default beneficial?
> > I'm not following your logic here.
>
> Well, the reverse of that is just as true: if we ship it without
> replication permissions on the postgres user, people can change that if
> they'd rather not create a separate role for replication. But I think
> we should encourage people to NOT do it that way. Setting it up that
> way by default hardly encourages use of a more secure arrangement.

I think this argument is a bit inconsistent in the extreme. You might
as well argue that a superuser shouldn't have any permissions by
default, to discourage users from using it. They can always grant
permissions back to it. I don't see why this particular one is so
different.

If we go down this road, we'll end up with a mess of permissions that a
superuser has and doesn't have.

On ons, 2010-12-29 at 11:09 +0100, Magnus Hagander wrote:
> I've applied this version (with some minor typo-fixes).

This page is now somewhat invalidated:

http://developer.postgresql.org/pgdocs/postgres/role-attributes.html

First, it doesn't mention the replication privilege, and second it
continues to claim that superuser status bypasses all permission checks.

On Thu, Dec 30, 2010 at 9:54 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On tor, 2010-12-23 at 17:29 -0500, Tom Lane wrote:
>> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> > On 12/23/10 2:21 PM, Tom Lane wrote:
>> >> Well, that's one laudable goal here, but "secure by default" is another
>> >> one that ought to be taken into consideration.
>>
>> > I don't see how *not* granting the superuser replication permissions
>> > makes things more secure.  The superuser can grant replication
>> > permissions to itself, so why is suspending them by default beneficial?
>> >  I'm not following your logic here.
>>
>> Well, the reverse of that is just as true: if we ship it without
>> replication permissions on the postgres user, people can change that if
>> they'd rather not create a separate role for replication.  But I think
>> we should encourage people to NOT do it that way.  Setting it up that
>> way by default hardly encourages use of a more secure arrangement.
>
> I think this argument is a bit inconsistent in the extreme.  You might
> as well argue that a superuser shouldn't have any permissions by
> default, to discourage users from using it.  They can always grant
> permissions back to it.  I don't see why this particular one is so
> different.
>
> If we go down this road, we'll end up with a mess of permissions that a
> superuser has and doesn't have.

+1.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> But yes, I see in commit 12c942383296bd626131241c012c2ab81b081738 the
> comment "convert some keywords.c symbols to KEYWORD_P to prevent
> conflict".

> Based on that, I should probably change it back, right? I just tried a
> patch for it and it compiles and checks just fine with the _P parts
> removed.

I'd leave it be, it's fine as-is.

regards, tom lane

On Thu, Dec 30, 2010 at 15:54, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On ons, 2010-12-29 at 11:09 +0100, Magnus Hagander wrote:
>> I've applied this version (with some minor typo-fixes).
>
> This page is now somewhat invalidated:
>
> http://developer.postgresql.org/pgdocs/postgres/role-attributes.html

Hmm. Somehow I missed that page completely when looking through the
docs. I'll go update that.

> First, it doesn't mention the replication privilege, and second it
> continues to claim that superuser status bypasses all permission checks.

Well, that was *already* wrong.

superuser doesn't bypass NOLOGIN.

That doesn't mean it shouldn't be fixed, but that's independent of the
replication role.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Fri, Dec 31, 2010 at 15:38, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Thu, Dec 30, 2010 at 15:54, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> On ons, 2010-12-29 at 11:09 +0100, Magnus Hagander wrote:
>>> I've applied this version (with some minor typo-fixes).
>>
>> This page is now somewhat invalidated:
>>
>> http://developer.postgresql.org/pgdocs/postgres/role-attributes.html
>
> Hmm. Somehow I missed that page completely when looking through the
> docs. I'll go update that.

BTW, shouldn't CONNECTION LIMIT be listed on that page? and INHERIT?
And VALID UNTIL? They're all role attributes, no? At least the last
two certainly interact with the authentication system...

>> First, it doesn't mention the replication privilege, and second it
>> continues to claim that superuser status bypasses all permission checks.
>
> Well, that was *already* wrong.
>
> superuser doesn't bypass NOLOGIN.
>
> That doesn't mean it shouldn't be fixed, but that's independent of the
> replication role.

I've committed a fix for this.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, Jan 3, 2011 at 6:00 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Fri, Dec 31, 2010 at 15:38, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> On Thu, Dec 30, 2010 at 15:54, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>>> On ons, 2010-12-29 at 11:09 +0100, Magnus Hagander wrote:
>>>> I've applied this version (with some minor typo-fixes).
>>>
>>> This page is now somewhat invalidated:
>>>
>>> http://developer.postgresql.org/pgdocs/postgres/role-attributes.html
>>
>> Hmm. Somehow I missed that page completely when looking through the
>> docs. I'll go update that.
>
> BTW, shouldn't CONNECTION LIMIT be listed on that page? and INHERIT?
> And VALID UNTIL? They're all role attributes, no?

+1.

>>> First, it doesn't mention the replication privilege, and second it
>>> continues to claim that superuser status bypasses all permission checks.
>>
>> Well, that was *already* wrong.
>>
>> superuser doesn't bypass NOLOGIN.
>>
>> That doesn't mean it shouldn't be fixed, but that's independent of the
>> replication role.
>
> I've committed a fix for this.

I still think this is the wrong approach. Saying superuser doesn't
bypass nologin is like saying that it doesn't bypass the need to enter
the correct password to authenticate to it. You have to BE the
superuser before you start bypassing permissions checks, and NOLOGIN
and a possible password prompts control WHO CAN BECOME superuser. On
the other hand, the REPLICATION privilege is denying you the right to
perform an operation *even though you already are authenticated as a
superuser*. I don't think there's anywhere else in the system where
we allow a privilege to non-super-users but deny that same privilege
to super-users, and I don't think we should be starting now.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On the other hand, the REPLICATION privilege is denying you the right to
> perform an operation *even though you already are authenticated as a
> superuser*. I don't think there's anywhere else in the system where
> we allow a privilege to non-super-users but deny that same privilege
> to super-users, and I don't think we should be starting now.

You might want to reflect on rolcatupdate a bit before asserting that
there are no cases where privileges are ever denied to superusers.

However, that precedent would suggest that the default should be to
grant the replication bit to superusers.

regards, tom lane

On Mon, Jan 3, 2011 at 11:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On the other hand, the REPLICATION privilege is denying you the right to
>> perform an operation *even though you already are authenticated as a
>> superuser*.  I don't think there's anywhere else in the system where
>> we allow a privilege to non-super-users but deny that same privilege
>> to super-users, and I don't think we should be starting now.
>
> You might want to reflect on rolcatupdate a bit before asserting that
> there are no cases where privileges are ever denied to superusers.

Oh, huh. I wasn't aware of that.

> However, that precedent would suggest that the default should be to
> grant the replication bit to superusers.

Yes it would.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jan 3, 2011 at 17:23, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jan 3, 2011 at 11:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On the other hand, the REPLICATION privilege is denying you the right to
>>> perform an operation *even though you already are authenticated as a
>>> superuser*.  I don't think there's anywhere else in the system where
>>> we allow a privilege to non-super-users but deny that same privilege
>>> to super-users, and I don't think we should be starting now.
>>
>> You might want to reflect on rolcatupdate a bit before asserting that
>> there are no cases where privileges are ever denied to superusers.
>
> Oh, huh.  I wasn't aware of that.
>
>> However, that precedent would suggest that the default should be to
>> grant the replication bit to superusers.
>
> Yes it would.

Just to be clear: are we saying that "CREATE ROLE foo SUPERUSER"
should grant both superuser and replication, as well as the default
"postgres" user also having replication as well?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, Jan 3, 2011 at 5:50 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Mon, Jan 3, 2011 at 17:23, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Mon, Jan 3, 2011 at 11:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> On the other hand, the REPLICATION privilege is denying you the right to
>>>> perform an operation *even though you already are authenticated as a
>>>> superuser*.  I don't think there's anywhere else in the system where
>>>> we allow a privilege to non-super-users but deny that same privilege
>>>> to super-users, and I don't think we should be starting now.
>>>
>>> You might want to reflect on rolcatupdate a bit before asserting that
>>> there are no cases where privileges are ever denied to superusers.
>>
>> Oh, huh.  I wasn't aware of that.
>>
>>> However, that precedent would suggest that the default should be to
>>> grant the replication bit to superusers.
>>
>> Yes it would.
>
> Just to be clear: are we saying that "CREATE ROLE foo SUPERUSER"
> should grant both superuser and replication, as well as the default
> "postgres" user also having replication as well?

I think that's what we're saying.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jan 5, 2011 at 04:24, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jan 3, 2011 at 5:50 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> On Mon, Jan 3, 2011 at 17:23, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Mon, Jan 3, 2011 at 11:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>>> On the other hand, the REPLICATION privilege is denying you the right to
>>>>> perform an operation *even though you already are authenticated as a
>>>>> superuser*.  I don't think there's anywhere else in the system where
>>>>> we allow a privilege to non-super-users but deny that same privilege
>>>>> to super-users, and I don't think we should be starting now.
>>>>
>>>> You might want to reflect on rolcatupdate a bit before asserting that
>>>> there are no cases where privileges are ever denied to superusers.
>>>
>>> Oh, huh.  I wasn't aware of that.
>>>
>>>> However, that precedent would suggest that the default should be to
>>>> grant the replication bit to superusers.
>>>
>>> Yes it would.
>>
>> Just to be clear: are we saying that "CREATE ROLE foo SUPERUSER"
>> should grant both superuser and replication, as well as the default
>> "postgres" user also having replication as well?
>
> I think that's what we're saying.

Ok, done and applied.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Wed, Jan 5, 2011 at 8:24 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Ok, done and applied.

Thanks.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On mån, 2011-01-03 at 11:20 -0500, Tom Lane wrote:
> You might want to reflect on rolcatupdate a bit before asserting that
> there are no cases where privileges are ever denied to superusers.

Arguably, the reason that that is hardly documented and slightly
deprecated is that the underlying design decision is questionable.

On tis, 2011-01-04 at 22:24 -0500, Robert Haas wrote:
> > Just to be clear: are we saying that "CREATE ROLE foo SUPERUSER"
> > should grant both superuser and replication, as well as the default
> > "postgres" user also having replication as well?
>
> I think that's what we're saying.

So now superusers have it by default but you can explicitly revoke it?

I guess that's still inconsistent with other superuser behavior. You
can't revoke a superuser's CREATEDB bit, for example. (You can, but it
won't have an effect, of course.)