On Wed, Jun 1, 2016 at 5:37 AM, Christian Boltz <appar...@cboltz.de> wrote:
> Hello,
>
> 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:
> - get_profile() missed a required parameter in a serialize_profile() call
> - when saving the profile, it was written to extra_profile_dir, not to
> profile_dir where it (as a now-active profile) should be. This is
> fixed by removing the filename from existing_profiles{} so that it can
> pick up the default name.
> - CMD_FINISHED (when asking if the extra profile should be used or a new
> one) behaved exactly like CMD_CREATE_PROFILE, but this is surprising
> for the user. Remove it to avoid confusion.
> - displaying the extra profile was only implemented in YaST mode
> - get_pager() returned None, not an actual pager. Since we have 'less'
> hardcoded at several places, also return it in get_pager()
>
> Finally, also remove CMD_FINISHED from the get_profile() test in
> test-translations.py.
>
>
> 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*
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)
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?
> +
> # If no profiles in repo and no inactive profiles
> if not profile_hash.keys():
> return None
> @@ -604,18 +607,13 @@
>
> q = aaui.PromptQuestion()
> q.headers = ['Profile', prof_name]
> - q.functions = ['CMD_VIEW_PROFILE', 'CMD_USE_PROFILE',
> 'CMD_CREATE_PROFILE',
> - 'CMD_ABORT', 'CMD_FINISHED']
> + q.functions = ['CMD_VIEW_PROFILE', 'CMD_USE_PROFILE',
> 'CMD_CREATE_PROFILE', 'CMD_ABORT']
> q.default = "CMD_VIEW_PROFILE"
> q.options = options
> q.selected = 0
>
> ans = ''
> while 'CMD_USE_PROFILE' not in ans and 'CMD_CREATE_PROFILE' not in
> ans:
> - if ans == 'CMD_FINISHED':
> - save_profiles()
> - return
> -
> ans, arg = q.promptUser()
> p = profile_hash[options[arg]]
> q.selected = options.index(options[arg])
> @@ -627,12 +625,13 @@
> 'profile_type': p['profile_type']
> })
> ypath, yarg = GetDataFromYast()
> - #else:
> - # pager = get_pager()
> - # proc = subprocess.Popen(pager, stdin=subprocess.PIPE)
> + else:
> + pager = get_pager()
> + proc = subprocess.Popen(pager, stdin=subprocess.PIPE)
> # proc.communicate('Profile submitted by %s:\n\n%s\n\n' %
> # (options[arg], p['profile']))
> - # proc.kill()
> + proc.communicate(p['profile'].encode())
>
space before encode
> + proc.kill()
> elif ans == 'CMD_USE_PROFILE':
> if p['profile_type'] == 'INACTIVE_LOCAL':
> profile_data = p['profile_data']
> @@ -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?
> extra_profile_dir filename
> attach_profile_data(aa, profile_data)
> attach_profile_data(original_aa, profile_data)
> if os.path.isfile(profile_dir + '/tunables/global'):
> @@ -1960,7 +1970,7 @@
> reload_base(profile_name)
>
> def get_pager():
> - pass
> + return 'less'
>
lol pager less
>
> def generate_diff(oldprofile, newprofile):
> oldtemp = tempfile.NamedTemporaryFile('w')
> @@ -2204,7 +2214,7 @@
> except:
> fatal_error(_("Can't read AppArmor profiles in %s") %
> extra_profile_dir)
>
> - for file in os.listdir(profile_dir):
> + for file in os.listdir(extra_profile_dir):
> if os.path.isfile(extra_profile_dir + '/' + file):
> if is_skippable_file(file):
> continue
>
> === modified file 'utils/test/test-translations.py'
> --- utils/test/test-translations.py 2016-05-14 11:25:15 +0000
> +++ utils/test/test-translations.py 2016-05-31 23:58:23 +0000
> @@ -24,7 +24,7 @@
> (['CMD_ALLOW', 'CMD_DENY', 'CMD_IGNORE_ENTRY', 'CMD_GLOB',
> 'CMD_GLOBEXT', 'CMD_NEW', 'CMD_AUDIT_OFF', 'CMD_ABORT', 'CMD_FINISHED'],
> True), # aa.py available_buttons() with CMD_AUDIT_OFF
> (['CMD_ALLOW', 'CMD_DENY', 'CMD_IGNORE_ENTRY', 'CMD_GLOB',
> 'CMD_GLOBEXT', 'CMD_NEW', 'CMD_AUDIT_NEW', 'CMD_ABORT', 'CMD_FINISHED'],
> True), # aa.py available_buttons() with CMD_AUDIT_NEW
> (['CMD_SAVE_CHANGES', 'CMD_SAVE_SELECTED', 'CMD_VIEW_CHANGES',
> 'CMD_VIEW_CHANGES_CLEAN', 'CMD_ABORT'],
> True), # aa.py save_profiles()
> - (['CMD_VIEW_PROFILE', 'CMD_USE_PROFILE', 'CMD_CREATE_PROFILE',
> 'CMD_ABORT', 'CMD_FINISHED'],
> True), # aa.py get_profile()
> + (['CMD_VIEW_PROFILE', 'CMD_USE_PROFILE', 'CMD_CREATE_PROFILE',
> 'CMD_ABORT'],
> True), # aa.py get_profile()
> (['CMD_UPLOAD_CHANGES', 'CMD_VIEW_CHANGES', 'CMD_ASK_LATER',
> 'CMD_ASK_NEVER', 'CMD_ABORT'],
> True), # aa.py console_select_and_upload_profiles()
> (['CMD_ix', 'CMD_pix', 'CMD_cix', 'CMD_nix', 'CMD_EXEC_IX_OFF',
> 'CMD_ux', 'CMD_DENY', 'CMD_ABORT', 'CMD_FINISHED'], True),
> # aa.py build_x_functions() with exec_toggle
> (['CMD_ix', 'CMD_cx', 'CMD_px', 'CMD_nx', 'CMD_ux',
> 'CMD_EXEC_IX_ON', 'CMD_DENY', 'CMD_ABORT', 'CMD_FINISHED'],
> True), # aa.py build_x_functions() without exec_toggle
>
>
> I agree to the sentiment that this is essentially a bugfix for a long
broken feature.
Acked-by: Kshitij Gupta <kgupta8...@gmail.com>
>
> Regards,
>
> Christian Boltz
> --
> > > Vielen Dank, daß du dir die Zeit nimmst, dran rumzutesten.
> > Wenn Du es nicht gemerkt hast: Ich empfehle Dir jetzt die Features,
> > die ich gern hätte. Geht schneller, als es selbst zu bauen *g*
> Verdammt. Ich wurde reingelegt! ;-))
> [Ratti zu > $me beim Testen seiner Fontlinge]
>
> --
> AppArmor mailing list
> AppArmor@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
>
>
--
Regards,
Kshitij Gupta
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
