On 11/23/2015 10:50 AM, Jan Cholasta wrote:
> On 23.11.2015 07:43, Jan Cholasta wrote:
>> On 19.11.2015 00:55, Petr Viktorin wrote:
>>> On 11/03/2015 02:39 PM, Petr Viktorin wrote:
>>>> Hello,
>>>> Python 3's strings are Unicode, so data coming to or leaving a Python
>>>> program needs to be decoded/encoded if it's to be handled as a string.
>>>> One of the boundaries where encoding is necessary is external programs,
>>>> specifically, ipautil.run.
>>>> Unfortunately there's no one set of options that would work for all
>>>> run() invocations, so I went through them all and specified the
>>>> stdin/stdout/stderr encoding where necessary. I've also updated the
>>>> call
>>>> sites to make it clearer if the return values are used or not.
>>>> If an encoding is not set, run() will accept/return bytes. (This is a
>>>> fail-safe setting, since it can't raise errors, and using bytes where
>>>> strings are expected generally fails loudly in py3.)
>>>>
>>>> Note that the changes are not effective under Python 2.
>>>
>>> ping,
>>> Could someone look at this patch?
>>
>> Looking.
> 
> 1) I think a better approach would be to use str for stdin/stdout/stderr
> by default in both Python 2 and 3, i.e. do nothing in Python 2 and
> encode/decode using locale.getpreferredencoding() in Python 3. This is
> consistent with sys.std{in,out,err} in both Python 2 and 3. Using bytes
> or different encoding should be opt-in.
> 
> Note that different encoding should be used only if the LC_ALL or LANG
> variables are overriden in the command's environment.

That would assume the output of *everything* run via ipautil.run can be
decoded using the locale encoding. Any stray invalid byte would make IPA
crash, even in cases where we don't care about the output. IPA calls too
many weird tools to assume they all output text.

> 2) The str() here is wrong:
> 
> +    arg_string = nolog_replace(' '.join(str(shell_quote(a)) for a in
> args), nolog)
> 
> It converts b'data' to "b'data'" in Python 3.
> 
> Popen accepts both bytes and text in both Python 2 and 3, so there is
> nothing to be done for bytes arguments.

Right, the logging for bytes arguments could be better. Changed.

> 3) I think the "surrogateescape" error handler would be better for
> non-strict decoding, as the text can be encoded back to bytes without
> loss of information.

Non-strict decoding is meant for logging, so we want backslash escapes
rather than invalid surrogates. If you need round-tripping then you
should use bytes.
I've changed the error strategy to 'replace' though, as I realized
'backslashreplace' doesn't work for decoding.

> 4) There are direct calls to subprocess.Popen() in a few places, have
> you checked them as well?

No. I'll do it in another patch; thanks for remembering that.


-- 
Petr Viktorin



From d2da5554a34fd76a62daac7480b31e582ba9f1f5 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Tue, 27 Oct 2015 17:54:54 +0100
Subject: [PATCH] Handle encoding for ipautil.run

For Python 2, nothing is changed, and all data discussed below is
bytestrings (str).

Parts of the "argv" for run() may be strings or bytes, and may even be
mixed.

Stdin must be bytes (or None) by default.
If stdin_encoding is given, it may be a string, and is encoded appropriately.

Stdout and stderr are returned as bytes, unless specified otherwise.
This means there will be no decoding errors in the (very common) case
where these are ignored.
For the logs, these output streams are encoded with 'backslashreplace',
so errors can't be raised, and also binary garbage won't show up in logs.

If the stdout_encoding and/or stderr_encoding arguments are given,
the corresponding stream is decoded. This may raise an exception
if the data can't be encoded.
To suppress errors, pass {stdout,stderr}_strict=False. This will cause
invalid characters to be replaced with backslash sequences. It's appropriate
when the stream is copied to a log file, NOT when the data is further
manipulated.
---
 .../certmonger/dogtag-ipa-ca-renew-agent-submit    |  9 ++-
 install/certmonger/ipa-server-guard                | 11 ++-
 install/oddjob/com.redhat.idm.trust-fetch-domains  |  5 +-
 install/tools/ipa-adtrust-install                  |  3 +-
 install/tools/ipa-replica-conncheck                | 16 +++--
 ipa-client/ipa-install/ipa-client-install          | 31 +++++----
 ipalib/plugins/pwpolicy.py                         |  3 +-
 ipaplatform/base/services.py                       | 26 +++----
 ipaplatform/redhat/services.py                     |  3 +-
 ipaplatform/redhat/tasks.py                        |  4 +-
 ipapython/certdb.py                                | 14 ++--
 ipapython/dnssec/bindmgr.py                        |  8 ++-
 ipapython/dnssec/odsmgr.py                         |  3 +-
 ipapython/ipautil.py                               | 79 +++++++++++++++++++---
 ipapython/kernel_keyring.py                        | 30 ++++++--
 ipaserver/dcerpc.py                                | 15 ++--
 ipaserver/install/cainstance.py                    | 24 ++++---
 ipaserver/install/certs.py                         |  2 +-
 ipaserver/install/httpinstance.py                  |  4 +-
 ipaserver/install/ipa_backup.py                    | 29 +++++---
 ipaserver/install/ipa_restore.py                   | 24 +++++--
 ipaserver/install/krbinstance.py                   |  7 +-
 ipaserver/install/opendnssecinstance.py            |  7 +-
 ipaserver/install/replication.py                   |  2 +-
 ipaserver/install/server/install.py                |  8 ++-
 ipatests/test_ipapython/test_ipautil.py            | 46 +++++++++++++
 ipatests/test_ipapython/test_keyring.py            | 17 ++---
 ipatests/test_xmlrpc/test_host_plugin.py           |  2 +-
 28 files changed, 313 insertions(+), 119 deletions(-)

diff --git a/install/certmonger/dogtag-ipa-ca-renew-agent-submit b/install/certmonger/dogtag-ipa-ca-renew-agent-submit
index 44993b038a38da60a25843147e86b64deda874e1..93399845bb5213739057e56531d63d9be96b637c 100755
--- a/install/certmonger/dogtag-ipa-ca-renew-agent-submit
+++ b/install/certmonger/dogtag-ipa-ca-renew-agent-submit
@@ -156,8 +156,13 @@ def request_cert():
     args = [path] + sys.argv[1:]
     if os.environ.get('CERTMONGER_CA_PROFILE') == 'caCACert':
         args += ['-N', '-O', 'bypassCAnotafter=true']
-    stdout, stderr, rc = ipautil.run(args, raiseonerr=False, env=os.environ)
-    sys.stderr.write(stderr)
+    stdout, stderr, rc = ipautil.run(args, raiseonerr=False, env=os.environ,
+                                     stdout_encoding='ascii')
+    if six.PY2:
+        sys.stderr.write(stderr)
+    else:
+        # Write bytes directly
+        sys.stderr.buffer.write(stderr)
     sys.stderr.flush()
 
     syslog.syslog(syslog.LOG_NOTICE, "dogtag-ipa-renew-agent returned %d" % rc)
diff --git a/install/certmonger/ipa-server-guard b/install/certmonger/ipa-server-guard
index 7ce3e43fce16ce9974e5db10af7cf851c0411943..abd384f481e9dd32ba0d6a5c5d780d20e40a8943 100755
--- a/install/certmonger/ipa-server-guard
+++ b/install/certmonger/ipa-server-guard
@@ -30,6 +30,8 @@ import sys
 import syslog
 import traceback
 
+import six
+
 from ipapython import ipautil
 from ipaserver.install import certs
 
@@ -41,9 +43,14 @@ def main():
     with certs.renewal_lock:
         stdout, stderr, rc = ipautil.run(sys.argv[1:], raiseonerr=False,
                                          env=os.environ)
-        sys.stdout.write(stdout)
+        if six.PY2:
+            sys.stdout.write(stdout)
+            sys.stderr.write(stderr)
+        else:
+            # Write bytes directly
+            sys.stdout.buffer.write(stdout)
+            sys.stderr.buffer.write(stderr)
         sys.stdout.flush()
-        sys.stderr.write(stderr)
         sys.stderr.flush()
 
     return rc
diff --git a/install/oddjob/com.redhat.idm.trust-fetch-domains b/install/oddjob/com.redhat.idm.trust-fetch-domains
index 019545b9340ac95f4e528618875df3dcd8170feb..ea82e086ef5bade9be3b9f30ae50504c4fcd5db7 100755
--- a/install/oddjob/com.redhat.idm.trust-fetch-domains
+++ b/install/oddjob/com.redhat.idm.trust-fetch-domains
@@ -26,9 +26,8 @@ def retrieve_keytab(api, ccache_name, oneway_keytab_name, oneway_principal):
     if os.path.isfile(oneway_keytab_name):
         os.unlink(oneway_keytab_name)
 
-    (stdout, stderr, retcode) = ipautil.run(getkeytab_args,
-                                            env={'KRB5CCNAME': ccache_name, 'LANG': 'C'},
-                                            raiseonerr=False)
+    ipautil.run(getkeytab_args, env={'KRB5CCNAME': ccache_name, 'LANG': 'C'},
+                raiseonerr=False)
     # Make sure SSSD is able to read the keytab
     try:
         sssd = pwd.getpwnam('sssd')
diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install
index ff69d69e2c11ce08b8b648a5a78777c472da2ac9..05be43b0894ef0457edfa4161e06fc0e01cd8b0e 100755
--- a/install/tools/ipa-adtrust-install
+++ b/install/tools/ipa-adtrust-install
@@ -200,7 +200,8 @@ def set_and_check_netbios_name(netbios_name, unattended):
 
 def ensure_admin_kinit(admin_name, admin_password):
     try:
-        ipautil.run(['kinit', admin_name], stdin=admin_password+'\n')
+        ipautil.run(['kinit', admin_name], stdin=admin_password+'\n',
+                    stdin_encoding='ascii')
     except ipautil.CalledProcessError as e:
         print("There was error to automatically re-kinit your admin user ticket.")
         return False
diff --git a/install/tools/ipa-replica-conncheck b/install/tools/ipa-replica-conncheck
index e4c259b7e3eed6a49163f4b9c748ce9ec6094033..0e05dcd9828a1681e56557db0287f77c6f9be79a 100755
--- a/install/tools/ipa-replica-conncheck
+++ b/install/tools/ipa-replica-conncheck
@@ -81,7 +81,9 @@ class SshExec(object):
         elif 'KRB5CCNAME' in os.environ:
             env['KRB5CCNAME'] = os.environ['KRB5CCNAME']
 
-        return ipautil.run(cmd, env=env, raiseonerr=False)
+        return ipautil.run(cmd, env=env, raiseonerr=False,
+                           stdout_encoding='ascii', stdout_strict=False,
+                           stderr_encoding='ascii', stderr_strict=False)
 
 
 class CheckedPort(object):
@@ -403,19 +405,19 @@ def main():
                         sys.exit("Principal password required")
 
 
-                stderr=''
-                (stdout, stderr, returncode) = ipautil.run([paths.KINIT, principal],
+                (_stdout, stderr, returncode) = ipautil.run([paths.KINIT, principal],
                      env={'KRB5_CONFIG':KRB5_CONFIG, 'KRB5CCNAME':CCACHE_FILE},
-                    stdin=password, raiseonerr=False)
+                    stdin=password, raiseonerr=False, stdin_encoding='ascii',
+                    stderr_encoding='ascii', stderr_strict=False)
                 if returncode != 0:
                     raise RuntimeError("Cannot acquire Kerberos ticket: %s" % stderr)
 
                 # Verify kinit was actually successful
-                stderr=''
-                (stdout, stderr, returncode) = ipautil.run([paths.BIN_KVNO,
+                (_stdout, stderr, returncode) = ipautil.run([paths.BIN_KVNO,
                      'host/%s' % options.master],
                     env={'KRB5_CONFIG':KRB5_CONFIG, 'KRB5CCNAME':CCACHE_FILE},
-                    raiseonerr=False)
+                    raiseonerr=False,
+                    stderr_encoding='ascii', stderr_strict=False)
                 if returncode != 0:
                     raise RuntimeError("Could not get ticket for master server: %s" % stderr)
 
diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index 05a550b11e74db84e46a126798c4db728226865c..645f5a84ec3128829ebbf8fda30110ef689a322c 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -37,6 +37,7 @@ try:
 
     import nss.nss as nss
     import SSSDConfig
+    import six
     from six.moves.urllib.parse import urlparse, urlunparse
 
     from ipapython.ipa_log_manager import standard_logging_setup, root_logger
@@ -602,7 +603,9 @@ def uninstall(options, env):
         if options.debug:
             join_args.append("-d")
             env['XMLRPC_TRACE_CURL'] = 'yes'
-        (stdout, stderr, returncode) = run(join_args, raiseonerr=False, env=env)
+        _stdout, stderr, returncode = run(
+            join_args, raiseonerr=False, env=env,
+            stderr_encoding='ascii', stderr_strict=False)
         if returncode != 0:
             root_logger.error("Unenrolling host failed: %s", stderr)
 
@@ -1437,7 +1440,7 @@ def configure_sshd_config(fstore, options):
                 args.append('-o')
                 args.append('%s=%s' % item)
 
-            (stdout, stderr, retcode) = ipautil.run(args, raiseonerr=False)
+            _stdout, _stderr, retcode = ipautil.run(args, raiseonerr=False)
             if retcode == 0:
                 authorized_keys_changes = candidate
                 break
@@ -1474,7 +1477,7 @@ def configure_automount(options):
         args.append('--no-sssd')
 
     try:
-        stdout, _, _ = run(args)
+        stdout, _, _ = run(args, stdout_encoding='ascii', stdout_strict=False)
     except Exception as e:
         root_logger.error('Automount configuration failed: %s', str(e))
     else:
@@ -1490,7 +1493,8 @@ def configure_nisdomain(options, domain):
     # First backup the old NIS domain name
     if os.path.exists(paths.BIN_NISDOMAINNAME):
         try:
-            nis_domain_name, _, _ = ipautil.run([paths.BIN_NISDOMAINNAME])
+            nis_domain_name, _, _ = ipautil.run([paths.BIN_NISDOMAINNAME],
+                                                stdout_encoding='ascii')
         except CalledProcessError as e:
             pass
 
@@ -1529,8 +1533,9 @@ def unconfigure_nisdomain():
 
 
 def get_iface_from_ip(ip_addr):
-    ipresult = ipautil.run([paths.IP, '-oneline', 'address', 'show'])
-    for line in ipresult[0].split('\n'):
+    sout, _serr, _rc = ipautil.run([paths.IP, '-oneline', 'address', 'show'],
+                                   stdout_encoding='ascii')
+    for line in sout.split('\n'):
         fields = line.split()
         if len(fields) < 6:
             continue
@@ -1547,8 +1552,8 @@ def get_local_ipaddresses(iface=None):
     args = [paths.IP, '-oneline', 'address', 'show']
     if iface:
         args += ['dev', iface]
-    ipresult = ipautil.run(args)
-    lines = ipresult[0].split('\n')
+    sout, _serr, _rc = ipautil.run(args, stdout_encoding='ascii')
+    lines = sout.split('\n')
     ips = []
     for line in lines:
         fields = line.split()
@@ -1919,7 +1924,8 @@ def get_ca_certs_from_http(url, warn=True):
     root_logger.debug("trying to retrieve CA cert via HTTP from %s", url)
     try:
 
-        stdout, stderr, rc = run([paths.BIN_WGET, "-O", "-", url])
+        stdout, _stderr, _rc = run([paths.BIN_WGET, "-O", "-", url],
+                                   stdout_encoding='ascii')
     except CalledProcessError as e:
         raise errors.NoCertificateError(entry=url)
 
@@ -2668,7 +2674,9 @@ def install(options, env, fstore, statestore):
                 return CLIENT_INSTALL_ERROR
 
             # Now join the domain
-            (stdout, stderr, returncode) = run(join_args, raiseonerr=False, env=env, nolog=nolog)
+            _stdout, stderr, returncode = run(
+                join_args, raiseonerr=False, env=env, nolog=nolog,
+                stderr_encoding='ascii')
 
             if returncode != 0:
                 root_logger.error("Joining realm failed: %s", stderr)
@@ -2691,8 +2699,7 @@ def install(options, env, fstore, statestore):
                 subject_base = DN(subject_base)
 
             if options.principal is not None:
-                stderr, stdout, returncode = run(
-                        ["kdestroy"], raiseonerr=False, env=env)
+                run(["kdestroy"], raiseonerr=False, env=env)
 
             # Obtain the TGT. We do it with the temporary krb5.conf, so that
             # only the KDC we're installing under is contacted.
diff --git a/ipalib/plugins/pwpolicy.py b/ipalib/plugins/pwpolicy.py
index 5e98d5469eac0a3f0094d0485dc5d0aaee299438..4d4d32a0615208d9d4a1a285e53d446315263de5 100644
--- a/ipalib/plugins/pwpolicy.py
+++ b/ipalib/plugins/pwpolicy.py
@@ -279,7 +279,8 @@ class pwpolicy(LDAPObject):
     has_lockout = False
     lockout_params = ()
 
-    (stdout, stderr, rc) = run(['klist', '-V'], raiseonerr=False)
+    stdout, _stderr, rc = run(['klist', '-V'], raiseonerr=False,
+                              stdout_encoding='ascii')
     if rc == 0:
         verstr = stdout.split()[-1]
         ver = version.LooseVersion(verstr)
diff --git a/ipaplatform/base/services.py b/ipaplatform/base/services.py
index 6dcb26bb938717fdbe6f7f92a43982fe7dbdcb05..2526ac41aa86f5a8ab0071036ce74355737443f2 100644
--- a/ipaplatform/base/services.py
+++ b/ipaplatform/base/services.py
@@ -323,9 +323,10 @@ class SystemdService(PlatformService):
 
         while True:
             try:
-                (sout, serr, rcode) = ipautil.run(
+                sout, _serr, rcode = ipautil.run(
                     [paths.SYSTEMCTL, "is-active", instance],
-                    capture_output=True
+                    capture_output=True,
+                    stdout_encoding='ascii', stdout_strict=False,
                 )
             except ipautil.CalledProcessError as e:
                 if e.returncode == 3 and 'activating' in str(e.output):
@@ -345,9 +346,9 @@ class SystemdService(PlatformService):
 
     def is_installed(self):
         try:
-            (sout, serr, rcode) = ipautil.run([paths.SYSTEMCTL,
-                                               "list-unit-files",
-                                               "--full"])
+            sout, _serr, rcode = ipautil.run(
+                [paths.SYSTEMCTL, "list-unit-files", "--full"],
+                stdout_encoding='ascii', stdout_strict=False)
             if rcode != 0:
                 return False
             else:
@@ -363,10 +364,9 @@ class SystemdService(PlatformService):
     def is_enabled(self, instance_name=""):
         enabled = True
         try:
-            (sout, serr, rcode) = ipautil.run(
-                                      [paths.SYSTEMCTL,
-                                       "is-enabled",
-                                        self.service_instance(instance_name)])
+            _sout, _serr, rcode = ipautil.run(
+                [paths.SYSTEMCTL, "is-enabled",
+                 self.service_instance(instance_name)])
 
             if rcode != 0:
                 enabled = False
@@ -378,10 +378,10 @@ class SystemdService(PlatformService):
     def is_masked(self, instance_name=""):
         masked = False
         try:
-            (sout, serr, rcode) = ipautil.run(
-                                      [paths.SYSTEMCTL,
-                                       "is-enabled",
-                                        self.service_instance(instance_name)])
+            sout, _serr, rcode = ipautil.run(
+                [paths.SYSTEMCTL, "is-enabled",
+                 self.service_instance(instance_name)],
+                stdout_encoding='ascii', stdout_strict=False)
 
             if rcode == 1 and sout == 'masked':
                 masked = True
diff --git a/ipaplatform/redhat/services.py b/ipaplatform/redhat/services.py
index 757908f9581d5f04176dd5243fc64bec4c074c7f..1ad1a4e66cf92ca13ba853a029079dd249d7e444 100644
--- a/ipaplatform/redhat/services.py
+++ b/ipaplatform/redhat/services.py
@@ -225,7 +225,8 @@ class RedHatCAService(RedHatService):
                     url
                 ]
 
-                stdout, stderr, returncode = ipautil.run(args)
+                stdout, _stderr, _returncode = ipautil.run(
+                    args, stdout_encoding='utf-8')
 
                 status = dogtag._parse_ca_status(stdout)
                 # end of workaround
diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py
index 94d2cb4e906965a20bcfdd55f38854005091c26f..12b5481ddd647be3e3a696a30c3ee953b1ddc9ac 100644
--- a/ipaplatform/redhat/tasks.py
+++ b/ipaplatform/redhat/tasks.py
@@ -375,7 +375,9 @@ class RedHatTaskNamespace(BaseTaskNamespace):
             if state is None:
                 continue
             try:
-                (stdout, stderr, rc) = ipautil.run([paths.GETSEBOOL, setting])
+                stdout, _stderr, _rc = ipautil.run(
+                    [paths.GETSEBOOL, setting],
+                    stdout_encoding='ascii', stdout_strict=False)
                 original_state = stdout.split()[2]
                 if backup_func is not None:
                     backup_func(setting, original_state)
diff --git a/ipapython/certdb.py b/ipapython/certdb.py
index 704bae528b44a3a3b978d25982a7e75a8e3af0f6..3f687c584102f7844555b79dd2dce0254f0c5370 100644
--- a/ipapython/certdb.py
+++ b/ipapython/certdb.py
@@ -110,7 +110,7 @@ class NSSDatabase(object):
     def run_certutil(self, args, stdin=None):
         new_args = [paths.CERTUTIL, "-d", self.secdir]
         new_args = new_args + args
-        return ipautil.run(new_args, stdin)
+        return ipautil.run(new_args, stdin, stdin_encoding='ascii')
 
     def create_db(self, password_filename):
         """Create cert DB
@@ -177,7 +177,7 @@ class NSSDatabase(object):
             pkcs12_passwd = pkcs12_passwd + '\n'
             args = args + ["-w", paths.DEV_STDIN]
         try:
-            ipautil.run(args, stdin=pkcs12_passwd)
+            ipautil.run(args, stdin=pkcs12_passwd, stdin_encoding='ascii')
         except ipautil.CalledProcessError as e:
             if e.returncode == 17:
                 raise RuntimeError("incorrect password for pkcs#12 file %s" %
@@ -247,7 +247,10 @@ class NSSDatabase(object):
                             '-print_certs',
                         ]
                         try:
-                            stdout, stderr, rc = ipautil.run(args, stdin=body)
+                            stdout, _stderr, rc = ipautil.run(
+                                args, stdin=body,
+                                stdin_encoding='ascii',
+                                stdout_encoding='ascii')
                         except ipautil.CalledProcessError as e:
                             if label == 'CERTIFICATE':
                                 root_logger.warning(
@@ -286,7 +289,10 @@ class NSSDatabase(object):
                                 '-passin', 'file:' + key_pwdfile.name,
                             ]
                         try:
-                            stdout, stderr, rc = ipautil.run(args, stdin=body)
+                            stdout, _stderr, _rc = ipautil.run(
+                                args, stdin=body,
+                                stdin_encoding='ascii',
+                                stdout_encoding='ascii')
                         except ipautil.CalledProcessError as e:
                             root_logger.warning(
                                 "Skipping private key in %s at line %s: %s",
diff --git a/ipapython/dnssec/bindmgr.py b/ipapython/dnssec/bindmgr.py
index 1822dacf2535e7c37062e4d639e01289edcf5074..31a17a51ac3ece4753f55dd0d6e9c301a5dbd8aa 100644
--- a/ipapython/dnssec/bindmgr.py
+++ b/ipapython/dnssec/bindmgr.py
@@ -40,8 +40,9 @@ class BINDMgr(object):
 
     def notify_zone(self, zone):
         cmd = ['rndc', 'sign', zone.to_text()]
-        output = ipautil.run(cmd)[0]
-        self.log.info(output)
+        output, _stderr, _rc = ipautil.run(cmd, stdout_encoding='ascii',
+                                           stdout_strict=False)
+        self.log.info('%s', output)
 
     def dn2zone_name(self, dn):
         """cn=KSK-20140813162153Z-cede9e182fc4af76c4bddbc19123a565,cn=keys,idnsname=test,cn=dns,dc=ipa,dc=example"""
@@ -110,7 +111,8 @@ class BINDMgr(object):
         cmd.append(zone.to_text())
 
         # keys has to be readable by ODS & named
-        basename = ipautil.run(cmd)[0].strip()
+        stdout, _stderr, _rc = ipautil.run(cmd, stdout_encoding='ascii')
+        basename = stdout.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 efbe16cc6ebf050d9cf347ed97b2b2e4b37c8a6e..f81f25b7d86f7d7132423cb018542569dbf43c88 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]
+        stdout, _stderr, _rc = ipautil.run(cmd, stdout_encoding='ascii')
+        return stdout
 
     def get_ods_zonelist(self):
         stdout = self.ksmutil(['zonelist', 'export'])
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4acdd1a98818bf311a8fef103e7219cc62a28ec1..8aca29075ec5e6e152a673846f85ad110dee1d7a 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -37,6 +37,7 @@ import gssapi
 import pwd
 import grp
 from contextlib import contextmanager
+import locale
 
 from dns import resolver, rdatatype
 from dns.exception import DNSException
@@ -154,8 +155,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')
+            stdout, _stderr, _rc = run(
+                [paths.IP, '-family', family, '-oneline', 'address', 'show'],
+                stdout_encoding='ascii')
+            lines = stdout.split('\n')
             for line in lines:
                 fields = line.split()
                 if len(fields) < 4:
@@ -255,11 +258,33 @@ 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
+
+def _errors_param(strict):
+    if strict:
+        return 'strict'
+    else:
+        return 'replace'
 
 def run(args, stdin=None, raiseonerr=True,
         nolog=(), env=None, capture_output=True, skip_output=False, cwd=None,
-        runas=None, timeout=None, suplementary_groups=[]):
+        runas=None, timeout=None, suplementary_groups=[],
+        stdin_encoding=None, stdout_encoding=None, stderr_encoding=None,
+        stdout_strict=True, stderr_strict=True):
     """
     Execute a command and return stdin, stdout and the process return code.
 
@@ -292,6 +317,17 @@ 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 stdin_encoding: For Python 3, the encoding of the stdin string.
+        If None, bytes are expected. Only effective if stdin is not None.
+    :param stdout_encoding: For Python 3, the encoding with which stdout
+        should be decoded. If None, bytes are returned.
+    :param stderr_encoding: Like stdout_encoding, but for stderr
+    :param stdout_strict: For Python 3: If True, unencodable bytes in stdout
+        cause an UnicodeError. False, invalid characters are encoded with
+        backslash sequences.
+        Setting this to False is suitable if the output goes to a log file
+        or user-visible message.
+    :param stderr_strict: Like stdout_strict, but for stderr
     """
     assert isinstance(suplementary_groups, list)
     p_in = None
@@ -318,12 +354,15 @@ def run(args, stdin=None, raiseonerr=True,
         p_out = subprocess.PIPE
         p_err = subprocess.PIPE
 
+    if six.PY3 and stdin is not None and stdin_encoding:
+        stdin = stdin.encode(stdin_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,7 +391,15 @@ def run(args, stdin=None, raiseonerr=True,
                              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
+        if six.PY2:
+            stdout, stderr = str(stdout), str(stderr)    # Make pylint happy
+        else:
+            if stdout_encoding:
+                stdout = stdout.decode(stdout_encoding,
+                                       errors=_errors_param(stdout_strict))
+            if stderr_encoding:
+                stderr = stderr.decode(stderr_encoding,
+                                       errors=_errors_param(stderr_strict))
     except KeyboardInterrupt:
         root_logger.debug('Process interrupted')
         p.wait()
@@ -374,8 +421,18 @@ def run(args, stdin=None, raiseonerr=True,
     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 six.PY3 and isinstance(stdout, bytes):
+            stdout_logstr = stdout.decode(locale.getpreferredencoding(),
+                                          errors='replace')
+        else:
+            stdout_logstr = stdout
+        if six.PY3 and isinstance(stderr, bytes):
+            stderr_logstr = stderr.decode(locale.getpreferredencoding(),
+                                          errors='replace')
+        else:
+            stderr_logstr = stderr
+        root_logger.debug('stdout=%s' % stdout_logstr)
+        root_logger.debug('stderr=%s' % stderr_logstr)
 
     if p.returncode != 0 and raiseonerr:
         raise CalledProcessError(p.returncode, arg_string, stdout)
@@ -1278,8 +1335,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)
+    _stdout, stderr, retcode = run(args, stdin=password, env=env,
+                                   raiseonerr=False,
+                                   stderr_encoding='ascii',
+                                   stderr_strict=False)
     if retcode:
         raise RuntimeError(stderr)
 
diff --git a/ipapython/kernel_keyring.py b/ipapython/kernel_keyring.py
index d30531cabaee5c12376f0821a21a6f63cd60397c..769f6806e36fa2b91979adca575677c3813bb304 100644
--- a/ipapython/kernel_keyring.py
+++ b/ipapython/kernel_keyring.py
@@ -44,13 +44,17 @@ 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)
+    assert isinstance(key, str)
+    stdout, _stderr, rc = run(['keyctl', 'search', KEYRING, KEYTYPE, key],
+                              raiseonerr=False)
     if rc:
         raise ValueError('key %s not found' % key)
     return stdout.rstrip()
 
 def get_persistent_key(key):
-    (stdout, stderr, rc) = run(['keyctl', 'get_persistent', KEYRING, key], raiseonerr=False)
+    assert isinstance(key, str)
+    stdout, _stderr, rc = run(['keyctl', 'get_persistent', KEYRING, key],
+                              raiseonerr=False)
     if rc:
         raise ValueError('persistent key %s not found' % key)
     return stdout.rstrip()
@@ -68,6 +72,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,8 +85,10 @@ 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)
+    stdout, stderr, rc = run(['keyctl', 'pipe', real_key], raiseonerr=False,
+                             stderr_encoding='ascii', stderr_strict=False)
     if rc:
         raise ValueError('keyctl pipe failed: %s' % stderr)
 
@@ -91,9 +98,13 @@ 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)
+        _stdout, stderr, rc = run(['keyctl', 'pupdate', real_key], stdin=value,
+                                  raiseonerr=False,
+                                  stderr_encoding='ascii', stderr_strict=False)
         if rc:
             raise ValueError('keyctl pupdate failed: %s' % stderr)
     else:
@@ -103,9 +114,13 @@ 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)
+    _stdout, stderr, rc = run(['keyctl', 'padd', KEYTYPE, key, KEYRING],
+                              stdin=value, raiseonerr=False,
+                              stderr_encoding='ascii', stderr_strict=False)
     if rc:
         raise ValueError('keyctl padd failed: %s' % stderr)
 
@@ -113,7 +128,10 @@ 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)
+    _stdout, stderr, rc = run(['keyctl', 'unlink', real_key, KEYRING],
+                               raiseonerr=False,
+                               stderr_encoding='ascii', stderr_strict=False)
     if rc:
         raise ValueError('keyctl unlink failed: %s' % stderr)
diff --git a/ipaserver/dcerpc.py b/ipaserver/dcerpc.py
index 2e412861ebc265a9b07c8634068151181a3e9b9e..7da72da93b99f635b66334ac7c14a0b4669fccfd 100644
--- a/ipaserver/dcerpc.py
+++ b/ipaserver/dcerpc.py
@@ -588,7 +588,7 @@ class DomainValidator(object):
         # Destroy the contents of the ccache
         root_logger.debug('Destroying the contents of the separate ccache')
 
-        (stdout, stderr, returncode) = ipautil.run(
+        ipautil.run(
             [paths.KDESTROY, '-A', '-c', ccache_path],
             env={'KRB5CCNAME': ccache_path},
             raiseonerr=False)
@@ -597,7 +597,7 @@ class DomainValidator(object):
         root_logger.debug('Running kinit from ipa.keytab to obtain HTTP '
                           'service principal with MS-PAC attached.')
 
-        (stdout, stderr, returncode) = ipautil.run(
+        (_stdout, _stderr, returncode) = ipautil.run(
             [paths.KINIT, '-kt', keytab, principal],
             env={'KRB5CCNAME': ccache_path},
             raiseonerr=False)
@@ -605,8 +605,6 @@ class DomainValidator(object):
         if returncode == 0:
             return (ccache_path, principal)
         else:
-            root_logger.debug('Kinit failed, stout: %s, stderr: %s'
-                              % (stdout, stderr))
             return (None, None)
 
     def kinit_as_administrator(self, domain):
@@ -634,7 +632,7 @@ class DomainValidator(object):
         # Destroy the contents of the ccache
         root_logger.debug('Destroying the contents of the separate ccache')
 
-        (stdout, stderr, returncode) = ipautil.run(
+        ipautil.run(
             [paths.KDESTROY, '-A', '-c', ccache_path],
             env={'KRB5CCNAME': ccache_path},
             raiseonerr=False)
@@ -642,17 +640,16 @@ class DomainValidator(object):
         # Destroy the contents of the ccache
         root_logger.debug('Running kinit with credentials of AD administrator')
 
-        (stdout, stderr, returncode) = ipautil.run(
+        (_stdout, _stderr, returncode) = ipautil.run(
             [paths.KINIT, principal],
             env={'KRB5CCNAME': ccache_path},
             stdin=password,
-            raiseonerr=False)
+            raiseonerr=False,
+            stdin_encoding='ascii')
 
         if returncode == 0:
             return (ccache_path, principal)
         else:
-            root_logger.debug('Kinit failed, stout: %s, stderr: %s'
-                              % (stdout, stderr))
             return (None, None)
 
     def search_in_dc(self, domain, filter, attrs, scope, basedn=None,
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 90edb362f496b89f433532bf8786c29da3902de9..90bebf2ce3e358f23d386f9ccf990870f8b84581 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -620,10 +620,11 @@ class CAInstance(DogtagInstance):
             x509.write_certificate(cert.der_data, cert_file.name)
             cert_file.flush()
 
-            cert_chain, stderr, rc = ipautil.run(
+            cert_chain, _stderr, _rc = ipautil.run(
                 [paths.OPENSSL, 'crl2pkcs7',
                  '-certfile', self.cert_chain_file,
-                 '-nocrl'])
+                 '-nocrl'],
+                stdout_encoding='ascii')
             # Dogtag chokes on the header and footer, remove them
             # https://bugzilla.redhat.com/show_bug.cgi?id=1127838
             cert_chain = re.search(
@@ -878,7 +879,8 @@ class CAInstance(DogtagInstance):
                 self.fqdn, self.dogtag_constants.AGENT_SECURE_PORT),
         ]
         (stdout, _stderr, _returncode) = ipautil.run(
-            args, nolog=(self.admin_password,))
+            args, nolog=(self.admin_password,),
+            stdout_encoding='ascii')
 
         data = stdout.split(self.dogtag_constants.RACERT_LINE_SEP)
         params = get_defList(data)
@@ -901,7 +903,8 @@ class CAInstance(DogtagInstance):
                 self.fqdn, self.dogtag_constants.AGENT_SECURE_PORT),
         ]
         (stdout, _stderr, _returncode) = ipautil.run(
-            args, nolog=(self.admin_password,))
+            args, nolog=(self.admin_password,),
+            stdout_encoding='ascii')
 
         data = stdout.split(self.dogtag_constants.RACERT_LINE_SEP)
         outputList = get_outputList(data)
@@ -994,14 +997,15 @@ class CAInstance(DogtagInstance):
 
         conn.disconnect()
 
-    def __run_certutil(self, args, database=None, pwd_file=None, stdin=None):
+    def __run_certutil(self, args, database=None, pwd_file=None, stdin=None,
+                       **kwargs):
         if not database:
             database = self.ra_agent_db
         if not pwd_file:
             pwd_file = self.ra_agent_pwd
         new_args = [paths.CERTUTIL, "-d", database, "-f", pwd_file]
         new_args = new_args + args
-        return ipautil.run(new_args, stdin, nolog=(pwd_file,))
+        return ipautil.run(new_args, stdin, nolog=(pwd_file,), **kwargs)
 
     def __create_ra_agent_db(self):
         if ipautil.file_exists(self.ra_agent_db + "/cert8.db"):
@@ -1062,7 +1066,7 @@ class CAInstance(DogtagInstance):
              "-inform",
              "DER",
              "-print_certs",
-             ], stdin=data)
+             ], stdin=data, stdout_encoding='ascii')
 
         # Ok, now we have all the certificates in certs, walk through it
         # and pull out each certificate and add it to our database
@@ -1106,7 +1110,8 @@ class CAInstance(DogtagInstance):
             (stdout, _stderr, _returncode) = self.__run_certutil(
                 ["-R", "-k", "rsa", "-g", "2048", "-s",
                  str(DN(('CN', 'IPA RA'), self.subject_base)),
-                 "-z", noise_name, "-a"])
+                 "-z", noise_name, "-a"],
+                stdout_encoding='utf-8')
         finally:
             os.remove(noise_name)
 
@@ -1298,7 +1303,8 @@ class CAInstance(DogtagInstance):
 
     def publish_ca_cert(self, location):
         args = ["-L", "-n", self.canickname, "-a"]
-        (cert, _err, _returncode) = self.__run_certutil(args)
+        (cert, _err, _returncode) = self.__run_certutil(
+            args, stdout_encoding='ascii')
         fd = open(location, "w+")
         fd.write(cert)
         fd.close()
diff --git a/ipaserver/install/certs.py b/ipaserver/install/certs.py
index 658e8ec4526c986bb56ed6b7263e3557e7dc0d81..fc7b8f1d3f3073c5193b803e6c319f2c1c0389b1 100644
--- a/ipaserver/install/certs.py
+++ b/ipaserver/install/certs.py
@@ -173,7 +173,7 @@ class CertDB(object):
         new_args = [paths.SIGNTOOL, "-d", self.secdir, "-p", password]
 
         new_args = new_args + args
-        ipautil.run(new_args, stdin)
+        ipautil.run(new_args, stdin, stdin_encoding='ascii')
 
     def create_noise_file(self):
         if ipautil.file_exists(self.noise_fname):
diff --git a/ipaserver/install/httpinstance.py b/ipaserver/install/httpinstance.py
index b7a15702acc77dbb76e69de5b8cac7d06666512d..ea33936a3ba86be95e4a2142efe1731c981cc7d0 100644
--- a/ipaserver/install/httpinstance.py
+++ b/ipaserver/install/httpinstance.py
@@ -64,7 +64,9 @@ def httpd_443_configured():
     False otherwise.
     """
     try:
-        (stdout, stderr, rc) = ipautil.run([paths.HTTPD, '-t', '-D', 'DUMP_VHOSTS'])
+        stdout, _stderr, _rc = ipautil.run([paths.HTTPD, '-t', '-D', 'DUMP_VHOSTS'],
+                                           stdout_encoding='ascii',
+                                           stdout_strict=False)
     except ipautil.CalledProcessError as e:
         service.print_msg("WARNING: cannot check if port 443 is already configured")
         service.print_msg("httpd returned error when checking: %s" % e)
diff --git a/ipaserver/install/ipa_backup.py b/ipaserver/install/ipa_backup.py
index 28054be3fdd0e062e3daae4850b506d4b8d2b5cf..e645d55cbf95a52f98cc9dbfc7291c2a0c4044e2 100644
--- a/ipaserver/install/ipa_backup.py
+++ b/ipaserver/install/ipa_backup.py
@@ -86,7 +86,8 @@ def encrypt_file(filename, keyring, remove_original=True):
     args.append('-e')
     args.append(source)
 
-    (stdout, stderr, rc) = run(args, raiseonerr=False)
+    _stdout, stderr, rc = run(args, raiseonerr=False,
+                              stderr_encoding='ascii', stderr_strict=False)
     if rc != 0:
         raise admintool.ScriptError('gpg failed: %s' % stderr)
 
@@ -441,7 +442,9 @@ class Backup(admintool.AdminTool):
                     '-r',
                     '-n', backend,
                     '-a', ldiffile]
-            (stdout, stderr, rc) = run(args, raiseonerr=False)
+            _stdout, stderr, rc = run(args, raiseonerr=False,
+                                      stderr_encoding='ascii',
+                                      stderr_strict=False)
             if rc != 0:
                 self.log.critical("db2ldif failed: %s", stderr)
 
@@ -485,7 +488,9 @@ class Backup(admintool.AdminTool):
             wait_for_task(conn, dn)
         else:
             args = [paths.DB2BAK, bakdir, '-Z', instance]
-            (stdout, stderr, rc) = run(args, raiseonerr=False)
+            _stdout, stderr, rc = run(args, raiseonerr=False,
+                                      stderr_encoding='ascii',
+                                      stderr_strict=False)
             if rc != 0:
                 self.log.critical("db2bak failed: %s" % stderr)
 
@@ -514,7 +519,9 @@ class Backup(admintool.AdminTool):
         if options.logs:
             args.extend(verify_directories(self.logs))
 
-        (stdout, stderr, rc) = run(args, raiseonerr=False)
+        _stdout, stderr, rc = run(args, raiseonerr=False,
+                                  stderr_encoding='ascii',
+                                  stderr_strict=False)
         if rc != 0:
             raise admintool.ScriptError('tar returned non-zero code '
                 '%d: %s' % (rc, stderr))
@@ -535,14 +542,18 @@ class Backup(admintool.AdminTool):
                    ]
             args.extend(missing_directories)
 
-            (stdout, stderr, rc) = run(args, raiseonerr=False)
+            _stdout, stderr, rc = run(args, raiseonerr=False,
+                                       stderr_encoding='ascii',
+                                       stderr_strict=False)
             if rc != 0:
                 raise admintool.ScriptError('tar returned non-zero %d when adding '
                     'directory structure: %s' % (rc, stderr))
 
         # Compress the archive. This is done separately, since 'tar' cannot
         # append to a compressed archive.
-        (stdout, stderr, rc) = run(['gzip', tarfile], raiseonerr=False)
+        _stdout, stderr, rc = run(['gzip', tarfile], raiseonerr=False,
+                                  stderr_encoding='ascii',
+                                  stderr_strict=False)
         if rc != 0:
             raise admintool.ScriptError('gzip returned non-zero %d when '
                 'compressing the backup: %s' % (rc, stderr))
@@ -617,9 +628,11 @@ class Backup(admintool.AdminTool):
                 filename,
                 '.'
                ]
-        (stdout, stderr, rc) = run(args, raiseonerr=False)
+        _stdout, stderr, rc = run(args, raiseonerr=False,
+                                  stderr_encoding='ascii',
+                                  stderr_strict=False)
         if rc != 0:
-            raise admintool.ScriptError('tar returned non-zero %d: %s' % (rc, stdout))
+            raise admintool.ScriptError('tar returned non-zero %d: %s' % (rc, stderr))
 
         if encrypt:
             self.log.info('Encrypting %s' % filename)
diff --git a/ipaserver/install/ipa_restore.py b/ipaserver/install/ipa_restore.py
index 792ad54c5707874f34e752887fcfb5df472fb937..79606506ffdc6163dc473d373c41a787a5ed7fb4 100644
--- a/ipaserver/install/ipa_restore.py
+++ b/ipaserver/install/ipa_restore.py
@@ -88,7 +88,9 @@ def decrypt_file(tmpdir, filename, keyring):
     args.append('-d')
     args.append(source)
 
-    (stdout, stderr, rc) = run(args, raiseonerr=False)
+    _stdout, stderr, rc = run(args, raiseonerr=False,
+                              stderr_encoding='ascii',
+                              stderr_strict=False)
     if rc != 0:
         raise admintool.ScriptError('gpg failed: %s' % stderr)
 
@@ -365,7 +367,9 @@ class Restore(admintool.AdminTool):
                     dirsrv.start(capture_output=False)
             else:
                 self.log.info('Stopping IPA services')
-                (stdout, stderr, rc) = run(['ipactl', 'stop'], raiseonerr=False)
+                _stdout, stderr, rc = run(['ipactl', 'stop'], raiseonerr=False,
+                                          stderr_encoding='ascii',
+                                          stderr_strict=False)
                 if rc not in [0, 6]:
                     self.log.warn('Stopping IPA failed: %s' % stderr)
 
@@ -576,7 +580,9 @@ class Restore(admintool.AdminTool):
                     '-Z', instance,
                     '-i', ldiffile,
                     '-n', backend]
-            (stdout, stderr, rc) = run(args, raiseonerr=False)
+            _stdout, stderr, rc = run(args, raiseonerr=False,
+                                      stderr_encoding='ascii',
+                                      stderr_strict=False)
             if rc != 0:
                 self.log.critical("ldif2db failed: %s" % stderr)
 
@@ -631,7 +637,9 @@ class Restore(admintool.AdminTool):
             if backend is not None:
                 args.append('-n')
                 args.append(backend)
-            (stdout, stderr, rc) = run(args, raiseonerr=False)
+            _stdout, stderr, rc = run(args, raiseonerr=False,
+                                       stderr_encoding='ascii',
+                                       stderr_strict=False)
             if rc != 0:
                 self.log.critical("bak2db failed: %s" % stderr)
 
@@ -653,7 +661,9 @@ class Restore(admintool.AdminTool):
                 paths.IPA_DEFAULT_CONF[1:],
                ]
 
-        (stdout, stderr, rc) = run(args, raiseonerr=False)
+        _stdout, stderr, rc = run(args, raiseonerr=False,
+                                   stderr_encoding='ascii',
+                                   stderr_strict=False)
         if rc != 0:
             self.log.critical('Restoring %s failed: %s' %
                               (paths.IPA_DEFAULT_CONF, stderr))
@@ -699,7 +709,9 @@ class Restore(admintool.AdminTool):
             args.append('--exclude')
             args.append('var/log')
 
-        (stdout, stderr, rc) = run(args, raiseonerr=False)
+        _stdout, stderr, rc = run(args, raiseonerr=False,
+                                   stderr_encoding='ascii',
+                                   stderr_strict=False)
         if rc != 0:
             self.log.critical('Restoring files failed: %s', stderr)
 
diff --git a/ipaserver/install/krbinstance.py b/ipaserver/install/krbinstance.py
index 1dd807c714aea52faeca732c2c1aba12d83fdc53..e87acf89769ad333dce6e683012087c96de9f017 100644
--- a/ipaserver/install/krbinstance.py
+++ b/ipaserver/install/krbinstance.py
@@ -280,7 +280,8 @@ class KrbInstance(service.Service):
             self.master_password + '\n',
         )
         try:
-            ipautil.run(args, nolog=(self.master_password,), stdin=''.join(dialogue))
+            ipautil.run(args, nolog=(self.master_password,), stdin=''.join(dialogue),
+                        stdin_encoding='ascii')
         except ipautil.CalledProcessError as e:
             print("Failed to initialize the realm container")
 
@@ -294,7 +295,9 @@ class KrbInstance(service.Service):
         MIN_KRB5KDC_WITH_WORKERS = "1.9"
         cpus = os.sysconf('SC_NPROCESSORS_ONLN')
         workers = False
-        (stdout, stderr, rc) = ipautil.run(['klist', '-V'], raiseonerr=False)
+        stdout, _stderr, rc = ipautil.run(['klist', '-V'], raiseonerr=False,
+                                          stdout_encoding='ascii',
+                                          stdout_strict=False)
         if rc == 0:
             verstr = stdout.split()[-1]
             ver = version.LooseVersion(verstr)
diff --git a/ipaserver/install/opendnssecinstance.py b/ipaserver/install/opendnssecinstance.py
index 4baf6b6bc3d1898aadade8c741f5105b5742d749..c3559741d94027e289422090432bc7c68a68608b 100644
--- a/ipaserver/install/opendnssecinstance.py
+++ b/ipaserver/install/opendnssecinstance.py
@@ -288,8 +288,9 @@ class OpenDNSSECInstance(service.Service):
             # regenerate zonelist.xml
             ods_enforcerd = services.knownservices.ods_enforcerd
             cmd = [paths.ODS_KSMUTIL, 'zonelist', 'export']
-            stdout, stderr, retcode = ipautil.run(cmd,
-                                          runas=ods_enforcerd.get_user_name())
+            stdout, _stderr, _retcode = ipautil.run(cmd,
+                                          runas=ods_enforcerd.get_user_name(),
+                                          stdout_encoding='ascii')
             with open(paths.OPENDNSSEC_ZONELIST_FILE, 'w') as zonelistf:
                 zonelistf.write(stdout)
                 os.chown(paths.OPENDNSSEC_ZONELIST_FILE,
@@ -304,7 +305,7 @@ class OpenDNSSECInstance(service.Service):
             ]
 
             ods_enforcerd = services.knownservices.ods_enforcerd
-            ipautil.run(command, stdin="y", runas=ods_enforcerd.get_user_name())
+            ipautil.run(command, stdin=b"y", runas=ods_enforcerd.get_user_name())
 
     def __setup_dnskeysyncd(self):
         # set up dnskeysyncd this is DNSSEC master
diff --git a/ipaserver/install/replication.py b/ipaserver/install/replication.py
index 443f7ca23bb40aeef861d679aa5bda1f0aa532ff..7fb78c5557b81c56a280c03aeeff8ae8e794687c 100644
--- a/ipaserver/install/replication.py
+++ b/ipaserver/install/replication.py
@@ -88,7 +88,7 @@ def replica_conn_check(master_host, host_name, realm, check_ca,
 
     if check_ca and dogtag_master_ds_port == dogtag.Dogtag9Constants.DS_PORT:
         args.append('--check-ca')
-    (stdin, stderr, returncode) = ipautil.run(
+    _stdin, _stderr, returncode = ipautil.run(
         args, raiseonerr=False, capture_output=False, nolog=nolog)
 
     if returncode != 0:
diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 6629e8ec12f50c83a691dcd60e2cbf1125598fcb..a68168c5a691200f7b68c16c3134eafa20f0ba6e 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -1048,7 +1048,7 @@ def uninstall(installer):
 
     print("Shutting down all IPA services")
     try:
-        (stdout, stderr, rc) = run([paths.IPACTL, "stop"], raiseonerr=False)
+        run([paths.IPACTL, "stop"], raiseonerr=False)
     except Exception as e:
         pass
 
@@ -1057,9 +1057,11 @@ def uninstall(installer):
 
     print("Removing IPA client configuration")
     try:
-        (stdout, stderr, rc) = run([paths.IPA_CLIENT_INSTALL, "--on-master",
+        stdout, _stderr, rc = run([paths.IPA_CLIENT_INSTALL, "--on-master",
                                     "--unattended", "--uninstall"],
-                                   raiseonerr=False)
+                                   raiseonerr=False,
+                                   stdout_encoding='ascii',
+                                   stdout_strict=False)
         if rc not in [0, 2]:
             root_logger.debug("ipa-client-install returned %d" % rc)
             raise RuntimeError(stdout)
diff --git a/ipatests/test_ipapython/test_ipautil.py b/ipatests/test_ipapython/test_ipautil.py
index d574bd809ab72a6459fd111e319b7bf4638f7b90..4e34a611c62df8ce1b6df88cd2a4e5092429c29c 100644
--- a/ipatests/test_ipapython/test_ipautil.py
+++ b/ipatests/test_ipapython/test_ipautil.py
@@ -1,3 +1,5 @@
+# encoding: utf-8
+
 # Authors:
 #   Jan Cholasta <jchol...@redhat.com>
 #
@@ -417,3 +419,47 @@ class TestTimeParser(object):
         nose.tools.assert_equal(-30, time.tzinfo.minoffset)
         offset = time.tzinfo.utcoffset(time.tzinfo.dst())
         nose.tools.assert_equal(((24 - 9) * 60 * 60) - (30 * 60), offset.seconds)
+
+
+def test_run():
+    out, _err, rc = ipautil.run(['echo', 'foo\x02bar'])
+    assert rc == 0
+    assert out == b'foo\x02bar\n'
+
+
+def test_run_bytes():
+    out, _err, rc = ipautil.run(['echo', b'\x01\x02'])
+    assert rc == 0
+    assert out == b'\x01\x02\n'
+
+
+def test_run_decode():
+    out, _err, rc = ipautil.run(['echo', u'á'.encode('utf-8')],
+                                stdout_encoding='utf-8')
+    assert rc == 0
+    if six.PY3:
+        assert out == 'á\n'
+    else:
+        assert out == 'á\n'.encode('utf-8')
+
+
+def test_run_decode_bad():
+    if six.PY3:
+        with pytest.raises(UnicodeDecodeError):
+            ipautil.run(['echo', b'\xa0\xa1'], stdout_encoding='utf-8')
+    else:
+        out, _err, rc = ipautil.run(['echo', '\xa0\xa1'],
+                                    stdout_encoding='utf-8')
+        assert rc == 0
+        assert out == '\xa0\xa1\n'
+
+
+def test_run_decode_not_strict():
+    out, _err, rc = ipautil.run(['echo', b'\xc3\xb1\xa0\xa1'],
+                                stdout_encoding='utf-8',
+                                stdout_strict=False)
+    assert rc == 0
+    if six.PY3:
+        assert out == 'ñ��\n'
+    else:
+        assert out == '\xc3\xb1\xa0\xa1\n'
diff --git a/ipatests/test_ipapython/test_keyring.py b/ipatests/test_ipapython/test_keyring.py
index 84b9f0ffa4252b4fdeb4302bf4b67212173dd685..79b4b3334a0e42779802ca96be4ce6f9506f6dda 100644
--- a/ipatests/test_ipapython/test_keyring.py
+++ b/ipatests/test_ipapython/test_keyring.py
@@ -28,8 +28,8 @@ import pytest
 pytestmark = pytest.mark.tier0
 
 TEST_KEY = 'ipa_test'
-TEST_VALUE = 'abc123'
-UPDATE_VALUE = '123abc'
+TEST_VALUE = b'abc123'
+UPDATE_VALUE = b'123abc'
 
 SIZE_256 = 'abcdefgh' * 32
 SIZE_512 = 'abcdefgh' * 64
@@ -94,9 +94,10 @@ class test_keyring(object):
 
         # Now update it 10 times
         for i in range(10):
-            kernel_keyring.update_key(TEST_KEY, 'test %d' %  i)
+            value = ('test %d' %  i).encode('ascii')
+            kernel_keyring.update_key(TEST_KEY, value)
             result = kernel_keyring.read_key(TEST_KEY)
-            assert(result == 'test %d' % i)
+            assert(result == value)
 
         kernel_keyring.del_key(TEST_KEY)
 
@@ -134,9 +135,9 @@ class test_keyring(object):
         """
         Test 512-bytes of data
         """
-        kernel_keyring.add_key(TEST_KEY, SIZE_512)
+        kernel_keyring.add_key(TEST_KEY, SIZE_512.encode('ascii'))
         result = kernel_keyring.read_key(TEST_KEY)
-        assert(result == SIZE_512)
+        assert(result == SIZE_512.encode('ascii'))
 
         kernel_keyring.del_key(TEST_KEY)
 
@@ -144,8 +145,8 @@ class test_keyring(object):
         """
         Test 1k bytes of data
         """
-        kernel_keyring.add_key(TEST_KEY, SIZE_1024)
+        kernel_keyring.add_key(TEST_KEY, SIZE_1024.encode('ascii'))
         result = kernel_keyring.read_key(TEST_KEY)
-        assert(result == SIZE_1024)
+        assert(result == SIZE_1024.encode('ascii'))
 
         kernel_keyring.del_key(TEST_KEY)
diff --git a/ipatests/test_xmlrpc/test_host_plugin.py b/ipatests/test_xmlrpc/test_host_plugin.py
index 868c09ce8b1974b9eacf08a28bc24454b3416902..052d70de8f2fd9c9e0a20de4b587d176de8e970d 100644
--- a/ipatests/test_xmlrpc/test_host_plugin.py
+++ b/ipatests/test_xmlrpc/test_host_plugin.py
@@ -694,7 +694,7 @@ class TestHostFalsePwdChange(XMLRPC_test):
         ]
 
         try:
-            out, err, rc = ipautil.run(new_args)
+            ipautil.run(new_args)
         except ipautil.CalledProcessError as e:
             # join operation may fail on 'adding key into keytab', but
             # the keytab is not necessary for further tests
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to