Refactor ipautil.run

The ipautil.run function now returns an object with returncode and
output are accessible as attributes.

The stdout and stderr of all commands are logged (unless skip_output is given).

The stdout/stderr contents must be explicitly requested with a keyword
argument, otherwise they are None.
This is because in Python 3, the output needs to be decoded, and that can
fail if it's not decodable (human-readable) text.

The raw (bytes) output is always available from the result object,
as is "leniently" decoded output suitable for logging.

All calls are changed to reflect this.

A use of Popen in cainstance is changed to ipautil.run.

Reviewed-By: Jan Cholasta <jcholast@redhat.com>

5 files changed, 179 insertions, 63 deletions

diff --git a/ipapython/certdb.py b/ipapython/certdb.py
index 704bae528..1e6c63a15 100644
--- a/ipapython/certdb.py
+++ b/ipapython/certdb.py
@@ -107,10 +107,10 @@ class NSSDatabase(object):
     def __exit__(self, type, value, tb):
         self.close()
 
-    def run_certutil(self, args, stdin=None):
+    def run_certutil(self, args, stdin=None, **kwargs):
         new_args = [paths.CERTUTIL, "-d", self.secdir]
         new_args = new_args + args
-        return ipautil.run(new_args, stdin)
+        return ipautil.run(new_args, stdin, **kwargs)
 
     def create_db(self, password_filename):
         """Create cert DB
@@ -124,8 +124,8 @@ class NSSDatabase(object):
 
         :return: List of (name, trust_flags) tuples
         """
-        certs, stderr, returncode = self.run_certutil(["-L"])
-        certs = certs.splitlines()
+        result = self.run_certutil(["-L"], capture_output=True)
+        certs = result.output.splitlines()
 
         # FIXME, this relies on NSS never changing the formatting of certutil
         certlist = []
@@ -157,9 +157,8 @@ class NSSDatabase(object):
         :return: List of certificate names
         """
         root_nicknames = []
-        chain, stderr, returncode = self.run_certutil([
-            "-O", "-n", nickname])
-        chain = chain.splitlines()
+        result = self.run_certutil(["-O", "-n", nickname], capture_output=True)
+        chain = result.output.splitlines()
 
         for c in chain:
             m = re.match('\s*"(.*)" \[.*', c)
@@ -247,7 +246,8 @@ class NSSDatabase(object):
                             '-print_certs',
                         ]
                         try:
-                            stdout, stderr, rc = ipautil.run(args, stdin=body)
+                            result = ipautil.run(
+                                args, stdin=body, capture_output=True)
                         except ipautil.CalledProcessError as e:
                             if label == 'CERTIFICATE':
                                 root_logger.warning(
@@ -259,7 +259,7 @@ class NSSDatabase(object):
                                     filename, line, e)
                             continue
                         else:
-                            extracted_certs += stdout + '\n'
+                            extracted_certs += result.output + '\n'
                             loaded = True
                             continue
 
@@ -286,14 +286,15 @@ class NSSDatabase(object):
                                 '-passin', 'file:' + key_pwdfile.name,
                             ]
                         try:
-                            stdout, stderr, rc = ipautil.run(args, stdin=body)
+                            result = ipautil.run(
+                                args, stdin=body, capture_output=True)
                         except ipautil.CalledProcessError as e:
                             root_logger.warning(
                                 "Skipping private key in %s at line %s: %s",
                                 filename, line, e)
                             continue
                         else:
-                            extracted_key = stdout
+                            extracted_key = result.output
                             key_file = filename
                             loaded = True
                             continue
@@ -401,10 +402,13 @@ class NSSDatabase(object):
         else:
             args.append('-r')
         try:
-            cert, err, returncode = self.run_certutil(args)
+            result = self.run_certutil(args, capture_output=pem)
         except ipautil.CalledProcessError:
             raise RuntimeError("Failed to get %s" % nickname)
-        return cert
+        if pem:
+            return result.output
+        else:
+            return result.raw_output
 
     def has_nickname(self, nickname):
         try:
diff --git a/ipapython/dnssec/bindmgr.py b/ipapython/dnssec/bindmgr.py
index 1822dacf2..a0a9f2eb2 100644
--- a/ipapython/dnssec/bindmgr.py
+++ b/ipapython/dnssec/bindmgr.py
@@ -40,8 +40,8 @@ class BINDMgr(object):
 
     def notify_zone(self, zone):
         cmd = ['rndc', 'sign', zone.to_text()]
-        output = ipautil.run(cmd)[0]
-        self.log.info(output)
+        result = ipautil.run(cmd, capture_output=True)
+        self.log.info('%s', result.output_log)
 
     def dn2zone_name(self, dn):
         """cn=KSK-20140813162153Z-cede9e182fc4af76c4bddbc19123a565,cn=keys,idnsname=test,cn=dns,dc=ipa,dc=example"""
@@ -110,7 +110,8 @@ class BINDMgr(object):
         cmd.append(zone.to_text())
 
         # keys has to be readable by ODS & named
-        basename = ipautil.run(cmd)[0].strip()
+        result = ipautil.run(cmd, capture_output=True)
+        basename = result.output.strip()
         private_fn = "%s/%s.private" % (workdir, basename)
         os.chmod(private_fn, FILE_PERM)
         # this is useful mainly for debugging
diff --git a/ipapython/dnssec/odsmgr.py b/ipapython/dnssec/odsmgr.py
index efbe16cc6..ebcd3aa24 100644
--- a/ipapython/dnssec/odsmgr.py
+++ b/ipapython/dnssec/odsmgr.py
@@ -128,7 +128,8 @@ class ODSMgr(object):
         Raises CalledProcessError if returncode != 0.
         """
         cmd = ['ods-ksmutil'] + params
-        return ipautil.run(cmd)[0]
+        result = ipautil.run(cmd, capture_output=True)
+        return result.output
 
     def get_ods_zonelist(self):
         stdout = self.ksmutil(['zonelist', 'export'])
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 89047b2e8..448074418 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -37,6 +37,8 @@ import gssapi
 import pwd
 import grp
 from contextlib import contextmanager
+import locale
+import collections
 
 from dns import resolver, rdatatype
 from dns.exception import DNSException
@@ -155,8 +157,10 @@ class CheckedIPAddress(netaddr.IPAddress):
             elif addr.version == 6:
                 family = 'inet6'
 
-            ipresult = run([paths.IP, '-family', family, '-oneline', 'address', 'show'])
-            lines = ipresult[0].split('\n')
+            result = run(
+                [paths.IP, '-family', family, '-oneline', 'address', 'show'],
+                capture_output=True)
+            lines = result.output.split('\n')
             for line in lines:
                 fields = line.split()
                 if len(fields) < 4:
@@ -256,13 +260,35 @@ def write_tmp_file(txt):
     return fd
 
 def shell_quote(string):
-    return "'" + string.replace("'", "'\\''") + "'"
+    if isinstance(string, str):
+        return "'" + string.replace("'", "'\\''") + "'"
+    else:
+        return b"'" + string.replace(b"'", b"'\\''") + b"'"
+
+
+if six.PY3:
+    def _log_arg(s):
+        """Convert string or bytes to a string suitable for logging"""
+        if isinstance(s, bytes):
+            return s.decode(locale.getpreferredencoding(),
+                            errors='replace')
+        else:
+            return s
+else:
+    _log_arg = str
+
+
+class _RunResult(collections.namedtuple('_RunResult',
+                                        'output error_output returncode')):
+    """Result of ipautil.run"""
+
 
-def run(args, stdin=None, raiseonerr=True,
-        nolog=(), env=None, capture_output=True, skip_output=False, cwd=None,
-        runas=None, timeout=None, suplementary_groups=[]):
+def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
+        capture_output=False, skip_output=False, cwd=None,
+        runas=None, timeout=None, suplementary_groups=[],
+        capture_error=False, encoding=None):
     """
-    Execute a command and return stdin, stdout and the process return code.
+    Execute an external command.
 
     :param args: List of arguments for the command
     :param stdin: Optional input to the command
@@ -283,8 +309,8 @@ def run(args, stdin=None, raiseonerr=True,
         If a value isn't found in the list it is silently ignored.
     :param env: Dictionary of environment variables passed to the command.
         When None, current environment is copied
-    :param capture_output: Capture stderr and stdout
-    :param skip_output: Redirect the output to /dev/null and do not capture it
+    :param capture_output: Capture stdout
+    :param skip_output: Redirect the output to /dev/null and do not log it
     :param cwd: Current working directory
     :param runas: Name of a user that the command should be run as. The spawned
         process will have both real and effective UID and GID set.
@@ -293,6 +319,31 @@ def run(args, stdin=None, raiseonerr=True,
     :param suplementary_groups: List of group names that will be used as
         suplementary groups for subporcess.
         The option runas must be specified together with this option.
+    :param capture_error: Capture stderr
+    :param encoding: For Python 3, the encoding to use for output,
+        error_output, and (if it's not bytes) stdin.
+        If None, the current encoding according to locale is used.
+
+    :return: An object with these attributes:
+
+        `returncode`: The process' exit status
+
+        `output` and `error_output`: captured output, as strings. Under
+        Python 3, these are encoded with the given `encoding`.
+        None unless `capture_output` or `capture_error`, respectively, are
+        given
+
+        `raw_output`, `raw_error_output`: captured output, as bytes.
+
+        `output_log` and `error_log`: The captured output, as strings, with any
+        unencodable characters discarded. These should only be used
+        for logging or error messages.
+
+    If skip_output is given, all output-related attributes on the result
+    (that is, all except `returncode`) are None.
+
+    For backwards compatibility, the return value can also be used as a
+    (output, error_output, returncode) triple.
     """
     assert isinstance(suplementary_groups, list)
     p_in = None
@@ -301,12 +352,16 @@ def run(args, stdin=None, raiseonerr=True,
 
     if isinstance(nolog, six.string_types):
         # We expect a tuple (or list, or other iterable) of nolog strings.
-        # Passing just a single string is bad: strings are also, so this
+        # Passing just a single string is bad: strings are iterable, so this
         # would result in every individual character of that string being
         # replaced by XXXXXXXX.
         # This is a sanity check to prevent that.
         raise ValueError('nolog must be a tuple of strings.')
 
+    if skip_output and (capture_output or capture_error):
+        raise ValueError('skip_output is incompatible with '
+                         'capture_output or capture_error')
+
     if env is None:
         # copy default env
         env = copy.deepcopy(os.environ)
@@ -315,16 +370,22 @@ def run(args, stdin=None, raiseonerr=True,
         p_in = subprocess.PIPE
     if skip_output:
         p_out = p_err = open(paths.DEV_NULL, 'w')
-    elif capture_output:
+    else:
         p_out = subprocess.PIPE
         p_err = subprocess.PIPE
 
+    if encoding is None:
+        encoding = locale.getpreferredencoding()
+
+    if six.PY3 and isinstance(stdin, str):
+        stdin = stdin.encode(encoding)
+
     if timeout:
         # If a timeout was provided, use the timeout command
         # to execute the requested command.
         args[0:0] = [paths.BIN_TIMEOUT, str(timeout)]
 
-    arg_string = nolog_replace(' '.join(shell_quote(a) for a in args), nolog)
+    arg_string = nolog_replace(' '.join(_log_arg(a) for a in args), nolog)
     root_logger.debug('Starting external process')
     root_logger.debug('args=%s' % arg_string)
 
@@ -352,8 +413,7 @@ def run(args, stdin=None, raiseonerr=True,
         p = subprocess.Popen(args, stdin=p_in, stdout=p_out, stderr=p_err,
                              close_fds=True, env=env, cwd=cwd,
                              preexec_fn=preexec_fn)
-        stdout,stderr = p.communicate(stdin)
-        stdout,stderr = str(stdout), str(stderr)    # Make pylint happy
+        stdout, stderr = p.communicate(stdin)
     except KeyboardInterrupt:
         root_logger.debug('Process interrupted')
         p.wait()
@@ -372,16 +432,50 @@ def run(args, stdin=None, raiseonerr=True,
 
     # The command and its output may include passwords that we don't want
     # to log. Replace those.
-    if capture_output and not skip_output:
-        stdout = nolog_replace(stdout, nolog)
-        stderr = nolog_replace(stderr, nolog)
-        root_logger.debug('stdout=%s' % stdout)
-        root_logger.debug('stderr=%s' % stderr)
+    if skip_output:
+        output_log = None
+        error_log = None
+    else:
+        if six.PY3:
+            output_log = stdout.decode(locale.getpreferredencoding(),
+                                       errors='replace')
+        else:
+            output_log = stdout
+        if six.PY3:
+            error_log = stderr.decode(locale.getpreferredencoding(),
+                                      errors='replace')
+        else:
+            error_log = stderr
+        output_log = nolog_replace(output_log, nolog)
+        root_logger.debug('stdout=%s' % output_log)
+        error_log = nolog_replace(error_log, nolog)
+        root_logger.debug('stderr=%s' % error_log)
+
+    if capture_output:
+        if six.PY2:
+            output = stdout
+        else:
+            output = stdout.encode(encoding)
+    else:
+        output = None
+
+    if capture_error:
+        if six.PY2:
+            error_output = stderr
+        else:
+            error_output = stderr.encode(encoding)
+    else:
+        error_output = None
 
     if p.returncode != 0 and raiseonerr:
-        raise CalledProcessError(p.returncode, arg_string, stdout)
+        raise CalledProcessError(p.returncode, arg_string, str(output))
 
-    return (stdout, stderr, p.returncode)
+    result = _RunResult(output, error_output, p.returncode)
+    result.raw_output = stdout
+    result.raw_error_output = stderr
+    result.output_log = output_log
+    result.error_log = error_log
+    return result
 
 
 def nolog_replace(string, nolog):
@@ -1269,10 +1363,10 @@ def kinit_password(principal, password, ccache_name, config=None,
 
     # this workaround enables us to capture stderr and put it
     # into the raised exception in case of unsuccessful authentication
-    (stdout, stderr, retcode) = run(args, stdin=password, env=env,
-                                    raiseonerr=False)
-    if retcode:
-        raise RuntimeError(stderr)
+    result = run(args, stdin=password, env=env, raiseonerr=False,
+                 capture_error=True)
+    if result.returncode:
+        raise RuntimeError(result.error_output)
 
 
 def dn_attribute_property(private_name):
diff --git a/ipapython/kernel_keyring.py b/ipapython/kernel_keyring.py
index d30531cab..7ba916ccb 100644
--- a/ipapython/kernel_keyring.py
+++ b/ipapython/kernel_keyring.py
@@ -36,24 +36,29 @@ def dump_keys():
     """
     Dump all keys
     """
-    (stdout, stderr, rc) = run(['keyctl', 'list', KEYRING], raiseonerr=False)
-    return stdout
+    result = run(['keyctl', 'list', KEYRING], raiseonerr=False,
+                 capture_output=True)
+    return result.output
 
 def get_real_key(key):
     """
     One cannot request a key based on the description it was created with
     so find the one we're looking for.
     """
-    (stdout, stderr, rc) = run(['keyctl', 'search', KEYRING, KEYTYPE, key], raiseonerr=False)
-    if rc:
+    assert isinstance(key, str)
+    result = run(['keyctl', 'search', KEYRING, KEYTYPE, key],
+                 raiseonerr=False, capture_output=True)
+    if result.returncode:
         raise ValueError('key %s not found' % key)
-    return stdout.rstrip()
+    return result.output.rstrip()
 
 def get_persistent_key(key):
-    (stdout, stderr, rc) = run(['keyctl', 'get_persistent', KEYRING, key], raiseonerr=False)
-    if rc:
+    assert isinstance(key, str)
+    result = run(['keyctl', 'get_persistent', KEYRING, key],
+                 raiseonerr=False, capture_output=True)
+    if result.returncode:
         raise ValueError('persistent key %s not found' % key)
-    return stdout.rstrip()
+    return result.output.rstrip()
 
 def is_persistent_keyring_supported():
     uid = os.geteuid()
@@ -68,6 +73,7 @@ def has_key(key):
     """
     Returns True/False whether the key exists in the keyring.
     """
+    assert isinstance(key, str)
     try:
         get_real_key(key)
         return True
@@ -80,22 +86,27 @@ def read_key(key):
 
     Use pipe instead of print here to ensure we always get the raw data.
     """
+    assert isinstance(key, str)
     real_key = get_real_key(key)
-    (stdout, stderr, rc) = run(['keyctl', 'pipe', real_key], raiseonerr=False)
-    if rc:
-        raise ValueError('keyctl pipe failed: %s' % stderr)
+    result = run(['keyctl', 'pipe', real_key], raiseonerr=False,
+                 capture_output=True)
+    if result.returncode:
+        raise ValueError('keyctl pipe failed: %s' % result.error_log)
 
-    return stdout
+    return result.output
 
 def update_key(key, value):
     """
     Update the keyring data. If they key doesn't exist it is created.
     """
+    assert isinstance(key, str)
+    assert isinstance(value, bytes)
     if has_key(key):
         real_key = get_real_key(key)
-        (stdout, stderr, rc) = run(['keyctl', 'pupdate', real_key], stdin=value, raiseonerr=False)
-        if rc:
-            raise ValueError('keyctl pupdate failed: %s' % stderr)
+        result = run(['keyctl', 'pupdate', real_key], stdin=value,
+                     raiseonerr=False)
+        if result.returncode:
+            raise ValueError('keyctl pupdate failed: %s' % result.error_log)
     else:
         add_key(key, value)
 
@@ -103,17 +114,22 @@ def add_key(key, value):
     """
     Add a key to the kernel keyring.
     """
+    assert isinstance(key, str)
+    assert isinstance(value, bytes)
     if has_key(key):
         raise ValueError('key %s already exists' % key)
-    (stdout, stderr, rc) = run(['keyctl', 'padd', KEYTYPE, key, KEYRING], stdin=value, raiseonerr=False)
-    if rc:
-        raise ValueError('keyctl padd failed: %s' % stderr)
+    result = run(['keyctl', 'padd', KEYTYPE, key, KEYRING],
+                 stdin=value, raiseonerr=False)
+    if result.returncode:
+        raise ValueError('keyctl padd failed: %s' % result.error_log)
 
 def del_key(key):
     """
     Remove a key from the keyring
     """
+    assert isinstance(key, str)
     real_key = get_real_key(key)
-    (stdout, stderr, rc) = run(['keyctl', 'unlink', real_key, KEYRING], raiseonerr=False)
-    if rc:
-        raise ValueError('keyctl unlink failed: %s' % stderr)
+    result = run(['keyctl', 'unlink', real_key, KEYRING],
+                 raiseonerr=False)
+    if result.returncode:
+        raise ValueError('keyctl unlink failed: %s' % result.error_log)