Re: [PATCH] Reworks for Access Control facilities (r2311)

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> Stephen Frost wrote:
> > The scenario you outline could happen without SE-PG, couldn't it?
> > Specifically, if a user makes a connection, creates a temporary table,
> > and then their rights to create temporary tables are revoked? What
> > should happen in that instance though? Should they continue to have
> > access to the tables they've created? Should they be dropped
> > immediately?
>
> The permission to be checked here is ACL_USAGE, not ACL_CREATE_TEMP.
> So, the default PG model does not prevent to access his temporary
> tables, even if ACL_CREATE_TEMP is revoked after the creation.
> In this case, he cannot create new temporay tables any more due to
> the lack of ACL_CREATE_TEMP. But it does not prevent accesses to the
> temporary objects because these are already created.

What I'm failing to understand is why SE-PG would be changing this then.
In general, the pg_temp stuff is a bit of a 'wart', as I understand it,
and is there mainly to be a place to put temporary tables. The fact
that it's a schema, which we use in other cases to implement access
controls, feels more like a side-effect of it being a schema than
something really necessary.

> > I'm not convinced this case is handled sufficiently.. We at least need
> > to think about what we want to happen and document it accordingly.
>
> It is a special case when pg_namespace_aclmask() is called towards
> a temporary namespace. It always returns ACL_USAGE bit at least
> independently from its acl setting. (ACL_CREATE bit depends on the
> ACL_CREATE_TEMP privilege on the current database.)
>
> Please see the pg_namespace_aclmask(). Its source code comment
> describes it and it always makes its decision to allow to search
> temporary namespace.
> I guess it is the reason why pg_namespace_aclcheck() is not called
> towards the temporary namespace, and it's right for the default PG
> model.
>
> But SE-PG model concerns the assumption.

The temporary namespace is just there to hold temporary tables which
have been created. It's not intended to be a point of access control.
I understand that there may be some cases where SE-PG wants to control
access when the default PG model doesn't, but this feels like a case
where the PG model doesn't because it's an implementation detail that's
really intended to be hidden from the user.

> The reason why I commented as ac_object_drop() should be done is that
> SE-PG model also need to apply checks on cascaded objects, not only
> the original one, as you mentioned. And I expected that code only used
> in SE-PG will be suggested to remove from the first patch.

I can understand that, and I think I even said that myself previously.
Things have changed a bit since then though, we're trying to develop a
generalized API. As I said before, I could go either way on this:
Either drop the comment, or add the function call. I'm leaning towards
adding the function call here since it can be done as part of the
generalized API and doesn't add alot of complexity. Removing the
comment is also an option, but that's not my preference.

> The dacSkip flag is a bit different issue.
> In some cases, cascaded deletion is not completely equivalent to the
> regular deletion.
>
> For example, the default PG model checks ownership of the relation
> when we create/drop a trigger object. The PE-PG model also follow
> the manner, so it also checks db_table:{setattr} permission.
> When we drop a table, it automatically drops triggers which depends
> on the table. In this case, if ac_object_drop() calls ac_trigger_drop()
> with dacSkip=true, SE-PG checks db_table:{setattr} first, then it also
> checks db_table:{drop} later. It is not incorrect, but redundant.
>
> But, it is not a fundamental differences between approaches.
> For example, we can skip SE-PG's check on triggers when dacSkip=true.
> In this case, the name of variable might be a bit strange.

That was part of the reason I was suggesting changing the name to
'depDrop' for 'dependency Drop', that would clear up the confusion about
it being dac or SE, etc.

> >> BTW, I wonder why ACL_EXECUTE is checked on creation of conversions,
> >> because it is equivalent to allow everyone to execute the conversion
> >> function. It seems to me ownership is more appropriate check.
> >
> > This is, again, something which should be discussed and dealt with
> > separately from the ac_* model implementation.
>
> Ahn, it is just my opinion when I saw the code.
> I have no intention to change the behavior in this patch.
> Is the source code comment also unconfortable?

I could probably go either way on this. In any case, it should be a
separate discussion in a separate thread, if we really want to discuss
it.

> > My concern is just that there might now be an error message seen be the
> > user first regarding the negator instead of a permissions error that
> > would have been seen first before, for the same situation.
> >
> > I doubt this is a problem, really, but we should at least bring up any
> > changes in behaviour like this and get agreement that they're
> > acceptable, even if it's just the particulars of error-messages (since
> > they could possibly contain information that shouldn't be available...).
>
> I don't think it is a problem.
> The default PG model (also SE-PG model) does not support to hide existence
> of a certain database objects, even if client does not have access permissions.
> For example, a client can SELECT from the pg_class which include relations
> stored within a certain namespace without ACL_USAGE.
> The default PG model prevent to resolve the relation name in the schema,
> but it is different from to hide its existent.

I know it doesn't hide existence of major database objects. Depending
on the situation, there might be other information that could be leaked.
I realize that's not the case here, but I still want to catch and
document any behavioral changes, even if it's clear they shouldn't be an
issue.

> I would like to use the revised comment.

Feel free to..

> > I like the latter of those (call ac_xxx_drop() with dacSkip set to
> > true). We might even consider changing the name of 'dacSkip' to be
> > 'depDrop' or similar. This would indicate that the function is being
> > called to drop an object due to a dependency, which is really the only
> > case we want to skip permissions checking in the default model (is this
> > correct?). That then makes it clear that other models (such as SE-PG)
> > can change the behaviour of permissions checking on objects dropped due
> > to dependencies.
>
> As I noted above. It is not a significant design issue.
>
> I prefere 'cascade' more than 'depDrop' as the name of flag variable.

That would be fine with me.

> > which is really the only
> > case we want to skip permissions checking in the default model (is this
> > correct?).
>
> If you saying the case is only MAC should be applied but no DAC,
> it is correct.
>
> I think four other cases that both of MAC/DAC should be bypassed.
> - when temporary database objects are cleaned up on session closing.
> - when ATRewriteTables() cleans up a temporary relation.
> - when CLUSTER command cleans up a temporary relation.
> - when autovacuum found an orphan temp table, and drop it.
>
> In this case, ac_object_drop() should not be called anyway,
> as if ac_proc_create() is not called when caller does not want.

Ok.

> >> In my current preference, I would like to remove dacSkip flag from the
> >> ac_xxx_drop() routines and we don't call it from the ac_object_drop().
> >> And, ac_object_drop() only calls MAC routines, as the SE-PG v8.4 doing.
> >
> > We could do this instead. Does it duplicate alot of code from the
> > ac_xxx_drop() routines though? If not, then I agree with this approach.
> > I still feel we should include calling ac_object_drop() in this patch
> > since it's clearly a hook we want to include in the API.
>
> The only difference is where sepgsql_xxx_drop() is called from.
> If we have the 'dacSkip' (or 'depDrop' or 'cascade') flag,
> the sepgsql_proc_drop() will be called from the ac_proc_drop().
> Otherwise, ac_object_drop() calls sepgsql_object_drop(), then
> it calls sepgsql_proc_drop().
>
> It needs micro adjustment for several object classes, such as
> trigger objects, but it is not a fundamental issue.
>
> At first, I follows your preference. (ac_object_drop() commented out
> and it calls ac_xxx_drop() with DAC bypassable flag.)
>
> > Again, it boils down to if there is sufficient information to implement
> > the controls we want. How about this- if we wanted (in some future PG)
> > to give users the option of "check permissions on objects dropped due to
> > dependencies" with regular PG, could we do that in ac_object_drop()
> > without alot of extra code? Or would we have to go add the dacSkip (my
> > depDrop) flag everywhere then? It seems like we could just have that
> > option checked in ac_object_drop() and have it set depDrop accordingly
> > for the other functions.
>
> If we expect such kind of future enhancement, it may be more flexible
> to deliver a flag value.

Yes, that's what I'm starting to think too, especially now that you've
explained how sepgsql_object_drop() would work. I'd rather it go
through the ac_* API and the sepgsql_proc_drop() be called from there
with the flag than have a separate path.

> >> In the current implementation, these permissions are checked on the
> >> separated timing just after obtaining OID of namespace and procedure.
> >
> > Checking immediately after resolving OIDs seems fine to me. Clearly,
> > that has to be done and it's really closer to 'parsing' than actually
> > doing anything.
>
> Please note that what factors are used depends on the security model
> for the same required action, such as CREATE CONVERSION.
> If we put ac_*() functions after the name->OID resolution, it breaks
> the purpose of security abstraction layer.
>
> In this example, it makes access control decision to create a new
> conversion, and raises an error if violated.
> But the way to make the decision is encapsulated from the caller.
> A configuration may make decision with the default PG model.
> An other configuration may make decision with the default PG and SE-PG
> model. The caller has to provide information to the ac_*() functions,
> but it should not depend on a certain security model.

I agree that what the caller provides shouldn't depend on the security
model. I wasn't suggesting that it would. The name->OID resolution I
was referring to was for things like getting the namespace OID, not
getting the OID for the new conversion being created. Does that clear
up the confusion here?

> >> The security checks raises an error on access violations, and it aborts
> >> whole of the current transaction. So, I don't think it is a big issue
> >> whether it should be put at the first or last.
> >
> > The issue is that, in my view, it's better to get 'permission denied'
> > earlier than some other error. I would be frustrated to work out how to
> > create a particular object only to then be given a 'permission denied'
> > right at the end. Some things have to be done first to make a decision
> > about permissions, such as name->OID resolution, but the real 'work' and
> > any issues related to that should be dealt with after permissions
> > checking.
>
> It is necessary to categolize the work into two cases.
> The one is a work without any side-effect. For example, constructing
> a memory object, looking up system caches and so on.
> The other is a work with updating system catalogs.
>
> I think the security checks can be reordered within the earlier operations,
> but should not move to the later of the updating system catalogs, even if
> an accee violation error cancels the updates.

Right, I was just suggesting moving it to immediately after the work
that does not have any side-effect. It must be checked before any
system catalog changes are done. Sorry for the confusion.

> >> It is only necessary to provide enough information to make decision for
> >> the ac_*() routines.
> >
> > Right.. Can we be consistant about having the permissions check done as
> > soon as we have enough information to call the ac_*() routines then? I
> > believe that's true in most cases, but not all, today. Any of those
> > changes which change behaviour should be discussed in some way (such as
> > an email to -hackers as you did for FindConversion()).
>
> At least, the current implementation deploys ac_*() routines as soon as
> the caller have enough information to provide it.

Good. We should document where that changed behaviour though, but also
document that this is a policy that we're going to try and stick with in
the future (running ac_*() as soon as there is enough information to).

> In the result, it had to be moved just before simple_heap_insert() in
> some cases.
>
> I'd like to discuss issues related to FindConversion() and EnableDisableRules()
> in other patch. And, ac_*() routines don't care about them in this stage.

Ok.

> >> The ac_proc_execute() is the only case which the caller has to give
> >> roleOid instead of GetUserId(), because the default PG model requires
> >> to check ACL_EXECUTE permission on the pair of owner of the aggregate
> >> function and trans/final function of the aggregate.
> >> In other case, GetUserId() is good enough.
> >>
> >> Is it really necessary to add roleOid argument for all the hooks?
> >
> > Ok.. It's just ac_proc_execute(), and really only in certain
> > circumstances, right? Most 'regular' usage of ac_proc_execute() still
> > uses GetUserId()? Perhaps we could address this by having the argument
> > called 'aggOid' instead, and pass 'InvalidOid' when it's not an
> > aggregate, and use GetUserId() in that case inside ac_proc_execute().
> > We could also change it to be ac_proc_execute() and
> > ac_proc_agg_execute(). That might be cleaner.
> >
> > What do you think?
>
> Hmm, it may be considerable.
>
> The ac_aggregate_execute(Oid aggOid) may be preferable for the naming
> convension.
> This check should be put on the ExecInitAgg() and ExecInitWindowAgg().
> The current window-func code checks permission on the finalfn/transfn
> at the initialize_peragg() called from the ExecInitWindowAgg().

Right.

Thanks,

Stephen