Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Mon, Dec 5, 2016 at 4:31 PM, Haribabu Kommi 
wrote:

>
>
> On Mon, Oct 3, 2016 at 3:11 PM, Michael Paquier  > wrote:
>
>> On Tue, Jul 19, 2016 at 3:42 PM, Haribabu Kommi
>>  wrote:
>> > The above changes are based on my understanding to the discussion
>> occurred in
>> > this mail. In case if I miss anything, please let me know, i will
>> > correct the same.
>>
>> The patch series still apply.
>>
>> +   " ((classid = (select oid from pg_class where
>> relname = 'pg_aggregate'))"
>> +   " OR (classid = (select oid from pg_class where
>> relname = 'pg_cast') AND has_cast_privilege(objid, 'any'))"
>> +   " OR (classid = (select oid from pg_class where
>> relname = 'pg_collation'))"
>> [... long list ...]
>> That's quite hard to digest...
>>
>> +static bool
>> +get_catalog_policy_string(Oid relationid, Form_pg_class
>> pg_class_tuple, char *buf)
>> This is an exceptionally weak interface at quick glance. This is using
>> SQL strings, and nothing is actually done regarding potentially
>> conflicting name types...
>>
>> The number of new files included in policy.c is impressive as well..
>>
>> This does not count as a full review though, so I am moving it to next
>> CF. Perhaps it will find its audience.
>>
>
> As the patch doesn't receive full review. Just kept in the commitfest to
> see any interest from others for this patch.
>
> Moved to next CF with "needs review" status.
>

This patch is not generating much interest from the community, may be
because of the design that is chosen to implement multi-tenancy.

Currently this patch is marked as rejected.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Mon, Oct 3, 2016 at 3:11 PM, Michael Paquier 
wrote:

> On Tue, Jul 19, 2016 at 3:42 PM, Haribabu Kommi
>  wrote:
> > The above changes are based on my understanding to the discussion
> occurred in
> > this mail. In case if I miss anything, please let me know, i will
> > correct the same.
>
> The patch series still apply.
>
> +   " ((classid = (select oid from pg_class where
> relname = 'pg_aggregate'))"
> +   " OR (classid = (select oid from pg_class where
> relname = 'pg_cast') AND has_cast_privilege(objid, 'any'))"
> +   " OR (classid = (select oid from pg_class where
> relname = 'pg_collation'))"
> [... long list ...]
> That's quite hard to digest...
>
> +static bool
> +get_catalog_policy_string(Oid relationid, Form_pg_class
> pg_class_tuple, char *buf)
> This is an exceptionally weak interface at quick glance. This is using
> SQL strings, and nothing is actually done regarding potentially
> conflicting name types...
>
> The number of new files included in policy.c is impressive as well..
>
> This does not count as a full review though, so I am moving it to next
> CF. Perhaps it will find its audience.
>

As the patch doesn't receive full review. Just kept in the commitfest to
see any interest from others for this patch.

Moved to next CF with "needs review" status.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Multi-tenancy with RLS

[Michael Paquier]

On Tue, Jul 19, 2016 at 3:42 PM, Haribabu Kommi
 wrote:
> The above changes are based on my understanding to the discussion occurred in
> this mail. In case if I miss anything, please let me know, i will
> correct the same.

The patch series still apply.

+   " ((classid = (select oid from pg_class where
relname = 'pg_aggregate'))"
+   " OR (classid = (select oid from pg_class where
relname = 'pg_cast') AND has_cast_privilege(objid, 'any'))"
+   " OR (classid = (select oid from pg_class where
relname = 'pg_collation'))"
[... long list ...]
That's quite hard to digest...

+static bool
+get_catalog_policy_string(Oid relationid, Form_pg_class
pg_class_tuple, char *buf)
This is an exceptionally weak interface at quick glance. This is using
SQL strings, and nothing is actually done regarding potentially
conflicting name types...

The number of new files included in policy.c is impressive as well..

This does not count as a full review though, so I am moving it to next
CF. Perhaps it will find its audience.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

JD,

* Joshua D. Drake (j...@commandprompt.com) wrote:
> pg_dump -U $non-super_user
> 
> Should just work, period.

That ship has sailed already, where you're running a pg_dump against
objects you don't own and which have RLS enabled on them.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Joshua D. Drake]

On 02/09/2016 12:05 PM, Robert Haas wrote:


That's true.  But I should also have an expectation that running
pg_dump won't trigger arbitrary code execution, which is why by
default, pg_dump sets row_security to OFF.  That way, if a row
security policy applies, I get an error rather than an incomplete,
possibly-invalid dump (and arbitrary code execution on the server
side).  If I'm OK with doing the dump subject to row security, I can
rerun with --enable-row-security.  But this proposal would force
non-superusers to always use that option, and that's not a good idea.



If I understand correctly what we are talking about here is:

1. If RLS is enabled and a non-super user issues a pg_dump, it will 
error unless I issue --enable-row-security


2. If RLS is not enabled and a non-super user issues a pg_dump the 
behavior is basically what it is now.


3. If RLS is enabled or not and I am a super user, it doesn't matter 
either way.


From my perspective, I should not have to enable row security as a 
non-super user to take a pg_dump. It should just work for what I am 
allowed to see. In other words:


pg_dump -U $non-super_user

Should just work, period.

Sincerely,

Joshua D. Drake

--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Robert Haas]

On Fri, Jan 15, 2016 at 11:53 AM, Stephen Frost  wrote:
>> Whereupon you'd have no certainty that what you got represented a
>> complete dump of your own data.
>
> It would be a dump of what you're allowed to see, rather than an error
> saying you couldn't dump something you couldn't see, which is the
> alternative we're talking about here.  Even if you've got a dependency
> to something-or-other, if you don't have access to it, then you're
> going to get an error.

I think you're dismissing Tom's concerns far too lightly.  The
row_security=off mode, which is the default, becomes unusable for
non-superusers under this proposal.  That's bad.  And if you switch to
the other mode, then you might accidentally fail to get all of the
data in some table you're trying to back up.  That's bad too: that's
why it isn't the default.  There's a big difference between saying
"I'm OK with not dumping the tables I can't see" and "I'm OK with not
dumping all of the data in some table I *can* see".

It seems to me that there's a big difference between policies we ship
out of the box and policies that are created be users: specifically,
the former can be assumed benign, while the latter can't.  I think
that difference matters here, although I'm not sure exactly where to
go with it.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Robert Haas]

On Tue, Feb 9, 2016 at 3:01 PM, Joe Conway  wrote:
> On 02/09/2016 11:47 AM, Robert Haas wrote:
>> On Fri, Jan 15, 2016 at 11:53 AM, Stephen Frost  wrote:
 Whereupon you'd have no certainty that what you got represented a
 complete dump of your own data.
>>>
>>> It would be a dump of what you're allowed to see, rather than an error
>>> saying you couldn't dump something you couldn't see, which is the
>>> alternative we're talking about here.  Even if you've got a dependency
>>> to something-or-other, if you don't have access to it, then you're
>>> going to get an error.
>>
>> I think you're dismissing Tom's concerns far too lightly.  The
>> row_security=off mode, which is the default, becomes unusable for
>> non-superusers under this proposal.  That's bad.  And if you switch to
>> the other mode, then you might accidentally fail to get all of the
>> data in some table you're trying to back up.  That's bad too: that's
>> why it isn't the default.  There's a big difference between saying
>> "I'm OK with not dumping the tables I can't see" and "I'm OK with not
>> dumping all of the data in some table I *can* see".
>
> I don't grok what you're saying here. If I, as a non-superuser could
> somehow see all the rows in a table just by running pg_dump, including
> rows that I could not normally see due to RLS policies, *that* would be
> bad. I should have no expectation of being able to dump rows I can't
> normally see.

That's true.  But I should also have an expectation that running
pg_dump won't trigger arbitrary code execution, which is why by
default, pg_dump sets row_security to OFF.  That way, if a row
security policy applies, I get an error rather than an incomplete,
possibly-invalid dump (and arbitrary code execution on the server
side).  If I'm OK with doing the dump subject to row security, I can
rerun with --enable-row-security.  But this proposal would force
non-superusers to always use that option, and that's not a good idea.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Feb 9, 2016 at 3:01 PM, Joe Conway  wrote:
> > On 02/09/2016 11:47 AM, Robert Haas wrote:
> >> On Fri, Jan 15, 2016 at 11:53 AM, Stephen Frost  wrote:
>  Whereupon you'd have no certainty that what you got represented a
>  complete dump of your own data.
> >>>
> >>> It would be a dump of what you're allowed to see, rather than an error
> >>> saying you couldn't dump something you couldn't see, which is the
> >>> alternative we're talking about here.  Even if you've got a dependency
> >>> to something-or-other, if you don't have access to it, then you're
> >>> going to get an error.
> >>
> >> I think you're dismissing Tom's concerns far too lightly.  The
> >> row_security=off mode, which is the default, becomes unusable for
> >> non-superusers under this proposal.  That's bad.  And if you switch to
> >> the other mode, then you might accidentally fail to get all of the
> >> data in some table you're trying to back up.  That's bad too: that's
> >> why it isn't the default.  There's a big difference between saying
> >> "I'm OK with not dumping the tables I can't see" and "I'm OK with not
> >> dumping all of the data in some table I *can* see".
> >
> > I don't grok what you're saying here. If I, as a non-superuser could
> > somehow see all the rows in a table just by running pg_dump, including
> > rows that I could not normally see due to RLS policies, *that* would be
> > bad. I should have no expectation of being able to dump rows I can't
> > normally see.
> 
> That's true.  But I should also have an expectation that running
> pg_dump won't trigger arbitrary code execution, which is why by
> default, pg_dump sets row_security to OFF.  That way, if a row
> security policy applies, I get an error rather than an incomplete,
> possibly-invalid dump (and arbitrary code execution on the server
> side).  If I'm OK with doing the dump subject to row security, I can
> rerun with --enable-row-security.  But this proposal would force
> non-superusers to always use that option, and that's not a good idea.

Arbitrary code execution is quite a different concern from the prior
concern regarding incomplete dumps.

To the extent that untrusted code execution is an issue (and my
experience with environments which would deploy RLS tells me that it
isn't a practical concern), an option could be created which would cause
an error to be thrown on non-catalog RLS being run.

When it comes to multi-tenancy environments, as this thread is about,
chances are the only tables you can see are ones which you own or are
owned by a trusted user, which is why I don't view this as a pratical
concern, but I'm not against having a solution to address the issue
raised regarding arbitrary code execution, provided it doesn't create
more problems than it purports to solve.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Dean Rasheed]

On 9 February 2016 at 19:47, Robert Haas  wrote:
> I think you're dismissing Tom's concerns far too lightly.  The
> row_security=off mode, which is the default, becomes unusable for
> non-superusers under this proposal.  That's bad.  And if you switch to
> the other mode, then you might accidentally fail to get all of the
> data in some table you're trying to back up.  That's bad too: that's
> why it isn't the default.  There's a big difference between saying
> "I'm OK with not dumping the tables I can't see" and "I'm OK with not
> dumping all of the data in some table I *can* see".
>
> It seems to me that there's a big difference between policies we ship
> out of the box and policies that are created be users: specifically,
> the former can be assumed benign, while the latter can't.  I think
> that difference matters here, although I'm not sure exactly where to
> go with it.
>

It sounds like there needs to be a separate system_row_security
setting that controls RLS on the system catalogs, and that it should
be on by default in pg_dump.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Robert Haas]

On Tue, Feb 9, 2016 at 3:26 PM, Stephen Frost  wrote:
> Arbitrary code execution is quite a different concern from the prior
> concern regarding incomplete dumps.

I've had both concerns all along, and I think I've mentioned them before.

> To the extent that untrusted code execution is an issue (and my
> experience with environments which would deploy RLS tells me that it
> isn't a practical concern), an option could be created which would cause
> an error to be thrown on non-catalog RLS being run.

There's a major release already in the wild that doesn't behave that
way.  And anyway I think that's missing the point: it's true that
features that are turned off don't cause problems, but features that
are turned on shouldn't break things either.

> When it comes to multi-tenancy environments, as this thread is about,
> chances are the only tables you can see are ones which you own or are
> owned by a trusted user, which is why I don't view this as a pratical
> concern, but I'm not against having a solution to address the issue
> raised regarding arbitrary code execution, provided it doesn't create
> more problems than it purports to solve.

Well, I'm against accepting this patch without such a solution.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Feb 9, 2016 at 3:26 PM, Stephen Frost  wrote:
> > To the extent that untrusted code execution is an issue (and my
> > experience with environments which would deploy RLS tells me that it
> > isn't a practical concern), an option could be created which would cause
> > an error to be thrown on non-catalog RLS being run.
> 
> There's a major release already in the wild that doesn't behave that
> way.

I'm at a loss as to what you're getting at there.  We don't have any
catalog RLS, and when it comes to non-catalog RLS, we do have an option
to throw an error when it's going to be run (and it's the default, as
you pointed out), in the one major version which supports RLS.

> And anyway I think that's missing the point: it's true that
> features that are turned off don't cause problems, but features that
> are turned on shouldn't break things either.

I don't, generally, disagree with that statement, but we have to agree
on what's on vs. off and what is broken vs. working correctly.  See
nearby comments from JD about how non-superuser pg_dump could be seen as
broken when running against an environment where RLS is in use.

> > When it comes to multi-tenancy environments, as this thread is about,
> > chances are the only tables you can see are ones which you own or are
> > owned by a trusted user, which is why I don't view this as a pratical
> > concern, but I'm not against having a solution to address the issue
> > raised regarding arbitrary code execution, provided it doesn't create
> > more problems than it purports to solve.
> 
> Well, I'm against accepting this patch without such a solution.

That's at least something which can be built upon then to help this
progress.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Joshua D. Drake]

On 02/09/2016 12:28 PM, Stephen Frost wrote:

JD,

* Joshua D. Drake (j...@commandprompt.com) wrote:

pg_dump -U $non-super_user

Should just work, period.


That ship has sailed already, where you're running a pg_dump against
objects you don't own and which have RLS enabled on them.


Just to be clear, what I was suggesting is that pg_dump would just work 
(and RLS would properly execute) or in other words, I shouldn't have to 
tell pg_dump to enable row security else throw an error. If RLS is 
enabled, then the backup just runs with appropriate permissions.


Or am I missing something?

Sincerely,

JD




Thanks!

Stephen




--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Joe Conway]

On 02/09/2016 01:22 PM, Tom Lane wrote:
> Maybe we need to restrict that somehow, or maybe some better solution
> exists that we've not thought of yet.  But in its current state, RLS
> is at least as much a security hazard as it is a security aid.
> I do not want to see it extended in ways that make pg_dump unsafe to
> use.

Ok, I can see that. Maybe we should have a specific GRANT for CREATE
POLICY which is distinct from the privilege to CREATE TABLE?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

* Dean Rasheed (dean.a.rash...@gmail.com) wrote:
> On 9 February 2016 at 19:47, Robert Haas  wrote:
> > I think you're dismissing Tom's concerns far too lightly.  The
> > row_security=off mode, which is the default, becomes unusable for
> > non-superusers under this proposal.  That's bad.  And if you switch to
> > the other mode, then you might accidentally fail to get all of the
> > data in some table you're trying to back up.  That's bad too: that's
> > why it isn't the default.  There's a big difference between saying
> > "I'm OK with not dumping the tables I can't see" and "I'm OK with not
> > dumping all of the data in some table I *can* see".
> >
> > It seems to me that there's a big difference between policies we ship
> > out of the box and policies that are created be users: specifically,
> > the former can be assumed benign, while the latter can't.  I think
> > that difference matters here, although I'm not sure exactly where to
> > go with it.
> 
> It sounds like there needs to be a separate system_row_security
> setting that controls RLS on the system catalogs, and that it should
> be on by default in pg_dump.

Right, that's what I had been thinking also.

Thanks (and congrats, btw!),

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Joe Conway]

On 02/09/2016 12:47 PM, Robert Haas wrote:
> On Tue, Feb 9, 2016 at 3:28 PM, Stephen Frost  wrote:
>> JD,
>>
>> * Joshua D. Drake (j...@commandprompt.com) wrote:
>>> pg_dump -U $non-super_user
>>>
>>> Should just work, period.
>>
>> That ship has sailed already, where you're running a pg_dump against
>> objects you don't own and which have RLS enabled on them.
> 
> But you'll get an error rather than an incomplete dump, and you won't
> run some code that you didn't want to run.  Those distinctions matter.

From the perspective of that unprivileged user, the dump is not
incomplete -- it is exactly as complete as it is supposed to be.

Personally I don't buy that the current situation is a good thing. I
know that the "ship has sailed" and regret not having participated in
the earlier discussions, but I agree with JD here -- the unprivileged
user should not have to even think about whether RLS exists, they should
only see what they have been allowed to see by the privileged users (and
in the context of their own objects, owners are privileged). I don't
think an unprivileged user should get to decide what code runs in order
to make that happen.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Robert Haas]

On Tue, Feb 9, 2016 at 4:22 PM, Tom Lane  wrote:
> Part of the problem here is that we have *not* created any hard and fast
> distinction between "privileged" and "unprivileged" users; I think that
> even speaking in those terms about RLS risks errors in your thinking.

+1.

> In particular, the code-execution issue arises from the fact that a table
> owner can now cause code to execute *with the permissions of someone else*
> if the someone else is foolish enough to select from his table.  No
> special privileges required, just the ability to create a table.  If we
> make pg_dump run with RLS enabled, then the "foolish" part doesn't need to
> be any more foolish than forgetting a -t switch when using pg_dump.

Yes.  That is exactly why I argued for the current situation to be the
way it is, and I think it would have been a huge mistake if we now
decided otherwise.  I don't have a ton of confidence that the database
is free of problems that would allow one user to assume the privileges
of another - but I certainly don't want to design more such problems
into the server.

> Maybe we need to restrict that somehow, or maybe some better solution
> exists that we've not thought of yet.  But in its current state, RLS
> is at least as much a security hazard as it is a security aid.
> I do not want to see it extended in ways that make pg_dump unsafe to
> use.

I could not agree more.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Robert Haas]

On Tue, Feb 9, 2016 at 3:28 PM, Stephen Frost  wrote:
> JD,
>
> * Joshua D. Drake (j...@commandprompt.com) wrote:
>> pg_dump -U $non-super_user
>>
>> Should just work, period.
>
> That ship has sailed already, where you're running a pg_dump against
> objects you don't own and which have RLS enabled on them.

But you'll get an error rather than an incomplete dump, and you won't
run some code that you didn't want to run.  Those distinctions matter.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

JD,

* Joshua D. Drake (j...@commandprompt.com) wrote:
> On 02/09/2016 12:28 PM, Stephen Frost wrote:
> >* Joshua D. Drake (j...@commandprompt.com) wrote:
> >>pg_dump -U $non-super_user
> >>
> >>Should just work, period.
> >
> >That ship has sailed already, where you're running a pg_dump against
> >objects you don't own and which have RLS enabled on them.
> 
> Just to be clear, what I was suggesting is that pg_dump would just
> work (and RLS would properly execute) or in other words, I shouldn't
> have to tell pg_dump to enable row security else throw an error. If
> RLS is enabled, then the backup just runs with appropriate
> permissions.
> 
> Or am I missing something?

You do have to tell pg_dump to enable RLS if you want it to be enabled
when performing a pg_dump.  There's multiple reasons for this, the first
being that, otherwise, you might get an incomplete dump, and secondly,
you might execute a function that some untrusted user wrote and included
in their RLS policy.  We want to avoid both of those, unless you've
specifically asked for it to be done.  That's why row_security is set to
'off' by pg_dump by default.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Tom Lane]

Joe Conway  writes:
> Personally I don't buy that the current situation is a good thing. I
> know that the "ship has sailed" and regret not having participated in
> the earlier discussions, but I agree with JD here -- the unprivileged
> user should not have to even think about whether RLS exists, they should
> only see what they have been allowed to see by the privileged users (and
> in the context of their own objects, owners are privileged). I don't
> think an unprivileged user should get to decide what code runs in order
> to make that happen.

Part of the problem here is that we have *not* created any hard and fast
distinction between "privileged" and "unprivileged" users; I think that
even speaking in those terms about RLS risks errors in your thinking.

In particular, the code-execution issue arises from the fact that a table
owner can now cause code to execute *with the permissions of someone else*
if the someone else is foolish enough to select from his table.  No
special privileges required, just the ability to create a table.  If we
make pg_dump run with RLS enabled, then the "foolish" part doesn't need to
be any more foolish than forgetting a -t switch when using pg_dump.

Maybe we need to restrict that somehow, or maybe some better solution
exists that we've not thought of yet.  But in its current state, RLS
is at least as much a security hazard as it is a security aid.
I do not want to see it extended in ways that make pg_dump unsafe to
use.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Joe Conway  writes:
> > Personally I don't buy that the current situation is a good thing. I
> > know that the "ship has sailed" and regret not having participated in
> > the earlier discussions, but I agree with JD here -- the unprivileged
> > user should not have to even think about whether RLS exists, they should
> > only see what they have been allowed to see by the privileged users (and
> > in the context of their own objects, owners are privileged). I don't
> > think an unprivileged user should get to decide what code runs in order
> > to make that happen.
> 
> Part of the problem here is that we have *not* created any hard and fast
> distinction between "privileged" and "unprivileged" users; I think that
> even speaking in those terms about RLS risks errors in your thinking.

I agree that where, exactly, that line is drawn is part of the issue,
and where's it's drawn is really system-dependent.  In many
environments, I view Joe's comments as entirely accurate- only
privileged users are allowed to create objects *at all*.  Of course,
there are a lot of environments where everyone is allowed to create
objects and, in those environments, all those users would be viewed as
"unprivileged", generally speaking.

> In particular, the code-execution issue arises from the fact that a table
> owner can now cause code to execute *with the permissions of someone else*
> if the someone else is foolish enough to select from his table.  No
> special privileges required, just the ability to create a table.  If we
> make pg_dump run with RLS enabled, then the "foolish" part doesn't need to
> be any more foolish than forgetting a -t switch when using pg_dump.

That distinction is really only relevant when it comes to pg_dump, as
those same users could use views to cause their code to be executed by
other users who are selecting from their view, and they could change if
it's a table or a view quite easily.  From a practical standpoint, we're
making a huge distinction between our client tools- pg_dump must be
protected from X, but we don't have any such qualms or concerns
regarding queries sent from psql.

Perhaps that's the right distinction to make, or perhaps we should come
up with a better answer for psql than what we have now, but I don't
agree that RLS is seriously moving the goalposts, overall, here,
particularly since you're not going to have any RLS policies being
executed by pg_dump when run as a superuser anyway, given Noah's change
to how BYPASSRLS works.

> Maybe we need to restrict that somehow, or maybe some better solution
> exists that we've not thought of yet.  But in its current state, RLS
> is at least as much a security hazard as it is a security aid.
> I do not want to see it extended in ways that make pg_dump unsafe to
> use.

I'm not against coming up with an approach which restricts cases where
user A can write code that will be run under another user's rights,
provided it doesn't make the system overly painful to use.  I don't see
RLS as changing the security risks all that much when you're talking
about regular user queries through psql, and the concern regarding
pg_dump has been addressed through the default of row_security being
off.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

* Joe Conway (m...@joeconway.com) wrote:
> On 02/09/2016 01:22 PM, Tom Lane wrote:
> > Maybe we need to restrict that somehow, or maybe some better solution
> > exists that we've not thought of yet.  But in its current state, RLS
> > is at least as much a security hazard as it is a security aid.
> > I do not want to see it extended in ways that make pg_dump unsafe to
> > use.
> 
> Ok, I can see that. Maybe we should have a specific GRANT for CREATE
> POLICY which is distinct from the privilege to CREATE TABLE?

Well, the only privilege we have now is "CREATE", which allows creation
of any kind of object inside a schema.  I'm generally in favor of
providing more granluar 'create table', 'create view', etc privileges
that can be granted out at the schema level, and 'create policy' would
be appropriate to include in such a set of object-creation permissions.

I don't have any particularly genius ideas about where we'd get the bits
to implement such a grant system though.  We could modify the existing
grant system to use larger bits, but given that this would only be
applicable for schemas, perhaps it'd make sense to have another field
in pg_namespace instead?  Not sure, just brainstorming here.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Joe Conway]

On 02/09/2016 11:47 AM, Robert Haas wrote:
> On Fri, Jan 15, 2016 at 11:53 AM, Stephen Frost  wrote:
>>> Whereupon you'd have no certainty that what you got represented a
>>> complete dump of your own data.
>>
>> It would be a dump of what you're allowed to see, rather than an error
>> saying you couldn't dump something you couldn't see, which is the
>> alternative we're talking about here.  Even if you've got a dependency
>> to something-or-other, if you don't have access to it, then you're
>> going to get an error.
> 
> I think you're dismissing Tom's concerns far too lightly.  The
> row_security=off mode, which is the default, becomes unusable for
> non-superusers under this proposal.  That's bad.  And if you switch to
> the other mode, then you might accidentally fail to get all of the
> data in some table you're trying to back up.  That's bad too: that's
> why it isn't the default.  There's a big difference between saying
> "I'm OK with not dumping the tables I can't see" and "I'm OK with not
> dumping all of the data in some table I *can* see".

I don't grok what you're saying here. If I, as a non-superuser could
somehow see all the rows in a table just by running pg_dump, including
rows that I could not normally see due to RLS policies, *that* would be
bad. I should have no expectation of being able to dump rows I can't
normally see.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Alvaro Herrera]

I've closed this as returned-with-feedback.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> Stephen Frost  writes:
> >>> I don't follow how this would destroy the ability to run pg_dump.
> >>> Ideally, we'd have a result where a user could run pg_dump without
> >>> having to apply any filters of their own and they'd get a dump of all
> >>> objects they're allowed to see.
> 
> >> You mean, other than the fact that pg_dump sets row_security = off
> >> to ensure that what it's seeing *isn't* filtered.
> 
> > There's a specific option to turn it back on already though.
> 
> Whereupon you'd have no certainty that what you got represented a
> complete dump of your own data.

It would be a dump of what you're allowed to see, rather than an error
saying you couldn't dump something you couldn't see, which is the
alternative we're talking about here.  Even if you've got a dependency
to something-or-other, if you don't have access to it, then you're
going to get an error.

In practice, you have to make sure to remember to include all of your
schemas when you pg_dump, and don't get it wrong or you'll get an error
(you don't have access to some schema referenced) or a subset of what
you intended (you forgot to include one you meant to).  That is not a
better user experience than being able to say "dump out everything I've
got access to."

In many, many use-cases that's exactly what you want.  pg_dump is more
than just a whole-database backup tool, and when it's used as a
whole-database backup tool, you'll need to make sure it has BYPASSRLS or
is a superuser or you could end up getting errors.  I don't see any
issue with that.

If the policies are incorrect then that'd be a problem, but I'm
certainly hopeful that we'd be able to get that right.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> However, by "not that much trouble" I only mean getting an implementation
> >> that works and doesn't create more security problems than it fixes.
> >> Usability is still likely to be a huge problem.  In particular it seems
> >> likely that any attempt to actually put RLS policies on the catalogs would
> >> completely destroy the ability to run pg_dump except as a BYPASSRLS role.
> >> That would be an unpleasant consequence.
> 
> > I don't follow how this would destroy the ability to run pg_dump.
> > Ideally, we'd have a result where a user could run pg_dump without
> > having to apply any filters of their own and they'd get a dump of all
> > objects they're allowed to see.
> 
> You mean, other than the fact that pg_dump sets row_security = off
> to ensure that what it's seeing *isn't* filtered.

There's a specific option to turn it back on already though.  This
wouldn't change that.

> The bigger picture here is that I do not think that you can just
> arbitrarily exclude non-owned objects from its view and still expect to
> get a valid dump; that will break dependency chains for example, possibly
> leading to stuff getting output in an order that doesn't restore.

We already have that issue when users select to dump out specific
schemas, I don't see this as being any different.  The idea behind
multi-tenancy is, generally speaking, you don't see or have any
references or dependencies with what other people have.  In those cases,
there won't be any dependencies to objects that you can't see.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Joe Conway  writes:
> > As Stephen mentioned, yes, I am very interested in at least some aspects
> > of this patch. The ability to apply RLS to system tables could be useful
> > to solve a number of problems we don't have a good story for today,
> > multi-tenancy only being one of them.
> 
> FWIW, it seems offhand like we might not have that much trouble with
> applying RLS to system catalogs as long as it's understood that RLS
> only has anything to do with SQL queries issued against the catalogs.

Right, that's what this patch set is about.

> If we imagine that RLS should somehow filter a backend's own operations on
> the catalogs, then I agree with Robert that the entire thing is deeply
> scary and probably incapable of being made to work robustly.

Personally, I like the idea of the capability, but I also agree that
it'd be a great deal more challenging to do and would require a lot of
pretty invasive and scary changes.  Hence, my thinking was that we'd
define our own set of policies which mimic what we already do through
the permissions system (thus, only impacting SQL queries against the
catalog and not anything about how the backend accesses the catalogs).

I'm on the fence about if we'd allow those policies to be modified by
users or not.

> However, by "not that much trouble" I only mean getting an implementation
> that works and doesn't create more security problems than it fixes.
> Usability is still likely to be a huge problem.  In particular it seems
> likely that any attempt to actually put RLS policies on the catalogs would
> completely destroy the ability to run pg_dump except as a BYPASSRLS role.
> That would be an unpleasant consequence.

I don't follow how this would destroy the ability to run pg_dump.
Ideally, we'd have a result where a user could run pg_dump without
having to apply any filters of their own and they'd get a dump of all
objects they're allowed to see.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> However, by "not that much trouble" I only mean getting an implementation
>> that works and doesn't create more security problems than it fixes.
>> Usability is still likely to be a huge problem.  In particular it seems
>> likely that any attempt to actually put RLS policies on the catalogs would
>> completely destroy the ability to run pg_dump except as a BYPASSRLS role.
>> That would be an unpleasant consequence.

> I don't follow how this would destroy the ability to run pg_dump.
> Ideally, we'd have a result where a user could run pg_dump without
> having to apply any filters of their own and they'd get a dump of all
> objects they're allowed to see.

You mean, other than the fact that pg_dump sets row_security = off
to ensure that what it's seeing *isn't* filtered.

The bigger picture here is that I do not think that you can just
arbitrarily exclude non-owned objects from its view and still expect to
get a valid dump; that will break dependency chains for example, possibly
leading to stuff getting output in an order that doesn't restore.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Stephen Frost  writes:
>>> I don't follow how this would destroy the ability to run pg_dump.
>>> Ideally, we'd have a result where a user could run pg_dump without
>>> having to apply any filters of their own and they'd get a dump of all
>>> objects they're allowed to see.

>> You mean, other than the fact that pg_dump sets row_security = off
>> to ensure that what it's seeing *isn't* filtered.

> There's a specific option to turn it back on already though.

Whereupon you'd have no certainty that what you got represented a
complete dump of your own data.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Tom Lane]

Joe Conway  writes:
> On 01/06/2016 12:15 PM, Robert Haas wrote:
>> Is any committer thinking about taking a serious look at this patch series?
>> 
>> I ask because (1) it seems like it could be nice to have but (2) it
>> frightens me terribly.  We are generally very sparing about assuming
>> that "stuff" (triggers, partial indexes, etc.) that works for user
>> tables can also be made to work for system tables.  I haven't thought
>> deeply about what might go wrong in this particular case, but it
>> strikes me that if Haribabu Kommi is building something that is doomed
>> for some reason, it would be good to figure that out before he spends
>> any more time on it than he already has.

> As Stephen mentioned, yes, I am very interested in at least some aspects
> of this patch. The ability to apply RLS to system tables could be useful
> to solve a number of problems we don't have a good story for today,
> multi-tenancy only being one of them.

FWIW, it seems offhand like we might not have that much trouble with
applying RLS to system catalogs as long as it's understood that RLS
only has anything to do with SQL queries issued against the catalogs.

If we imagine that RLS should somehow filter a backend's own operations on
the catalogs, then I agree with Robert that the entire thing is deeply
scary and probably incapable of being made to work robustly.

However, by "not that much trouble" I only mean getting an implementation
that works and doesn't create more security problems than it fixes.
Usability is still likely to be a huge problem.  In particular it seems
likely that any attempt to actually put RLS policies on the catalogs would
completely destroy the ability to run pg_dump except as a BYPASSRLS role.
That would be an unpleasant consequence.

I've not read the patch set, so maybe it contains some ideas about how
to mitigate the usability issues, in which case never mind ...

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Joe Conway]

On 01/06/2016 12:15 PM, Robert Haas wrote:
> On Tue, Jan 5, 2016 at 11:07 PM, Haribabu Kommi
>  wrote:
>> May be you missed to apply the 3_shared_catalog_tenancy_v4 path,
>> because 4_database_catalog_tenancy_v5 patch depends on it.
>>
>> Here I attached all the patches for your convenience, I am able to
>> apply all patches in the order without any problem.
> 
> Is any committer thinking about taking a serious look at this patch series?
> 
> I ask because (1) it seems like it could be nice to have but (2) it
> frightens me terribly.  We are generally very sparing about assuming
> that "stuff" (triggers, partial indexes, etc.) that works for user
> tables can also be made to work for system tables.  I haven't thought
> deeply about what might go wrong in this particular case, but it
> strikes me that if Haribabu Kommi is building something that is doomed
> for some reason, it would be good to figure that out before he spends
> any more time on it than he already has.

As Stephen mentioned, yes, I am very interested in at least some aspects
of this patch. The ability to apply RLS to system tables could be useful
to solve a number of problems we don't have a good story for today,
multi-tenancy only being one of them.

> Apart from the issue of whether this is doomed for some architectural
> reason, it is not entirely clear to me that there's any consensus that
> we want this.  I don't think that I understand the issues here well
> enough to proffer an opinion of my own just yet... but I'd like to
> hear what other people think.

As said above, I definitely see a need for something like this if not
this specifically.

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Jan 5, 2016 at 11:07 PM, Haribabu Kommi
>  wrote:
> > May be you missed to apply the 3_shared_catalog_tenancy_v4 path,
> > because 4_database_catalog_tenancy_v5 patch depends on it.
> >
> > Here I attached all the patches for your convenience, I am able to
> > apply all patches in the order without any problem.
> 
> Is any committer thinking about taking a serious look at this patch series?

Joe took a look at it earlier and I'm definitely interested in it.
Furhter, he and I have chatted about it a few times.

Reference here to comments from Joe:
http://www.postgresql.org/message-id/55f1fb2e.8020...@joeconway.com

> I ask because (1) it seems like it could be nice to have but (2) it
> frightens me terribly.  We are generally very sparing about assuming
> that "stuff" (triggers, partial indexes, etc.) that works for user
> tables can also be made to work for system tables.  I haven't thought
> deeply about what might go wrong in this particular case, but it
> strikes me that if Haribabu Kommi is building something that is doomed
> for some reason, it would be good to figure that out before he spends
> any more time on it than he already has.

I'm not aware of any particular reason for it to be doomed out of the
gate.  That isn't to say that we won't find an issue with it or that
I've given it an in depth look (I've not), but the concept seems
reasonable enough and I can't think of any off-hand reasons why it
won't work.

I will note that there's a couple different patches involved here and
they really deserve their own review and consideration.

> Apart from the issue of whether this is doomed for some architectural
> reason, it is not entirely clear to me that there's any consensus that
> we want this.  I don't think that I understand the issues here well
> enough to proffer an opinion of my own just yet... but I'd like to
> hear what other people think.

I'm certainly of the opinion that we want this or something similar.

The big caveat kicking around in my head is if we want to have our own
set of defined policies or if we want to give flexibility to the
administrator to define their own policies.  In particular, I'm
wondering about things like:

CREATE POLICY namespace_limit ON pg_namespace TO company1 USING
  (substring(nspname,1,8) = 'company1_');

Which is a bit different, as I understand it, from what Haribadu has
been proposing and quite a bit more complicated, as we'd then have to
make the internal lookups respect the policy (so things like CREATE
SCHEMA would have to check if you're allowed to actually create that
schema, which would be based on the policy...).

I don't know if we want to try and support that level of flexibility
or if we'd like to simply go based on the concept of 'you only get to
see what you have access to', which I'm thinking will allow us to avoid
changing the existing functions as they are already doing permissions
checks.

My general thinking here is that we could support one set of provided
policies via these patches and then, if there is sufficient interest,
generalize how policies on system catalogs are handled and eventually
allow users to redefine the system catalog policies.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Amit Langote]

On 2016/01/06 13:07, Haribabu Kommi wrote:
> On Wed, Jan 6, 2016 at 1:43 PM, Amit Langote
>>
>> Patch 4_database_catalog_tenancy_v5 fails to apply:
> 
> May be you missed to apply the 3_shared_catalog_tenancy_v4 path,
> because 4_database_catalog_tenancy_v5 patch depends on it.

Oops, I even missed patches 1 and 2 at all.

> 
> Here I attached all the patches for your convenience, I am able to
> apply all patches in the order without any problem.

Okay, thanks!

I applied all the patches. I have a basic question. Sorry though if I've
entirely missed the point (and/or scope) of your proposal. I wonder if
something like the following should not have failed with the patch:

postgres=# CREATE POLICY class_policy ON pg_class TO PUBLIC USING
(relowner = current_user);
ERROR:  permission denied: "pg_class" is a system catalog

Is there no support yet for user-defined catalog policies?

Regards,
Amit




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Thu, Jan 7, 2016 at 2:29 PM, Stephen Frost  wrote:
> Robert,
>
> * Robert Haas (robertmh...@gmail.com) wrote:
>
>> Apart from the issue of whether this is doomed for some architectural
>> reason, it is not entirely clear to me that there's any consensus that
>> we want this.  I don't think that I understand the issues here well
>> enough to proffer an opinion of my own just yet... but I'd like to
>> hear what other people think.
>
> I'm certainly of the opinion that we want this or something similar.
>
> The big caveat kicking around in my head is if we want to have our own
> set of defined policies or if we want to give flexibility to the
> administrator to define their own policies.  In particular, I'm
> wondering about things like:
>
> CREATE POLICY namespace_limit ON pg_namespace TO company1 USING
>   (substring(nspname,1,8) = 'company1_');
>
> Which is a bit different, as I understand it, from what Haribadu has
> been proposing and quite a bit more complicated, as we'd then have to
> make the internal lookups respect the policy (so things like CREATE
> SCHEMA would have to check if you're allowed to actually create that
> schema, which would be based on the policy...).

I feel we may needed both our own set of policies and also providing
the user to create/alter/drop the catalog policies. This way we can
support both simple and complex scenarios. With default policies
an user can setup multi-tenancy easily. With the help of edit option,
user can tune the policies according to their scenarios.

The one problem with either approach as i am thinking, currently with
our own set of policies, the objects entries that are present on the
catalog tables are visible to the users, those are having any kind of
privileges on those objects. In case if a user tries to create an object
that is already present in the catalog relation will produce an error, but
user cannot view that object because of permissions problem.

To avoid such problem, administrator has to add policies such as
"namespace_prefix" needs to be added to all catalog tables.

Regards,
Hari Babu
Fujitsu Australia


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Robert Haas]

On Tue, Jan 5, 2016 at 11:07 PM, Haribabu Kommi
 wrote:
> May be you missed to apply the 3_shared_catalog_tenancy_v4 path,
> because 4_database_catalog_tenancy_v5 patch depends on it.
>
> Here I attached all the patches for your convenience, I am able to
> apply all patches in the order without any problem.

Is any committer thinking about taking a serious look at this patch series?

I ask because (1) it seems like it could be nice to have but (2) it
frightens me terribly.  We are generally very sparing about assuming
that "stuff" (triggers, partial indexes, etc.) that works for user
tables can also be made to work for system tables.  I haven't thought
deeply about what might go wrong in this particular case, but it
strikes me that if Haribabu Kommi is building something that is doomed
for some reason, it would be good to figure that out before he spends
any more time on it than he already has.

Apart from the issue of whether this is doomed for some architectural
reason, it is not entirely clear to me that there's any consensus that
we want this.  I don't think that I understand the issues here well
enough to proffer an opinion of my own just yet... but I'd like to
hear what other people think.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Amit Langote]

On 2016/01/06 10:17, Haribabu Kommi wrote:
> On Mon, Jan 4, 2016 at 10:43 PM, Haribabu Kommi
>>
>> Thanks for the test. Yes, the issue happens at backend startup itself.
>> I will give a try by separating the initialization of security
>> policies after init phase 3.
> 
> Here I attached updated patches with the fix of infinite recursion in
> RelationBuildRowSecurity function by checking with a variable that
> whether the build row security is already in progress for a system
> relation or not. If it is already in progress for a relation, then it doesn't
> build the row security description for this relation.

Thanks for updating the patch.

Patch 4_database_catalog_tenancy_v5 fails to apply:

patching file src/backend/commands/policy.c
Hunk #3 succeeded at 112 with fuzz 2 (offset 3 lines).
Hunk #4 succeeded at 269 with fuzz 1 (offset 13 lines).
Hunk #5 succeeded at 298 (offset 13 lines).
Hunk #6 succeeded at 365 (offset 12 lines).
Hunk #7 FAILED at 466.
Hunk #8 succeeded at 577 (offset 22 lines).
Hunk #9 succeeded at 607 with fuzz 2 (offset 22 lines).
Hunk #10 succeeded at 633 with fuzz 2 (offset 22 lines).
Hunk #11 FAILED at 801.
Hunk #12 FAILED at 813.
3 out of 12 hunks FAILED -- saving rejects to file
src/backend/commands/policy.c.rej

Thanks,
Amit





-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Mon, Jan 4, 2016 at 8:34 PM, Amit Langote
 wrote:
> On 2016/01/04 14:43, Haribabu Kommi wrote:
>>>
>>> Here I attached new series of patches with a slightly different approach.
>>> Instead of creating the policies on the system catalog tables whenever
>>> the catalog security command is executed, just enable row level security
>>> on the system catalog tables. During the relation build, in
>>> RelationBuildRowSecurity function, if it is a system relation, frame the
>>> policy using the policy query which we earlier used to create by parsing it.
>>>
>>> With the above approach, in case of any problems in the policy, to use
>>> the corrected policy, user just needs to replace the binaries. whereas in
>>> earlier approach, either pg_upgrade or disabling and enabling of catalog
>>> security is required.
>>>
>>> Currently it is changed only for shared system catalog tables and also the
>>> way of enabling catalog security on shared system catalog tables is through
>>> initdb only. This also can be changed later. I will do similar changes for
>>> remaining catalog tables.
>>>
>>> Any comments on the approach?
>>
>> Instead of creating policies during the "alter database" command for database
>> catalog tables, generating at relation building is leading to an
>> infinite recursion
>> loop because of transformExpr call for the qual. Any ideas to handle the 
>> same?
>
> I tried your latest patch to see what may have caused the infinite
> recursion. The recursion occurs during backend startup itself, right?
>
> ISTM, doing transformWhereClause during RelationCacheInitializePhase3()
> would not work. Things like operators, functions within the policy qual
> require namespace lookup which down the line would call
> RelationBuildRowSecurity for pg_namespace build and so on thus causing the
> infinite recursion. Perhaps, it would have to be done in a separate phase
> after the phase 3 but I'm not sure.

Thanks for the test. Yes, the issue happens at backend startup itself.
I will give a try by separating the initialization of security
policies after init phase 3.

> I wonder why do the policy quals need to be built from scratch every time
> in RelationBuildRowSecurity for system tables (shared or otherwise)? Why
> not just read from the pg_policy catalog like regular relations if ALTER
> DATABASE CATALOG SECURITY TRUE already created those entries? Maybe I
> missing something though.

Yes, creating policies at start and using them every time when
relation is building works
until there is no problem is found in the policies. The row level
security policies on catalog
tables are created automatically when user enables catalog security,
so user don't have
any control on these policies.

In case if we found any problem in these policies, later if we want to
correct them, for
shared system catalog tables policies user needs to do a pg_upgrade to
correct them.
And for the other catalog tables, user needs to disable and enable the
catalog security
on all databases.

Instead of the above, if we built the policy at run time always for
catalog tables, user
can just replace binaries with latest works. Currently it is working
fine for shared system
catalog tables. I will give a try by separating
RelationBuildRowSecurity from init phase 3.

Regards,
Hari Babu
Fujitsu Australia


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Amit Langote]

On 2016/01/04 14:43, Haribabu Kommi wrote:
>>
>> Here I attached new series of patches with a slightly different approach.
>> Instead of creating the policies on the system catalog tables whenever
>> the catalog security command is executed, just enable row level security
>> on the system catalog tables. During the relation build, in
>> RelationBuildRowSecurity function, if it is a system relation, frame the
>> policy using the policy query which we earlier used to create by parsing it.
>>
>> With the above approach, in case of any problems in the policy, to use
>> the corrected policy, user just needs to replace the binaries. whereas in
>> earlier approach, either pg_upgrade or disabling and enabling of catalog
>> security is required.
>>
>> Currently it is changed only for shared system catalog tables and also the
>> way of enabling catalog security on shared system catalog tables is through
>> initdb only. This also can be changed later. I will do similar changes for
>> remaining catalog tables.
>>
>> Any comments on the approach?
> 
> Instead of creating policies during the "alter database" command for database
> catalog tables, generating at relation building is leading to an
> infinite recursion
> loop because of transformExpr call for the qual. Any ideas to handle the same?

I tried your latest patch to see what may have caused the infinite
recursion. The recursion occurs during backend startup itself, right?

ISTM, doing transformWhereClause during RelationCacheInitializePhase3()
would not work. Things like operators, functions within the policy qual
require namespace lookup which down the line would call
RelationBuildRowSecurity for pg_namespace build and so on thus causing the
infinite recursion. Perhaps, it would have to be done in a separate phase
after the phase 3 but I'm not sure.

I wonder why do the policy quals need to be built from scratch every time
in RelationBuildRowSecurity for system tables (shared or otherwise)? Why
not just read from the pg_policy catalog like regular relations if ALTER
DATABASE CATALOG SECURITY TRUE already created those entries? Maybe I
missing something though.

Thanks,
Amit




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Thu, Dec 17, 2015 at 12:46 PM, Haribabu Kommi
 wrote:
> Rebased patch is attached as it is having an OID conflict with the
> latest set of changes
> in the master branch.

Here I attached new series of patches with a slightly different approach.
Instead of creating the policies on the system catalog tables whenever
the catalog security command is executed, just enable row level security
on the system catalog tables. During the relation build, in
RelationBuildRowSecurity function, if it is a system relation, frame the
policy using the policy query which we earlier used to create by parsing it.

With the above approach, in case of any problems in the policy, to use
the corrected policy, user just needs to replace the binaries. whereas in
earlier approach, either pg_upgrade or disabling and enabling of catalog
security is required.

Currently it is changed only for shared system catalog tables and also the
way of enabling catalog security on shared system catalog tables is through
initdb only. This also can be changed later. I will do similar changes for
remaining catalog tables.

Any comments on the approach?

Regards,
Hari Babu
Fujitsu Australia


3_shared_catalog_tenancy_v2.patch
Description: Binary data


1_any_privilege_option_v2.patch
Description: Binary data


2_view_security_definer_v2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

Rebased patch is attached as it is having an OID conflict with the
latest set of changes
in the master branch.

Regards,
Hari Babu
Fujitsu Australia


4_database_catalog_tenancy_v3.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Wed, Oct 21, 2015 at 2:42 PM, Haribabu Kommi
 wrote:
> Pending items:
> 1. Need to add some more tests to verify all database catalog tables.
> 2. Documentation changes for database catalog tenancy.

Here I attached the updated database-catalog-security with more tests
including system views,
information schema views and documentation.

>Known issues:
>2. If user (U2) executes a query on an object (tbl2) which the user
>(U2) don't have
>permissions, as he cannot able to see that object from catalog 
> views/tables,
>but the query returns an error message as "permission denied", but in case
>if multi-tenancy is enabled, the error message should be "relation
>doesn't exist".

To handle the above problem, we can add a check to verify whether the
corresponding
catalog relation has the row level security is enabled or not? in all
*_aclmask or similar
functions. Based on the ACL result, if the row security is enabled,
through an error as
"object does not exist", instead of permission denied by the
aclcheck_error function.
This will increase the extra processing time for queries irrespective
of whether the
multi-tenancy is enabled or not?

comments?

Regards,
Hari Babu
Fujitsu Australia


4_database_catalog_tenancy_v2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Sat, Oct 10, 2015 at 1:54 AM, Stephen Frost  wrote:
> * Haribabu Kommi (kommi.harib...@gmail.com) wrote:
>> On Fri, Oct 9, 2015 at 2:04 PM, Stephen Frost  wrote:
>> > * Robert Haas (robertmh...@gmail.com) wrote:
>> >> We've got one reloption for views already - security_barrier.  Maybe
>> >> we could have another one that effectively changes a particular view
>> >> from "security definer" as it is today to "security invoker".
>> >
>> > As I recall, there was a previous suggestion (honestly, I thought it was
>> > your idea) to have a reloption which made views "fully" security
>> > definer, in that functions in the view definition would run as the view
>> > owner instead of the view invoker.
>> >
>> > I liked that idea, though we would need to have a function to say "who
>> > is the 'outer' user?" (CURRENT_USER always being the owner with the
>> > above described reloption).
>> >
>> > I'm less sure about the idea of having a view which runs entirely as the
>> > view invoker, but I'm not against it either.
>>
>> I changed in function check_enable_rls to use the invoker id instead of 
>> owner id
>> for all the system objects, the catalog table policies are getting
>> applied and it is
>> working fine till now in my multi-tenancy testing.
>>
>> Currently I am writing tests to validate it against all user objects also.
>> If this change works for all user objects also, then we may not needed
>> the security invoker
>> reloption.
>
> The reloption would be to allow the user to decide which behavior they
> wanted, as there are use-cases for both.


Any_privilege_option:
Patch that adds 'any' type as a privilege option to verify whether the user
is having any privileges on the object, instead of specifying each and every
privilege type that object supports. Using of this option at grant and revoke
commands throw an error.

View_security_definer:
Patch that adds "security_definer" as a view option to specify whether the
view owner needs to be used for all operations on the view, otherwise the
current user is used.

Currently by default the view owner is used to check against all privileges,
so changing it as invoker instead of owner leads to backward compatibility
problems as permission denied on the base relation and etc. To minimize
the impact, currently the invoker id is used only when the view is rewritten
to base relation for 1) updatable views 2) while applying the row security
policies to the base relations.

Instead of the above change, if we treat all the views by default as security
definer, then to support multi-tenancy we need to change all the system views
as security_definer=false.

comments?

shared_catalog_tenancy:
Patch adds an initdb option -C or --shared-catalog-security to add row level
security policies on shared catalog tables that are eligible for tenancy.
With this option, user gets the tenancy at database level, means user can
get the database list that he has some privileges, but not all. It is
not possible
to disable the shared catalog security once it is set at initdb time.


database_catalog_tenancy:
Patch that adds an database option of "catalog security". This can be used
with alter database only not possible with create database command.
With this option, user gets the tenancy at table level. Once user enables
the catalog security at database level, row level security policies are created
on catalog tables that are eligible. User can disable catalog security if wants.

Known issues:
1. If user (U1) grants permissions on object (tbl1) to user (U2), the user U2
can get the information that there exists an user (U1) in the system, but
U2 cannot get the details of U1.

2. If user (U2) executes a query on an object (tbl2) which the user
(U2) don't have
permissions, as he cannot able to see that object from catalog views/tables,
but the query returns an error message as "permission denied", but in case
if multi-tenancy is enabled, the error message should be "relation
doesn't exist".

Pending items:
1. Need to add some more tests to verify all database catalog tables.
2. Documentation changes for database catalog tenancy.

Regards,
Hari Babu
Fujitsu Australia


1_any_privilege_option_v1.patch
Description: Binary data


2_view_security_definer_v1.patch
Description: Binary data


3_shared_catalog_tenancy_v1.patch
Description: Binary data


4_database_catalog_tenancy_v1.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Joe Conway]

On 10/08/2015 11:04 PM, Stephen Frost wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> We've got one reloption for views already - security_barrier.  Maybe
>> we could have another one that effectively changes a particular view
>> from "security definer" as it is today to "security invoker".
> 
> As I recall, there was a previous suggestion (honestly, I thought it was
> your idea) to have a reloption which made views "fully" security
> definer, in that functions in the view definition would run as the view
> owner instead of the view invoker.

I'd love to see a way for views to behave in an entirely view definer or
entirely view invoker way. The current mixed mode is bad/confusing IMHO,
although I guess we need some backward compatibility mode?

> I liked that idea, though we would need to have a function to say "who
> is the 'outer' user?" (CURRENT_USER always being the owner with the
> above described reloption).
> 
> I'm less sure about the idea of having a view which runs entirely as the
> view invoker, but I'm not against it either.
> 
> I do think both of those are independent of supporting policies for
> views and foreign tables though, which we'd want even if we had
> reloptions for the above ideas.

Agreed

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

* Haribabu Kommi (kommi.harib...@gmail.com) wrote:
> On Fri, Oct 9, 2015 at 2:04 PM, Stephen Frost  wrote:
> > * Robert Haas (robertmh...@gmail.com) wrote:
> >> We've got one reloption for views already - security_barrier.  Maybe
> >> we could have another one that effectively changes a particular view
> >> from "security definer" as it is today to "security invoker".
> >
> > As I recall, there was a previous suggestion (honestly, I thought it was
> > your idea) to have a reloption which made views "fully" security
> > definer, in that functions in the view definition would run as the view
> > owner instead of the view invoker.
> >
> > I liked that idea, though we would need to have a function to say "who
> > is the 'outer' user?" (CURRENT_USER always being the owner with the
> > above described reloption).
> >
> > I'm less sure about the idea of having a view which runs entirely as the
> > view invoker, but I'm not against it either.
> 
> I changed in function check_enable_rls to use the invoker id instead of owner 
> id
> for all the system objects, the catalog table policies are getting
> applied and it is
> working fine till now in my multi-tenancy testing.
> 
> Currently I am writing tests to validate it against all user objects also.
> If this change works for all user objects also, then we may not needed
> the security invoker
> reloption.

The reloption would be to allow the user to decide which behavior they
wanted, as there are use-cases for both.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Robert Haas]

On Tue, Oct 6, 2015 at 7:29 AM, Stephen Frost  wrote:
> * Haribabu Kommi (kommi.harib...@gmail.com) wrote:
>> On Tue, Oct 6, 2015 at 10:56 AM, Haribabu Kommi
>>  wrote:
>> > Here I attached an updated version of the patch with the following changes.
>>
>> I found some problems related to providing multi-tenancy on a system
>> catalog view.
>> This is because, system catalog view uses the owner that is created
>> the user instead
>> of the current user by storing the user information in "checkAsUser"
>> field in RangeTblEntry
>> structure.
>
> Right, when querying through a view to tables underneath, we use the
> permissions of the view owner.  View creators should be generally aware
> of this already.
>
> I agree that it adds complications to the multi-tenancy idea since the
> system views, today, allow viewing of all objects.  There are two ways
> to address that:
>
> Modify the system catalog views to include the same constraints that the
> policies on the tables do
>
> or
>
> Allow RLS policies against views and then create the necessary policies
> on the views in the catalog.
>
> My inclination is to work towards the latter as that's a capability we'd
> like to have anyway.

We've got one reloption for views already - security_barrier.  Maybe
we could have another one that effectively changes a particular view
from "security definer" as it is today to "security invoker".

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> We've got one reloption for views already - security_barrier.  Maybe
> we could have another one that effectively changes a particular view
> from "security definer" as it is today to "security invoker".

As I recall, there was a previous suggestion (honestly, I thought it was
your idea) to have a reloption which made views "fully" security
definer, in that functions in the view definition would run as the view
owner instead of the view invoker.

I liked that idea, though we would need to have a function to say "who
is the 'outer' user?" (CURRENT_USER always being the owner with the
above described reloption).

I'm less sure about the idea of having a view which runs entirely as the
view invoker, but I'm not against it either.

I do think both of those are independent of supporting policies for
views and foreign tables though, which we'd want even if we had
reloptions for the above ideas.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Fri, Oct 9, 2015 at 2:04 PM, Stephen Frost  wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> We've got one reloption for views already - security_barrier.  Maybe
>> we could have another one that effectively changes a particular view
>> from "security definer" as it is today to "security invoker".
>
> As I recall, there was a previous suggestion (honestly, I thought it was
> your idea) to have a reloption which made views "fully" security
> definer, in that functions in the view definition would run as the view
> owner instead of the view invoker.
>
> I liked that idea, though we would need to have a function to say "who
> is the 'outer' user?" (CURRENT_USER always being the owner with the
> above described reloption).
>
> I'm less sure about the idea of having a view which runs entirely as the
> view invoker, but I'm not against it either.

I changed in function check_enable_rls to use the invoker id instead of owner id
for all the system objects, the catalog table policies are getting
applied and it is
working fine till now in my multi-tenancy testing.

Currently I am writing tests to validate it against all user objects also.
If this change works for all user objects also, then we may not needed
the security invoker
reloption.

Regards,
Hari Babu
Fujitsu Australia


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Tue, Oct 6, 2015 at 10:56 AM, Haribabu Kommi
 wrote:
> Here I attached an updated version of the patch with the following changes.

I found some problems related to providing multi-tenancy on a system
catalog view.
This is because, system catalog view uses the owner that is created
the user instead
of the current user by storing the user information in "checkAsUser"
field in RangeTblEntry
structure.

The same "checkAsUser" is used in check_enable_rls function before
getting the policies for the table. All the system catalog views are
created by the super user, so no row level security policies
are applied to the views.

Ex-
SET SESSION ROLE tenancy_user1;

select relname from pg_class where relname = 'tenancy_user2_tbl1';
 relname
-
(0 rows)

select schemaname, relname from pg_stat_all_tables where relname =
'tenancy_user2_tbl1';
 schemaname |  relname
+
 public | tenancy_user2_tbl1
(1 row)

The policy that is created on pg_class system catalog table is, get
all the objects that current
user have permissions. Permissions can be anything.

To fix the problem, I thought of using current session id instead of
"checkAsUser" while applying
row level security policies to system catalog objects. This doesn't
affect the normal objects. But this solution has given some problems
for foreign_data.sql while running the regress tests as the planner is
generating targetlist as NULL.

Is the above specified solution is the correct approach to handle this
problem? If it is i will check the foreign_data.sql problem, otherwise
is there any good approach to handle the same?


Regards,
Hari Babu
Fujitsu Australia


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Tue, Oct 6, 2015 at 10:29 PM, Stephen Frost  wrote:
> * Haribabu Kommi (kommi.harib...@gmail.com) wrote:
>> On Tue, Oct 6, 2015 at 10:56 AM, Haribabu Kommi
>>  wrote:
>> > Here I attached an updated version of the patch with the following changes.
>>
>> I found some problems related to providing multi-tenancy on a system
>> catalog view.
>> This is because, system catalog view uses the owner that is created
>> the user instead
>> of the current user by storing the user information in "checkAsUser"
>> field in RangeTblEntry
>> structure.
>
> Right, when querying through a view to tables underneath, we use the
> permissions of the view owner.  View creators should be generally aware
> of this already.
>
> I agree that it adds complications to the multi-tenancy idea since the
> system views, today, allow viewing of all objects.  There are two ways
> to address that:
>
> Modify the system catalog views to include the same constraints that the
> policies on the tables do
>
> or
>
> Allow RLS policies against views and then create the necessary policies
> on the views in the catalog.
>
> My inclination is to work towards the latter as that's a capability we'd
> like to have anyway.

Thanks for the solutions to handle the problem.

Currently I thought of providing two multi-tenancy solutions to the user.
They are:

1. Tenancy at shared system catalog tables level
2. Tenancy at database system catalog tables.

User can create views on system catalog tables, even though I want to provide
tenancy on those views also. I will do further analysis and provide
details of which
solution gives the benefit of two tenancy levels and then I can proceed for
implementation after discussion.

Regards,
Hari Babu
Fujitsu Australia


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

* Haribabu Kommi (kommi.harib...@gmail.com) wrote:
> On Tue, Oct 6, 2015 at 10:56 AM, Haribabu Kommi
>  wrote:
> > Here I attached an updated version of the patch with the following changes.
> 
> I found some problems related to providing multi-tenancy on a system
> catalog view.
> This is because, system catalog view uses the owner that is created
> the user instead
> of the current user by storing the user information in "checkAsUser"
> field in RangeTblEntry
> structure.

Right, when querying through a view to tables underneath, we use the
permissions of the view owner.  View creators should be generally aware
of this already.

I agree that it adds complications to the multi-tenancy idea since the
system views, today, allow viewing of all objects.  There are two ways
to address that:

Modify the system catalog views to include the same constraints that the
policies on the tables do

or

Allow RLS policies against views and then create the necessary policies
on the views in the catalog.

My inclination is to work towards the latter as that's a capability we'd
like to have anyway.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Fri, Sep 11, 2015 at 7:50 AM, Joe Conway  wrote:
> On 09/01/2015 11:25 PM, Haribabu Kommi wrote:
>> If any user is granted any permissions on that object then that user
>> can view it's meta data of that object from the catalog tables.
>> To check the permissions of the user on the object, instead of
>> checking each and every available option, I just added a new
>> privilege check option called "any". If user have any permissions on
>> the object, the corresponding permission check function returns
>> true. Patch attached for the same.
>>
>> Any thoughts/comments?
>
> Thanks for working on this! Overall I like the concept and know of use
> cases where it is critical and should be supported. Some comments:

Here I attached an updated version of the patch with the following changes.

Two options to the user to create catalog security on system catalog tables.

./initdb -C -D data

With the above option during initdb, the catalog security is enabled
on all shared system catalog
tables. With this way the user can achieve the catalog security at
database level. For some users
this may be enough. Currently enabling catalog security is supported
only at initdb.

ALTER DATABASE  WITH CATALOG SECURITY=true;
ALTER DATABASE  WITH CATALOG SECURITY=false;

With the above commands, user can enable/disable catalog security on a
database system catalog
tables if multi-tenancy requires at table level.

Currently setting catalog security at create database command is not
supported. And also with
alter database command to enable/disable to database where the backend
is not connected.
This is because of a restriction to execute the policy commands
without connecting to a database.


Pending things needs to be taken care:

1. select * from tenancy_user1_tbl1;
ERROR:  permission denied for relation tenancy_user1_tbl1

As we are not able to see the above user table in any catalog relation
because of the multi-tenancy policies,
but if user tries to select the data of the table directly, The error
message comes as permission denied, I feel
instead of the permission denied error, in case of multi-tenancy is
enabled, the error message should be
"relation doesn't exist".

2. Correct all catalog relation policies
3. Add regression tests for all system catalog relations and views.
4. Documentation changes

Any comments?

Regards,
Hari Babu
Fujitsu Australia


multi-tenancy_with_rls_poc_3.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Robert Haas]

On Tue, Sep 15, 2015 at 9:18 AM, Jim Nasby  wrote:
> Also, we've faced issues in the past with making catalog changes due to fear
> of breaking user scripts. Instead of doubling down on that with RLS on top
> of catalog tables, would it be better to move the tables to a different
> schema, make them accessible only to superusers and put views in pg_catalog?

Uggh.  -1 on that option from me.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Tue, Sep 15, 2015 at 9:18 AM, Jim Nasby  wrote:
> > Also, we've faced issues in the past with making catalog changes due to fear
> > of breaking user scripts. Instead of doubling down on that with RLS on top
> > of catalog tables, would it be better to move the tables to a different
> > schema, make them accessible only to superusers and put views in pg_catalog?
> 
> Uggh.  -1 on that option from me.

Yeah, -1 from here too...  That way leads to madness (note that we still
haven't managed to get rid of the pg_user, et al,
backwards-compatibility views from, uh, 8.2?).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Jim Nasby]

On 9/14/15 7:38 PM, Haribabu Kommi wrote:

On Fri, Sep 11, 2015 at 7:50 AM, Joe Conway  wrote:

On 09/01/2015 11:25 PM, Haribabu Kommi wrote:

If any user is granted any permissions on that object then that user
can view it's meta data of that object from the catalog tables.
To check the permissions of the user on the object, instead of
checking each and every available option, I just added a new
privilege check option called "any". If user have any permissions on
the object, the corresponding permission check function returns
true. Patch attached for the same.

Any thoughts/comments?


Thanks for working on this! Overall I like the concept and know of use
cases where it is critical and should be supported. Some comments:


Thanks for the review, I will take care of the comments in the next patch.

I didn't find any better approach other than creating policies automatically
or providing permission to superuser on system catalog tables. If everyone
feels as this is the best approach, then i will create policies for all catalog
tables in the next version.


Instead of adding or removing the rules, couldn't they just stay in 
place and be governed by what the field in the database was set to? It 
would also be nice if we could grant full access to roles instead of 
requiring superuser to see everything. Perhaps instead of a boolean 
store a role name in pg_database; anyone granted that role can see the 
full catalogs.


Also, we've faced issues in the past with making catalog changes due to 
fear of breaking user scripts. Instead of doubling down on that with RLS 
on top of catalog tables, would it be better to move the tables to a 
different schema, make them accessible only to superusers and put views 
in pg_catalog?

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Fri, Sep 11, 2015 at 7:50 AM, Joe Conway  wrote:
> On 09/01/2015 11:25 PM, Haribabu Kommi wrote:
>> If any user is granted any permissions on that object then that user
>> can view it's meta data of that object from the catalog tables.
>> To check the permissions of the user on the object, instead of
>> checking each and every available option, I just added a new
>> privilege check option called "any". If user have any permissions on
>> the object, the corresponding permission check function returns
>> true. Patch attached for the same.
>>
>> Any thoughts/comments?
>
> Thanks for working on this! Overall I like the concept and know of use
> cases where it is critical and should be supported. Some comments:

Thanks for the review, I will take care of the comments in the next patch.

I didn't find any better approach other than creating policies automatically
or providing permission to superuser on system catalog tables. If everyone
feels as this is the best approach, then i will create policies for all catalog
tables in the next version.

Regards,
Hari Babu
Fujitsu Australia


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multi-tenancy with RLS

[Joe Conway]

On 09/01/2015 11:25 PM, Haribabu Kommi wrote:
> If any user is granted any permissions on that object then that user
> can view it's meta data of that object from the catalog tables.
> To check the permissions of the user on the object, instead of
> checking each and every available option, I just added a new
> privilege check option called "any". If user have any permissions on
> the object, the corresponding permission check function returns
> true. Patch attached for the same.
> 
> Any thoughts/comments?

Thanks for working on this! Overall I like the concept and know of use
cases where it is critical and should be supported. Some comments:

1.) "... WITH ROW LEVEL SECURITY ...": name is not really accurate. This
feature does not, for example automatically add row security to all
tables in the database. It is really a specific set of RLS policies on
system tables to accomplish one very specific goal -- restrict metadata
access to those objects on which the user has some access. So maybe
something like one of these?
  ... WITH METADATA SECURITY ...
  ... WITH CATALOG SECURITY ...
  ... WITH METADATA RESTRICT ...
  ... WITH CATALOG RESTRICT ...

2.) Related to #1, I wonder if we should even provide a set of policies,
or should we just allow RLS on system tables and let people create their
own policies to suit their own needs. Maybe that could be dangerous, but
so are a lot of things we allow superusers do.

3.) In RelationBuildRowSecurity:

8<--
+   /* */
+   if (!criticalRelcachesBuilt || !criticalSharedRelcachesBuilt ||
relation->rd_id == PolicyRelationId)
+   return;
8<--

Bypassing RelationBuildRowSecurity() for relation->rd_id ==
PolicyRelationId causes a segfault for me and prevents me from even
logging into the database. Additionally I would want row security to
apply to pg_policy as well. Maybe not the best approach, but I solved
the infinite recursion issue like this:

8<--
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 45326a3..49db767 100644
*** a/src/backend/commands/policy.c
--- b/src/backend/commands/policy.c
*** static char parse_policy_command(const c
*** 52,57 
--- 105,116 
  static Datum *policy_role_list_to_array(List *roles, int *num_roles);

  /*
+  * used to prevent infinite recursion when building
+  * row security for pg_policy itself
+  */
+ int rdepth = 0;
+
+ /*
   * Callback to RangeVarGetRelidExtended().
   *
   * Checks the following:
*** RelationBuildRowSecurity(Relation relati
*** 197,202 
--- 257,273 
MemoryContext oldcxt = CurrentMemoryContext;
RowSecurityDesc *volatile rsdesc = NULL;

+   /* */
+   if (!criticalRelcachesBuilt || !criticalSharedRelcachesBuilt)
+   return;
+   if (relation->rd_id == PolicyRelationId && rdepth > 0)
+   {
+   rdepth = 0;
+   return;
+   }
+   if (relation->rd_id == PolicyRelationId)
+   rdepth = 1;
+
/*
 * Create a memory context to hold everything associated with this
 * relation's row security policy.  This makes it easy to clean
up during
*** RelationBuildRowSecurity(Relation relati
*** 366,377 
--- 437,450 
/* Delete rscxt, first making sure it isn't active */
MemoryContextSwitchTo(oldcxt);
MemoryContextDelete(rscxt);
+   rdepth = 0;
PG_RE_THROW();
}
PG_END_TRY();

/* Success --- attach the policy descriptor to the relcache entry */
relation->rd_rsdesc = rsdesc;
+   rdepth = 0;
  }

  /*
8<--

4.) Currently, policies are always installed in the current database
rather than the target database. Didn't try to figure out how to fix:
8<--
postgres=# create database test with ROW LEVEL SECURITY = true;
CREATE DATABASE
postgres=# select count(1) from pg_policy;
 count
---
30
(1 row)

postgres=# \c test
You are now connected to database "test" as user "postgres".
test=# select count(1) from pg_policy;
 count
---
 0
(1 row)
8<--

5.) I'm not thrilled with all the additional included headers added to
policy.c for this, but I don't offhand see a better way either.

6.) I would like to see this work in conjunction with sepgsql (and/or
other security label providers) as well. That could be accomplished
either as mentioned in #2 above (let me create custom policies to suit
my needs), or by providing security label hooks somewhere that they
affect the output of the has__privilege*() functions.

HTH,

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Multi-tenancy with RLS

[Haribabu Kommi]

On Fri, Aug 14, 2015 at 12:00 PM, Haribabu Kommi
 wrote:
>
> Here I attached the proof concept patch.

Here I attached an updated patch by adding policies to the most of the
system catalog tables, except the following.
AggregateRelationId

AccessMethodRelationId
AccessMethodOperatorRelationId
AccessMethodProcedureRelationId

AuthMemRelationId
CastRelationId
EnumRelationId
EventTriggerRelationId
ExtensionRelationId

LargeObjectRelationId
LargeObjectMetadataRelationId

PLTemplateRelationId
RangeRelationId
RewriteRelationId
TransformRelationId

TSConfigRelationId
TSConfigMapRelationId
TSDictionaryRelationId
TSParserRelationId
TSTemplateRelationId

Following catalog tables needs to create the policy based on the
class, so currently didn't added any policy for the same.

SecLabelRelationId
SharedDependRelationId
SharedDescriptionRelationId
SharedSecLabelRelationId


If any user is granted any permissions on that object then that user
can view it's meta data of that object from the catalog tables.
To check the permissions of the user on the object, instead of
checking each and every available option, I just added a new
privilege check option called "any". If user have any permissions on
the object, the corresponding permission check function returns
true. Patch attached for the same.

Any thoughts/comments?

Regards,
Hari Babu
Fujitsu Australia


multi-tenancy_with_rls_poc_2.patch
Description: Binary data


any_privilege_check_option.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers