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

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> Yes, it is reasonable both of MAC/DAC to handle temporary schema as
> an exception of access controls on schemas.

Great.

> > 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 agree that it should be documented.
> Where should I document them on? I guess the purpose of the description
> is to inform these behavior changes for users, not only developers.
> The official documentation sgml? wiki.postgresql.org? or, source code
> comments are enough?

What I would suggest is having a README or similar which accompanies the
patch. This could then be included by reference in the commit message,
or directly in the commit depending on what the committer prefers. Or,
it could just go into the mailing list and commitfest archives. The
point is to make sure the committer understands and isn't suprised when
reviewing the changes and comes across places where the code changes
result in a behaviour change.

If the changes are significant enough (and I don't think they will be,
to be honest..), they should be included by the committer in the commit
message and then picked up by Bruce, et al, when the release notes are
developed. I don't believe it needs to be in the formal PG
documentation, unless there's something documented there today which is
changing (very unlikely..).

> The shdepDropOwned() launched using DROP OWNED BY statement is a case that
> we have no DAC for each database objects but MAC should be applied on them.
> We can consider it as a variety of cascaded deletion, so ac_object_drop()
> should be also put here.

Right, that makes sense to me.

> > 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?
>
> Sorry, confusable description.
>
> What I would like to say is something like:
>
> CreateXXXX()
> {
> namespaceId = LookupCreationNamespace();
> ac_xxx_create_namespace(); --> only one use it, but other doesn't use?
> :
> tablespaceId = get_tablespace_oid();
> ac_xxx_create_tablespace(); --> only DAC use it?
> :
> ac_xxx_create(); --> only MAC use it?
> :
> values[ ... ] = ObjectIdGetDatum(namespaceId);
> values[ ... ] = ObjectIdGetDatum(tablespaceId);
> simple_heap_insert();
> :
> }
>
> When we create a new object X which needs OID of namespace and tablespace,
> these names have to be resolved prior to updating system catalogs.
> If we put ac_*() routines for each name resolution, it implicitly assumes
> a certain security model and the ac_*() routine getting nonsense.

No, no. What I was suggesting and what I think we already do in most
places (but not everywhere and it's not really a policy) is this:

CreateXXXX()
{
namespaceId = LookupCreationNamespace();
tablespaceId = get_tablespace_oid();
ac_xxx_create();
:
values[ ... ] = ObjectIdGetDatum(namespaceId);
values[ ... ] = ObjectIdGetDatum(tablespaceId);
simple_heap_insert();
:
}

Which I think is what you're doing with this, it just might be a change
from what was done before when there were multiple permission checks
done.

> >> 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).
>
> This policy focuses on developers, so it is enough to be source code comments?

Source code comments would be good for this. The only other place I
could think of it going would be on the developer part of the wiki.

Thanks,

Stephen