[apparmor] Bug#735470: Fwd: Bug#735470: Could be implemented centrally with a dpkg trigger instead of requiring every package shipping an apparmor file to use dh_apparmor

[John Johansen]

On 01/16/2014 03:03 PM, Kees Cook wrote:
> On Thu, Jan 16, 2014 at 02:59:54PM -0800, John Johansen wrote:
>> On 01/16/2014 02:57 PM, John Johansen wrote:
>>> On 01/16/2014 02:49 PM, Kees Cook wrote:
>>>> On Thu, Jan 16, 2014 at 07:37:04PM +0100, Didier 'OdyX' Raboud wrote:
>>>>> Le jeudi, 16 janvier 2014 10.14:14, vous avez écrit :
>>>>>> On Thu, Jan 16, 2014 at 11:11:22AM +0100, Didier 'OdyX' Raboud wrote:
>>>>>>> As far as I understand deb-triggers' manpage, this can be enforced
>>>>>>> using 'activate /etc/apparmor.d/', which will then make the trigger
>>>>>>> run "at the start of the configure operation", which ensures
>>>>>>> exactly what you want.
>>>>>>
>>>>>> Per-policy reloads must happen before a daemon restarts, so they
>>>>>> cannot be triggers.
>>>>>
>>>>> Err…
>>>>>
>>>>> man deb-trigggers contradicts you, in my reading; an 'activate 
>>>>> /etc/apparmor.d' triggers' file in apparmor would make its action run 
>>>>> _before_ cups (which would have shipped /etc/apparmor.d/usr.sbin.cupsd) 
>>>>> would be 'configured' (hence its postinst run).
>>>>>
>>>>> Isn't it?
>>>>
>>>> Right, sorry, you are right, but my original observation stands: we should
>>>> never reload all apparmor profiles when installing a single profile. Just
>>>> the single profile should be reloaded. Otherwise we end up doing very
>>>> CPU expensive work for no reason. The point of dh-apparmor is to reload a
>>>> single profile, not all of them. Doing a trigger for all-profile reload
>>>> isn't something we want. Think of the situation where someone has 5000
>>>> apache virtual host profiles and they update cups. We never want to wait
>>>> for those 5000 to be reloaded when cups's profile is installed. Hence,
>>>> dh_apparmor.
>>>>
>>> Is there a way for a trigger to notice which file was updated?
>>> That way we could use a trigger.
>>>
>>> If not another option that comes to mind is we could add a new flag to the
>>> parser that would say reload only if the cache is out of date.
>>>
>>> The trigger would have to still do work figuring out which cache files
>>> are out of date but thats still better than reloading everything to the
>>> kernel.
>>>
>> by trigger, I mean the parser/script called by the trigger.
> 
> I can't remember -- does the parser do the right thing in the face of
> included files timestamps? If so, yeah, this could work.
> 
Here is a start on the patch,

It does not at the moment consider what is loaded into the kernel, but only
works off of the cache time stamps. For older kernels there really isn't
a way to check if what is loaded in the kernel is up to date. However
for 3.12+ kernels we could leverage the policy dir to compare timestamps
or if policy hashing is enabled we could compare to the sha1 stored for
each profile that is loaded.

we can of course also change the letter chosen

---

---
 parser/parser.h        |    1 +
 parser/parser_common.c |    1 +
 parser/parser_main.c   |   15 ++++++++++++++-
 3 files changed, 16 insertions(+), 1 deletion(-)

--- 2.9-test.orig/parser/parser.h
+++ 2.9-test/parser/parser.h
@@ -229,6 +229,7 @@
 extern int conf_quiet;
 extern int names_only;
 extern int option;
+extern int update_only;
 extern int current_lineno;
 extern dfaflags_t dfaflags;
 extern const char *progname;
--- 2.9-test.orig/parser/parser_common.c
+++ 2.9-test/parser/parser_common.c
@@ -33,6 +33,7 @@
 int names_only = 0;
 int current_lineno = 1;
 int option = OPTION_ADD;
+int update_only = 0;
 
 dfaflags_t dfaflags = (dfaflags_t)(DFA_CONTROL_TREE_NORMAL | DFA_CONTROL_TREE_SIMPLE | DFA_CONTROL_MINIMIZE );
 
--- 2.9-test.orig/parser/parser_main.c
+++ 2.9-test/parser/parser_main.c
@@ -96,6 +96,7 @@
 	{"help",		2, 0, 'h'},
 	{"replace",		0, 0, 'r'},
 	{"reload",		0, 0, 'r'},	/* undocumented reload option == replace */
+	{"update",		0, 0, 'u'},
 	{"version",		0, 0, 'V'},
 	{"complain",		0, 0, 'C'},
 	{"Complain",		0, 0, 'C'},	/* Erk, apparently documented as --Complain */
@@ -144,6 +145,7 @@
 	       "-a, --add		Add apparmor definitions [default]\n"
 	       "-r, --replace		Replace apparmor definitions\n"
 	       "-R, --remove		Remove apparmor definitions\n"
+	       "-u, --update		Only load/replace if updated, ie. cache is out of date\n"
 	       "-C, --Complain		Force the profile into complain mode\n"
 	       "-B, --binary		Input is precompiled profile\n"
 	       "-N, --names		Dump names of profiles in input.\n"
@@ -294,6 +296,9 @@
 		option = OPTION_REMOVE;
 		skip_cache = 1;
 		break;
+	case 'u':
+		update_only++;
+		break;
 	case 'V':
 		display_version();
 		exit(0);
@@ -430,7 +435,7 @@
 	int count = 0;
 	option = OPTION_ADD;
 
-	while ((c = getopt_long(argc, argv, "adf:h::rRVvI:b:BCD:NSm:qQn:XKTWkO:po:", long_options, &o)) != -1)
+	while ((c = getopt_long(argc, argv, "adf:h::rRuVvI:b:BCD:NSm:qQn:XKTWkO:po:", long_options, &o)) != -1)
 	{
 		count += process_arg(c, optarg);
 	}
@@ -873,6 +878,14 @@
 		if (!skip_read_cache &&
 		    stat(cachename, &stat_bin) == 0 &&
 		    stat_bin.st_size > 0 && (mru_t_cmp(stat_bin.st_mtim))) {
+			if (update_only) {
+				/* cache hit means profile is not out of
+				 * date so don't load
+				 */
+				if (show_cache)
+					PERROR("skipping load of up to date: %s\n", cachename);
+				goto out;	/* retval already 0 */
+			}
 			if (show_cache)
 				PERROR("Cache hit: %s\n", cachename);
 			retval = process_binary(option, cachename);