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

[Nicolas Iooss <nicolas.iooss@m4x.org>]

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?

Anyway, thanks for the patches!
Nicolas