Re: security label support, part.2

Thanks for your reviewing.

On Mon, 9 Aug 2010 16:02:12 -0400
Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> 2010/7/26 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> > The attached patches are revised ones, as follows.
>
> I think this is pretty good, and I'm generally in favor of committing
> it. Some concerns:
>
> 1. Since nobody has violently objected to the comment.c refactoring
> patch I recently proposed, I'm hopeful that can go in. And if that's
> the case, then I'd prefer to see that committed first, and then rework
> this to use that code. That would eliminate some code here, and it
> would also make it much easier to support labels on other types of
> objects.
>
It seems to me fair enough. This parse_object.c can also provide
a facility to resolve the name of object to be labeled.

> 2. Some of this code refers to "local" security labels. I'm not sure
> what's "local" about them - aren't they just security labels? On a
> related note, I don't like the trivial wrappers you have here, with
> DeleteSecurityLabel around DeleteLocalSecLabel, SetSecurityLabel
> around SetLocalSecLabel, etc. Just collapse these into a single set
> of functions.
>
In the feature, I plan to assign security labels on the shared database
objects such as pg_database. The "local" is a contradistinction
towards these "shared" objects.

> 3. Is it really appropriate for ExecRelationSecLabel() to have an
> "Exec" prefix? I don't think so.
>
I don't have any preferences about

> 4. Please get rid of the nkeys++ stuff in DeleteLocalSecLabel() and
> just use fixed offsets as we do everywhere else.
>
OK, I'll fix it.

> 5. Why do we think that the relabel hook needs to be passed the number
> of expected parents?
>
We need to prevent relabeling on inherited relations/columns from
the multiple origin, like ALTER RENAME TO.
It requires to pass the expected parents into the provider, or
to check it in the caller.

> 6. What are we doing about the assignment of initial security labels?
> I had initially thought that perhaps new objects would just start out
> unlabeled, and the user would be responsible for labeling them as
> needed. But maybe we can do better. Perhaps we should extend the
> security provider hook API with a function that gets called when a
> labellable object gets created, and let each loaded security provider
> return any label it would like attached. Even if we don't do that
> now, esp_relabel_hook_entry needs to be renamed to something more
> generic; we will certainly want to add more fields to that structure
> later.
>
Starting with "unlabeled" is wrong, because it does not distinguish
an object created by security sensitive users and insensitive users,
for example. It is similar to relation's relowner is InvalidOid.

I plan the security provider hook on the creation time works two things.
1. It checks user's privilege to create this object.
2. It returns security labels to be assigned.

Then, the caller assigns these returned labels on the new object,
if one or more valid labels are returned.

> 7. I think we need to write and include in the fine documentation some
> "big picture" documentation about enhanced security providers. Of
> course, we have to decide what we want to say. But the SECURITY LABEL
> documentation is just kind of hanging out there in space right now; it
> needs to connect to a broad introduction to the subject.
>
OK, I'll try to describe with appropriate granularity.
Do we need an independent section in addition to the introduction of
SECURITY LABEL syntax?

> 8. Generally, the English in both the comments and documentation needs
> work. However, we can address that problem when we're closer to
> commit.
>
OK

BTW, I'll go on the area where internet unconnectable during next
two days. Perhaps, my reply will run late.

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