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

Stephen, thanks for your comments.

Stephen Frost wrote:
>> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> >> Could you post any review comments, even if it is not comprehensive yet?
>>
>> In general, you don't need to preface your comments with 'MEMO:'. I
>> would encourage removing that. You might use 'FIXME:' instead, if it is
>> something which needs to be corrected in the future. Additionally,
>> please think about PG as a whole project. Talking about 'native
>> privilege mechanism' implies there are 'native' and 'foreign' ones. I
>> would recommend using the term 'default' instead.

OK, indeed, we don't have such a manner something like 'MEMO:' and 'FIXME:'.

I could not find an appropriate name for the current database acl mechanism.
OK, I'll revise the source code comment using the "default".

>> Also, rather than
>> saying it is "not a correct way" when referring to decisions made about
>> the permissions checks for the 'default' mechanism, just say it's the
>> default way and that other modules might not implement it the same.
>> Saying it's "not a correct way" implies a problem with the existing
>> code. If that's true, then it should be addressed separatly from this
>> patch.
>>
>> Specifically, in LookupExplicitNamespace, you added:
>>
>> ---------------------
>> MEMO: The native privilege mechanism always allows everyone to apply
>> ACL_USAGE permission on the temporary namespaces implicitly, so it was
>> omitted to check the obvious one here. But it is a heuristic, not a
>> correct way.
>> ---------------------
>>
>> My recommendation for how to reword this, if it's accurate, is:
>> ---------------------
>> By default, everyone is permitted ACL_USAGE on temporary namespaces
>> implicitly, so a check for it was omitted here. Other security models
>> may wish to implement a check, so call ac_schema_search() to check.
>> ---------------------

OK, indeed, it might be an inappropriate representation.

>> You might also provide a specific example of where and why this check
>> matters. I'm not entirely convinced it's necessary or makes sense, to
>> be honest..

By the default, it is 100% correct to omit checks here.

But it can make an issue, when we port SE-PG.
SELinux has its working mode (Enforcing or Permissive). On the permissive
mode, it also check security policy but does not prevent anything except
for generating access violation logs. We can toggle the working mode at the
runtime, so it is necessary to consider the following scenario.

1. A client tries to create a temporary table, the security policy does not
allow him to search his temporary namespace but it can be bypassed due to
the permissive mode.
2. Security administrator toggles it to enforcing mode.
3. The client tries to use the temporary table, but the security policy does
not allow him to search the temporary namespace, and it shall be enforced
in this time.

>> Also, does it make sense to still have LookupCreationNamespace? Given
>> its charter and the changes, is it still different from
>> LookupExplicitNamespace?

These have a bit of difference. The LookupCreationNamespace() can set up
a new temporary namespace, if "pg_temp" is given but the temporary namespace
it not still set up. LookupExplicitNamespace() does not set it up.

>> I would recommend adding back the comments above the calls to
>> ac_*_grant() which explicitly say:
>>
>> ---------------------
>> If we found no grant options, consider whether to issue a hard error.
>> Per spec, having any privilege at all on the object will get you by
>> here.
>> ---------------------
>>
>> The issue here is to make it clear to any callers that ac_*_grant() does
>> not check if everything being attempted will succeed but rather if
>> any one thing will. Same thing with the comments above the ac_*_grant()
>> functions themselves.

OK, I'll add this comments.

>> I'm not sure that the comment in restrict_grant() regarding this is
>> really necessary, considering that post-change there shouldn't be a
>> pg_aclmask() anymore, so referring to it in a comment seems odd.

Hmm, it may not be necessary after the code reviewing. I'll remove it.

>> In general, I like what you've done with splitting
>> restrict_and_check_grant() up.

Thanks,

>> Regarding your comment in dependency.c about objects which are dropped
>> due to dependencies:
>>
>> Have you already developed the code to resolve this issue? Is there
>> additional information required to check if the object can be dropped?
>> Should the ac_object_drop() just call the regular drop routines? But
>> they'd have to have a flag added which indicates if it's a dependency
>> drop or not, right? If the regular drop routine isn't called, then what
>> would ac_object_drop() use to determine if the drop is permitted or not?
>>
>> My concern here is that we don't want to develop a new API and then
>> immediately discover that it doesn't cover the cases we need it for.
>> If you have already developed an ac_object_drop() which would work for
>> existing PG and would be easily changed to support SE, I would recommend
>> you include it in this patch.

At the SE-PgSQL v8.4.x series, I already added checks on deleteOneObject().
http://code.google.com/p/sepgsql/source/browse/branches/pgsql-8.4.x/sepgsql/src/backend/catalog/dependency.c#959

I added a new boolean argument for the function to inform whether the
deletion should be checked, or not. In some cases, it is necessary to
bypass permission checks on cascaded deletion, such as cleaning up
database objects within temporary namespace.

It is not difficult the caller to distinguish whether the given deletion
is cascaded or not.
The findDependentObjects() returns a list of dependency objects.
Any entries to be removed in cascade deletion does not have DEPFLAG_ORIGINAL
flag in the ObjectAddressExtra structure. If the object has the flag, it is
already checked, so we don't need to check permission at the deleteOneObject()
twice.

>> Should the ac_object_drop() just call the regular drop routines?

In this patch, ac_xxx_drop() has dacSkip flag to bypass permission
checks on cascaded deletion. Perhaps, it might be necessary to put
two different ac_ routine to check object deletion.
The one is deployed on regular drop routines, such as ac_relation_drop().
The other is deloped on dependency drop routine, such as ac_object_drop().
If SE-PG feature checks all the drop permission at the ac_object_drop()
(except for objects which does not use dependency deletion), it is not
necessary to distinguish whether the given object is original or cascaded.

>> I don't find the comment regarding what happened with FindConversion to
>> be nearly descriptive enough. Can you elaborate on why the check wasn't
>> necessary and has now been removed? If it really isn't needed, why have
>> that function at all?

http://archives.postgresql.org/message-id/26499.1250706473@sss.pgh.pa.us

I'll add a comment about the reason why this check was simply eliminated.
It already checks permission when we create a new conversion, and it
is enough for the purpose.

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.

>> Regarding OperatorCreate, it looks like there may be some functional
>> changes. Specifically, get_other_operator() could now be called even if
>> a given user doesn't own the operator shell he's trying to fill out. It
>> could then error-out with the "operator cannot be its own negator or
>> sort operator" error. I'm not sure that's a big deal at all, but it's a
>> difference which we might want to document as expected. This assumes,
>> of course, that such a situation could really happen. If I'm missing
>> something and that's not possible, let me know.

When the get_other_operator() returns valid OID of the negator and
commutator, these OIDs are delivered to ac_operator_create(), then
ac_operator_create() calls pg_oper_ownercheck() to check ownership
on the negator and commutator.
I think it keeps compatibility in behavior.

>> The new 'permission' argument to ProcedureCreate should be documented in
>> the function definition, or at least where it's used, as to what it's
>> for and why it's needed and why ac_proc_create is conditional on it.

I'll add the following comment. Is it natural?
----
/*
* The 'permission' argument should be false, when an internal stuff
* tries to define a new obviously safe function. It allows to bypass
* security checks to create a new function.
* Otherwise, it should be true.
*/
----

>> Again, regarding shdepend, if you have the code already which works with
>> the default permissions model for PG, you should include it in this
>> patch as part of the API. I realize this contradicts a bit what I said
>> earlier, but the main concern here is making sure that it will actually
>> work for SEPG. If you vouch that it will, then perhaps we don't need to
>> add them now, but I would probably also remove the comments then. One
>> or the other.. Let's not add comments which will immediately be
>> removed, assuming everything goes well.

I guess your requirement is to proof the ac_object_drop() is implementable
with reasonable changes.
Yes, in the default permission model for PG, it does not check anything
here. So, if we add ac_object_drop() here, it should be an empty function
or it calls ac_xxx_drop() with dacSkip equals true which means do nothing.

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.

However, either of them are possible.

>> There is a similar change in CreateConversionCommand. Again, I don't
>> think it's a big deal, but I wonder if we should make a decision about
>> if permission checks should be first or last and then be consistant
>> about it. My gut feeling is that we should be doing them first and
>> doing all of them, if at all possible.. There are a couple of other
>> places like this.

The default PG model requires two privilege to create a new conversion.
The one is ACL_CREATE on the namespace, the other is ACL_EXECUTE on
the conversion procedure. The caller (CreateConversionCommand) has to
resolve both of objects identified by human readable name, prior to
the ac_conversion_create() invocation.

In the current implementation, these permissions are checked on the
separated timing just after obtaining OID of namespace and procedure.

>> My gut feeling is that we should be doing them first and
>> doing all of them, if at all possible

The 'doing all of them' will contain resolving database object identified
by its name. It has to be done prior to the security checks.
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.
It is only necessary to provide enough information to make decision for
the ac_*() routines.

>> I'm also concerned about the inconsistancy regarding if the roleOid is
>> passed into the function of if GetUserId() is used. I would recommend
>> being consistant with this. Either GetUserId() is always 'good enough',
>> or it's not, and we should require the roleOid to be passed into all of
>> the ac_* functions. I'm tending towards the latter, since it appears to
>> be necessary in some cases. If there is some division of the ac_*
>> functions where it's consistant within a division, that might be
>> alright, but it should be discussed somewhere.

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?

>> I've glanced through the rest and in general I feel like it's starting
>> to look good. Thanks for your efforts towards this. I'd really like to
>> see this get committed eventually.

Thanks for your reviewing. It is great heplful to improve the project.

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>