Re: security label support, revised

On Thu, Sep 16, 2010 at 11:12 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> 2010/9/14 KaiGai Kohei <kaigai@ak.jp.nec.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.

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.

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 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.

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.  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).

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.  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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company