From aa22e19a621e7e56cd73713040d1a32f4fd5d5d8 Mon Sep 17 00:00:00 2001 From: Michael Komitee Date: Mon, 1 Jul 2019 18:27:22 -0400 Subject: [PATCH 1/3] Revert "Revert "Resolve potential concurrency issue"" This reverts commit 4fb11604404aeef410c481b485aead655aafbd32. --- requests_kerberos/kerberos_.py | 44 +++++++++++++------------- tests/test_requests_kerberos.py | 56 ++++++++++++++++++--------------- 2 files changed, 52 insertions(+), 48 deletions(-) diff --git a/requests_kerberos/kerberos_.py b/requests_kerberos/kerberos_.py index 86e31c6..4610af7 100644 --- a/requests_kerberos/kerberos_.py +++ b/requests_kerberos/kerberos_.py @@ -179,14 +179,13 @@ def __init__( self.hostname_override = hostname_override self.sanitize_mutual_error_response = sanitize_mutual_error_response self.auth_done = False - self.winrm_encryption_available = hasattr(kerberos, 'authGSSWinRMEncryptMessage') # Set the CBT values populated after the first response self.send_cbt = send_cbt self.cbt_binding_tried = False self.cbt_struct = None - def generate_request_header(self, response, host, is_preemptive=False): + def generate_request_header(self, host, request=None, response=None, is_preemptive=False): """ Generates the GSSAPI authentication token with kerberos. @@ -195,6 +194,9 @@ def generate_request_header(self, response, host, is_preemptive=False): """ + if request is None: + request = response.request + # Flags used by kerberos module. gssflags = kerberos.GSS_C_MUTUAL_FLAG | kerberos.GSS_C_SEQUENCE_FLAG if self.delegate: @@ -209,9 +211,11 @@ def generate_request_header(self, response, host, is_preemptive=False): kerb_host = self.hostname_override if self.hostname_override is not None else host kerb_spn = "{0}@{1}".format(self.service, kerb_host) - result, self.context[host] = kerberos.authGSSClientInit(kerb_spn, + result, ctx = kerberos.authGSSClientInit(kerb_spn, gssflags=gssflags, principal=self.principal) + self.context[request] = ctx + if result < 1: raise EnvironmentError(result, kerb_stage) @@ -222,18 +226,18 @@ def generate_request_header(self, response, host, is_preemptive=False): kerb_stage = "authGSSClientStep()" # If this is set pass along the struct to Kerberos if self.cbt_struct: - result = kerberos.authGSSClientStep(self.context[host], + result = kerberos.authGSSClientStep(ctx, negotiate_resp_value, channel_bindings=self.cbt_struct) else: - result = kerberos.authGSSClientStep(self.context[host], + result = kerberos.authGSSClientStep(ctx, negotiate_resp_value) if result < 0: raise EnvironmentError(result, kerb_stage) kerb_stage = "authGSSClientResponse()" - gss_response = kerberos.authGSSClientResponse(self.context[host]) + gss_response = kerberos.authGSSClientResponse(ctx) return "Negotiate {0}".format(gss_response) @@ -258,7 +262,7 @@ def authenticate_user(self, response, **kwargs): host = urlparse(response.url).hostname try: - auth_header = self.generate_request_header(response, host) + auth_header = self.generate_request_header(host, response=response) except KerberosExchangeError: # GSS Failure, return existing response return response @@ -298,6 +302,9 @@ def handle_other(self, response): log.debug("handle_other(): Handling: %d" % response.status_code) + setattr(response, 'requests_kerberos_context', + self.context.get(response.request)) + if self.mutual_authentication in (REQUIRED, OPTIONAL) and not self.auth_done: is_http_error = response.status_code >= 400 @@ -348,16 +355,19 @@ def authenticate_server(self, response): log.debug("authenticate_server(): Authenticate header: {0}".format( _negotiate_value(response))) - host = urlparse(response.url).hostname + ctx = response.requests_kerberos_context + if ctx is None: + log.exception("authenticate_server(): no established context") + return False try: # If this is set pass along the struct to Kerberos if self.cbt_struct: - result = kerberos.authGSSClientStep(self.context[host], + result = kerberos.authGSSClientStep(ctx, _negotiate_value(response), channel_bindings=self.cbt_struct) else: - result = kerberos.authGSSClientStep(self.context[host], + result = kerberos.authGSSClientStep(ctx, _negotiate_value(response)) except kerberos.GSSError: log.exception("authenticate_server(): authGSSClientStep() failed:") @@ -416,25 +426,13 @@ def deregister(self, response): """Deregisters the response handler""" response.request.deregister_hook('response', self.handle_response) - def wrap_winrm(self, host, message): - if not self.winrm_encryption_available: - raise NotImplementedError("WinRM encryption is not available on the installed version of pykerberos") - - return kerberos.authGSSWinRMEncryptMessage(self.context[host], message) - - def unwrap_winrm(self, host, message, header): - if not self.winrm_encryption_available: - raise NotImplementedError("WinRM encryption is not available on the installed version of pykerberos") - - return kerberos.authGSSWinRMDecryptMessage(self.context[host], message, header) - def __call__(self, request): if self.force_preemptive and not self.auth_done: # add Authorization header before we receive a 401 # by the 401 handler host = urlparse(request.url).hostname - auth_header = self.generate_request_header(None, host, is_preemptive=True) + auth_header = self.generate_request_header(host, request=request, is_preemptive=True) log.debug("HTTPKerberosAuth: Preemptive Authorization header: {0}".format(auth_header)) diff --git a/tests/test_requests_kerberos.py b/tests/test_requests_kerberos.py index ebaca37..0f1c13b 100644 --- a/tests/test_requests_kerberos.py +++ b/tests/test_requests_kerberos.py @@ -115,7 +115,7 @@ def test_generate_request_header(self): host = urlparse(response.url).hostname auth = requests_kerberos.HTTPKerberosAuth() self.assertEqual( - auth.generate_request_header(response, host), + auth.generate_request_header(host, response=response), "Negotiate GSSRESPONSE" ) clientInit_complete.assert_called_with( @@ -138,7 +138,7 @@ def test_generate_request_header_init_error(self): host = urlparse(response.url).hostname auth = requests_kerberos.HTTPKerberosAuth() self.assertRaises(requests_kerberos.exceptions.KerberosExchangeError, - auth.generate_request_header, response, host + auth.generate_request_header, host, response=response, ) clientInit_error.assert_called_with( "HTTP@www.example.org", @@ -160,7 +160,7 @@ def test_generate_request_header_step_error(self): host = urlparse(response.url).hostname auth = requests_kerberos.HTTPKerberosAuth() self.assertRaises(requests_kerberos.exceptions.KerberosExchangeError, - auth.generate_request_header, response, host + auth.generate_request_header, host, response=response, ) clientInit_complete.assert_called_with( "HTTP@www.example.org", @@ -265,14 +265,13 @@ def test_authenticate_server(self): with patch.multiple(kerberos_module_name, authGSSClientStep=clientStep_complete): response_ok = requests.Response() - response_ok.url = "http://www.example.org/" + response_ok.requests_kerberos_context = "CTX" response_ok.status_code = 200 response_ok.headers = { 'www-authenticate': 'negotiate servertoken', 'authorization': 'Negotiate GSSRESPONSE'} auth = requests_kerberos.HTTPKerberosAuth() - auth.context = {"www.example.org": "CTX"} result = auth.authenticate_server(response_ok) self.assertTrue(result) @@ -281,15 +280,16 @@ def test_authenticate_server(self): def test_handle_other(self): with patch(kerberos_module_name+'.authGSSClientStep', clientStep_complete): + request = requests.PreparedRequest() response_ok = requests.Response() - response_ok.url = "http://www.example.org/" + response_ok.request = request response_ok.status_code = 200 response_ok.headers = { 'www-authenticate': 'negotiate servertoken', 'authorization': 'Negotiate GSSRESPONSE'} auth = requests_kerberos.HTTPKerberosAuth() - auth.context = {"www.example.org": "CTX"} + auth.context = {request: "CTX"} r = auth.handle_other(response_ok) @@ -299,15 +299,16 @@ def test_handle_other(self): def test_handle_response_200(self): with patch(kerberos_module_name+'.authGSSClientStep', clientStep_complete): + request = requests.PreparedRequest() response_ok = requests.Response() - response_ok.url = "http://www.example.org/" + response_ok.request = request response_ok.status_code = 200 response_ok.headers = { 'www-authenticate': 'negotiate servertoken', 'authorization': 'Negotiate GSSRESPONSE'} auth = requests_kerberos.HTTPKerberosAuth() - auth.context = {"www.example.org": "CTX"} + auth.context = {request: "CTX"} r = auth.handle_response(response_ok) @@ -317,13 +318,14 @@ def test_handle_response_200(self): def test_handle_response_200_mutual_auth_required_failure(self): with patch(kerberos_module_name+'.authGSSClientStep', clientStep_error): + request = requests.PreparedRequest() response_ok = requests.Response() - response_ok.url = "http://www.example.org/" + response_ok.request = request response_ok.status_code = 200 response_ok.headers = {} auth = requests_kerberos.HTTPKerberosAuth() - auth.context = {"www.example.org": "CTX"} + auth.context = {request: "CTX"} self.assertRaises(requests_kerberos.MutualAuthenticationError, auth.handle_response, @@ -334,15 +336,16 @@ def test_handle_response_200_mutual_auth_required_failure(self): def test_handle_response_200_mutual_auth_required_failure_2(self): with patch(kerberos_module_name+'.authGSSClientStep', clientStep_exception): + request = requests.PreparedRequest() response_ok = requests.Response() - response_ok.url = "http://www.example.org/" + response_ok.request = request response_ok.status_code = 200 response_ok.headers = { 'www-authenticate': 'negotiate servertoken', 'authorization': 'Negotiate GSSRESPONSE'} auth = requests_kerberos.HTTPKerberosAuth() - auth.context = {"www.example.org": "CTX"} + auth.context = {request: "CTX"} self.assertRaises(requests_kerberos.MutualAuthenticationError, auth.handle_response, @@ -353,8 +356,9 @@ def test_handle_response_200_mutual_auth_required_failure_2(self): def test_handle_response_200_mutual_auth_optional_hard_failure(self): with patch(kerberos_module_name+'.authGSSClientStep', clientStep_error): + request = requests.PreparedRequest() response_ok = requests.Response() - response_ok.url = "http://www.example.org/" + response_ok.request = request response_ok.status_code = 200 response_ok.headers = { 'www-authenticate': 'negotiate servertoken', @@ -362,7 +366,7 @@ def test_handle_response_200_mutual_auth_optional_hard_failure(self): auth = requests_kerberos.HTTPKerberosAuth( requests_kerberos.OPTIONAL) - auth.context = {"www.example.org": "CTX"} + auth.context = {request: "CTX"} self.assertRaises(requests_kerberos.MutualAuthenticationError, auth.handle_response, @@ -373,13 +377,14 @@ def test_handle_response_200_mutual_auth_optional_hard_failure(self): def test_handle_response_200_mutual_auth_optional_soft_failure(self): with patch(kerberos_module_name+'.authGSSClientStep', clientStep_error): + request = requests.PreparedRequest() response_ok = requests.Response() - response_ok.url = "http://www.example.org/" + response_ok.request = request response_ok.status_code = 200 auth = requests_kerberos.HTTPKerberosAuth( requests_kerberos.OPTIONAL) - auth.context = {"www.example.org": "CTX"} + auth.context = {request: "CTX"} r = auth.handle_response(response_ok) @@ -390,8 +395,9 @@ def test_handle_response_200_mutual_auth_optional_soft_failure(self): def test_handle_response_500_mutual_auth_required_failure(self): with patch(kerberos_module_name+'.authGSSClientStep', clientStep_error): + request = requests.PreparedRequest() response_500 = requests.Response() - response_500.url = "http://www.example.org/" + response_500.request = request response_500.status_code = 500 response_500.headers = {} response_500.request = "REQUEST" @@ -402,7 +408,7 @@ def test_handle_response_500_mutual_auth_required_failure(self): response_500.cookies = "COOKIES" auth = requests_kerberos.HTTPKerberosAuth() - auth.context = {"www.example.org": "CTX"} + auth.context = {request: "CTX"} r = auth.handle_response(response_500) @@ -422,7 +428,6 @@ def test_handle_response_500_mutual_auth_required_failure(self): # re-test with error response sanitizing disabled auth = requests_kerberos.HTTPKerberosAuth(sanitize_mutual_error_response=False) - auth.context = {"www.example.org": "CTX"} r = auth.handle_response(response_500) @@ -431,8 +436,9 @@ def test_handle_response_500_mutual_auth_required_failure(self): def test_handle_response_500_mutual_auth_optional_failure(self): with patch(kerberos_module_name+'.authGSSClientStep', clientStep_error): + request = requests.PreparedRequest() response_500 = requests.Response() - response_500.url = "http://www.example.org/" + response_500.request = request response_500.status_code = 500 response_500.headers = {} response_500.request = "REQUEST" @@ -444,7 +450,7 @@ def test_handle_response_500_mutual_auth_optional_failure(self): auth = requests_kerberos.HTTPKerberosAuth( requests_kerberos.OPTIONAL) - auth.context = {"www.example.org": "CTX"} + auth.context = {request: "CTX"} r = auth.handle_response(response_500) @@ -562,7 +568,7 @@ def test_generate_request_header_custom_service(self): response.headers = {'www-authenticate': 'negotiate token'} host = urlparse(response.url).hostname auth = requests_kerberos.HTTPKerberosAuth(service="barfoo") - auth.generate_request_header(response, host), + auth.generate_request_header(host, response=response), clientInit_complete.assert_called_with( "barfoo@www.example.org", gssflags=( @@ -627,7 +633,7 @@ def test_principal_override(self): response.headers = {'www-authenticate': 'negotiate token'} host = urlparse(response.url).hostname auth = requests_kerberos.HTTPKerberosAuth(principal="user@REALM") - auth.generate_request_header(response, host) + auth.generate_request_header(host, response=response) clientInit_complete.assert_called_with( "HTTP@www.example.org", gssflags=( @@ -645,7 +651,7 @@ def test_realm_override(self): response.headers = {'www-authenticate': 'negotiate token'} host = urlparse(response.url).hostname auth = requests_kerberos.HTTPKerberosAuth(hostname_override="otherhost.otherdomain.org") - auth.generate_request_header(response, host) + auth.generate_request_header(host, response=response) clientInit_complete.assert_called_with( "HTTP@otherhost.otherdomain.org", gssflags=( From 7f4327ba84f48816456e0ea391d9f1b637bad74e Mon Sep 17 00:00:00 2001 From: Michael Komitee Date: Mon, 1 Jul 2019 18:27:53 -0400 Subject: [PATCH 2/3] Avoid memory leak We should not store the request or the established kerberos context on the authentication handler forever. Once it's no longer necessary (we've authenticated the server and attached the context to the response we're returning to the user for them to do with as they wish), we should drop it so it can potentially be collected. --- requests_kerberos/kerberos_.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requests_kerberos/kerberos_.py b/requests_kerberos/kerberos_.py index 4610af7..c248ada 100644 --- a/requests_kerberos/kerberos_.py +++ b/requests_kerberos/kerberos_.py @@ -303,7 +303,7 @@ def handle_other(self, response): log.debug("handle_other(): Handling: %d" % response.status_code) setattr(response, 'requests_kerberos_context', - self.context.get(response.request)) + self.context.pop(response.request, None)) if self.mutual_authentication in (REQUIRED, OPTIONAL) and not self.auth_done: From 3e3da1c180c25bdee9606ba5a37c27703fb2e276 Mon Sep 17 00:00:00 2001 From: Michael Komitee Date: Mon, 1 Jul 2019 18:56:06 -0400 Subject: [PATCH 3/3] Drop support for winrm. - Indicate so in the HISTORY file. - Bump version (though we're on 0.x, so we're only bumping the minor version) - Change the argument order for generate_request_header to make the changes minimally invasive. --- HISTORY.rst | 8 ++++++++ requests_kerberos/__init__.py | 2 +- requests_kerberos/kerberos_.py | 25 ++++++++++++++++++++++--- tests/test_requests_kerberos.py | 12 ++++++------ 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index 44bfc4b..95931c4 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -1,6 +1,14 @@ History ======= +0.14.0.dev0: 2019-07-01 +----------------------- + +- Dropped winrm support. The kerberos context is now attached to response + objects so applications like winrm can be implemented external to + requests-kerberos. +- Corrected a concurrency issue exposed by threaded applications. + 0.12.0: 2017-12-20 ------------------------ diff --git a/requests_kerberos/__init__.py b/requests_kerberos/__init__.py index 63c7db5..0b3c757 100644 --- a/requests_kerberos/__init__.py +++ b/requests_kerberos/__init__.py @@ -22,4 +22,4 @@ __all__ = ('HTTPKerberosAuth', 'MutualAuthenticationError', 'REQUIRED', 'OPTIONAL', 'DISABLED') -__version__ = '0.13.0.dev0' +__version__ = '0.14.0.dev0' diff --git a/requests_kerberos/kerberos_.py b/requests_kerberos/kerberos_.py index c248ada..c1371d7 100644 --- a/requests_kerberos/kerberos_.py +++ b/requests_kerberos/kerberos_.py @@ -185,7 +185,7 @@ def __init__( self.cbt_binding_tried = False self.cbt_struct = None - def generate_request_header(self, host, request=None, response=None, is_preemptive=False): + def generate_request_header(self, response, host, request=None, is_preemptive=False): """ Generates the GSSAPI authentication token with kerberos. @@ -262,7 +262,7 @@ def authenticate_user(self, response, **kwargs): host = urlparse(response.url).hostname try: - auth_header = self.generate_request_header(host, response=response) + auth_header = self.generate_request_header(response, host) except KerberosExchangeError: # GSS Failure, return existing response return response @@ -426,13 +426,32 @@ def deregister(self, response): """Deregisters the response handler""" response.request.deregister_hook('response', self.handle_response) + def wrap_winrm(self, host, message): + raise NotImplementedError( + "WinRM encryption is no longer supported. The established " + "kerberos is now made available on the returned response objects " + "with the attribute named 'requests_kerberos_context' so WinRM " + "and other similar applications can be implemented external to " + "requests_kerberos." + ) + + def unwrap_winrm(self, host, message, header): + raise NotImplementedError( + "WinRM encryption is no longer supported. The established " + "kerberos is now made available on the returned response objects " + "with the attribute named 'requests_kerberos_context' so WinRM " + "and other similar applications can be implemented external to " + "requests_kerberos." + ) + + def __call__(self, request): if self.force_preemptive and not self.auth_done: # add Authorization header before we receive a 401 # by the 401 handler host = urlparse(request.url).hostname - auth_header = self.generate_request_header(host, request=request, is_preemptive=True) + auth_header = self.generate_request_header(None, host, request=request, is_preemptive=True) log.debug("HTTPKerberosAuth: Preemptive Authorization header: {0}".format(auth_header)) diff --git a/tests/test_requests_kerberos.py b/tests/test_requests_kerberos.py index 0f1c13b..69092c1 100644 --- a/tests/test_requests_kerberos.py +++ b/tests/test_requests_kerberos.py @@ -115,7 +115,7 @@ def test_generate_request_header(self): host = urlparse(response.url).hostname auth = requests_kerberos.HTTPKerberosAuth() self.assertEqual( - auth.generate_request_header(host, response=response), + auth.generate_request_header(response, host), "Negotiate GSSRESPONSE" ) clientInit_complete.assert_called_with( @@ -138,7 +138,7 @@ def test_generate_request_header_init_error(self): host = urlparse(response.url).hostname auth = requests_kerberos.HTTPKerberosAuth() self.assertRaises(requests_kerberos.exceptions.KerberosExchangeError, - auth.generate_request_header, host, response=response, + auth.generate_request_header, response, host ) clientInit_error.assert_called_with( "HTTP@www.example.org", @@ -160,7 +160,7 @@ def test_generate_request_header_step_error(self): host = urlparse(response.url).hostname auth = requests_kerberos.HTTPKerberosAuth() self.assertRaises(requests_kerberos.exceptions.KerberosExchangeError, - auth.generate_request_header, host, response=response, + auth.generate_request_header, response, host ) clientInit_complete.assert_called_with( "HTTP@www.example.org", @@ -568,7 +568,7 @@ def test_generate_request_header_custom_service(self): response.headers = {'www-authenticate': 'negotiate token'} host = urlparse(response.url).hostname auth = requests_kerberos.HTTPKerberosAuth(service="barfoo") - auth.generate_request_header(host, response=response), + auth.generate_request_header(response, host) clientInit_complete.assert_called_with( "barfoo@www.example.org", gssflags=( @@ -633,7 +633,7 @@ def test_principal_override(self): response.headers = {'www-authenticate': 'negotiate token'} host = urlparse(response.url).hostname auth = requests_kerberos.HTTPKerberosAuth(principal="user@REALM") - auth.generate_request_header(host, response=response) + auth.generate_request_header(response, host) clientInit_complete.assert_called_with( "HTTP@www.example.org", gssflags=( @@ -651,7 +651,7 @@ def test_realm_override(self): response.headers = {'www-authenticate': 'negotiate token'} host = urlparse(response.url).hostname auth = requests_kerberos.HTTPKerberosAuth(hostname_override="otherhost.otherdomain.org") - auth.generate_request_header(host, response=response) + auth.generate_request_header(response, host) clientInit_complete.assert_called_with( "HTTP@otherhost.otherdomain.org", gssflags=(