Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 9 Nov 2017 15:52:46 -0500
From:   Richard Guy Briggs <rgb@redhat.com>
To:     Paul Moore <paul@paul-moore.com>
Cc:     Steve Grubb <sgrubb@redhat.com>, linux-audit@redhat.com,
        linux-kernel@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with
 anonymous parents
Message-ID: <20171109205246.ohro2gbsnvc7hpij@madcap2.tricolour.ca>
References: <fa0938a05c2d783556d67d4dafef208b156a35ad.1503484357.git.rgb@redhat.com>
 <5662600.QY0GDuKsRv@x2>
 <CAHC9VhSb5Q8hSwvzvyY6rfZZDyC0NnOghokTf=FqFKXX6RUt9Q@mail.gmail.com>
 <2668378.rW6Oo1akt8@x2>
 <CAHC9VhQMzR06Tqr8WNDuV9zgnDfH8reaKam+e_sAFCm1ZK3QQQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHC9VhQMzR06Tqr8WNDuV9zgnDfH8reaKam+e_sAFCm1ZK3QQQ@mail.gmail.com>
User-Agent: NeoMutt/20171013
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2017-11-09 10:59, Paul Moore wrote:
> On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
> >> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> 
> ...
> 
> >> > Late reply...but I just noticed that this changes the format of the "name"
> >> > field - which is undesirable. Please put the file system type in a field
> >> > all by itself called "fstype". You can just leave it as the hex magic
> >> > number prepended with 0x and user space can do the lookup from there,
> >> >
> >> > It might be simplest to just apply a corrective patch over top of this one
> >> > so that you don't have to muck about with git branches and commit
> >> > messages.
> >>
> >> A quick note on the "corrective patch": given we are just days away
> >> from the merge window opening, it is *way* to late for something like
> >> that, at this point the only options are to leave it as-is or
> >> yank/revert and make another pass during the next development phase.
> >
> > Then yank it. I think that is overreacting but given the options you presented
> > its the only one that avoids changing a critical field format.
> 
> It's not overreacting Steve, there is simply no way we can test and
> adequately soak new changes in the few days we have left.  Event
> yanks/reverts carry a risk at this stage, but I consider that the less
> risky option for these patches.  Neither is a great option, and that
> is why I'm rather annoyed.

I don't really see that this is my choice to include it or not.  This is
the upstream maintainer's decision.

I can't say I'd be thrilled to have my name on something that stuffs up
the system though.  It still isn't clear to me why an incomplete path
from some seemingly random place in the filesystem tree is preferable to
something that gives it an anchor point, at least to human interpreters.
Adding an fstype to the record is an interesting idea, but then creates
a void for all the rest of the properly formed records that don't need
it and will need more work to find it, wasting bandwidth with
"fstype=?".  How are the analysis tools stymied by a text prefix to a
path that it can't find anyways?

Since we have a chance to fix it before it goes upstream, I think it
should either be yanked and respun, or add a corrective patch and submit
them together.

> >> As for the objection itself: ungh.  There is really no good reason why
> >> you couldn't have seen this in the *several* *months* prior to this;
> >> Richard wrote a nice patch description which *included* sample audit
> >> events, and you were involved in discussions regarding this patchset.
> >> To say I'm disappointed would be an understatement.
> >
> > I am also disappointed to find that we are modifying a searchable field that
> > has been defined since 2005. The "name" field is very important. It's used in
> > quite a few reports, its used in the text format, it's searchable, and we have
> > a dictionary that defines exactly what it is. Fields that are searchable and
> > used in common reports cannot be changed without a whole lot of coordination.
> > I'm also disappointed to have to point out that new information should go in
> > its own field. I thought this was common knowledge. In any event, it was
> > caught and problems can be avoided.

So why does this make it unsearchable?  I still don't understand any
explanations that have been made so far.

> There are plenty of things to say about the above comment, but in the
> interest of brevity I'm just going to leave it at the assumptions and
> inflexibility in your audit userspace continue to amaze me in all the
> worst ways.  Regardless, as you say, the problem can likely be avoided
> this time.
> 
> >> I need to look at the rest of audit/next to see what a mess things
> >> would be if I yanked this patch.  I don't expect it to be bad, but
> >> taking a look will also give Richard a chance to voice his thoughts;
> >> it is his patch after all, it would be nice to see an "OK" from him.
> >> Whatever we do, it needs to happen by the of the day today (Thursday,
> >> November 9th) as we need time to build and test the revised patches.
> 
> FWIW, I just went through audit/next and it looks like yanking patch
> 1/2 isn't going to be too painful; I'm waiting on the build to finish
> now.  Also, as a FYI, Richard's 2/2 filtering patch is going to remain
> in audit/next as that appears unrelated to the pathname objection,
> applies cleanly, and still offers value.

The irony here stuns me.  2/2 was supposed to be the more controvertial
one.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

From 1583604972006085773@xxx Thu Nov 09 16:03:09 +0000 2017
X-GM-THRID: 1576519731154696149
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread