From 23c290fa9b791a8a7c6e2c353fb287b66ed16364 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Sep 29 2016 12:09:51 +0000 Subject: [PATCH 1/5] Merge remote-tracking branch 'xning/issue655' into issue32 --- diff --git a/koji/ssl/SSLConnection.py b/koji/ssl/SSLConnection.py index de77d0d..0cdb0ce 100644 --- a/koji/ssl/SSLConnection.py +++ b/koji/ssl/SSLConnection.py @@ -5,7 +5,7 @@ # Author: Mihai Ibanescu # Modifications by Dan Williams - +import errno from OpenSSL import SSL, crypto import os, string, time, socket, select @@ -50,7 +50,13 @@ class SSLConnection: and Connection.shutdown() doesn't take an argument. So we just discard the argument. """ - self.__dict__["conn"].shutdown() + try: + self.__dict__["conn"].shutdown() + except SSL.SysCallError, e: + if e.args[0] == errno.EPIPE: + pass + else: + raise SSL.SysCallError(str(e)) def accept(self): """ From eb9ebdeab9ec41850928486cb3bb323f5b0f1172 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Sep 29 2016 12:10:43 +0000 Subject: [PATCH 2/5] Don't try to close already closed connection --- diff --git a/koji/__init__.py b/koji/__init__.py index 1810042..0fb7ddb 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2052,8 +2052,13 @@ class ClientSession(object): def _close_connection(self): if self._connection: - self._connection[1].close() - self._connection = None + try: + self._connection[1].close() + except Exception, e: + if ssl.SSLCommon.is_already_closed_error(e): + pass + finally: + self._connection = None def _read_xmlrpc_response(self, response, handler=''): #XXX honor debug_xmlrpc diff --git a/koji/ssl/SSLCommon.py b/koji/ssl/SSLCommon.py index 345d4ea..e0f47fc 100644 --- a/koji/ssl/SSLCommon.py +++ b/koji/ssl/SSLCommon.py @@ -28,9 +28,21 @@ def our_verify(connection, x509, errNum, errDepth, preverifyOK): # correctly authenticates against the CA chain return preverifyOK +def _is_error_type(e, labels): + """Determine in an OpenSSL Exception is of specific type as defined by ssl_reason component. -def is_cert_error(e): - """Determine if an OpenSSL error is due to a bad cert""" + :param e: Exception tested + :type e: Exception + :param labels: Strings tests + :type labels: str | list[str] + :rtype: bool + """ + + if isinstance(labels, str): + labels = [labels] + + if not isinstance(labels, (list, tuple)) or not all([isinstance(s, str) for s in labels]): + raise ValueError("strs argument of _is_error_type needs to be string or list of strings") if not isinstance(e, SSL.Error): return False @@ -45,7 +57,7 @@ def is_cert_error(e): except TypeError: continue - # We do all this so that we can detect cert expiry + # We do all this so that we can detect cert expiry and closed socket, # so we can avoid retrying those over and over. for items in arg: try: @@ -58,14 +70,36 @@ def is_cert_error(e): _, _, ssl_reason = items - if ('certificate revoked' in ssl_reason or - 'certificate expired' in ssl_reason): - return True + if not isinstance(ssl_reason, str): + continue - #otherwise + # any of required strings could appear in ssl_reason + return any([s in ssl_reason for s in labels]) + + # otherwise return False +def is_cert_error(e): + """Determine if an OpenSSL exception is due to a bad cert + + :param e: Exception + :type e: Exception + :rtype: bool""" + + return _is_error_type(e, ['certificate revoked', 'certificate expired']) + + +def is_already_closed_error(e): + """Determine if an OpenSSL exception has happened when closing socket. + In such case it needn't to be cared about in most cases. + + :param e: Exception + :type e: Exception + :rtype: bool""" + + return _is_error_type(e, ['protocol is shutdown', 'shutdown while in init']) + def CreateSSLContext(certs): key_and_cert = certs['key_and_cert'] peer_ca_cert = certs['peer_ca_cert'] From 279c4586b85259829f8ce657f9c7bfb57c773197 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Sep 29 2016 12:12:42 +0000 Subject: [PATCH 3/5] Tests for ssl._is_error_type --- diff --git a/tests/test_ssl.py b/tests/test_ssl.py new file mode 100644 index 0000000..5c1f649 --- /dev/null +++ b/tests/test_ssl.py @@ -0,0 +1,55 @@ +from OpenSSL import SSL +from unittest import TestCase + +from koji.ssl.SSLCommon import _is_error_type, is_already_closed_error, is_cert_error + +class SSLTestCase(TestCase): + def test_is_error_type(self): + # invalid calls + self.assertRaises(ValueError, _is_error_type, Exception(), 123) + self.assertRaises(ValueError, _is_error_type, Exception(), None) + + # random exceptions + invalid_exceptions = ( + Exception(), + Exception([(1, 2, 3)]), + Exception([(1, 2)]), + Exception([(1, 2, "xxx")]), + RuntimeError(), + SSL.Error(123), + SSL.Error([(1, 2, 3)]), + SSL.Error([(1, 2)]), + SSL.Error([1, 2, 3]), + SSL.Error([(1, 2, 'x')]), + ) + for e in invalid_exceptions: + self.assertFalse(_is_error_type(e, 'xxx')) + self.assertFalse(is_cert_error(e)) + self.assertFalse(is_already_closed_error(e)) + + # valid exceptions + e = SSL.Error([(1, 2, 'xxx')]) + self.assertTrue(_is_error_type(e, 'xxx')) + + e = SSL.Error([(1, 2, 'certificate revoked')]) + self.assertTrue(is_cert_error(e)) + self.assertFalse(is_already_closed_error(e)) + + e = SSL.Error([(1, 2, 'certificate expired')]) + self.assertTrue(is_cert_error(e)) + self.assertFalse(is_already_closed_error(e)) + + e = SSL.Error([(1, 2, 'shutdown while in init')]) + self.assertFalse(is_cert_error(e)) + self.assertTrue(is_already_closed_error(e)) + + e = SSL.Error([(1, 2, 'protocol is shutdown')]) + self.assertFalse(is_cert_error(e)) + self.assertTrue(is_already_closed_error(e)) + + # is ok and should ever return False ([] != "") + self.assertFalse(_is_error_type(e, [])) + self.assertTrue(_is_error_type(e, "")) + self.assertTrue(_is_error_type(e, [""])) + + From 489f0378dd036fefffa4bd9bae9be37c917d900b Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Sep 29 2016 14:26:31 +0000 Subject: [PATCH 4/5] move catching ssl exception to SSLConnection class --- diff --git a/koji/__init__.py b/koji/__init__.py index 0fb7ddb..1810042 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2052,13 +2052,8 @@ class ClientSession(object): def _close_connection(self): if self._connection: - try: - self._connection[1].close() - except Exception, e: - if ssl.SSLCommon.is_already_closed_error(e): - pass - finally: - self._connection = None + self._connection[1].close() + self._connection = None def _read_xmlrpc_response(self, response, handler=''): #XXX honor debug_xmlrpc diff --git a/koji/ssl/SSLConnection.py b/koji/ssl/SSLConnection.py index 0cdb0ce..1c8255c 100644 --- a/koji/ssl/SSLConnection.py +++ b/koji/ssl/SSLConnection.py @@ -8,6 +8,7 @@ import errno from OpenSSL import SSL, crypto import os, string, time, socket, select +import SSLCommon class SSLConnection: @@ -86,8 +87,13 @@ class SSLConnection: self.__dict__["close_refcount"] = self.__dict__["close_refcount"] - 1 if self.__dict__["close_refcount"] == 0: self.shutdown() - self.__dict__["conn"].close() - self.__dict__["closed"] = True + try: + self.__dict__["conn"].close() + except Exception, e: + if SSLCommon.is_already_closed_error(e): + pass + finally: + self.__dict__["closed"] = True def sendall(self, data, flags=0): """ From 265f04f7fc78238cfb7f581aae58bada1c77af77 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Sep 29 2016 14:28:21 +0000 Subject: [PATCH 5/5] remove unused imports --- diff --git a/koji/ssl/SSLConnection.py b/koji/ssl/SSLConnection.py index 1c8255c..4e568fa 100644 --- a/koji/ssl/SSLConnection.py +++ b/koji/ssl/SSLConnection.py @@ -6,8 +6,8 @@ # Modifications by Dan Williams import errno -from OpenSSL import SSL, crypto -import os, string, time, socket, select +from OpenSSL import SSL +import time, socket, select import SSLCommon