Thread: Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)
Stephen Frost wrote: > Row-Level Security Policies (RLS) What do we need RowSecurityPolicy->policy_id for? It seems to me that it is only used to determine whether the policy is the "default deny" one, so that it can later be removed if a hook adds a different one. This seems contrived as well as under-documented. Why isn't a boolean flag sufficient? (Not an actual review of this patch. I was merely skimming the code.) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro,
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> What do we need RowSecurityPolicy->policy_id for? It seems to me that
> it is only used to determine whether the policy is the "default deny"
> one, so that it can later be removed if a hook adds a different one.
> This seems contrived as well as under-documented. Why isn't a boolean
> flag sufficient?
Thanks for taking a look!
It's also used during relcache updates (see equalPolicy()). That wasn't
originally the case (I had missed adding the necessary bits to relcache
in the original patch), but I wouldn't want to remove that piece now
and, given that it's there, using InvalidOid to indicate when it's the
default-deny policy (and therefore this is no actual Oid) seems
sensible.
Thanks again!
Stephen
Stephen Frost wrote: > Alvaro, > > * Alvaro Herrera (alvherre@2ndquadrant.com) wrote: > > What do we need RowSecurityPolicy->policy_id for? It seems to me that > > it is only used to determine whether the policy is the "default deny" > > one, so that it can later be removed if a hook adds a different one. > > This seems contrived as well as under-documented. Why isn't a boolean > > flag sufficient? > > Thanks for taking a look! > > It's also used during relcache updates (see equalPolicy()). Hmm, but the policy name is unique also, right? So the policy_id check is redundant ... -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro,
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > * Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> > > What do we need RowSecurityPolicy->policy_id for? It seems to me that
> > > it is only used to determine whether the policy is the "default deny"
> > > one, so that it can later be removed if a hook adds a different one.
> > > This seems contrived as well as under-documented. Why isn't a boolean
> > > flag sufficient?
> >
> > Thanks for taking a look!
> >
> > It's also used during relcache updates (see equalPolicy()).
>
> Hmm, but the policy name is unique also, right? So the policy_id check
> is redundant ...
I don't disagree with that, but surely checking if it's the same OID and
exiting immediately is going to be faster than comparing the policy
names.
Now, looking at the code, I'm actually failing to see a case where we
use the RowSecurityPolicy->policy_name.. Perhaps *that's* what we
should be looking to remove?
Thanks!
Stephen
On 27 May 2015 at 02:42, Stephen Frost <sfrost@snowman.net> wrote: > Now, looking at the code, I'm actually failing to see a case where we > use the RowSecurityPolicy->policy_name.. Perhaps *that's* what we > should be looking to remove? > If we add support for restrictive policies, it would be possible, and I think desirable, to report on which policy was violated. For that, having the policy name would be handy. We might also arguably decide to enforce restrictive RLS policies in name order, like check constraints. Of course none of that means it must be kept now, but it feels like a useful field to keep nonetheless. Regards, Dean
Dean,
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 27 May 2015 at 02:42, Stephen Frost <sfrost@snowman.net> wrote:
> > Now, looking at the code, I'm actually failing to see a case where we
> > use the RowSecurityPolicy->policy_name.. Perhaps *that's* what we
> > should be looking to remove?
>
> If we add support for restrictive policies, it would be possible, and
> I think desirable, to report on which policy was violated. For that,
> having the policy name would be handy. We might also arguably decide
> to enforce restrictive RLS policies in name order, like check
> constraints. Of course none of that means it must be kept now, but it
> feels like a useful field to keep nonetheless.
I agree that it could be useful, but we really shouldn't have fields in
the current code base which are "just in case".. The one exception
which comes to mind is if we should keep it for use by extensions.
Those operate on an independent cycle from our major releases and would
likely find having the name field useful.
One thing which now occurs to me, however, is that, while the current
coding is fine, using InvalidOid as an indicator for the default-deny
policy, in general, does fall over when we consider policies added by
extensions. Those policies are necessairly going to need to put
InvalidOid into that field, unless they acquire an OID somehow
themselves (it doesn't seem reasonable to make that a requirement,
though I suppose we could..), and, therefore, perhaps we should add a
boolean field which indicates which is the defaultDeny policy anyway and
not use that field for that purpose.
Thoughts?
Thanks!
Stephen
On 27 May 2015 at 14:51, Stephen Frost <sfrost@snowman.net> wrote: >> On 27 May 2015 at 02:42, Stephen Frost <sfrost@snowman.net> wrote: >> > Now, looking at the code, I'm actually failing to see a case where we >> > use the RowSecurityPolicy->policy_name.. Perhaps *that's* what we >> > should be looking to remove? >> >> If we add support for restrictive policies, it would be possible, and >> I think desirable, to report on which policy was violated. For that, >> having the policy name would be handy. We might also arguably decide >> to enforce restrictive RLS policies in name order, like check >> constraints. Of course none of that means it must be kept now, but it >> feels like a useful field to keep nonetheless. > > I agree that it could be useful, but we really shouldn't have fields in > the current code base which are "just in case".. The one exception > which comes to mind is if we should keep it for use by extensions. > Those operate on an independent cycle from our major releases and would > likely find having the name field useful. > Hmm, when I wrote that I had forgotten that we already allow extensions to add restrictive policies. I think it would be good to allow those policies to have names, and if they do, to copy those names to any WCOs created. Then, if a WCO is violated, and it has a non-NULL policy name associated with it, we should include that in the error report. > One thing which now occurs to me, however, is that, while the current > coding is fine, using InvalidOid as an indicator for the default-deny > policy, in general, does fall over when we consider policies added by > extensions. Those policies are necessairly going to need to put > InvalidOid into that field, unless they acquire an OID somehow > themselves (it doesn't seem reasonable to make that a requirement, > though I suppose we could..), and, therefore, perhaps we should add a > boolean field which indicates which is the defaultDeny policy anyway and > not use that field for that purpose. > > Thoughts? > Actually I think a new boolean field is unnecessary, and probably the policy_id field is too. Re-reading the code in rowsecurity.c, I think it could use a bit of refactoring. Essentially what it needs to do (for permissive policies at least) is just * fetch a list of internal policies * fetch a list of external policies * concatenate them * if the resulting list is empty, add a default-deny qual and/or WCO By only building the default-deny qual/WCO at the end, rather than half-way through and pretending that it's a fully-fledged policy, the code ought to be simpler. I might get some time at the weekend, so I can take a look and see if refactoring it this way works out in practice. BTW, I just spotted another problem with the current code, which is that policies from extensions aren't being checked against the current role (policy->roles is just being ignored). I think it would be neater and safer to move the check_role_for_policy() check up and apply it to the entire concatenated list of policies, rather than having the lower level code have to worry about that. Regards, Dean
Dean,
* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 27 May 2015 at 14:51, Stephen Frost <sfrost@snowman.net> wrote:
> >> On 27 May 2015 at 02:42, Stephen Frost <sfrost@snowman.net> wrote:
> >> > Now, looking at the code, I'm actually failing to see a case where we
> >> > use the RowSecurityPolicy->policy_name.. Perhaps *that's* what we
> >> > should be looking to remove?
> >>
> >> If we add support for restrictive policies, it would be possible, and
> >> I think desirable, to report on which policy was violated. For that,
> >> having the policy name would be handy. We might also arguably decide
> >> to enforce restrictive RLS policies in name order, like check
> >> constraints. Of course none of that means it must be kept now, but it
> >> feels like a useful field to keep nonetheless.
> >
> > I agree that it could be useful, but we really shouldn't have fields in
> > the current code base which are "just in case".. The one exception
> > which comes to mind is if we should keep it for use by extensions.
> > Those operate on an independent cycle from our major releases and would
> > likely find having the name field useful.
>
> Hmm, when I wrote that I had forgotten that we already allow
> extensions to add restrictive policies. I think it would be good to
> allow those policies to have names, and if they do, to copy those
> names to any WCOs created. Then, if a WCO is violated, and it has a
> non-NULL policy name associated with it, we should include that in the
> error report.
I'm certainly not against that and I agree that it'd be nicer than
simply reporting that there was a violation, but we combine all of the
various pieces together, no? Are we really going to be able to
confidently say which policy was violated? Even if we are able to say
the first which was violated, more than one might be. Is that an issue
we need to address? Perhaps not, but it's something to consider.
> > One thing which now occurs to me, however, is that, while the current
> > coding is fine, using InvalidOid as an indicator for the default-deny
> > policy, in general, does fall over when we consider policies added by
> > extensions. Those policies are necessairly going to need to put
> > InvalidOid into that field, unless they acquire an OID somehow
> > themselves (it doesn't seem reasonable to make that a requirement,
> > though I suppose we could..), and, therefore, perhaps we should add a
> > boolean field which indicates which is the defaultDeny policy anyway and
> > not use that field for that purpose.
>
> Actually I think a new boolean field is unnecessary, and probably the
> policy_id field is too. Re-reading the code in rowsecurity.c, I think
> it could use a bit of refactoring. Essentially what it needs to do
> (for permissive policies at least) is just
>
> * fetch a list of internal policies
> * fetch a list of external policies
> * concatenate them
> * if the resulting list is empty, add a default-deny qual and/or WCO
>
> By only building the default-deny qual/WCO at the end, rather than
> half-way through and pretending that it's a fully-fledged policy, the
> code ought to be simpler.
I tend to agree.
> I might get some time at the weekend, so I can take a look and see if
> refactoring it this way works out in practice.
That would certainly be fantastic and most appreciated. Beyond that, we
have the add-a-default-deny-policy logic in multiple places, as I
recall, and I wonder if that's overkill and done out of paranoia rather
than sound judgement. I'm certainly not suggesting that we take
unnecessary risks, and so perhaps we should keep it, but I do think it's
something which should be reviewed and considered (I've been planning to
do so for a while, in fact).
> BTW, I just spotted another problem with the current code, which is
> that policies from extensions aren't being checked against the current
> role (policy->roles is just being ignored). I think it would be neater
> and safer to move the check_role_for_policy() check up and apply it to
> the entire concatenated list of policies, rather than having the lower
> level code have to worry about that.
Excellent point... We should address that and your proposed approach
sounds reasonable to me. If you're able to work on that this weekend,
I'd be happy to review next week.
Thanks!
Stephen
On 28 May 2015 at 08:51, Stephen Frost <sfrost@snowman.net> wrote: > * Dean Rasheed (dean.a.rasheed@gmail.com) wrote: >> Actually I think a new boolean field is unnecessary, and probably the >> policy_id field is too. Re-reading the code in rowsecurity.c, I think >> it could use a bit of refactoring. Essentially what it needs to do >> (for permissive policies at least) is just >> >> * fetch a list of internal policies >> * fetch a list of external policies >> * concatenate them >> * if the resulting list is empty, add a default-deny qual and/or WCO >> >> By only building the default-deny qual/WCO at the end, rather than >> half-way through and pretending that it's a fully-fledged policy, the >> code ought to be simpler. > > I tend to agree. > >> I might get some time at the weekend, so I can take a look and see if >> refactoring it this way works out in practice. > > That would certainly be fantastic and most appreciated. Beyond that, we > have the add-a-default-deny-policy logic in multiple places, as I > recall, and I wonder if that's overkill and done out of paranoia rather > than sound judgement. I'm certainly not suggesting that we take > unnecessary risks, and so perhaps we should keep it, but I do think it's > something which should be reviewed and considered (I've been planning to > do so for a while, in fact). > So I had a go at refactoring the code in rowsecurity.c to simplify the default-deny policy handling, as described. In the end my changes were quite extensive, so I'll understand if you don't have time to review it, but I think that it's worth it for the improved code clarity, simplicity and more detailed error messages for restrictive policies. In any case, there are a couple of bug fixes in there that ought to be considered. The main changes are: * pull_row_security_policies() is replaced by get_policies_for_relation(), whose remit is to fetch both the internal and external policies that apply to the relation. It returns the permissive and restrictive policies as separate lists, each of which contains any internal policies followed by any external policies (except of course that internal restrictive policies aren't possible yet). Unlike pull_row_security_policies() this does not try to build a default-deny policy, and instead may return empty lists. All the returned policies are filtered according to the current role, thus fixing the bug that external policies weren't being filtered. * process_policies() is replaced by build_security_quals() and build_with_check_options(), whose remits are to build the lists of security quals and WithCheckOption checks from the lists of permissive and restrictive policies. These new functions now have sole responsibility for handling the default-deny case if there are no explicit policies to apply, which means that it is no longer necessary to build RowSecurityPolicy objects representing the default-deny case (hence no more InvalidOid policy_id). * get_row_security_policies() is now greatly simplified, since it no longer has to merge internal and external policies, or worry about default-deny policies. The guts of the work is now done by the new functions described above. * The way that ON CONFLICT DO UPDATE is handled is also simplified --- rather than recursively calling get_row_security_policies() and turning the returned list of security quals into WCOs, it now simply calls build_with_check_options() a couple more times to build the additional kinds of WCOs needed for the auxiliary UPDATE. * RelationBuildRowSecurity() no longer builds a default-deny policy, and the resulting policy list is now allowed to be empty. This removes the need for RowSecurityPolicy's policy_id field. Actually the old code had 3 separate places with default-deny policy handling. That is now all localised in the functions that build security quals and WCOs from the policy lists. * Finally, WCOs for restrictive policies now report the name of the policy violated. Of course this means that the actual error might depend on the order in which the policies are checked. I've tackled that in the same way as was recently used for CHECK constraints, which is to always check restrictive policies in a well-defined order (name order) so that self-test output is predictable. Overall, I think this reduces the code complexity (although I think the total line count is about the same), and there is now a clearer separation of concerns across the various functions. Also I think it will make it easier to add support for internal restrictive policies in the future. While going through this, I spotted another issue --- in a DML query with additional non-target relations, such as UPDATE t1 .. FROM t2 .., the old code was checking the UPDATE policies of both t1 and t2, but really I think it ought to be checking the SELECT policies of t2 (in the same way as this query requires SELECT table permissions on t2, not UPDATE permissions). I've changed that and added new regression tests to test that change. Regards, Dean
Attachment
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 06/01/2015 02:21 AM, Dean Rasheed wrote: > While going through this, I spotted another issue --- in a DML > query with additional non-target relations, such as UPDATE t1 .. > FROM t2 .., the old code was checking the UPDATE policies of both > t1 and t2, but really I think it ought to be checking the SELECT > policies of t2 (in the same way as this query requires SELECT table > permissions on t2, not UPDATE permissions). I've changed that and > added new regression tests to test that change. I assume the entire refactoring patch needs a fair bit of work to rebase against current HEAD, but I picked out the attached to address just the above issue. Does this look correct, and if so does it make sense to apply at least this part right now? Thanks, Joe -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVuXFuAAoJEDfy90M199hlOMkP/RZoBU0MJF64GjHfuLVa3T5w PnDrnIoLBMOgOzkXrI+Ov0w0ESXltNvYjyIxkuyaB5PuoeDOdyYnnzbpPe7hH4pv zDjSinJnfNmFEcm0UjBzuBiSH0dv52PEIToexTKu6SnjvH3Co/Hk4Ar2uZ0r7bRF krl+Dd4kZX1uuRIigsSFqy873C79m3o11Szs5aW8c5od9adGxSvRHqZNf/UIDIbZ CxAo0E3Tlw0DmGl1cw1tdN8gWMzmvx5dQ0ih3+0/hkVUqN9p3Pg8BGajSvxxFlb2 l4+6RZGUw5ZTpJxRZf/zPc4updhq0zf/Z5g7GUYddrhO29eLS6al2ySlb7HL5G9M VPMjEzXuhFwhxSMdgHfz8UQh82KuNENMTKp81BvtIgZ7w86A9lWrKxCLaVMGhi8m MnfCQ4cdOmnE2vEHi0Ue3Dg/rvYO8QpVW0JKDOdzoqPErC7to9vorXI5X3vcfLbF fYfiJe6wUwbDEdxh/z0oKVFxWlf2NRk4pd+jZ7C+ibraLIwgEgZe7G4GEje5LxSP h4zTfx0sj3IyrvqziurO/aZqIBXBZEsm3Gv6OQs26C5Xvkx/QmgROB42lHwcDYri BTk6+uzNYKbX+kW56vEY0f0WMTLYZzc6jZRIVr3s+aLeyG0P9acY/n3uY1xBFCZZ Xb7gmepAN3rY1CF7By9o =e5hz -----END PGP SIGNATURE-----
Attachment
On 30 July 2015 at 01:35, Joe Conway <mail@joeconway.com> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 06/01/2015 02:21 AM, Dean Rasheed wrote: >> While going through this, I spotted another issue --- in a DML >> query with additional non-target relations, such as UPDATE t1 .. >> FROM t2 .., the old code was checking the UPDATE policies of both >> t1 and t2, but really I think it ought to be checking the SELECT >> policies of t2 (in the same way as this query requires SELECT table >> permissions on t2, not UPDATE permissions). I've changed that and >> added new regression tests to test that change. > > I assume the entire refactoring patch needs a fair bit of work to > rebase against current HEAD, Actually, there haven't been any conflicting changes so far, so a git rebase was able to automatically merge correctly -- new patch attached, with some minor comment rewording (not affecting the bug-fix part). Even so, I agree that it makes sense to apply the bug-fix separately, since it's not really anything to do with the refactoring. > but I picked out the attached to address > just the above issue. Does this look correct, and if so does it make > sense to apply at least this part right now? > Looks correct to me. Thanks. Regards, Dean
Attachment
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 07/30/2015 06:52 AM, Dean Rasheed wrote: > On 30 July 2015 at 01:35, Joe Conway <mail@joeconway.com> wrote: >> On 06/01/2015 02:21 AM, Dean Rasheed wrote: >>> While going through this, I spotted another issue --- in a DML >>> query with additional non-target relations, such as UPDATE t1 >>> .. FROM t2 .., the old code was checking the UPDATE policies of >>> both t1 and t2, but really I think it ought to be checking the >>> SELECT policies of t2 (in the same way as this query requires >>> SELECT table permissions on t2, not UPDATE permissions). I've >>> changed that and added new regression tests to test that >>> change. >> >> I assume the entire refactoring patch needs a fair bit of work >> to rebase against current HEAD, > > Actually, there haven't been any conflicting changes so far, so a > git rebase was able to automatically merge correctly -- new patch > attached, with some minor comment rewording (not affecting the > bug-fix part). Good to hear. > Even so, I agree that it makes sense to apply the bug-fix > separately, since it's not really anything to do with the > refactoring. > >> but I picked out the attached to address just the above issue. >> Does this look correct, and if so does it make sense to apply at >> least this part right now? > > Looks correct to me. Thanks -- committed and pushed to HEAD and 9.5 - -- Joe Conway -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVulOVAAoJEDfy90M199hlURUP/2TF7r77taLPhQtEk3CFFQEn mrt90N4DJ43VwGwC7mfdBsKXoJ+3jF3Hpghw/7ulI731/Io7C514gYDDvwGkrJWu vK3vzXEQu9CIIfh97CsJ5mmenaaxrF9ZSaWDbYvQQMwMnxUS5CGWwR6VSp+NhCZ9 cnfA7idbxNjfBsjG0nQvtSgV/HOp0tP3A6dlYPTXPiIzbT9IiOpxWPwoQYgMSTcP TBgK5MHG5JWrK1Bcg3BlQzYZefKEwN+LGU6zal4P4AjM14FfyMKfMc9A6EP9vtRl jFekmRUddbXxWddl0ZSSV5BY9BLTRL2CZFhMQNQ9xKDlsK1cuYORN4v+TgRCjPKy PdMH2tgndWsNNRTmj/vWUJxMXnHARl8MmtY8pau5Z6PKNUcASqd5Xq+zfKxOw8vf lS8c4eRsLcCD+TZ1rv5K6VULmwBViU0gKP6Xs62yDHsz/Zwvt2ar1fW/25peohhb m4j8vOCsdik9DDcJf6dF8sbb/z0FVh+JQqWhjbSJYioX9BOw/1AoNbi44wS7HzV1 vdhWx6qGWZnxi5qtb9j8BU0eFr/Q65kU3hsp2smRA/IK0bCQaO08rDQlPYsIHq10 SFodULNKFzpGvkzQ2Yqv1oyJ6jMvLtWgr9vBUZvPo8OHUyAkR8kfrZyzWyr/L/Mm 6jcuFNdYSS5F7W/S7ost =GJw9 -----END PGP SIGNATURE-----