Merge pull request #551 from skuhl/which-fix

Improve consistency of PATH, environments, and which()

sshuttle/client.py

@@ -15,7 +15,7 @@ import sshuttle.ssyslog as ssyslog
 import sshuttle.sdnotify as sdnotify
 from sshuttle.ssnet import SockWrapper, Handler, Proxy, Mux, MuxWrapper
 from sshuttle.helpers import log, debug1, debug2, debug3, Fatal, islocal, \
-    resolvconf_nameservers
+    resolvconf_nameservers, which
 from sshuttle.methods import get_method, Features
 from sshuttle import __version__
 try:
@@ -196,11 +196,19 @@ class FirewallClient:
                     ['--firewall'])
         if ssyslog._p:
             argvbase += ['--syslog']
-        # Default to sudo unless on OpenBSD in which case use built in `doas`
+
+        # Determine how to prefix the command in order to elevate privileges.
         if platform.platform().startswith('OpenBSD'):
-            elev_prefix = ['doas']
+            elev_prefix = ['doas']  # OpenBSD uses built in `doas`
         else:
             elev_prefix = ['sudo', '-p', '[local sudo] Password: ']
+
+        # Look for binary and switch to absolute path if we can find
+        # it.
+        path = which(elev_prefix[0])
+        if path:
+            elev_prefix[0] = path
+
         if sudo_pythonpath:
             elev_prefix += ['/usr/bin/env',
                             'PYTHONPATH=%s' % python_path]

sshuttle/helpers.py

@@ -1,6 +1,7 @@
 import sys
 import socket
 import errno
+import os
 
 logprefix = ''
 verbose = 0
@@ -99,3 +100,75 @@ def family_to_string(family):
         return "AF_INET"
     else:
         return str(family)
+
+
+def get_env():
+    """An environment for sshuttle subprocesses. See get_path()."""
+    env = {
+        'PATH': get_path(),
+        'LC_ALL': "C",
+    }
+    return env
+
+
+def get_path():
+    """Returns a string of paths separated by os.pathsep.
+
+    Users might not have all of the programs sshuttle needs in their
+    PATH variable (i.e., some programs might be in /sbin). Use PATH
+    and a hardcoded set of paths to search through. This function is
+    used by our which() and get_env() functions. If which() and the
+    subprocess environments differ, programs that which() finds might
+    not be found at run time (or vice versa).
+    """
+    path = []
+    if "PATH" in os.environ:
+        path += os.environ["PATH"].split(os.pathsep)
+    # Python default paths.
+    path += os.defpath.split(os.pathsep)
+    # /sbin, etc are not in os.defpath and may not be in PATH either.
+    # /bin/ and /usr/bin below are probably redundant.
+    path += ['/bin', '/usr/bin', '/sbin', '/usr/sbin']
+
+    # Remove duplicates. Not strictly necessary.
+    path_dedup = []
+    for i in path:
+        if i not in path_dedup:
+            path_dedup.append(i)
+
+    return os.pathsep.join(path_dedup)
+
+
+if sys.version_info >= (3, 3):
+    from shutil import which as _which
+else:
+    # Although sshuttle does not officially support older versions of
+    # Python, some still run the sshuttle server on remote machines
+    # with old versions of python.
+    def _which(file, mode=os.F_OK | os.X_OK, path=None):
+        if path is not None:
+            search_paths = path.split(os.pathsep)
+        elif "PATH" in os.environ:
+            search_paths = os.environ["PATH"].split(os.pathsep)
+        else:
+            search_paths = os.defpath.split(os.pathsep)
+
+        for p in search_paths:
+            filepath = os.path.join(p, file)
+            if os.path.exists(filepath) and os.access(filepath, mode):
+                return filepath
+        return None
+
+
+def which(file, mode=os.F_OK | os.X_OK):
+    """A wrapper around shutil.which() that searches a predictable set of
+    paths and is more verbose about what is happening. See get_path()
+    for more information.
+    """
+    path = get_path()
+    rv = _which(file, mode, path)
+    if rv:
+        debug2("which() found '%s' at %s\n" % (file, rv))
+    else:
+        debug2("which() could not find '%s' in %s\n" % (file, path))
+    return rv

sshuttle/hostwatch.py

@@ -9,7 +9,7 @@ import platform
 
 import subprocess as ssubprocess
 import sshuttle.helpers as helpers
-from sshuttle.helpers import log, debug1, debug2, debug3
+from sshuttle.helpers import log, debug1, debug2, debug3, get_env
 
 POLL_TIME = 60 * 15
 NETSTAT_POLL_TIME = 30
@@ -125,14 +125,10 @@ def _check_dns(hostname):
 
 def _check_netstat():
     debug2(' > netstat\n')
-    env = {
-        'PATH': os.environ['PATH'],
-        'LC_ALL': "C",
-    }
     argv = ['netstat', '-n']
     try:
         p = ssubprocess.Popen(argv, stdout=ssubprocess.PIPE, stderr=null,
-                              env=env)
+                              env=get_env())
         content = p.stdout.read().decode("ASCII")
         p.wait()
     except OSError:
@@ -151,14 +147,10 @@ def _check_smb(hostname):
     if not _smb_ok:
         return
     debug2(' > smb: %s\n' % hostname)
-    env = {
-        'PATH': os.environ['PATH'],
-        'LC_ALL': "C",
-    }
     argv = ['smbclient', '-U', '%', '-L', hostname]
     try:
         p = ssubprocess.Popen(argv, stdout=ssubprocess.PIPE, stderr=null,
-                              env=env)
+                              env=get_env())
         lines = p.stdout.readlines()
         p.wait()
     except OSError:
@@ -214,14 +206,10 @@ def _check_nmb(hostname, is_workgroup, is_master):
     if not _nmb_ok:
         return
     debug2(' > n%d%d: %s\n' % (is_workgroup, is_master, hostname))
-    env = {
-        'PATH': os.environ['PATH'],
-        'LC_ALL': "C",
-    }
     argv = ['nmblookup'] + ['-M'] * is_master + ['--', hostname]
     try:
         p = ssubprocess.Popen(argv, stdout=ssubprocess.PIPE, stderr=null,
-                              env=env)
+                              env=get_env)
         lines = p.stdout.readlines()
         rv = p.wait()
     except OSError:

sshuttle/linux.py

@@ -1,7 +1,6 @@
-import os
 import socket
 import subprocess as ssubprocess
-from sshuttle.helpers import log, debug1, Fatal, family_to_string
+from sshuttle.helpers import log, debug1, Fatal, family_to_string, get_env
 
 
 def nonfatal(func, *args):
@@ -19,12 +18,8 @@ def ipt_chain_exists(family, table, name):
     else:
         raise Exception('Unsupported family "%s"' % family_to_string(family))
     argv = [cmd, '-t', table, '-nL']
-    env = {
-        'PATH': os.environ['PATH'],
-        'LC_ALL': "C",
-    }
     try:
-        output = ssubprocess.check_output(argv, env=env)
+        output = ssubprocess.check_output(argv, env=get_env())
         for line in output.decode('ASCII').split('\n'):
             if line.startswith('Chain %s ' % name):
                 return True
@@ -40,11 +35,7 @@ def ipt(family, table, *args):
     else:
         raise Exception('Unsupported family "%s"' % family_to_string(family))
     debug1('%s\n' % ' '.join(argv))
-    env = {
+    rv = ssubprocess.call(argv, env=get_env())
-        'PATH': os.environ['PATH'],
-        'LC_ALL': "C",
-    }
-    rv = ssubprocess.call(argv, env=env)
     if rv:
         raise Fatal('fw: %r returned %d' % (argv, rv))
 
@@ -55,11 +46,7 @@ def nft(family, table, action, *args):
     else:
         raise Exception('Unsupported family "%s"' % family_to_string(family))
     debug1('%s\n' % ' '.join(argv))
-    env = {
+    rv = ssubprocess.call(argv, env=get_env())
-        'PATH': os.environ['PATH'],
-        'LC_ALL': "C",
-    }
-    rv = ssubprocess.call(argv, env=env)
     if rv:
         raise Fatal('fw: %r returned %d' % (argv, rv))
 

sshuttle/methods/__init__.py

@@ -1,10 +1,9 @@
-import os
 import importlib
 import socket
 import struct
 import errno
 import ipaddress
-from sshuttle.helpers import Fatal, debug3
+from sshuttle.helpers import Fatal, debug3, which
 
 
 def original_dst(sock):
@@ -97,27 +96,19 @@ class BaseMethod(object):
         return False
 
 
-def _program_exists(name):
-    paths = (os.getenv('PATH') or os.defpath).split(os.pathsep)
-    for p in paths:
-        fn = '%s/%s' % (p, name)
-        if os.path.exists(fn):
-            return not os.path.isdir(fn) and os.access(fn, os.X_OK)
-
-
 def get_method(method_name):
     module = importlib.import_module("sshuttle.methods.%s" % method_name)
     return module.Method(method_name)
 
 
 def get_auto_method():
-    if _program_exists('iptables'):
+    if which('iptables'):
         method_name = "nat"
-    elif _program_exists('nft'):
+    elif which('nft'):
         method_name = "nft"
-    elif _program_exists('pfctl'):
+    elif which('pfctl'):
         method_name = "pf"
-    elif _program_exists('ipfw'):
+    elif which('ipfw'):
         method_name = "ipfw"
     else:
         raise Fatal(

sshuttle/methods/ipfw.py

@@ -2,7 +2,7 @@ import os
 import subprocess as ssubprocess
 from sshuttle.methods import BaseMethod
 from sshuttle.helpers import log, debug1, debug3, \
-    Fatal, family_to_string
+    Fatal, family_to_string, get_env
 
 recvmsg = None
 try:
@@ -61,11 +61,7 @@ else:
 
 def ipfw_rule_exists(n):
     argv = ['ipfw', 'list']
-    env = {
+    p = ssubprocess.Popen(argv, stdout=ssubprocess.PIPE, env=get_env())
-        'PATH': os.environ['PATH'],
-        'LC_ALL': "C",
-    }
-    p = ssubprocess.Popen(argv, stdout=ssubprocess.PIPE, env=env)
 
     found = False
     for line in p.stdout:
@@ -85,11 +81,7 @@ _oldctls = {}
 
 def _fill_oldctls(prefix):
     argv = ['sysctl', prefix]
-    env = {
+    p = ssubprocess.Popen(argv, stdout=ssubprocess.PIPE, env=get_env())
-        'PATH': os.environ['PATH'],
-        'LC_ALL': "C",
-    }
-    p = ssubprocess.Popen(argv, stdout=ssubprocess.PIPE, env=env)
     for line in p.stdout:
         line = line.decode()
         assert(line[-1] == '\n')
@@ -105,7 +97,7 @@ def _fill_oldctls(prefix):
 def _sysctl_set(name, val):
     argv = ['sysctl', '-w', '%s=%s' % (name, val)]
     debug1('>> %s\n' % ' '.join(argv))
-    return ssubprocess.call(argv, stdout=open(os.devnull, 'w'))
+    return ssubprocess.call(argv, stdout=open(os.devnull, 'w'), env=get_env())
     # No env: No output. (Or error that won't be parsed.)
 
 
@@ -139,7 +131,7 @@ def sysctl_set(name, val, permanent=False):
 def ipfw(*args):
     argv = ['ipfw', '-q'] + list(args)
     debug1('>> %s\n' % ' '.join(argv))
-    rv = ssubprocess.call(argv)
+    rv = ssubprocess.call(argv, env=get_env())
     # No env: No output. (Or error that won't be parsed.)
     if rv:
         raise Fatal('%r returned %d' % (argv, rv))
@@ -148,7 +140,7 @@ def ipfw(*args):
 def ipfw_noexit(*args):
     argv = ['ipfw', '-q'] + list(args)
     debug1('>> %s\n' % ' '.join(argv))
-    ssubprocess.call(argv)
+    ssubprocess.call(argv, env=get_env())
     # No env: No output. (Or error that won't be parsed.)
 
 

sshuttle/methods/pf.py

@@ -11,7 +11,8 @@ from fcntl import ioctl
 from ctypes import c_char, c_uint8, c_uint16, c_uint32, Union, Structure, \
     sizeof, addressof, memmove
 from sshuttle.firewall import subnet_weight
-from sshuttle.helpers import debug1, debug2, debug3, Fatal, family_to_string
+from sshuttle.helpers import debug1, debug2, debug3, Fatal, family_to_string, \
+    get_env
 from sshuttle.methods import BaseMethod
 
 
@@ -179,7 +180,7 @@ class FreeBsd(Generic):
         return freebsd
 
     def enable(self):
-        returncode = ssubprocess.call(['kldload', 'pf'])
+        returncode = ssubprocess.call(['kldload', 'pf'], env=get_env())
         # No env: No output.
         super(FreeBsd, self).enable()
         if returncode == 0:
@@ -189,7 +190,7 @@ class FreeBsd(Generic):
         super(FreeBsd, self).disable(anchor)
         if _pf_context['loaded_by_sshuttle'] and \
                 _pf_context['started_by_sshuttle'] == 0:
-            ssubprocess.call(['kldunload', 'pf'])
+            ssubprocess.call(['kldunload', 'pf'], env=get_env())
             # No env: No output.
 
     def add_anchors(self, anchor):
@@ -386,15 +387,10 @@ else:
 def pfctl(args, stdin=None):
     argv = ['pfctl'] + shlex.split(args)
     debug1('>> %s\n' % ' '.join(argv))
-
-    env = {
-        'PATH': os.environ['PATH'],
-        'LC_ALL': "C",
-    }
     p = ssubprocess.Popen(argv, stdin=ssubprocess.PIPE,
                           stdout=ssubprocess.PIPE,
                           stderr=ssubprocess.PIPE,
-                          env=env)
+                          env=get_env())
     o = p.communicate(stdin)
     if p.returncode:
         raise Fatal('%r returned %d' % (argv, p.returncode))

sshuttle/server.py

@@ -7,24 +7,6 @@ import sys
 import os
 import platform
 
-if sys.version_info >= (3, 0):
-    from shutil import which
-else:
-    # python2 compat: shutil.which is not available so we provide our
-    # own which command
-    def which(file, mode=os.F_OK | os.X_OK, path=None):
-        if path is not None:
-            search_paths = [path]
-        elif "PATH" in os.environ:
-            search_paths = os.environ["PATH"].split(os.pathsep)
-        else:
-            search_paths = os.defpath.split(os.pathsep)
-
-        for p in search_paths:
-            filepath = os.path.join(p, file)
-            if os.path.exists(filepath) and os.access(filepath, mode):
-                return filepath
-        return None
 
 import sshuttle.ssnet as ssnet
 import sshuttle.helpers as helpers
@@ -32,7 +14,7 @@ import sshuttle.hostwatch as hostwatch
 import subprocess as ssubprocess
 from sshuttle.ssnet import Handler, Proxy, Mux, MuxWrapper
 from sshuttle.helpers import b, log, debug1, debug2, debug3, Fatal, \
-    resolvconf_random_nameserver
+    resolvconf_random_nameserver, which, get_env
 
 
 def _ipmatch(ipstr):
@@ -100,11 +82,7 @@ def _route_iproute(line):
 
 def _list_routes(argv, extract_route):
     # FIXME: IPv4 only
-    env = {
+    p = ssubprocess.Popen(argv, stdout=ssubprocess.PIPE, env=get_env())
-        'PATH': os.environ['PATH'],
-        'LC_ALL': "C",
-    }
-    p = ssubprocess.Popen(argv, stdout=ssubprocess.PIPE, env=env)
     routes = []
     for line in p.stdout:
         if not line.strip():
@@ -119,7 +97,6 @@ def _list_routes(argv, extract_route):
     rv = p.wait()
     if rv != 0:
         log('WARNING: %r returned %d\n' % (argv, rv))
-        log('WARNING: That prevents --auto-nets from working.\n')
 
     return routes
 
@@ -130,7 +107,8 @@ def list_routes():
     elif which('netstat'):
         routes = _list_routes(['netstat', '-rn'], _route_netstat)
     else:
-        log('WARNING: Neither ip nor netstat were found on the server.\n')
+        log('WARNING: Neither "ip" nor "netstat" were found on the server. '
+            '--auto-nets feature will not work.\n')
         routes = []
 
     for (family, ip, width) in routes:

sshuttle/ssh.py

@@ -12,7 +12,7 @@ import ipaddress
 from urllib.parse import urlparse
 
 import sshuttle.helpers as helpers
-from sshuttle.helpers import debug2
+from sshuttle.helpers import debug2, which, get_path, Fatal
 
 
 def get_module_source(name):
@@ -141,6 +141,17 @@ def connect(ssh_cmd, rhostport, python, stderr, options):
             argv = (sshl +
                     portl +
                     [rhost, '--', pycmd])
+
+    # Our which() function searches for programs in get_path()
+    # directories (which include PATH). This step isn't strictly
+    # necessary if ssh is already in the user's PATH, but it makes the
+    # error message friendlier if the user incorrectly passes in a
+    # custom ssh command that we cannot find.
+    abs_path = which(argv[0])
+    if abs_path is None:
+        raise Fatal("Failed to find '%s' in path %s" % (argv[0], get_path()))
+    argv[0] = abs_path
+
     (s1, s2) = socket.socketpair()
 
     def setup():
@@ -148,6 +159,7 @@ def connect(ssh_cmd, rhostport, python, stderr, options):
         s2.close()
     s1a, s1b = os.dup(s1.fileno()), os.dup(s1.fileno())
     s1.close()
+
     debug2('executing: %r\n' % argv)
     p = ssubprocess.Popen(argv, stdin=s1a, stdout=s1b, preexec_fn=setup,
                           close_fds=True, stderr=stderr)

tests/client/test_methods_pf.py

@@ -4,7 +4,7 @@ from socket import AF_INET, AF_INET6
 import pytest
 from mock import Mock, patch, call, ANY
 from sshuttle.methods import get_method
-from sshuttle.helpers import Fatal
+from sshuttle.helpers import Fatal, get_env
 from sshuttle.methods.pf import FreeBsd, Darwin, OpenBsd
 
 
@@ -316,7 +316,8 @@ def test_setup_firewall_freebsd(mock_pf_get_dev, mock_ioctl, mock_pfctl,
              b'to <dns_servers> port 53 keep state\n'),
         call('-e'),
     ]
-    assert call(['kldload', 'pf']) in mock_subprocess_call.mock_calls
+    assert call(['kldload', 'pf'], env=get_env()) in \
+        mock_subprocess_call.mock_calls
     mock_pf_get_dev.reset_mock()
     mock_ioctl.reset_mock()
     mock_pfctl.reset_mock()