From fc1617531e96e654f03a3bc758c3dde4a8dda59b Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Thu, 19 Feb 2015 17:49:27 -0800 Subject: [PATCH 1/3] Use psutil instead of netstat subprocess --- .../client/standalone_authenticator.py | 41 ++--- .../tests/standalone_authenticator_test.py | 141 +++++++++++------- setup.py | 1 + 3 files changed, 103 insertions(+), 80 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index e2b1d7872..4bd6d3379 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -1,8 +1,8 @@ """Standalone authenticator.""" import os +import psutil import signal import socket -import subprocess import sys import time @@ -266,30 +266,23 @@ class StandaloneAuthenticator(object): If so, also tell the user via a display notification. .. warning:: - The current implementation is Linux-specific. (On other - operating systems, it will simply not detect bound ports.) - This function can only usefully be run as root. + On some operating systems, this function can only usefully be + run as root. :param int port: The TCP port in question. :returns: True or False.""" + listeners = [conn.pid for conn in psutil.net_connections() \ + if conn.status == 'LISTEN' and \ + conn.type == socket.SOCK_STREAM and \ + (conn.laddr == ('0.0.0.0', port) or \ + conn.laddr == ('::', port))] try: - proc = subprocess.Popen( - [constants.NETSTAT, "-nta", "--program"], - stdout=subprocess.PIPE, stderr=subprocess.PIPE) - stdout, _ = proc.communicate() - if proc.wait() != 0: - raise OSError("netstat subprocess failed") - lines = [x.split() for x in stdout.split("\n")[2:] if x] - listeners = [L[6] for L in lines if - # IPv4 socket case - (L[0] == 'tcp' and L[5] == 'LISTEN' \ - and L[3] == '0.0.0.0:{0}'.format(port)) or \ - # IPv6 socket case - (L[0] == 'tcp6' and L[5] == 'LISTEN' \ - and L[3] == ':::{0}'.format(port))] - if listeners: - pid, name = listeners[0].split("/") + if listeners and listeners[0]: + # conn.pid may be None if the current process doesn't have + # permission to identify the listening process! + pid = listeners[0] + name = psutil.Process(pid).name() display = zope.component.getUtility(interfaces.IDisplay) display.generic_notification( "The program {0} (process ID {1}) is already listening " @@ -297,11 +290,9 @@ class StandaloneAuthenticator(object): "that port. Please stop the {0} program temporarily " "and then try again.".format(name, pid, port)) return True - except (OSError, ValueError, IndexError): - # A sign that this command isn't available or usable this - # way on this operating system, or there was something - # unexpected about the format of the netstat output; we will - # not be able to recover from this condition. + except psutil.NoSuchProcess: + # Perhaps the result of a race where the process could have + # exited or relinquished the port. pass return False diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index 60a1ba600..abe3ff42d 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -187,71 +187,102 @@ class AlreadyListeningTest(unittest.TestCase): StandaloneAuthenticator self.authenticator = StandaloneAuthenticator() - @mock.patch("letsencrypt.client.standalone_authenticator.subprocess.Popen") - def test_subprocess_fails(self, mock_popen): - subprocess_object = mock.MagicMock() - subprocess_object.communicate.return_value = ("foo", "bar") - subprocess_object.wait.return_value = 1 - mock_popen.return_value = subprocess_object - result = self.authenticator.already_listening(17) - self.assertFalse(result) - subprocess_object.wait.assert_called_once_with() - - @mock.patch("letsencrypt.client.standalone_authenticator.subprocess.Popen") - def test_no_relevant_line(self, mock_popen): - # pylint: disable=line-too-long,trailing-whitespace - subprocess_object = mock.MagicMock() - subprocess_object.communicate.return_value = ( - """Active Internet connections (servers and established) -Proto Recv-Q Send-Q Local Address Foreign Address State PID/Program name -tcp 0 0 127.0.1.1:53 0.0.0.0:* LISTEN 1234/foo -tcp 0 0 127.0.0.1:631 0.0.0.0:* LISTEN 2345/bar -tcp 0 0 0.0.0.0:180 0.0.0.0:* LISTEN 11111/hello """, - "I am the standard error") - subprocess_object.wait.return_value = 0 - mock_popen.return_value = subprocess_object - result = self.authenticator.already_listening(17) - self.assertFalse(result) - - @mock.patch("letsencrypt.client.standalone_authenticator.subprocess.Popen") + @mock.patch("letsencrypt.client.standalone_authenticator.psutil." + "net_connections") + @mock.patch("letsencrypt.client.standalone_authenticator.psutil.Process") @mock.patch("letsencrypt.client.standalone_authenticator." "zope.component.getUtility") - def test_has_relevant_line(self, mock_get_utility, mock_popen): - # pylint: disable=line-too-long,trailing-whitespace - subprocess_object = mock.MagicMock() - subprocess_object.communicate.return_value = ( - """Active Internet connections (servers and established) -Proto Recv-Q Send-Q Local Address Foreign Address State PID/Program name -tcp 0 0 127.0.1.1:53 0.0.0.0:* LISTEN 1234/foo -tcp 0 0 127.0.0.1:631 0.0.0.0:* LISTEN 2345/bar -tcp 0 0 0.0.0.0:17 0.0.0.0:* LISTEN 11111/hello -tcp 0 0 0.0.0.0:1728 0.0.0.0:* LISTEN 2345/bar """, - "I am the standard error") - subprocess_object.wait.return_value = 0 - mock_popen.return_value = subprocess_object + def test_race_condition(self, mock_get_utility, mock_process, mock_net): + # This tests a race condition, or permission problem, or OS + # incompatibility in which, for some reason, no process name can be + # found to match the identified listening PID. + from psutil._common import sconn + from psutil import NoSuchProcess + conns = [ + sconn(fd=-1, family=2, type=1, laddr=('0.0.0.0', 30), + raddr=(), status='LISTEN', pid=None), + sconn(fd=3, family=2, type=1, laddr=('192.168.5.10', 32783), + raddr=('20.40.60.80', 22), status='ESTABLISHED', pid=1234), + sconn(fd=-1, family=10, type=1, laddr=('::1', 54321), + raddr=('::1', 111), status='CLOSE_WAIT', pid=None), + sconn(fd=3, family=2, type=1, laddr=('0.0.0.0', 17), + raddr=(), status='LISTEN', pid=4416)] + mock_net.return_value = conns + mock_process.side_effect = NoSuchProcess("No such PID") + # We simulate being unable to find the process name of PID 4416, + # which results in returning False. + self.assertFalse(self.authenticator.already_listening(17)) + self.assertEqual(mock_get_utility.generic_notification.call_count, 0) + mock_process.assert_called_once_with(4416) + + @mock.patch("letsencrypt.client.standalone_authenticator.psutil." + "net_connections") + @mock.patch("letsencrypt.client.standalone_authenticator.psutil.Process") + @mock.patch("letsencrypt.client.standalone_authenticator." + "zope.component.getUtility") + def test_not_listening(self, mock_get_utility, mock_process, mock_net): + from psutil._common import sconn + conns = [ + sconn(fd=-1, family=2, type=1, laddr=('0.0.0.0', 30), + raddr=(), status='LISTEN', pid=None), + sconn(fd=3, family=2, type=1, laddr=('192.168.5.10', 32783), + raddr=('20.40.60.80', 22), status='ESTABLISHED', pid=1234), + sconn(fd=-1, family=10, type=1, laddr=('::1', 54321), + raddr=('::1', 111), status='CLOSE_WAIT', pid=None)] + mock_net.return_value = conns + mock_process.name.return_value = "inetd" + self.assertFalse(self.authenticator.already_listening(17)) + self.assertEqual(mock_get_utility.generic_notification.call_count, 0) + self.assertEqual(mock_process.call_count, 0) + + @mock.patch("letsencrypt.client.standalone_authenticator.psutil." + "net_connections") + @mock.patch("letsencrypt.client.standalone_authenticator.psutil.Process") + @mock.patch("letsencrypt.client.standalone_authenticator." + "zope.component.getUtility") + def test_listening_ipv4(self, mock_get_utility, mock_process, mock_net): + from psutil._common import sconn + conns = [ + sconn(fd=-1, family=2, type=1, laddr=('0.0.0.0', 30), + raddr=(), status='LISTEN', pid=None), + sconn(fd=3, family=2, type=1, laddr=('192.168.5.10', 32783), + raddr=('20.40.60.80', 22), status='ESTABLISHED', pid=1234), + sconn(fd=-1, family=10, type=1, laddr=('::1', 54321), + raddr=('::1', 111), status='CLOSE_WAIT', pid=None), + sconn(fd=3, family=2, type=1, laddr=('0.0.0.0', 17), + raddr=(), status='LISTEN', pid=4416)] + mock_net.return_value = conns + mock_process.name.return_value = "inetd" result = self.authenticator.already_listening(17) self.assertTrue(result) self.assertEqual(mock_get_utility.call_count, 1) + mock_process.assert_called_once_with(4416) - @mock.patch("letsencrypt.client.standalone_authenticator.subprocess.Popen") + @mock.patch("letsencrypt.client.standalone_authenticator.psutil." + "net_connections") + @mock.patch("letsencrypt.client.standalone_authenticator.psutil.Process") @mock.patch("letsencrypt.client.standalone_authenticator." "zope.component.getUtility") - def test_has_relevant_ipv6_line(self, mock_get_utility, mock_popen): - # pylint: disable=line-too-long,trailing-whitespace - subprocess_object = mock.MagicMock() - subprocess_object.communicate.return_value = ( - """Active Internet connections (servers and established) -Proto Recv-Q Send-Q Local Address Foreign Address State PID/Program name -tcp 0 0 127.0.1.1:53 0.0.0.0:* LISTEN 1234/foo -tcp 0 0 127.0.0.1:631 0.0.0.0:* LISTEN 2345/bar -tcp6 0 0 :::17 :::* LISTEN 11111/hello -tcp 0 0 0.0.0.0:1728 0.0.0.0:* LISTEN 2345/bar """, - "I am the standard error") - subprocess_object.wait.return_value = 0 - mock_popen.return_value = subprocess_object - result = self.authenticator.already_listening(17) + def test_listening_ipv6(self, mock_get_utility, mock_process, mock_net): + from psutil._common import sconn + conns = [ + sconn(fd=-1, family=2, type=1, laddr=('0.0.0.0', 30), + raddr=(), status='LISTEN', pid=None), + sconn(fd=3, family=2, type=1, laddr=('192.168.5.10', 32783), + raddr=('20.40.60.80', 22), status='ESTABLISHED', pid=1234), + sconn(fd=-1, family=10, type=1, laddr=('::1', 54321), + raddr=('::1', 111), status='CLOSE_WAIT', pid=None), + sconn(fd=3, family=10, type=1, laddr=('::', 12345), raddr=(), + status='LISTEN', pid=4420), + sconn(fd=3, family=2, type=1, laddr=('0.0.0.0', 17), + raddr=(), status='LISTEN', pid=4416)] + mock_net.return_value = conns + mock_process.name.return_value = "inetd" + result = self.authenticator.already_listening(12345) self.assertTrue(result) self.assertEqual(mock_get_utility.call_count, 1) + mock_process.assert_called_once_with(4420) + class PerformTest(unittest.TestCase): """Tests for perform() method.""" diff --git a/setup.py b/setup.py index f2550446f..1136202ff 100755 --- a/setup.py +++ b/setup.py @@ -26,6 +26,7 @@ install_requires = [ 'ConfArgParse', 'jsonschema', 'mock', + 'psutil', 'pycrypto', 'PyOpenSSL', 'python-augeas', From daf7c81e5db6fd9bb89bd5dd02f11e041f1f9d5e Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 20 Feb 2015 13:22:20 -0800 Subject: [PATCH 2/3] Formatting and small fixes --- .../client/standalone_authenticator.py | 23 +++++++++++-------- .../tests/standalone_authenticator_test.py | 4 ++-- setup.py | 2 +- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index 4bd6d3379..ef926f0ea 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -272,15 +272,18 @@ class StandaloneAuthenticator(object): :param int port: The TCP port in question. :returns: True or False.""" - listeners = [conn.pid for conn in psutil.net_connections() \ - if conn.status == 'LISTEN' and \ - conn.type == socket.SOCK_STREAM and \ - (conn.laddr == ('0.0.0.0', port) or \ - conn.laddr == ('::', port))] + listeners = [conn.pid for conn in psutil.net_connections() + if conn.status == 'LISTEN' and + conn.type == socket.SOCK_STREAM and + (conn.laddr[1] == port)] try: - if listeners and listeners[0]: + if listeners and listeners[0] is not None: # conn.pid may be None if the current process doesn't have - # permission to identify the listening process! + # permission to identify the listening process! Additionally, + # listeners may have more than one element if separate + # sockets have bound the same port on separate interfaces. + # We currently only have UI to notify the user about one + # of them at a time. pid = listeners[0] name = psutil.Process(pid).name() display = zope.component.getUtility(interfaces.IDisplay) @@ -290,9 +293,11 @@ class StandaloneAuthenticator(object): "that port. Please stop the {0} program temporarily " "and then try again.".format(name, pid, port)) return True - except psutil.NoSuchProcess: + except (psutil.NoSuchProcess, psutil.AccessDenied): # Perhaps the result of a race where the process could have - # exited or relinquished the port. + # exited or relinquished the port (NoSuchProcess), or the result + # of an OS policy where we're not allowed to look up the process + # name (AccessDenied). pass return False diff --git a/letsencrypt/client/tests/standalone_authenticator_test.py b/letsencrypt/client/tests/standalone_authenticator_test.py index abe3ff42d..542915c7a 100644 --- a/letsencrypt/client/tests/standalone_authenticator_test.py +++ b/letsencrypt/client/tests/standalone_authenticator_test.py @@ -1,6 +1,7 @@ """Tests for letsencrypt.client.standalone_authenticator.""" import os import pkg_resources +import psutil import signal import socket import unittest @@ -197,7 +198,6 @@ class AlreadyListeningTest(unittest.TestCase): # incompatibility in which, for some reason, no process name can be # found to match the identified listening PID. from psutil._common import sconn - from psutil import NoSuchProcess conns = [ sconn(fd=-1, family=2, type=1, laddr=('0.0.0.0', 30), raddr=(), status='LISTEN', pid=None), @@ -208,7 +208,7 @@ class AlreadyListeningTest(unittest.TestCase): sconn(fd=3, family=2, type=1, laddr=('0.0.0.0', 17), raddr=(), status='LISTEN', pid=4416)] mock_net.return_value = conns - mock_process.side_effect = NoSuchProcess("No such PID") + mock_process.side_effect = psutil.NoSuchProcess("No such PID") # We simulate being unable to find the process name of PID 4416, # which results in returning False. self.assertFalse(self.authenticator.already_listening(17)) diff --git a/setup.py b/setup.py index 1136202ff..d8243af8a 100755 --- a/setup.py +++ b/setup.py @@ -26,7 +26,7 @@ install_requires = [ 'ConfArgParse', 'jsonschema', 'mock', - 'psutil', + 'psutil>2.1.0', # net_connections introduced in 2.1.0 'pycrypto', 'PyOpenSSL', 'python-augeas', From e17b9907f8b57bc83f36d4e33e257a1c880d2e85 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Fri, 20 Feb 2015 14:37:23 -0800 Subject: [PATCH 3/3] psutil>=2.1.0; remove redundant parentheses --- letsencrypt/client/standalone_authenticator.py | 2 +- setup.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt/client/standalone_authenticator.py b/letsencrypt/client/standalone_authenticator.py index ef926f0ea..f025cbe6d 100644 --- a/letsencrypt/client/standalone_authenticator.py +++ b/letsencrypt/client/standalone_authenticator.py @@ -275,7 +275,7 @@ class StandaloneAuthenticator(object): listeners = [conn.pid for conn in psutil.net_connections() if conn.status == 'LISTEN' and conn.type == socket.SOCK_STREAM and - (conn.laddr[1] == port)] + conn.laddr[1] == port] try: if listeners and listeners[0] is not None: # conn.pid may be None if the current process doesn't have diff --git a/setup.py b/setup.py index d8243af8a..236cb4ff9 100755 --- a/setup.py +++ b/setup.py @@ -26,7 +26,7 @@ install_requires = [ 'ConfArgParse', 'jsonschema', 'mock', - 'psutil>2.1.0', # net_connections introduced in 2.1.0 + 'psutil>=2.1.0', # net_connections introduced in 2.1.0 'pycrypto', 'PyOpenSSL', 'python-augeas',