URL: https://github.com/freeipa/freeipa/pull/818 Author: stlaz Title: #818: Avoid possible endless recursion in RPC call from client Action: synchronized
To pull the PR as Git branch: git remote add ghfreeipa https://github.com/freeipa/freeipa git fetch ghfreeipa pull/818/head:pr818 git checkout pr818
From 8b2824c01cf74e43a61ebe4d62332dba344c5dc8 Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka <slazn...@redhat.com> Date: Fri, 26 May 2017 08:37:36 +0200 Subject: [PATCH 1/3] Avoid possible endless recursion in RPC call This commit removes recursion in RPCClient.forward() which may lack end condition. https://pagure.io/freeipa/issue/6796 --- ipalib/rpc.py | 95 +++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 54 insertions(+), 41 deletions(-) diff --git a/ipalib/rpc.py b/ipalib/rpc.py index e23ca3d061..297ed80414 100644 --- a/ipalib/rpc.py +++ b/ipalib/rpc.py @@ -1088,50 +1088,63 @@ def forward(self, name, *args, **kw): :param kw: Keyword arguments to pass to remote command. """ server = getattr(context, 'request_url', None) - self.log.info("Forwarding '%s' to %s server '%s'", - name, self.protocol, server) command = getattr(self.conn, name) params = [args, kw] - try: - return self._call_command(command, params) - except Fault as e: - e = decode_fault(e) - self.debug('Caught fault %d from server %s: %s', e.faultCode, - server, e.faultString) - if e.faultCode in errors_by_code: - error = errors_by_code[e.faultCode] - raise error(message=e.faultString) - raise UnknownError( - code=e.faultCode, - error=e.faultString, - server=server, - ) - except SSLError as e: - raise NetworkError(uri=server, error=str(e)) - except ProtocolError as e: - # By catching a 401 here we can detect the case where we have - # a single IPA server and the session is invalid. Otherwise - # we always have to do a ping(). - session_cookie = getattr(context, 'session_cookie', None) - if session_cookie and e.errcode == 401: - # Unauthorized. Remove the session and try again. - delattr(context, 'session_cookie') - try: - principal = getattr(context, 'principal', None) - delete_persistent_client_session_data(principal) - except Exception as e: - # This shouldn't happen if we have a session but it isn't fatal. - pass - # Create a new serverproxy with the non-session URI - serverproxy = self.create_connection(os.environ.get('KRB5CCNAME'), self.env.verbose, self.env.fallback, self.env.delegate) - setattr(context, self.id, Connection(serverproxy, self.disconnect)) - return self.forward(name, *args, **kw) - raise NetworkError(uri=server, error=e.errmsg) - except socket.error as e: - raise NetworkError(uri=server, error=str(e)) - except (OverflowError, TypeError) as e: - raise XMLRPCMarshallError(error=str(e)) + # we'll be trying to connect multiple times with a new session cookie + # each time should we be getting UNAUTHORIZED error from the server + max_tries = 5 + for try_num in range(0, max_tries): + self.log.info("[try %d]: Forwarding '%s' to %s server '%s'", + try_num+1, name, self.protocol, server) + try: + return self._call_command(command, params) + except Fault as e: + e = decode_fault(e) + self.debug('Caught fault %d from server %s: %s', e.faultCode, + server, e.faultString) + if e.faultCode in errors_by_code: + error = errors_by_code[e.faultCode] + raise error(message=e.faultString) + raise UnknownError( + code=e.faultCode, + error=e.faultString, + server=server, + ) + except ProtocolError as e: + # By catching a 401 here we can detect the case where we have + # a single IPA server and the session is invalid. Otherwise + # we always have to do a ping(). + session_cookie = getattr(context, 'session_cookie', None) + if session_cookie and e.errcode == 401: + # Unauthorized. Remove the session and try again. + delattr(context, 'session_cookie') + try: + principal = getattr(context, 'principal', None) + delete_persistent_client_session_data(principal) + except Exception as e: + # This shouldn't happen if we have a session + # but it isn't fatal. + self.debug("Error trying to remove persisent session " + "data: {err}".format(err=e)) + + # Create a new serverproxy with the non-session URI + serverproxy = self.create_connection( + os.environ.get('KRB5CCNAME'), self.env.verbose, + self.env.fallback, self.env.delegate) + + setattr(context, self.id, + Connection(serverproxy, self.disconnect)) + # try to connect again with the new session cookie + continue + raise NetworkError(uri=server, error=e.errmsg) + except (SSLError, socket.error) as e: + raise NetworkError(uri=server, error=str(e)) + except (OverflowError, TypeError) as e: + raise XMLRPCMarshallError(error=str(e)) + raise NetworkError( + uri=server, + error=_("Exceeded number of tries to forward a request.")) class xmlclient(RPCClient): From 641cb67741e7e352c95ea29625c6b9adcc192b52 Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka <slazn...@redhat.com> Date: Wed, 31 May 2017 15:45:19 +0200 Subject: [PATCH 2/3] rpc: preparations for recursion fix Made several improvements to coding style: - same use of KerberosError throughout the module - removed some unused variables - moved code from try-except blocks if it didn't have to be there - preparations for putting most of RPCClient.create_connection() to loop https://pagure.io/freeipa/issue/6796 --- ipalib/rpc.py | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/ipalib/rpc.py b/ipalib/rpc.py index 297ed80414..b12ce4c536 100644 --- a/ipalib/rpc.py +++ b/ipalib/rpc.py @@ -52,7 +52,7 @@ from ipalib.backend import Connectible from ipalib.constants import LDAP_GENERALIZED_TIME_FORMAT from ipalib.errors import (public_errors, UnknownError, NetworkError, - KerberosError, XMLRPCMarshallError, JSONError) + XMLRPCMarshallError, JSONError) from ipalib import errors, capabilities from ipalib.request import context, Connection from ipapython.ipa_log_manager import root_logger @@ -653,7 +653,7 @@ def _auth_complete(self, response): except (TypeError, UnicodeError): pass if not token: - raise KerberosError( + raise errors.KerberosError( message=u"No valid Negotiate header in server response") token = self._sec_context.step(token=token) if self._sec_context.complete: @@ -979,8 +979,10 @@ def create_connection(self, ccache=None, verbose=None, fallback=None, delegate = self.api.env.delegate if ca_certfile is None: ca_certfile = self.api.env.tls_ca_cert + context.ca_certfile = ca_certfile + + rpc_uri = self.env[self.env_rpc_uri_key] try: - rpc_uri = self.env[self.env_rpc_uri_key] principal = get_principal(ccache_name=ccache) stored_principal = getattr(context, 'principal', None) if principal != stored_principal: @@ -996,12 +998,14 @@ def create_connection(self, ccache=None, verbose=None, fallback=None, except (errors.CCacheError, ValueError): # No session key, do full Kerberos auth pass - context.ca_certfile = ca_certfile urls = self.get_url_list(rpc_uri) serverproxy = None for url in urls: - kw = dict(allow_none=True, encoding='UTF-8') - kw['verbose'] = verbose + kw = { + 'allow_none': True, + 'encoding': 'UTF-8', + 'verbose': verbose + } if url.startswith('https://'): if delegate: transport_class = DelegatedKerbTransport @@ -1036,21 +1040,24 @@ def create_connection(self, ccache=None, verbose=None, fallback=None, ) # We don't care about the response, just that we got one break - except KerberosError as krberr: + except errors.KerberosError: # kerberos error on one server is likely on all - raise errors.KerberosError(message=unicode(krberr)) + raise except ProtocolError as e: if hasattr(context, 'session_cookie') and e.errcode == 401: # Unauthorized. Remove the session and try again. delattr(context, 'session_cookie') try: delete_persistent_client_session_data(principal) - except Exception as e: + except Exception: # This shouldn't happen if we have a session but it isn't fatal. pass - return self.create_connection(ccache, verbose, fallback, delegate) + return self.create_connection( + ccache, verbose, fallback, delegate) if not fallback: raise + else: + self.log.info('Connection to %s failed with %s', url, e) serverproxy = None except Exception as e: if not fallback: From 71450eb14f85b04581efa0670ceb481e20fa14e8 Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka <slazn...@redhat.com> Date: Thu, 1 Jun 2017 09:00:15 +0200 Subject: [PATCH 3/3] rpc: avoid possible recursion in create_connection There was a recursion in RPCClient.create_connection() which under rare circumstances would not have an ending condition. This commit removes it and cleans up the code a bit as well. https://pagure.io/freeipa/issue/6796 --- ipalib/rpc.py | 140 +++++++++++++++++++++++++++++++--------------------------- 1 file changed, 74 insertions(+), 66 deletions(-) diff --git a/ipalib/rpc.py b/ipalib/rpc.py index b12ce4c536..e3b8d67d69 100644 --- a/ipalib/rpc.py +++ b/ipalib/rpc.py @@ -999,77 +999,85 @@ def create_connection(self, ccache=None, verbose=None, fallback=None, # No session key, do full Kerberos auth pass urls = self.get_url_list(rpc_uri) - serverproxy = None + + proxy_kw = { + 'allow_none': True, + 'encoding': 'UTF-8', + 'verbose': verbose + } + for url in urls: - kw = { - 'allow_none': True, - 'encoding': 'UTF-8', - 'verbose': verbose - } - if url.startswith('https://'): - if delegate: - transport_class = DelegatedKerbTransport + # should we get ProtocolError (=> error in HTTP response) and + # 401 (=> Unauthorized), we'll be re-trying with new session + # cookies several times + for _try_num in range(0, 5): + if url.startswith('https://'): + if delegate: + transport_class = DelegatedKerbTransport + else: + transport_class = KerbTransport else: - transport_class = KerbTransport - else: - transport_class = LanguageAwareTransport - kw['transport'] = transport_class(protocol=self.protocol, - service='HTTP', ccache=ccache) - self.log.info('trying %s' % url) - setattr(context, 'request_url', url) - serverproxy = self.server_proxy_class(url, **kw) - if len(urls) == 1: - # if we have only 1 server and then let the - # main requester handle any errors. This also means it - # must handle a 401 but we save a ping. - return serverproxy - try: - command = getattr(serverproxy, 'ping') + transport_class = LanguageAwareTransport + proxy_kw['transport'] = transport_class( + protocol=self.protocol, service='HTTP', ccache=ccache) + self.log.info('trying %s' % url) + setattr(context, 'request_url', url) + serverproxy = self.server_proxy_class(url, **proxy_kw) + if len(urls) == 1: + # if we have only 1 server and then let the + # main requester handle any errors. This also means it + # must handle a 401 but we save a ping. + return serverproxy try: - command([], {}) - except Fault as e: - e = decode_fault(e) - if e.faultCode in errors_by_code: - error = errors_by_code[e.faultCode] - raise error(message=e.faultString) - else: - raise UnknownError( - code=e.faultCode, - error=e.faultString, - server=url, - ) - # We don't care about the response, just that we got one - break - except errors.KerberosError: - # kerberos error on one server is likely on all - raise - except ProtocolError as e: - if hasattr(context, 'session_cookie') and e.errcode == 401: - # Unauthorized. Remove the session and try again. - delattr(context, 'session_cookie') + command = getattr(serverproxy, 'ping') try: - delete_persistent_client_session_data(principal) - except Exception: - # This shouldn't happen if we have a session but it isn't fatal. - pass - return self.create_connection( - ccache, verbose, fallback, delegate) - if not fallback: + command([], {}) + except Fault as e: + e = decode_fault(e) + if e.faultCode in errors_by_code: + error = errors_by_code[e.faultCode] + raise error(message=e.faultString) + else: + raise UnknownError( + code=e.faultCode, + error=e.faultString, + server=url, + ) + # We don't care about the response, just that we got one + return serverproxy + except errors.KerberosError: + # kerberos error on one server is likely on all raise - else: - self.log.info('Connection to %s failed with %s', url, e) - serverproxy = None - except Exception as e: - if not fallback: - raise - else: - self.log.info('Connection to %s failed with %s', url, e) - serverproxy = None - - if serverproxy is None: - raise NetworkError(uri=_('any of the configured servers'), - error=', '.join(urls)) - return serverproxy + except ProtocolError as e: + if hasattr(context, 'session_cookie') and e.errcode == 401: + # Unauthorized. Remove the session and try again. + delattr(context, 'session_cookie') + try: + delete_persistent_client_session_data(principal) + except Exception: + # This shouldn't happen if we have a session but + # it isn't fatal. + pass + # try the same url once more with a new session cookie + continue + if not fallback: + raise + else: + self.log.info( + 'Connection to %s failed with %s', url, e) + # try the next url + break + except Exception as e: + if not fallback: + raise + else: + self.log.info( + 'Connection to %s failed with %s', url, e) + # try the next url + break + # finished all tries but no serverproxy was found + raise NetworkError(uri=_('any of the configured servers'), + error=', '.join(urls)) def destroy_connection(self): conn = getattr(context, self.id, None)
_______________________________________________ FreeIPA-devel mailing list -- freeipa-devel@lists.fedorahosted.org To unsubscribe send an email to freeipa-devel-le...@lists.fedorahosted.org
