URL: https://github.com/freeipa/freeipa/pull/649 Author: simo5 Title: #649: Session cookie storage and handling fixes Action: synchronized
To pull the PR as Git branch: git remote add ghfreeipa https://github.com/freeipa/freeipa git fetch ghfreeipa pull/649/head:pr649 git checkout pr649
From 9fd0b4ce68daac2edbc38ccc743d4b7c1fafdf9d Mon Sep 17 00:00:00 2001 From: Simo Sorce <s...@redhat.com> Date: Wed, 22 Mar 2017 18:25:38 -0400 Subject: [PATCH 1/4] Avoid growing FILE ccaches unnecessarily Related https://pagure.io/freeipa/issue/6775 Signed-off-by: Simo Sorce <s...@redhat.com> --- ipapython/session_storage.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ipapython/session_storage.py b/ipapython/session_storage.py index bcf0947..f208827 100644 --- a/ipapython/session_storage.py +++ b/ipapython/session_storage.py @@ -111,6 +111,12 @@ def store_data(princ_name, key, value): if not isinstance(value, bytes): value = value.encode('utf-8') + # FILE ccaches grow every time an entry is stored, so we need + # to avoid storing the same entry multiple times. + oldvalue = get_data(princ_name, key) + if oldvalue == value: + return + context = krb5_context() principal = krb5_principal() ccache = krb5_ccache() From 7653192d67de8d6b19259ece49f6c1d31f788665 Mon Sep 17 00:00:00 2001 From: Simo Sorce <s...@redhat.com> Date: Wed, 22 Mar 2017 18:38:22 -0400 Subject: [PATCH 2/4] Handle failed authentication via cookie If cookie authentication fails and we get back a 401 see if we tried a SPNEGO auth by checking if we had a GSSAPI context. If not it means our session cookie was invalid or expired or some other error happened on the server that requires us to try a full SPNEGO handshake, so go ahead and try it. Fixes https://pagure.io/freeipa/issue/6775 Signed-off-by: Simo Sorce <s...@redhat.com> --- ipalib/rpc.py | 52 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/ipalib/rpc.py b/ipalib/rpc.py index 303b22a..f597ce0 100644 --- a/ipalib/rpc.py +++ b/ipalib/rpc.py @@ -586,22 +586,33 @@ def _handle_exception(self, e, service=None): else: raise errors.KerberosError(message=unicode(e)) - def get_host_info(self, host): + def _get_host(self): + return self._connection[0] + + def _remove_extra_header(self, name): + for (h, v) in self._extra_headers: + if h == name: + self._extra_headers.remove((h, v)) + break + + def get_auth_info(self, use_cookie=True): """ Two things can happen here. If we have a session we will add a cookie for that. If not we will set an Authorization header. """ - (host, extra_headers, x509) = SSLTransport.get_host_info(self, host) - - if not isinstance(extra_headers, list): - extra_headers = [] + if not isinstance(self._extra_headers, list): + self._extra_headers = [] - session_cookie = getattr(context, 'session_cookie', None) - if session_cookie: - extra_headers.append(('Cookie', session_cookie)) - return (host, extra_headers, x509) + # Remove any existing Cookie first + self._remove_extra_header('Cookie') + if use_cookie: + session_cookie = getattr(context, 'session_cookie', None) + if session_cookie: + self._extra_headers.append(('Cookie', session_cookie)) + return # Set the remote host principal + host = self._get_host() service = self.service + "@" + host.split(':')[0] try: @@ -616,18 +627,14 @@ def get_host_info(self, host): except gssapi.exceptions.GSSError as e: self._handle_exception(e, service=service) - self._set_auth_header(extra_headers, response) - - return (host, extra_headers, x509) + self._set_auth_header(response) - def _set_auth_header(self, extra_headers, token): - for (h, v) in extra_headers: - if h == 'Authorization': - extra_headers.remove((h, v)) - break + def _set_auth_header(self, token): + # Remove any existing authorization header first + self._remove_extra_header('Authorization') if token: - extra_headers.append( + self._extra_headers.append( ('Authorization', 'negotiate %s' % base64.b64encode(token).decode('ascii')) ) @@ -651,18 +658,23 @@ def _auth_complete(self, response): if self._sec_context.complete: self._sec_context = None return True - self._set_auth_header(self._extra_headers, token) + self._set_auth_header(token) + return False + elif response.status == 401: + self.get_auth_info(use_cookie=False) return False return True def single_request(self, host, handler, request_body, verbose=0): # Based on Python 2.7's xmllib.Transport.single_request try: - h = SSLTransport.make_connection(self, host) + h = self.make_connection(host) if verbose: h.set_debuglevel(1) + self.get_auth_info() + while True: if six.PY2: # pylint: disable=no-value-for-parameter From ee987faf2e744c77ee77d1cfe6357751ab166b3b Mon Sep 17 00:00:00 2001 From: Simo Sorce <s...@redhat.com> Date: Thu, 23 Mar 2017 13:02:00 -0400 Subject: [PATCH 3/4] Work around issues fetching session data Unfortunately the MIT krb5 library has a severe limitation with FILE ccaches when retrieving config data. It will always only search until the first entry is found and return that one. For FILE caches MIT krb5 does not support removing old entries when a new one is stored, and storage happens only in append mode, so the end result is that even if an update is stored it is never returned with the standard krb5_cc_get_config() call. To work around this issue we simply implement what krb5_cc_get_config() does under the hood with the difference that we do not stop at the first match but keep going until all ccache entries have been checked. Related https://pagure.io/freeipa/issue/6775 Signed-off-by: Simo Sorce <s...@redhat.com> --- ipapython/session_storage.py | 216 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 192 insertions(+), 24 deletions(-) diff --git a/ipapython/session_storage.py b/ipapython/session_storage.py index f208827..874b850 100644 --- a/ipapython/session_storage.py +++ b/ipapython/session_storage.py @@ -13,6 +13,12 @@ except OSError as e: # pragma: no cover raise ImportError(str(e)) +krb5_int32 = ctypes.c_int32 +krb5_error_code = krb5_int32 +krb5_magic = krb5_error_code +krb5_enctype = krb5_int32 +krb5_octet = ctypes.c_uint8 +krb5_timestamp = krb5_int32 class _krb5_context(ctypes.Structure): # noqa """krb5/krb5.h struct _krb5_context""" @@ -27,7 +33,7 @@ class _krb5_ccache(ctypes.Structure): # noqa class _krb5_data(ctypes.Structure): # noqa """krb5/krb5.h struct _krb5_data""" _fields_ = [ - ("magic", ctypes.c_int32), + ("magic", krb5_magic), ("length", ctypes.c_uint), ("data", ctypes.c_char_p), ] @@ -38,6 +44,63 @@ class krb5_principal_data(ctypes.Structure): # noqa _fields_ = [] +class _krb5_keyblock(ctypes.Structure): # noqa + """krb5/krb5.h struct _krb5_keyblock""" + _fields_ = [ + ("magic", krb5_magic), + ("enctype", krb5_enctype), + ("length", ctypes.c_uint), + ("contents", ctypes.POINTER(krb5_octet)) + ] + + +class _krb5_ticket_times(ctypes.Structure): # noqa + """krb5/krb5.h struct _krb5_ticket_times""" + _fields_ = [ + ("authtime", krb5_timestamp), + ("starttime", krb5_timestamp), + ("endtime", krb5_timestamp), + ("renew_till", krb5_timestamp), + ] + + +class _krb5_address(ctypes.Structure): # noqa + """krb5/krb5.h struct _krb5_address""" + _fields_ = [] + + +class _krb5_authdata(ctypes.Structure): # noqa + """krb5/krb5.h struct _krb5_authdata""" + _fields_ = [] + + +krb5_principal = ctypes.POINTER(krb5_principal_data) +krb5_keyblock = _krb5_keyblock +krb5_ticket_times = _krb5_ticket_times +krb5_boolean = ctypes.c_uint +krb5_flags = krb5_int32 +krb5_data = _krb5_data +krb5_address_p = ctypes.POINTER(_krb5_address) +krb5_authdata_p = ctypes.POINTER(_krb5_authdata) + + +class _krb5_creds(ctypes.Structure): # noqa + """krb5/krb5.h struct _krb5_creds""" + _fields_ = [ + ("magic", krb5_magic), + ("client", krb5_principal), + ("server", krb5_principal), + ("keyblock", krb5_keyblock), + ("times", krb5_ticket_times), + ("is_skey", krb5_boolean), + ("ticket_flags", krb5_flags), + ("addresses", ctypes.POINTER(krb5_address_p)), + ("ticket", krb5_data), + ("second_ticket", krb5_data), + ("authdata", ctypes.POINTER(krb5_authdata_p)) + ] + + class KRB5Error(Exception): pass @@ -48,11 +111,11 @@ def krb5_errcheck(result, func, arguments): raise KRB5Error(result, func.__name__, arguments) -krb5_principal = ctypes.POINTER(krb5_principal_data) krb5_context = ctypes.POINTER(_krb5_context) krb5_ccache = ctypes.POINTER(_krb5_ccache) krb5_data_p = ctypes.POINTER(_krb5_data) -krb5_error = ctypes.c_int32 +krb5_creds = _krb5_creds +krb5_error = krb5_int32 krb5_init_context = LIBKRB5.krb5_init_context krb5_init_context.argtypes = (ctypes.POINTER(krb5_context), ) @@ -61,15 +124,15 @@ def krb5_errcheck(result, func, arguments): krb5_free_context = LIBKRB5.krb5_free_context krb5_free_context.argtypes = (krb5_context, ) -krb5_free_context.retval = None +krb5_free_context.restype = None krb5_free_principal = LIBKRB5.krb5_free_principal krb5_free_principal.argtypes = (krb5_context, krb5_principal) -krb5_free_principal.retval = None +krb5_free_principal.restype = None krb5_free_data_contents = LIBKRB5.krb5_free_data_contents krb5_free_data_contents.argtypes = (krb5_context, krb5_data_p) -krb5_free_data_contents.retval = None +krb5_free_data_contents.restype = None krb5_cc_default = LIBKRB5.krb5_cc_default krb5_cc_default.argtypes = (krb5_context, ctypes.POINTER(krb5_ccache), ) @@ -78,26 +141,82 @@ def krb5_errcheck(result, func, arguments): krb5_cc_close = LIBKRB5.krb5_cc_close krb5_cc_close.argtypes = (krb5_context, krb5_ccache, ) -krb5_cc_close.retval = krb5_error +krb5_cc_close.restype = krb5_error krb5_cc_close.errcheck = krb5_errcheck krb5_parse_name = LIBKRB5.krb5_parse_name krb5_parse_name.argtypes = (krb5_context, ctypes.c_char_p, ctypes.POINTER(krb5_principal), ) -krb5_parse_name.retval = krb5_error +krb5_parse_name.restype = krb5_error krb5_parse_name.errcheck = krb5_errcheck krb5_cc_set_config = LIBKRB5.krb5_cc_set_config krb5_cc_set_config.argtypes = (krb5_context, krb5_ccache, krb5_principal, ctypes.c_char_p, krb5_data_p, ) -krb5_cc_set_config.retval = krb5_error +krb5_cc_set_config.restype = krb5_error krb5_cc_set_config.errcheck = krb5_errcheck -krb5_cc_get_config = LIBKRB5.krb5_cc_get_config -krb5_cc_get_config.argtypes = (krb5_context, krb5_ccache, krb5_principal, - ctypes.c_char_p, krb5_data_p, ) -krb5_cc_get_config.retval = krb5_error -krb5_cc_get_config.errcheck = krb5_errcheck +krb5_cc_get_principal = LIBKRB5.krb5_cc_get_principal +krb5_cc_get_principal.argtypes = (krb5_context, krb5_ccache, + ctypes.POINTER(krb5_principal), ) +krb5_cc_get_principal.restype = krb5_error +krb5_cc_get_principal.errcheck = krb5_errcheck + +# krb5_build_principal is a variadic function but that can't be expressed +# in a ctypes argtypes definition, so I explicitly listed the number of +# arguments we actually use through the code for type checking purposes +krb5_build_principal = LIBKRB5.krb5_build_principal +krb5_build_principal.argtypes = (krb5_context, ctypes.POINTER(krb5_principal), + ctypes.c_uint, ctypes.c_char_p, + ctypes.c_char_p, ctypes.c_char_p, + ctypes.c_char_p, ctypes.c_char_p, ) +krb5_build_principal.restype = krb5_error +krb5_build_principal.errcheck = krb5_errcheck + +krb5_pointer = ctypes.c_void_p +krb5_cc_cursor = krb5_pointer + +krb5_cc_start_seq_get = LIBKRB5.krb5_cc_start_seq_get +krb5_cc_start_seq_get.argtypes = (krb5_context, krb5_ccache, + ctypes.POINTER(krb5_cc_cursor), ) +krb5_cc_start_seq_get.restype = krb5_error +krb5_cc_start_seq_get.errcheck = krb5_errcheck + +krb5_cc_next_cred = LIBKRB5.krb5_cc_next_cred +krb5_cc_next_cred.argtypes = (krb5_context, krb5_ccache, + ctypes.POINTER(krb5_cc_cursor), + ctypes.POINTER(krb5_creds), ) +krb5_cc_next_cred.restype = krb5_error +krb5_cc_next_cred.errcheck = krb5_errcheck + +krb5_cc_end_seq_get = LIBKRB5.krb5_cc_end_seq_get +krb5_cc_end_seq_get.argtypes = (krb5_context, krb5_ccache, + ctypes.POINTER(krb5_cc_cursor), ) +krb5_cc_end_seq_get.restype = krb5_error +krb5_cc_end_seq_get.errcheck = krb5_errcheck + +krb5_free_cred_contents = LIBKRB5.krb5_free_cred_contents +krb5_free_cred_contents.argtypes = (krb5_context, ctypes.POINTER(krb5_creds)) +krb5_free_cred_contents.restype = krb5_error +krb5_free_cred_contents.errcheck = krb5_errcheck + +krb5_principal_compare = LIBKRB5.krb5_principal_compare +krb5_principal_compare.argtypes = (krb5_context, krb5_principal, + krb5_principal, ) +krb5_principal_compare.restype = krb5_boolean + +krb5_unparse_name = LIBKRB5.krb5_unparse_name +krb5_unparse_name.argtypes = (krb5_context, krb5_principal, + ctypes.POINTER(ctypes.c_char_p), ) +krb5_unparse_name.restype = krb5_error +krb5_unparse_name.errcheck = krb5_errcheck + +krb5_free_unparsed_name = LIBKRB5.krb5_free_unparsed_name +krb5_free_unparsed_name.argtypes = (krb5_context, ctypes.c_char_p, ) +krb5_free_unparsed_name.restype = None + +CONF_REALM = "X-CACHECONF:" +CONF_NAME = "krb5_ccache_conf_data" def store_data(princ_name, key, value): @@ -156,29 +275,78 @@ def get_data(princ_name, key): context = krb5_context() principal = krb5_principal() + srv_princ = krb5_principal() ccache = krb5_ccache() - data = _krb5_data() + pname_princ = krb5_principal() + pname = ctypes.c_char_p(0) try: krb5_init_context(ctypes.byref(context)) - krb5_parse_name(context, ctypes.c_char_p(princ_name), - ctypes.byref(principal)) - krb5_cc_default(context, ctypes.byref(ccache)) + krb5_cc_get_principal(context, ccache, ctypes.byref(principal)) - krb5_cc_get_config(context, ccache, principal, key, - ctypes.byref(data)) - - return data.data.decode('utf-8') + # We need to parse and then unparse the name in case the pric_name + # passed in comes w/o a realm attached + krb5_parse_name(context, ctypes.c_char_p(princ_name), + ctypes.byref(pname_princ)) + krb5_unparse_name(context, pname_princ, ctypes.byref(pname)) + + krb5_build_principal(context, ctypes.byref(srv_princ), + len(CONF_REALM), ctypes.c_char_p(CONF_REALM), + ctypes.c_char_p(CONF_NAME), ctypes.c_char_p(key), + pname, ctypes.c_char_p(None)) + + # Unfortunately we can't just use krb5_cc_get_config() + # because if bugs in some ccache handling code in krb5 + # libraries that would always return the first entry + # stored and not the last one, which is the one we want. + cursor = krb5_cc_cursor(0) + creds = krb5_creds() + got_creds = False + krb5_cc_start_seq_get(context, ccache, ctypes.byref(cursor)) + try: + while True: + checkcreds = krb5_creds() + # the next function will throw an error and break out of the + # while loop when we try to access past the last cred + krb5_cc_next_cred(context, ccache, ctypes.byref(cursor), + ctypes.byref(checkcreds)) + if (krb5_principal_compare(context, principal, + checkcreds.client) == 1 and + krb5_principal_compare(context, srv_princ, + checkcreds.server) == 1): + if got_creds: + krb5_free_cred_contents(context, ctypes.byref(creds)) + creds = checkcreds + got_creds = True + # We do not stop here, as we want the LAST entry + # in the ccache for those ccaches that cannot delete + # but only always append, like FILE + else: + krb5_free_cred_contents(context, + ctypes.byref(checkcreds)) + except KRB5Error: + pass + finally: + krb5_cc_end_seq_get(context, ccache, ctypes.byref(cursor)) + + if got_creds: + data = creds.ticket.data.decode('utf-8') + krb5_free_cred_contents(context, ctypes.byref(creds)) + return data finally: if principal: krb5_free_principal(context, principal) + if srv_princ: + krb5_free_principal(context, srv_princ) + if pname_princ: + krb5_free_principal(context, pname_princ) + if pname: + krb5_free_unparsed_name(context, pname) if ccache: krb5_cc_close(context, ccache) - if data: - krb5_free_data_contents(context, data) if context: krb5_free_context(context) From 75183b059dc33981d631c961c2a56f91648a395e Mon Sep 17 00:00:00 2001 From: Simo Sorce <s...@redhat.com> Date: Thu, 23 Mar 2017 17:49:27 -0400 Subject: [PATCH 4/4] Prevent churn on ccaches We slice down the received cookie so that just the content that matter is preserved. Thi is ok because servers can't trust anything else anyway and will accept a cookie with the ancillary data missing. By removing variable parts like the expiry component added by mod_session or the Expiration or Max-Age metadata we keep only the part of the cookie that changes only when a new session is generated. This way when storing the cookie we actually add a new entry in the ccache only when the session actually changes, and this prevents churn on FILE based ccaches. Related https://pagure.io/freeipa/issue/6775 Signed-off-by: Simo Sorce <s...@redhat.com> --- ipalib/rpc.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/ipalib/rpc.py b/ipalib/rpc.py index f597ce0..e23ca3d 100644 --- a/ipalib/rpc.py +++ b/ipalib/rpc.py @@ -38,6 +38,7 @@ import locale import base64 import json +import re import socket import gzip @@ -737,6 +738,20 @@ def __send_request(self, connection, host, handler, request_body, debug): self.send_content(connection, request_body) return connection + # Find all occurrences of the expiry component + expiry_re = re.compile(r'.*?(&expiry=\d+).*?') + + def _slice_session_cookie(self, session_cookie): + # Keep only the cookie value and strip away all other info. + # This is to reduce the churn on FILE ccaches which grow every time we + # set new data. The expiration time for the cookie is set in the + # encrypted data anyway and will be enforced by the server + http_cookie = session_cookie.http_cookie() + # We also remove the "expiry" part from the data which is not required + for exp in self.expiry_re.findall(http_cookie): + http_cookie = http_cookie.replace(exp, '') + return http_cookie + def store_session_cookie(self, cookie_header): ''' Given the contents of a Set-Cookie header scan the header and @@ -787,7 +802,7 @@ def store_session_cookie(self, cookie_header): if session_cookie is None: return - cookie_string = str(session_cookie) + cookie_string = self._slice_session_cookie(session_cookie) root_logger.debug("storing cookie '%s' for principal %s", cookie_string, principal) try: update_persistent_client_session_data(principal, cookie_string)
-- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
