Re: [PATCH 3/4] python/sepolicy: Add sepolicy.load_store_policy(store)

[Petr Lautrbach <plautrba@redhat.com>]

Nicolas Iooss <nicolas.iooss@m4x.org> writes:

> On Thu, Dec 20, 2018 at 4:14 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>>
>> load_store_policy() allows to (re)load SELinux policy based on a store name. It
>> is useful when SELinux is disabled and default policy is not installed; or when
>> a user wants to query or manipulate another policy.
>>
>> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1558861
>>
>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>> ---
>>  python/sepolicy/sepolicy/__init__.py | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/python/sepolicy/sepolicy/__init__.py b/python/sepolicy/sepolicy/__init__.py
>> index fbeb731d..b69a6b94 100644
>> --- a/python/sepolicy/sepolicy/__init__.py
>> +++ b/python/sepolicy/sepolicy/__init__.py
>> @@ -129,6 +129,13 @@ def get_installed_policy(root="/"):
>>          pass
>>      raise ValueError(_("No SELinux Policy installed"))
>>
>> +def get_store_policy(store, root="/"):
>> +    try:
>> +        policies = glob.glob("%s%s/policy/policy.*" % (selinux.selinux_path(), store))
>> +        policies.sort()
>> +        return policies[-1]
>> +    except:
>> +        return None
>
> Hi, I agree this function is useful. Nevertheless the sorting order
> seems to be fragile because '100' < '99', so the policy filename needs
> to be parsed in order to extract the version as an integer and sort
> according to it. Moreover its second parameter ("root") is not used
> and I would rather avoid adding new bare excepts to the code base.
>
> I suggest the following implementation of this function:
>
> def get_store_policy(store):
>     """Get the path to the policy file located in the given store name"""
>     def policy_sortkey(policy_path):
>         # Parse the extension of a policy path which looks like
> .../policy/policy.31
>         extension = policy_path.rsplit('/policy.', 1)[1]
>         try:
>             return int(extension), policy_path
>         except ValueError:
>             # Fallback with sorting on the full path
>             return 0, policy_path
>     policies = glob.glob("%s%s/policy/policy.*" %
> (selinux.selinux_path(), store))
>     if not policies:
>         return None
>     # Return the policy with the higher version number
>     policies.sort(key=policy_sortkey)
>     return policies[-1] if policies else None
>
> It is more complex but fixes the issues I have identified. If you want
> to keep "root", it may be possible to use it with both
> "glob.glob("%s/%s/%s/policy/policy.*" % (root, selinux.selinux_path(),
> store))" and "return os.path.realpath(policies[-1]) if policies else
> None" (in order to simplify double-slashes into a single "/"
> character). What do you think of this?
>

It looks good to me. I'll only move policy_sortkey out of this function
and use it in also get_installed_policy() as this function use the
original sort method.

Petr