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

[Christian Boltz]

Hello,

Am Mittwoch, 1. Juni 2016, 23:06:41 CEST schrieb Kshitij Gupta:
> On Wed, Jun 1, 2016 at 5:37 AM, Christian Boltz <apparmor at cboltz.de> 
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? ;-)
...
> > I propose this patch for 2.9, 2.10 and trunk
> > (test-translations.py is only in trunk, therefore this part of the
> > patch is obviously trunk-only.)
> > 
> > 
> > [ 01-genprof-ask-for-extra-dir.diff ]
> > 
> > === modified file ./utils/apparmor/aa.py
> > --- utils/apparmor/aa.py        2016-05-30 23:16:05.713448348 +0200
> > +++ utils/apparmor/aa.py        2016-06-01 01:25:31.242505830 +0200
> > @@ -578,8 +578,11 @@
> > 
> >          inactive_profile[prof_name][prof_name].pop('filename')
> >          profile_hash[uname]['username'] = uname
> >          profile_hash[uname]['profile_type'] = 'INACTIVE_LOCAL'
> > 
> > -        profile_hash[uname]['profile'] =
> > serialize_profile(inactive_profile[prof_name], prof_name)
> > +        profile_hash[uname]['profile'] =
> > serialize_profile(inactive_profile[prof_name], prof_name, None)
> 
> Wouldn't it be better to then make the parameter optional with default
> value as None, I have two reasons:
> 1. I see the two parameter form littered around in aa.py *hides*

I noticed that and intentionally didn't fix it because it points out dead 
code ;-)  Well, at least to someone who knows about the function 
parameters, so "me yesterday" ;-)

> 2. The function has built in handling for cases when the options are
> not present and sort of makes sense to have an option free serialiser
> with a default
> 3. The None there looks absolutely meaningless and confusing (to me
> and explains why I above said two reasonsand not 3)

Sounds like good arguments - patches welcome ;-)


> >          profile_hash[uname]['profile_data'] = inactive_profile
> > +
> > +        existing_profiles.pop(prof_name)  # remove profile filename
> > from list to force storing in /etc/apparmor.d/ instead of
> > extra_profile_dir
> force storing in profile_dir you mean?

Yes, of course. I just used /etc/apparmor.d to make the command easier 
to understand ;-)

> > +                proc.communicate(p['profile'].encode())
> 
> space before encode

[comment ignored as discussed on IRC]

> > @@ -683,6 +682,7 @@
> > 
> >      if not profile_data:
> >          profile_data = create_new_profile(pname)
> >      
> >      file = get_profile_filename(pname)
> > 
> > +    profile_data[pname][pname]['filename'] = None  # will be stored
> > in /etc/apparmor.d when saving, so it shouldn't carry the
> 
> profile_dir?

see above ;-)

> I agree to the sentiment that this is essentially a bugfix for a
> long broken feature.
> 
> Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>

Thanks!


Regards,

Christian Boltz
-- 
Die drei Todfeinde des Programmieres:
Sonnenlicht, frische Luft und das unerträgliche Gebrüll der Vögel.
-------------- 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/a968d5ce/attachment.pgp>