Re: add a MAC check for TRUNCATE

On Fri, Sep 6, 2019 at 10:40 AM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,

Hello Stephen,

>
> * Yuli Khodorkovskiy (yuli.khodorkovskiy@crunchydata.com) wrote:
> > On Tue, Sep 3, 2019 at 3:25 PM Stephen Frost <sfrost@snowman.net> wrote:
> > > * Kohei KaiGai (kaigai@heterodb.com) wrote:
> > > > 2019年7月25日(木) 3:52 Yuli Khodorkovskiy <yuli.khodorkovskiy@crunchydata.com>:
> > > > > Since all DAC checks should have corresponding MAC, this patch adds a
> > > > > hook to allow extensions to implement a MAC check on TRUNCATE. I have
> > > > > also implemented this access check in the sepgsql extension.
> > > > >
> > > > > One important thing to note is that refpolicy [1] and Redhat based
> > > > > distributions do not have the SELinux permission for db_table {truncate}
> > > > > implemented.
> > > > >
> > > > How db_table:{delete} permission is different from truncate?
> > > > >From the standpoint of data access, TRUNCATE is equivalent to DELETE
> > > > without WHERE, isn't it?
> > > > Of course, there are some differences between them. TRUNCATE takes
> > > > exclusive locks and eliminates underlying data blocks, on the other hands,
> > > > DELETE removes rows under MVCC manner. However, both of them
> > > > eventually removes data from the target table.
> > > >
> > > > I like to recommend to reuse "db_table:{delete}" permission for TRUNCATE.
> > > > How about your opinions?
> > >
> > > There's been much discussion and justifcation for adding an independent
> > > TRUNCATE privilege to GRANT (which actually took many years to be
> > > allowed).  I don't see why we wouldn't represent that as a different
> > > privilege to external MAC systems.  If the external MAC system wishes to
> > > use db_table:{delete} to decide if the privilege is allowed or not, they
> > > can, but I don't think core should force that when we have them as
> > > independent permissions.
> > >
> > > So, perhaps we can argue about what the sepgsql extension should do, but
> > > it's clear that we should have an independent hook for this in core.
> > >
> > > Isn't there a way to allow an admin to control if db_table:{truncate} is
> > > allowed for users with db_table:{delete}, or not?  Ideally, this could
> > > be managed at the SELinux level instead of having to have something
> > > different in sepgsql or core, but if it needs to be configurable there
> > > too then hopefully we can come up with a good solution.
> >
> > If I understand you correctly, you are asking if an SELinux domain can
> > have the db_table:{truncate} permission but not db_table:{delete}
> > using SELinux policy? This would only work if the userspace object
> > manager, sepgsql in this case, reaches out to the policy server to
> > check if db_table:{truncate} is allowed for a subject accessing an
> > object.
>
> I was saying that, I believe, it would be pretty straight-forward for an
> SELinux admin to add db_table:{truncate} to whatever set of individuals
> are allowed to use db_table:{delete}.

Okay that makes sense. Yes that can definitely be done, and the
original sepgsql patch accomplished what you are describing. I did not
add tests or SELinux policy granting `db_table: { truncate }` in the
regressions of the original patch. If the community decides a new
SELinux permission in sepgsql for TRUNCATE is the correct path, I will
gladly update the original patch.

>
> > I think it should be okay to use db_table:{delete} as the permission
> > to check for TRUNCATE in the object manager. I have attached a second
> > version of the hook and sepgsql changes to demonstrate this.
>
> There are actual reasons why the 'DELETE' privilege is *not* the same as
> 'TRUNCATE' in PostgreSQL and I'm really not convinced that we should
> just be tossing that distinction out the window for users of SELinux.  A
> pretty obvious one is that DELETE triggers don't get fired for a
> TRUNCATE command, but TRUNCATE also doesn't follow the same MVCC rules
> that the rest of the system does.

I do agree with you there should be a distinction between TRUNCATE and
DELETE in the SELinux perms. I'll wait a few days for more discussion
and send an updated patch.

Thank you.

>
> If TRUNCATE and DELETE were the same, we'd only have DELETE and we would
> just make it super-fast by implementing it the way we implement
> TRUNCATE.
>
> Thanks,
>
> Stephen