PR#50658: Issue 50634 - Clean up CLI errors output - 389-ds-base

389-ds-base

#50658 Issue 50634 - Clean up CLI errors output

Closed 3 years ago by spichugi. Opened 4 years ago by spichugi.

spichugi/389-ds-base error_cleanup into master

Issue 50634 - Clean up CLI errors output

Simon Pichugin • 4 years ago

a2e3c02

src/lib389/cli/dsconf

file modified

+4 -7

		`@@ -15,7 +15,6 @@`
		`import sys`
		`import signal`
		`import json`
		`- import ast`
		`from lib389._constants import DSRC_HOME`
		`from lib389.cli_conf import config as cli_config`
		`from lib389.cli_conf import backend as cli_backend`
		`@@ -34,6 +33,7 @@`
		`from lib389.cli_base import disconnect_instance, connect_instance`
		`from lib389.cli_base.dsrc import dsrc_to_ldap, dsrc_arg_concat`
		`from lib389.cli_base import setup_script_logger`
		`+ from lib389.cli_base import format_error_to_dict`


		`parser = argparse.ArgumentParser(allow_abbrev=True)`
		`@@ -138,15 +138,12 @@`
		`log.info("Command successful.")`
		`except Exception as e:`
		`log.debug(e, exc_info=True)`
		`- errmsg = str(e)`
		`+ msg = format_error_to_dict(e)`
		`+`
		`if args and args.json:`
		`- try:`
		`- msg = ast.literal_eval(errmsg)`
		`- except:`
		`- msg = {'desc': errmsg}`
		`sys.stderr.write(json.dumps(msg))`
		`else:`
		`- log.error("Error: %s" % errmsg)`
		`+ log.error("Error: %s" % " - ".join(msg.values()))`
		`result = False`

		`disconnect_instance(inst)`

src/lib389/cli/dscreate

file modified

+3 -1

		`@@ -17,6 +17,7 @@`
		`from lib389 import DirSrv`
		`from lib389.cli_ctl import instance as cli_instance`
		`from lib389.cli_base import setup_script_logger`
		`+ from lib389.cli_base import format_error_to_dict`

		`parser = argparse.ArgumentParser()`
		`parser.add_argument('-v', '--verbose',`
		`@@ -74,7 +75,8 @@`
		`result = args.func(inst, log, args)`
		`except Exception as e:`
		`log.debug(e, exc_info=True)`
		`- log.error("Error: %s" % str(e))`
		`+ msg = format_error_to_dict(e)`
		`+ log.error("Error: %s" % " - ".join(msg.values()))`
		`result = False`

		`# Done!`

src/lib389/cli/dsctl

file modified

+7 -4

		`@@ -21,6 +21,7 @@`
		`from lib389.cli_ctl import instance as cli_instance`
		`from lib389.cli_ctl import dbtasks as cli_dbtasks`
		`from lib389.cli_base import disconnect_instance, setup_script_logger`
		`+ from lib389.cli_base import format_error_to_dict`
		`from lib389.cli_ctl.instance import instance_remove_all`
		`from lib389._constants import DSRC_CONTAINER`

		`@@ -106,10 +107,12 @@`
		`insts = inst.list(serverid=args.instance)`
		`except (PermissionError, IOError) as e:`
		`log.error("Unable to access instance information. Are you running as the correct user? (usually dirsrv or root)")`
		`- log.error("Error: %s" % str(e))`
		`+ msg = format_error_to_dict(e)`
		`+ log.error("Error: %s" % " - ".join(msg.values()))`
		`sys.exit(1)`
		`except Exception as e:`
		`- log.error("Error: %s" % str(e))`
		`+ msg = format_error_to_dict(e)`
		`+ log.error("Error: %s" % " - ".join(msg.values()))`
		`sys.exit(1)`
		`if len(insts) != 1:`
		`log.error("No such instance '%s'" % args.instance)`
		`@@ -123,11 +126,11 @@`
		`result = args.func(inst, log, args)`
		`except Exception as e:`
		`log.debug(e, exc_info=True)`
		`- log.error("Error: %s" % str(e))`
		`+ msg = format_error_to_dict(e)`
		`+ log.error("Error: %s" % " - ".join(msg.values()))`
		`result = False`
		`disconnect_instance(inst)`

		`# Done!`
		`if result is False:`
		`sys.exit(1)`
		`-`

src/lib389/cli/dsidm

file modified

+3 -1

		`@@ -26,6 +26,7 @@`
		`from lib389.cli_idm import role as cli_role`
		`from lib389.cli_base import connect_instance, disconnect_instance, setup_script_logger`
		`from lib389.cli_base.dsrc import dsrc_to_ldap, dsrc_arg_concat`
		`+ from lib389.cli_base import format_error_to_dict`


		`parser = argparse.ArgumentParser(allow_abbrev=True)`
		`@@ -131,7 +132,8 @@`
		`log.info("Command successful.")`
		`except Exception as e:`
		`log.debug(e, exc_info=True)`
		`- log.error("Error: %s" % str(e))`
		`+ msg = format_error_to_dict(e)`
		`+ log.error("Error: %s" % " - ".join(msg.values()))`
		`result = False`

		`disconnect_instance(inst)`

src/lib389/lib389/cli_base/__init__.py

file modified

+20

		`@@ -7,6 +7,7 @@`
		`# See LICENSE for details.`
		`# --- END COPYRIGHT BLOCK ---`

		`+ import ast`
		`import logging`
		`import sys`
		`import json`
		`@@ -406,3 +407,22 @@`
		`root.addHandler(log_handler)`

		`return log`
		`+`
		`+`
		`+ def format_error_to_dict(exception):`
		`+ """python-ldap str(exception) processing is not consistent.`
		`+ This function makes sure that the result is dict`
		`+`
		`+ :param exception: Exception you need to print`
		`+ :type exception: ldap.LDAPError`
		`+ :returns: dict`
		`+ """`
		`+`
		`+ # The issue was raise on python-ldap repo: https://github.com/python-ldap/python-ldap/issues/304`
		`+ # We should fix the code here after the issue is fixed`
		`+ errmsg = str(exception)`
		`+ try:`
		`+ msg = ast.literal_eval(errmsg)`
firstyear commented 4 years ago This is a security risk, because if an attacker can control any input that becomes put into an error message, it will be run here. This is not safe.
		`+ except ValueError:`
		`+ msg = {'desc': errmsg}`
		`+ return msg`

spichugi commented 4 years ago

Description: CLI tools should print human easy readable messages
if something went wrong.
As discussed here: https://pagure.io/389-ds-base/pull-request/50624

Change the CLI error processing so the dict type is always transformed.

https://pagure.io/389-ds-base/issue/50634

Reviewed by: ?

mreynolds commented 4 years ago

LGTM

mhonek commented 4 years ago

Couldn't we abstract that sequence of several lines that are literally the same into a procedure?

1 new commit added

Encupsulate code

4 years ago

spichugi commented 4 years ago

Good catch, thanks!

mhonek commented 4 years ago

Looks good, thanks!

rebased onto b74ddc0d1656ca2d487141c14d7c3a967a47f8ae

4 years ago

rebased onto a2e3c02

4 years ago

Pull-Request has been merged by spichugi

4 years ago

firstyear commented on line 28 of src/lib389/lib389/cli_base/__init__.py 4 years ago

This is a security risk, because if an attacker can control any input that becomes put into an error message, it will be run here. This is not safe.

spichugi commented 4 years ago

This is a security risk, because if an attacker can control any input that becomes put into an error message, it will be run here. This is not safe.

Could you please provide some examples of a possible attack?..
Looking at the code and at the docstring, it looks safe - https://hg.python.org/cpython/file/tip/Lib/ast.py#l40

firstyear commented 4 years ago

My apologies - eval normal leads to horrible things, but indeed you chose the safe one! Okay, could you comment (with the oneline rule maybe?) about this discussion above the use of literal_eval incase this discussion point comes up in a security review?

spichugi commented 3 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3713

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

3 years ago