Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Steve Grubb <sgrubb@redhat.com>]

On Thursday, October 12, 2017 6:51:19 PM EDT Paul Moore wrote:
> On Thu, Oct 12, 2017 at 6:13 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Thursday, October 12, 2017 5:04:41 PM EDT Paul Moore wrote:
> >> Another reminder that in general I'm not going to accept patches that
> >> shuffle the fields or insert fields in the middle of a record; if you
> >> want to add new fields to a record, add them at the end.  I see no
> >> reason to break with the rule for this patch.
> > 
> > This has never been a rule ...
> 
> Yes it has, one I've mentioned to you several times both on and off
> the list.  You may disagree with it, but that doesn't mean you are
> exempt.

I'm speaking on behalf of everyone that has to deal with the events. If we 
need to have 9 parsers for the same event, its really doing a disservice to 
everyone that works on audit events. Besides, since you don't write any user 
space code or do things that have to make sense of it, why would you block 
something that is fixing problems? Did you run any tests with the events I 
supplied to verify things for yourself?

Look at this mess:

1)	audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
	audit_log_session_info(ab);
	rc = audit_log_task_context(ab);

2) 	audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
	audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);

3)	audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
	audit_log_format(ab, " op=trim res=1");

4)  audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
	audit_log_format(ab, " op=make_equiv old=");
	audit_log_untrustedstring(ab, old);
	audit_log_format(ab, " new=");
	audit_log_untrustedstring(ab, new);
	audit_log_format(ab, " res=%d", !err);

5)	audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
	audit_log_format(ab, " op=tty_set old-enabled=%d new-enabled=%d"
		" old-log_passwd=%d new-log_passwd=%d res=%d",
		old.enabled, s.enabled, old.log_passwd,
		s.log_passwd, !err);

6)	audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid);
	audit_log_task_context(ab);
	audit_log_format(ab, " op=%s", action);
	audit_log_key(ab, rule->filterkey);
	audit_log_format(ab, " list=%d res=%d", rule->listnr, res);

7)	audit_log_format(ab, "auid=%u ses=%u op=%s",
		from_kuid(&init_user_ns, audit_get_loginuid(current)),
		audit_get_sessionid(current), op);
	audit_log_format(ab, " path=");
	audit_log_untrustedstring(ab, audit_mark->path);
	audit_log_key(ab, rule->filterkey);
	audit_log_format(ab, " list=%d res=1", rule->listnr);

8)	audit_log_format(ab, "op=remove_rule");
	audit_log_format(ab, " dir=");
	audit_log_untrustedstring(ab, rule->tree->pathname);
	audit_log_key(ab, rule->filterkey);
	audit_log_format(ab, " list=%d res=1", rule->listnr);

9)	audit_log_format(ab, "auid=%u ses=%u op=%s",
		from_kuid(&init_user_ns, audit_get_loginuid(current)),
		audit_get_sessionid(current), op);
	audit_log_format(ab, " path=");
	audit_log_untrustedstring(ab, w->path);
	audit_log_key(ab, r->filterkey);
	audit_log_format(ab, " list=%d res=1", r->listnr);

Of these 7 & 9 are the same. So that means following your suggestion, everyone 
has to write 8 parsers for the same event. Does that sound like good 
engineering practice? Also, it will cause subtle changes that break bigger 
rules than this - like all events everywhere end with the results. They are 
always the last item. Everywhere. This is a self evident rule that you are 
suggesting we break. We can't do that.

Meanwhile...there is a problem that needs to be fixed....

-Steve