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

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

[Petr Lautrbach]

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>
---
 python/semanage/semanage    | 25 ++++++++++++-------------
 python/semanage/seobject.py | 10 ++++++++--
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/python/semanage/semanage b/python/semanage/semanage
index 6afeac14..9b737fa8 100644
--- a/python/semanage/semanage
+++ b/python/semanage/semanage
@@ -609,14 +609,14 @@ def setupInterfaceParser(subparsers):
 
 def handleModule(args):
     OBJECT = seobject.moduleRecords(args)
-    if args.action == "add":
-        OBJECT.add(args.module_name, args.priority)
-    if args.action == "enable":
-        OBJECT.set_enabled(args.module_name, True)
-    if args.action == "disable":
-        OBJECT.set_enabled(args.module_name, False)
-    if args.action == "remove":
-        OBJECT.delete(args.module_name, args.priority)
+    if args.action_add:
+        OBJECT.add(args.action_add, args.priority)
+    if args.action_enable:
+        OBJECT.set_enabled(args.action_enable, True)
+    if args.action_disable:
+        OBJECT.set_enabled(args.action_disable, False)
+    if args.action_remove:
+        OBJECT.delete(args.action_remove, args.priority)
     if args.action == "deleteall":
         OBJECT.deleteall()
     if args.action == "list":
@@ -635,14 +635,13 @@ def setupModuleParser(subparsers):
     parser_add_priority(moduleParser, "module")
 
     mgroup = moduleParser.add_mutually_exclusive_group(required=True)
-    parser_add_add(mgroup, "module")
     parser_add_list(mgroup, "module")
     parser_add_extract(mgroup, "module")
     parser_add_deleteall(mgroup, "module")
-    mgroup.add_argument('-r', '--remove', dest='action', action='store_const', const='remove', help=_("Remove a module"))
-    mgroup.add_argument('-d', '--disable', dest='action', action='store_const', const='disable', help=_("Disable a module"))
-    mgroup.add_argument('-e', '--enable', dest='action', action='store_const', const='enable', help=_("Enable a module"))
-    moduleParser.add_argument('module_name', nargs='?', default=None, help=_('Name of the module to act on'))
+    mgroup.add_argument('-a', '--add', dest='action_add', action='store', nargs=1, metavar='module_name', help=_("Add a module"))
+    mgroup.add_argument('-r', '--remove', dest='action_remove', action='store', nargs='+', metavar='module_name', help=_("Remove a module"))
+    mgroup.add_argument('-d', '--disable', dest='action_disable', action='store', nargs='+', metavar='module_name', help=_("Disable a module"))
+    mgroup.add_argument('-e', '--enable', dest='action_enable', action='store', nargs='+', metavar='module_name', help=_("Enable a module"))
     moduleParser.set_defaults(func=handleModule)
 
 
diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py
index 556d3ba5..cd2d3457 100644
--- a/python/semanage/seobject.py
+++ b/python/semanage/seobject.py
@@ -401,6 +401,8 @@ class moduleRecords(semanageRecords):
             print("%-25s %-9s %-5s %s" % (t[0], t[2], t[3], disabled))
 
     def add(self, file, priority):
+        if type(file) == list:
+            file = file[0]
         if not os.path.exists(file):
             raise ValueError(_("Module does not exist: %s ") % file)
 
@@ -413,7 +415,9 @@ class moduleRecords(semanageRecords):
             self.commit()
 
     def set_enabled(self, module, enable):
-        for m in module.split():
+        if type(module) == str:
+            module = module.split()
+        for m in module:
             rc, key = semanage_module_key_create(self.sh)
             if rc < 0:
                 raise ValueError(_("Could not create module key"))
@@ -435,7 +439,9 @@ class moduleRecords(semanageRecords):
         if rc < 0:
             raise ValueError(_("Invalid priority %d (needs to be between 1 and 999)") % priority)
 
-        for m in module.split():
+        if type(module) == str:
+            module = module.split()
+        for m in module:
             rc = semanage_module_remove(self.sh, m)
             if rc < 0 and rc != -2:
                 raise ValueError(_("Could not remove module %s (remove failed)") % m)
-- 
2.20.1

[PATCH 2/2] python/semanage: Use standard argparse.error() method in handlePermissive

[Petr Lautrbach]

This method prints a usage message including the message to the standard error
and terminates the program with a status code of 2.

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 python/semanage/semanage | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/python/semanage/semanage b/python/semanage/semanage
index 9b737fa8..a9fcb319 100644
--- a/python/semanage/semanage
+++ b/python/semanage/semanage
@@ -743,9 +743,7 @@ def handlePermissive(args):
         if args.action == "delete":
             OBJECT.delete(args.type)
     else:
-        args.parser.print_usage(sys.stderr)
-        sys.stderr.write(_('semanage permissive: error: the following argument is required: type\n'))
-        sys.exit(1)
+        args.parser.error(message=_('semanage permissive: error: the following argument is required: type\n'))
 
 
 def setupPermissiveParser(subparsers):
-- 
2.20.1

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

[Nicolas Iooss]

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). 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)").

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():
                # ...

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)

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".

> ---
>  python/semanage/semanage    | 25 ++++++++++++-------------
>  python/semanage/seobject.py | 10 ++++++++--
>  2 files changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/python/semanage/semanage b/python/semanage/semanage
> index 6afeac14..9b737fa8 100644
> --- a/python/semanage/semanage
> +++ b/python/semanage/semanage
> @@ -609,14 +609,14 @@ def setupInterfaceParser(subparsers):
>
>  def handleModule(args):
>      OBJECT = seobject.moduleRecords(args)
> -    if args.action == "add":
> -        OBJECT.add(args.module_name, args.priority)
> -    if args.action == "enable":
> -        OBJECT.set_enabled(args.module_name, True)
> -    if args.action == "disable":
> -        OBJECT.set_enabled(args.module_name, False)
> -    if args.action == "remove":
> -        OBJECT.delete(args.module_name, args.priority)
> +    if args.action_add:
> +        OBJECT.add(args.action_add, args.priority)
> +    if args.action_enable:
> +        OBJECT.set_enabled(args.action_enable, True)
> +    if args.action_disable:
> +        OBJECT.set_enabled(args.action_disable, False)
> +    if args.action_remove:
> +        OBJECT.delete(args.action_remove, args.priority)
>      if args.action == "deleteall":
>          OBJECT.deleteall()
>      if args.action == "list":
> @@ -635,14 +635,13 @@ def setupModuleParser(subparsers):
>      parser_add_priority(moduleParser, "module")
>
>      mgroup = moduleParser.add_mutually_exclusive_group(required=True)
> -    parser_add_add(mgroup, "module")
>      parser_add_list(mgroup, "module")
>      parser_add_extract(mgroup, "module")
>      parser_add_deleteall(mgroup, "module")
> -    mgroup.add_argument('-r', '--remove', dest='action', action='store_const', const='remove', help=_("Remove a module"))
> -    mgroup.add_argument('-d', '--disable', dest='action', action='store_const', const='disable', help=_("Disable a module"))
> -    mgroup.add_argument('-e', '--enable', dest='action', action='store_const', const='enable', help=_("Enable a module"))
> -    moduleParser.add_argument('module_name', nargs='?', default=None, help=_('Name of the module to act on'))
> +    mgroup.add_argument('-a', '--add', dest='action_add', action='store', nargs=1, metavar='module_name', help=_("Add a module"))
> +    mgroup.add_argument('-r', '--remove', dest='action_remove', action='store', nargs='+', metavar='module_name', help=_("Remove a module"))
> +    mgroup.add_argument('-d', '--disable', dest='action_disable', action='store', nargs='+', metavar='module_name', help=_("Disable a module"))
> +    mgroup.add_argument('-e', '--enable', dest='action_enable', action='store', nargs='+', metavar='module_name', help=_("Enable a module"))
>      moduleParser.set_defaults(func=handleModule)
>
>
> diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py
> index 556d3ba5..cd2d3457 100644
> --- a/python/semanage/seobject.py
> +++ b/python/semanage/seobject.py
> @@ -401,6 +401,8 @@ class moduleRecords(semanageRecords):
>              print("%-25s %-9s %-5s %s" % (t[0], t[2], t[3], disabled))
>
>      def add(self, file, priority):
> +        if type(file) == list:
> +            file = file[0]
>          if not os.path.exists(file):
>              raise ValueError(_("Module does not exist: %s ") % file)
>
> @@ -413,7 +415,9 @@ class moduleRecords(semanageRecords):
>              self.commit()
>
>      def set_enabled(self, module, enable):
> -        for m in module.split():
> +        if type(module) == str:
> +            module = module.split()
> +        for m in module:
>              rc, key = semanage_module_key_create(self.sh)
>              if rc < 0:
>                  raise ValueError(_("Could not create module key"))
> @@ -435,7 +439,9 @@ class moduleRecords(semanageRecords):
>          if rc < 0:
>              raise ValueError(_("Invalid priority %d (needs to be between 1 and 999)") % priority)
>
> -        for m in module.split():
> +        if type(module) == str:
> +            module = module.split()
> +        for m in module:
>              rc = semanage_module_remove(self.sh, m)
>              if rc < 0 and rc != -2:
>                  raise ValueError(_("Could not remove module %s (remove failed)") % m)
> --
> 2.20.1
>

Re: [PATCH 2/2] python/semanage: Use standard argparse.error() method in handlePermissive

[Nicolas Iooss]

On Wed, Feb 6, 2019 at 8:43 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> This method prints a usage message including the message to the standard error
> and terminates the program with a status code of 2.
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>

For this patch:
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

> ---
>  python/semanage/semanage | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/python/semanage/semanage b/python/semanage/semanage
> index 9b737fa8..a9fcb319 100644
> --- a/python/semanage/semanage
> +++ b/python/semanage/semanage
> @@ -743,9 +743,7 @@ def handlePermissive(args):
>          if args.action == "delete":
>              OBJECT.delete(args.type)
>      else:
> -        args.parser.print_usage(sys.stderr)
> -        sys.stderr.write(_('semanage permissive: error: the following argument is required: type\n'))
> -        sys.exit(1)
> +        args.parser.error(message=_('semanage permissive: error: the following argument is required: type\n'))
>
>
>  def setupPermissiveParser(subparsers):
> --
> 2.20.1
>

Re: [PATCH 2/2] python/semanage: Use standard argparse.error() method in handlePermissive

[Nicolas Iooss]

On Thu, Feb 7, 2019 at 10:47 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Wed, Feb 6, 2019 at 8:43 PM Petr Lautrbach <plautrba@redhat.com> wrote:
> >
> > This method prints a usage message including the message to the standard error
> > and terminates the program with a status code of 2.
> >
> > Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>
> For this patch:
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Merged.

Nicolas

> > ---
> >  python/semanage/semanage | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/python/semanage/semanage b/python/semanage/semanage
> > index 9b737fa8..a9fcb319 100644
> > --- a/python/semanage/semanage
> > +++ b/python/semanage/semanage
> > @@ -743,9 +743,7 @@ def handlePermissive(args):
> >          if args.action == "delete":
> >              OBJECT.delete(args.type)
> >      else:
> > -        args.parser.print_usage(sys.stderr)
> > -        sys.stderr.write(_('semanage permissive: error: the following argument is required: type\n'))
> > -        sys.exit(1)
> > +        args.parser.error(message=_('semanage permissive: error: the following argument is required: type\n'))
> >
> >
> >  def setupPermissiveParser(subparsers):
> > --
> > 2.20.1
> >

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

[Petr Lautrbach]

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 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)



> 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.



>
>> ---
>>  python/semanage/semanage    | 25 ++++++++++++-------------
>>  python/semanage/seobject.py | 10 ++++++++--
>>  2 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/python/semanage/semanage b/python/semanage/semanage
>> index 6afeac14..9b737fa8 100644
>> --- a/python/semanage/semanage
>> +++ b/python/semanage/semanage
>> @@ -609,14 +609,14 @@ def setupInterfaceParser(subparsers):
>>
>>  def handleModule(args):
>>      OBJECT = seobject.moduleRecords(args)
>> -    if args.action == "add":
>> -        OBJECT.add(args.module_name, args.priority)
>> -    if args.action == "enable":
>> -        OBJECT.set_enabled(args.module_name, True)
>> -    if args.action == "disable":
>> -        OBJECT.set_enabled(args.module_name, False)
>> -    if args.action == "remove":
>> -        OBJECT.delete(args.module_name, args.priority)
>> +    if args.action_add:
>> +        OBJECT.add(args.action_add, args.priority)
>> +    if args.action_enable:
>> +        OBJECT.set_enabled(args.action_enable, True)
>> +    if args.action_disable:
>> +        OBJECT.set_enabled(args.action_disable, False)
>> +    if args.action_remove:
>> +        OBJECT.delete(args.action_remove, args.priority)
>>      if args.action == "deleteall":
>>          OBJECT.deleteall()
>>      if args.action == "list":
>> @@ -635,14 +635,13 @@ def setupModuleParser(subparsers):
>>      parser_add_priority(moduleParser, "module")
>>
>>      mgroup = moduleParser.add_mutually_exclusive_group(required=True)
>> -    parser_add_add(mgroup, "module")
>>      parser_add_list(mgroup, "module")
>>      parser_add_extract(mgroup, "module")
>>      parser_add_deleteall(mgroup, "module")
>> -    mgroup.add_argument('-r', '--remove', dest='action', action='store_const', const='remove', help=_("Remove a module"))
>> -    mgroup.add_argument('-d', '--disable', dest='action', action='store_const', const='disable', help=_("Disable a module"))
>> -    mgroup.add_argument('-e', '--enable', dest='action', action='store_const', const='enable', help=_("Enable a module"))
>> -    moduleParser.add_argument('module_name', nargs='?', default=None, help=_('Name of the module to act on'))
>> +    mgroup.add_argument('-a', '--add', dest='action_add', action='store', nargs=1, metavar='module_name', help=_("Add a module"))
>> +    mgroup.add_argument('-r', '--remove', dest='action_remove', action='store', nargs='+', metavar='module_name', help=_("Remove a module"))
>> +    mgroup.add_argument('-d', '--disable', dest='action_disable', action='store', nargs='+', metavar='module_name', help=_("Disable a module"))
>> +    mgroup.add_argument('-e', '--enable', dest='action_enable', action='store', nargs='+', metavar='module_name', help=_("Enable a module"))
>>      moduleParser.set_defaults(func=handleModule)
>>
>>
>> diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py
>> index 556d3ba5..cd2d3457 100644
>> --- a/python/semanage/seobject.py
>> +++ b/python/semanage/seobject.py
>> @@ -401,6 +401,8 @@ class moduleRecords(semanageRecords):
>>              print("%-25s %-9s %-5s %s" % (t[0], t[2], t[3], disabled))
>>
>>      def add(self, file, priority):
>> +        if type(file) == list:
>> +            file = file[0]
>>          if not os.path.exists(file):
>>              raise ValueError(_("Module does not exist: %s ") % file)
>>
>> @@ -413,7 +415,9 @@ class moduleRecords(semanageRecords):
>>              self.commit()
>>
>>      def set_enabled(self, module, enable):
>> -        for m in module.split():
>> +        if type(module) == str:
>> +            module = module.split()
>> +        for m in module:
>>              rc, key = semanage_module_key_create(self.sh)
>>              if rc < 0:
>>                  raise ValueError(_("Could not create module key"))
>> @@ -435,7 +439,9 @@ class moduleRecords(semanageRecords):
>>          if rc < 0:
>>              raise ValueError(_("Invalid priority %d (needs to be between 1 and 999)") % priority)
>>
>> -        for m in module.split():
>> +        if type(module) == str:
>> +            module = module.split()
>> +        for m in module:
>>              rc = semanage_module_remove(self.sh, m)
>>              if rc < 0 and rc != -2:
>>                  raise ValueError(_("Could not remove module %s (remove failed)") % m)
>> --
>> 2.20.1
>>

[PATCH v2 1/3] python/semanage: Drop python shebang from seobject.py

[Petr Lautrbach]

seobject.py is not supposed to be used as entrypoint therefore the shebang is
unnecessary. It also doesn't need execute bits.

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 python/semanage/Makefile    | 2 +-
 python/semanage/seobject.py | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/python/semanage/Makefile b/python/semanage/Makefile
index 2e262ef8..37065268 100644
--- a/python/semanage/Makefile
+++ b/python/semanage/Makefile
@@ -27,7 +27,7 @@ install: all
 		fi ; \
 	done
 	test -d $(DESTDIR)/$(PACKAGEDIR) || install -m 755 -d $(DESTDIR)/$(PACKAGEDIR)
-	install -m 755 seobject.py $(DESTDIR)/$(PACKAGEDIR)
+	install -m 644 seobject.py $(DESTDIR)/$(PACKAGEDIR)
 	-mkdir -p $(DESTDIR)$(BASHCOMPLETIONDIR)
 	install -m 644 $(BASHCOMPLETIONS) $(DESTDIR)$(BASHCOMPLETIONDIR)/semanage
 
diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py
index b31a90c1..13fdf531 100644
--- a/python/semanage/seobject.py
+++ b/python/semanage/seobject.py
@@ -1,4 +1,3 @@
-#! /usr/bin/python -Es
 # Copyright (C) 2005-2013 Red Hat
 # see file 'COPYING' for use and warranty information
 #
-- 
2.20.1

[PATCH v2 2/3] python/semanage: Update semanage to use python3

[Petr Lautrbach]

semanage uses seobject which uses setools which is python 3 only.

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 python/semanage/semanage | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/semanage/semanage b/python/semanage/semanage
index 4b544bfc..18191c13 100644
--- a/python/semanage/semanage
+++ b/python/semanage/semanage
@@ -1,4 +1,4 @@
-#! /usr/bin/python -Es
+#! /usr/bin/python3 -Es
 # Copyright (C) 2012-2013 Red Hat
 # AUTHOR: Miroslav Grepl <mgrepl@redhat.com>
 # AUTHOR: David Quigley <selinux@davequigley.com>
-- 
2.20.1

[PATCH v2 3/3] python/semanage module: Fix handling of -a/-e/-d/-r options

[Petr Lautrbach]

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 it's needed to convert list to str
before it's sent to moduleRecords class.

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>
---
 python/semanage/semanage | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/python/semanage/semanage b/python/semanage/semanage
index 18191c13..d6d68248 100644
--- a/python/semanage/semanage
+++ b/python/semanage/semanage
@@ -609,14 +609,14 @@ def setupInterfaceParser(subparsers):
 
 def handleModule(args):
     OBJECT = seobject.moduleRecords(args)
-    if args.action == "add":
-        OBJECT.add(args.module_name, args.priority)
-    if args.action == "enable":
-        OBJECT.set_enabled(args.module_name, True)
-    if args.action == "disable":
-        OBJECT.set_enabled(args.module_name, False)
-    if args.action == "remove":
-        OBJECT.delete(args.module_name, args.priority)
+    if args.action_add:
+        OBJECT.add(args.action_add[0], args.priority)
+    if args.action_enable:
+        OBJECT.set_enabled(" ".join(args.action_enable), True)
+    if args.action_disable:
+        OBJECT.set_enabled(" ".join(args.action_disable), False)
+    if args.action_remove:
+        OBJECT.delete(" ".join(args.action_remove), args.priority)
     if args.action == "deleteall":
         OBJECT.deleteall()
     if args.action == "list":
@@ -635,14 +635,13 @@ def setupModuleParser(subparsers):
     parser_add_priority(moduleParser, "module")
 
     mgroup = moduleParser.add_mutually_exclusive_group(required=True)
-    parser_add_add(mgroup, "module")
     parser_add_list(mgroup, "module")
     parser_add_extract(mgroup, "module")
     parser_add_deleteall(mgroup, "module")
-    mgroup.add_argument('-r', '--remove', dest='action', action='store_const', const='remove', help=_("Remove a module"))
-    mgroup.add_argument('-d', '--disable', dest='action', action='store_const', const='disable', help=_("Disable a module"))
-    mgroup.add_argument('-e', '--enable', dest='action', action='store_const', const='enable', help=_("Enable a module"))
-    moduleParser.add_argument('module_name', nargs='?', default=None, help=_('Name of the module to act on'))
+    mgroup.add_argument('-a', '--add', dest='action_add', action='store', nargs=1, metavar='module_name', help=_("Add a module"))
+    mgroup.add_argument('-r', '--remove', dest='action_remove', action='store', nargs='+', metavar='module_name', help=_("Remove a module"))
+    mgroup.add_argument('-d', '--disable', dest='action_disable', action='store', nargs='+', metavar='module_name', help=_("Disable a module"))
+    mgroup.add_argument('-e', '--enable', dest='action_enable', action='store', nargs='+', metavar='module_name', help=_("Enable a module"))
     moduleParser.set_defaults(func=handleModule)
 
 
-- 
2.20.1

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

[Nicolas Iooss]

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

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

[Nicolas Iooss]

On Fri, Feb 15, 2019 at 5:00 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 it's needed to convert list to str
> before it's sent to moduleRecords class.
>
> 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>

For the three patches: Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

> ---
>  python/semanage/semanage | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/python/semanage/semanage b/python/semanage/semanage
> index 18191c13..d6d68248 100644
> --- a/python/semanage/semanage
> +++ b/python/semanage/semanage
> @@ -609,14 +609,14 @@ def setupInterfaceParser(subparsers):
>
>  def handleModule(args):
>      OBJECT = seobject.moduleRecords(args)
> -    if args.action == "add":
> -        OBJECT.add(args.module_name, args.priority)
> -    if args.action == "enable":
> -        OBJECT.set_enabled(args.module_name, True)
> -    if args.action == "disable":
> -        OBJECT.set_enabled(args.module_name, False)
> -    if args.action == "remove":
> -        OBJECT.delete(args.module_name, args.priority)
> +    if args.action_add:
> +        OBJECT.add(args.action_add[0], args.priority)
> +    if args.action_enable:
> +        OBJECT.set_enabled(" ".join(args.action_enable), True)
> +    if args.action_disable:
> +        OBJECT.set_enabled(" ".join(args.action_disable), False)
> +    if args.action_remove:
> +        OBJECT.delete(" ".join(args.action_remove), args.priority)
>      if args.action == "deleteall":
>          OBJECT.deleteall()
>      if args.action == "list":
> @@ -635,14 +635,13 @@ def setupModuleParser(subparsers):
>      parser_add_priority(moduleParser, "module")
>
>      mgroup = moduleParser.add_mutually_exclusive_group(required=True)
> -    parser_add_add(mgroup, "module")
>      parser_add_list(mgroup, "module")
>      parser_add_extract(mgroup, "module")
>      parser_add_deleteall(mgroup, "module")
> -    mgroup.add_argument('-r', '--remove', dest='action', action='store_const', const='remove', help=_("Remove a module"))
> -    mgroup.add_argument('-d', '--disable', dest='action', action='store_const', const='disable', help=_("Disable a module"))
> -    mgroup.add_argument('-e', '--enable', dest='action', action='store_const', const='enable', help=_("Enable a module"))
> -    moduleParser.add_argument('module_name', nargs='?', default=None, help=_('Name of the module to act on'))
> +    mgroup.add_argument('-a', '--add', dest='action_add', action='store', nargs=1, metavar='module_name', help=_("Add a module"))
> +    mgroup.add_argument('-r', '--remove', dest='action_remove', action='store', nargs='+', metavar='module_name', help=_("Remove a module"))
> +    mgroup.add_argument('-d', '--disable', dest='action_disable', action='store', nargs='+', metavar='module_name', help=_("Disable a module"))
> +    mgroup.add_argument('-e', '--enable', dest='action_enable', action='store', nargs='+', metavar='module_name', help=_("Enable a module"))
>      moduleParser.set_defaults(func=handleModule)
>
>
> --
> 2.20.1
>

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

[Nicolas Iooss]

On Sun, Feb 17, 2019 at 9:42 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Fri, Feb 15, 2019 at 5:00 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 it's needed to convert list to str
> > before it's sent to moduleRecords class.
> >
> > 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>
>
> For the three patches: Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Merged. Thanks!

Nicolas

> > ---
> >  python/semanage/semanage | 25 ++++++++++++-------------
> >  1 file changed, 12 insertions(+), 13 deletions(-)
> >
> > diff --git a/python/semanage/semanage b/python/semanage/semanage
> > index 18191c13..d6d68248 100644
> > --- a/python/semanage/semanage
> > +++ b/python/semanage/semanage
> > @@ -609,14 +609,14 @@ def setupInterfaceParser(subparsers):
> >
> >  def handleModule(args):
> >      OBJECT = seobject.moduleRecords(args)
> > -    if args.action == "add":
> > -        OBJECT.add(args.module_name, args.priority)
> > -    if args.action == "enable":
> > -        OBJECT.set_enabled(args.module_name, True)
> > -    if args.action == "disable":
> > -        OBJECT.set_enabled(args.module_name, False)
> > -    if args.action == "remove":
> > -        OBJECT.delete(args.module_name, args.priority)
> > +    if args.action_add:
> > +        OBJECT.add(args.action_add[0], args.priority)
> > +    if args.action_enable:
> > +        OBJECT.set_enabled(" ".join(args.action_enable), True)
> > +    if args.action_disable:
> > +        OBJECT.set_enabled(" ".join(args.action_disable), False)
> > +    if args.action_remove:
> > +        OBJECT.delete(" ".join(args.action_remove), args.priority)
> >      if args.action == "deleteall":
> >          OBJECT.deleteall()
> >      if args.action == "list":
> > @@ -635,14 +635,13 @@ def setupModuleParser(subparsers):
> >      parser_add_priority(moduleParser, "module")
> >
> >      mgroup = moduleParser.add_mutually_exclusive_group(required=True)
> > -    parser_add_add(mgroup, "module")
> >      parser_add_list(mgroup, "module")
> >      parser_add_extract(mgroup, "module")
> >      parser_add_deleteall(mgroup, "module")
> > -    mgroup.add_argument('-r', '--remove', dest='action', action='store_const', const='remove', help=_("Remove a module"))
> > -    mgroup.add_argument('-d', '--disable', dest='action', action='store_const', const='disable', help=_("Disable a module"))
> > -    mgroup.add_argument('-e', '--enable', dest='action', action='store_const', const='enable', help=_("Enable a module"))
> > -    moduleParser.add_argument('module_name', nargs='?', default=None, help=_('Name of the module to act on'))
> > +    mgroup.add_argument('-a', '--add', dest='action_add', action='store', nargs=1, metavar='module_name', help=_("Add a module"))
> > +    mgroup.add_argument('-r', '--remove', dest='action_remove', action='store', nargs='+', metavar='module_name', help=_("Remove a module"))
> > +    mgroup.add_argument('-d', '--disable', dest='action_disable', action='store', nargs='+', metavar='module_name', help=_("Disable a module"))
> > +    mgroup.add_argument('-e', '--enable', dest='action_enable', action='store', nargs='+', metavar='module_name', help=_("Enable a module"))
> >      moduleParser.set_defaults(func=handleModule)
> >
> >
> > --
> > 2.20.1
> >