Re: security label support, revised

Robert, thanks for your reviewing and revising.

(2010/09/23 13:28), Robert Haas wrote:
> On Thu, Sep 16, 2010 at 11:12 PM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>> 2010/9/14 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> The attached patch is a revised version, but a bit difference from what
>>> I introduced yesterday.
>>
>> I am working through this patch and fixing a variety of things things
>> that seem to need fixing. Please hang tight and don't send any new
>> versions for now.
>
> There's no particularly good way to say this, so I'm just going to
> spit it out: this patch was a real mess. In particular, there are a
> huge number of cases where the identifier names were poorly chosen,
> which I have mostly gone through and fixed now. There may yet be some
> arguable and/or wrong cases remaining, and it's certainly possible
> that not everyone may agree with all the choices I made, but it's
> certainly a lot better than it was. I also had to rewrite pretty much
> all of the documentation, comments, and error messages. I reorganized
> a fair amount of the code, too; and ripped out a bunch of stuff that
> looked irrelevant. In theory, this was supposed to be patterned off
> the COMMENT code, but there were various changes which mostly did not
> seem like improvements to me, and which in at least one case were
> plain wrong.
>
Thanks for this revising that I didn't found out.

> Most of the contents of the new documentation section on external
> security providers seemed irrelevant to me, which I guess I can only
> blame myself for since I was the one who asked for that section to be
> created, and I didn't specify what it should contain all that well. I
> took a try at rewriting it to be more on-topic, but it didn't amount
> to much so I ended up just ripping that part out completely.
>
One headache thing when I tried to describe the new documentation section
was what we should describe here, because whole of the chapters in Server
Administration are on the standpoint of users, not developers.

Under the previous discussion, I suggested to move the most of descriptions
about external security providers into chapters in Internals, except for
a mention about a fact we have external security provider at the tail of
Database Roles and Privileges. How about the idea?
Perhaps, the chapters in Internals are appropriate place to introduce
specification of security hooks.

I also think it is irrelevant to describe label based mandatory access
control in the PgSQL documentation. It should be moved to the package of
SE-PgSQL.

> For a couple of reasons, I decided that it made sense to broaden the
> set of objects to which the SECURITY LABEL command can apply. My
> meeting with the NSA folks at BWPUG more or less convinced me that
> we're not going to get very far with this unless we have suitable
> hooks for additional permissions-checking when functions are executed
> or large objects are accessed, so I added labels for those, as well as
> for types, schemas, and procedural languages. It is possible that we
> need more than that, but supporting all of these rather than just
> relations and attributes requires only fairly trivial code changes,
> and I'd like to have at least a month or two go by before I have to
> look at another patch in this area. It's worth noting that labels on
> schemas can be useful even if we don't have a hook for schema-related
> permissions checking, once we have hooks to set labels at object
> creation time: the label for a newly assigned table can be a function
> of the user's label and the schema's label.
>
I agree this enhancement. Early or late, security labels of these objects
become eventually necessary to apply permission checks rather than relations
and attributes.

> I removed the crock that let one label provider veto another label
> provider's label. I understand that MAC will require a control there,
> but (as I said before) that's not the right way to do it. Let's leave
> that as material for a separate patch that solves the whole problem
> well instead of 5% of it poorly.
>
OK, I'll revise this matter later.

> I think the backend code here is now in pretty good shape, but there
> are still a number of things that need to be fixed. The pg_dump
> support is broken at the moment, because of the change to the set of
> objects that can be labeled. I also don't think it's right to dump
> security labels only when asked to do so. I think that the option
> should be --no-security-label in pg_dump(all) just as it is in
> pg_restore.

OK, I'll fix up the specification.

> Also, the pg_dump support for security labels should
> really reuse the existing design for comments, rather than inventing a
> new and less efficient method, unless there is some really compelling
> reason why the method used for comments won't work. Please send a
> reworked patch for just this directory (src/bin/pg_dump).
>
I intended to follow on the existing design for comments.
Could you suggest me how should it be fixed up the design?

Because of the --no-security-label option, we need to dump security
labels in a separated section from comments. So, we cannot pack them
into "COMMENT" sections.

> There are a few other problems. First, there's no psql support of any
> kind. Now, this is kind of a corner-case feature: so maybe we don't
> really need it. And as I mentioned on another thread, there aren't a
> lot of good letters left for backslash-d commands. So I'd be just as
> happy to add a system view along the lines I previously proposed for
> comments and call it good. Alternatively, or in addition, we could
> add a \d command after all. The best way forward is debatable, but we
> certainly need *something*, because interpreting the pg_seclabel
> catalog by hand is not for the faint of heart.

Do you suggest the new system views should be defined for each supported
object classes, such as pg_largeobject_seclabel? It seems to me a bit
inflation of number of system views.
My preference is psql's \d commands at first.

> Second, there are no
> regression tests. It's a bit tricky to think about how to crack that
> nut because this feature is somewhat unusual in that it can't be used
> without loading an appropriate loadable module. I'm wondering if we
> can ship a "dummy_seclabel" contrib module that can be loaded during
> the regression test run and then run various tests using that, but I'm
> not quite sure what the best way to set that up is. SECURITY LABEL is
> a core feature, so it would be nice to test it in the core regression
> tests... but maybe that's too complicated to get working, and we
> should just test it from the contrib module.
>

As you suggested in the following topic, I think it is the best way to
use LOAD command at the head of regression test (or just after SECURITY
LABEL command being failed due to no modules).
I'll add a dummy module into contrib, and regression test for labels.

Right now, I don't have any complaint about the patch to the backend
you revised, so I'd like to submit the next patch as an incremental
one to the seclabel-v4.patch, except for src/bin/pg_dump.

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