Re: security label support, revised

Robert, although you suggested that only one ESP module shall be
invoked on relabeling an object before, and I agreed this design
at that time, but I noticed that we cannot implement the following
behavior correctly.

SELinux defines these permissions corresponding to table relabeling.
- db_table:{setattr}
It is necessary to change *any* properties of the table.
Security label is one of properties of the table, so, needs to be
checked on relabeling, not only ALTER TABLE and so on.
- db_table:{relabelfrom relabelto}
It is neccesary to change label of the table.
User must have {relabelfrom} permission on the older security label,
and {relabelto} permission on the new security label.

If and when multiple ESP modules are installed, we need to consider
the way to handle SECURITY LABEL statement for other modules.
When user tries to relabel security label of a table managed by
something except for selinux, it is not relevant to {relabelfrom
relabelto} permission in selinux, but we want to check {setattr}
permission on the operation, because it is a property of the table,
although not a security label in selinux.

In the current patch, the core PG (ExecSecurityLabel()) invokes only
one hook that matches with the given "FOR <esp>" option.
However, I reconsidered this hook should be simply invoked like other
hooks. Then, the ESP module decides whether ignores or handles the
invocation, and also decides to call secondary hook when multiple
modules are loaded. If so, selinux module can check {setattr} and
also calls secondary modules.

In the previous discussion, I missed the possibility of the case
when we want to check relabeling a security label managed by other
ESP. But it might be also necessary to call all the modules which
want to get control on SECURITY LABEL statement, apart from whether
it manages the supplied security label, or not.

Thanks,

(2010/08/31 15:27), KaiGai Kohei wrote:
> The attached patch is a revised version of security label support.
>
> summary of changes:
> * The logic to translate object-name to object-id was rewritten
> with the new get_object_address().
>
> * Right now, it does not support labeling on shared database object
> (ie; pg_database), so wrapper functions to XXXLocalSecLabel() were
> removed.
>
> * The restriction of same security label within whole of inheritance
> tree has gone. And, the 'num_parents' argument was also removed
> from the security hook.
>
> * ExecRelationSecLabel() was also removed, although you suggested
> to rename it, because it translate the supplied relation name
> into relation id and handled child tables, but it get unnecessary.
>
> * The chapter of 'External Security Provider' was added.
> It introduces overview of ESP concept and MAC features.
> Perhaps, other structures of chapters are more preferable,
> but I also think we need a draft at the begining of discussion.
>
> * The '--security-label' option was added to pg_dump/pg_dumpall;
> it allows to include security label of objects in the archives.
> The '--no-security-label' option was also added to pg_restore;
> it allows to skip security labels, even if the archive contains
> security labels.
>
> Thanks,
>
> (2010/08/10 5:02), Robert Haas 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.
>>
>> 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.
>>
>> 3. Is it really appropriate for ExecRelationSecLabel() to have an
>> "Exec" prefix? I don't think so.
>>
>> 4. Please get rid of the nkeys++ stuff in DeleteLocalSecLabel() and
>> just use fixed offsets as we do everywhere else.
>>
>> 5. Why do we think that the relabel hook needs to be passed the number
>> of expected parents?
>>
>> 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.
>>
>> 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.
>>
>> 8. Generally, the English in both the comments and documentation needs
>> work. However, we can address that problem when we're closer to
>> commit.
>>
>> I am going to mark this Returned with Feedback because I don't believe
>> it's realistic to get the comment code committed in the next week,
>> rework this patch, and then get this patch committed also. However,
>> I'm feeling pretty good about this effort and I think we're making
>> good progress toward getting this done.
>>
>
>
>
>
>

--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>