[apparmor] [patch] aa-genprof: ask about profiles in extra dir (again)

[Christian Boltz]

Hello,

Am Dienstag, 31. Mai 2016, 17:22:16 CEST schrieb Seth Arnold:
> On Wed, Jun 01, 2016 at 02:07:10AM +0200, Christian Boltz wrote:
> > thanks to reading the wrong directory in read_inactive_profiles()
> > (profile_dir instead of extra_profile_dir), aa-genprof never asked
> > about using a profile from the extra_profile_dir.
> > 
> > Sounds like an easy fix, right? ;-)
> > 
> > After fixing this (last chunk), several other errors popped up, one
> > after the other:
...
> > I propose this patch for 2.9, 2.10 and trunk
> 
> Acked-by: Seth Arnold <seth.arnold at canonical.com>
> for trunk
> 
> Nice set of fixes; however I'm uncomfortable with making that large a
> change on 2.9, 2.10. I know it's only a wee tiny little code change
> but the semantic idea of the change is quite a bit larger, and it
> doesn't feel appropriate to me to make the change anywhere except
> trunk. (I won't object if someone else ACKs it for 2.9, 2.10.. I just
> don't feel comfortable myself.)

Well, it introduces a behaviour change if a profile exists in extra, 
(asking if the user wants to use this profile as starting point), but
I'd argue that this fixes a regression that was introduced in 2.9 during 
the rewrite to python ;-)

BTW: I tested the patch on top of trunk and the 2.10 and 2.9 bzr branches, 
and it works everywhere.

@John: is your ACK also valid for 2.10 and 2.9?

> I'm also confused by the removal of the CMD_FINISHED; how do you end
> an aa-genprof or aa-logprof session?

CMD_FINISHED is only removed in the question that asks if you want to 
use the profile in extra or if you want to create a new one. And you 
still have Abo(r)t as an option:

# python3 aa-genprof /usr/bin/skype

Profile: /usr/bin/skype

 [1 - Inactive local profile for /usr/bin/skype]                                                                                                                                  
[(V)iew Profile] / (U)se Profile / (C)reate New Profile / Abo(r)t    <--- (F)inish removed here
Writing updated profile for /usr/bin/skype.                                                                                                                                       
Setting /usr/bin/skype to complain mode.                                                                                                                                          

Before you begin, [...]

Profiling: /usr/bin/skype                                                                                                                                                                                                                                                      

[(S)can system log for AppArmor events] / (F)inish                                                                                                                                                                                                                             
[...]

Without my patch, there also was (F)inish at the first question.
However, it behaved exactly like (C)reate New Profile, and continued
with the   (S)can log / (F)inish   question. This means it was a) useless
and b) confusing.

> And, final question:
> > +    profile_data[pname][pname]['filename'] = None  # will be stored
> > in /etc/apparmor.d when saving, so it shouldn't carry the
> > extra_profile_dir filename
> Is this correct? Should it instead be:
> 
> del profile_data[pname][pname]['filename']

Both work. Setting it to None has the advantage that the data structure 
stays complete ('filename' is still there, just empty/None) and can be
used without a .get('filename') safeguard.

> I'm not sure what else may depend upon this being present-but-None vs
> not-even-present. (Half of why I'm excited about the death of the
> hasher..  :)

I'm using a POC "strict storage" patch since a while which changes
profile_storage() to return a dict() with a specified set of sub-items
(so I can notice access to funny[tm] uninitialized places without breaking 
it for users - and actually found some of those places, so I don't want
to merge this patch yet ;-)  I can of course post this patch in case
someone wants to play with it.

The code expects 'filename' to be in the data structure - all checks 
basically use   if ...['filename']:   so setting it to None doesn't 
cause any problems. I'm not aware of any   is None   or   == ''   checks
for it.

TL;DR: I'm quite sure something would explode when using "del" ;-) as
soon as my "strict storage" patch enters the stage.

In theory I could set it to the final filename instead of None - but
write_profile() basically does this itsself (using get_profile_filename())
if 'filename' is empty, so why should I do superfluous work? ;-)

That all said - hasher is not dead yet ;-)


Regards,

Christian Boltz
-- 
<BartOtten> and devs, well they are just a different type of
            human :) [...]
<suseROCKs> Devs are human??  :-)
[from #opensuse-project]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160601/3ade8605/attachment.pgp>