Re: [PATCH 1/2] python/semanage module: Fix handling of -a/-e/-d/-r options

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

On Fri, Feb 15, 2019 at 3:28 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> Nicolas Iooss <nicolas.iooss@m4x.org> writes:
>
> > On Wed, Feb 6, 2019 at 8:43 PM Petr Lautrbach <plautrba@redhat.com> wrote:
> >>
> >> Previous code traceback-ed when one of the mentioned option was used without
> >> any argument as this state was not handled by the argument parser.
> >>
> >> action='store' stores arguments as a list while the original
> >> action='store_const' used str therefore the particular interfaces in
> >> moduleRecords are changed to be compatible with both.
> >>
> >> Fixes:
> >> ^_^ semanage module -a
> >> Traceback (most recent call last):
> >>   File "/usr/sbin/semanage", line 963, in <module>
> >>     do_parser()
> >>   File "/usr/sbin/semanage", line 942, in do_parser
> >>     args.func(args)
> >>   File "/usr/sbin/semanage", line 608, in handleModule
> >>     OBJECT.add(args.module_name, args.priority)
> >>   File "/usr/lib/python3.7/site-packages/seobject.py", line 402, in add
> >>     if not os.path.exists(file):
> >>   File "/usr/lib64/python3.7/genericpath.py", line 19, in exists
> >>     os.stat(path)
> >> TypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType
> >>
> >> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> >
> > Nice bug :) Nevertheless "if type(module) == str" troubles me because
> > I except a function to only accept one kind of arguments (either a
> > list of strings or a string, but not both).
>
> The idea was to have backward compatible code. Originally, semanage used
> to send a string, while the new code sends a list. I didn't want to
> break 3rd party and I wanted to avoid converting of the list from
> action_enable to string and the converting it back to list in
> moduleRecords.set_enabled()
>
>
> > Moreover this is
> > Python3-only code and semanage's shebang does not specify a Python
> > version (the Python2-equivalent code would have been "if
> > isinstance(module, basestring)").
>
> Works for me:
>
> > python2
> Python 2.7.15 (default, Feb  2 2019, 16:04:51)
> [GCC 9.0.1 20190129 (Red Hat 9.0.1-0.2)] on linux2
> Type "help", "copyright", "credits" or "license" for more information.
> >>> type("name")
> <type 'str'>
> >>> type(["name"])
> <type 'list'>
> >>> type("name") == str
> True
> >>> type(["name"]) == str
> False
>
>
> https://docs.python.org/2/library/functions.html#type

I had unicode strings in mind:

python2
>>> type(u'abc')
<type 'unicode'>
>>> type(u'abc') == str
False
>>> isinstance(u'abc', basestring)
True

Nevertheless, it seems that argparse does not introduce unicode
strings when UTF-8 characters are provided as command-line parameters,
so your code worked.

> >
> > I would prefer if the new code looked like this (that I have not tested):
> >
> >     def set_enabled(self, modules, enable):
> >         for item_modules in modules:
> >             for m in item_modules.split():
> >                 # ...
>
> Or just convert action_enable to string before it's sent to
> moduleRecords.set_enabled():
>
> --- a/python/semanage/semanage
> +++ b/python/semanage/semanage
> @@ -612,9 +612,9 @@ def handleModule(args):
>      if args.action_add:
>          OBJECT.add(args.action_add, args.priority)
>      if args.action_enable:
> -        OBJECT.set_enabled(args.action_enable, True)
> +        OBJECT.set_enabled(args.action_enable.join(" "), True)
>      if args.action_disable:
> -        OBJECT.set_enabled(args.action_disable, False)
> +        OBJECT.set_enabled(args.action_disable.join(" "), False)
>      if args.action_remove:
>          OBJECT.delete(args.action_remove, args.priority)

Indeed. I like it better, thanks!

> > Moreover the "file = file[0]" in moduleRecords.add() looks strange
> > without a context, which is in handleModule(). I would prefer if this
> > operation occurred in semanage, where it is clear that args.action_add
> > always has a single item (because « action='store', nargs=1 »):
> >
> >     if args.action_add:
> >         OBJECT.add(args.action_add[0], args.priority)
>
> Good idea.
>
> >
> > Nicolas
> >
> > PS: As setools is now Python3-only and seobject.py requires it, it
> > seems to be a good time to update the shebang to "#!/usr/bin/python3
> > -Es".
>
> seobject.py is just a module which is not supposed to be entry point.
> The shebang does not make sense here and should be removed.
>
> But I'd change at least semanage and chcat as they both directly imports
> seobject.
>
> Or rather change the whole project to python3 by default.

I agree with changing all the shebangs in the project to python3 (as
python/sepolicy/sepolicy/__init__.py also imports setools, there are
more tools that are broken with a Python3-only setools and a Python2
"python" binary).

Nicolas