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 ef5948b9bcb49335929f6a6fb62f19b611c2a57d 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 | 213 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 190 insertions(+), 23 deletions(-)

diff --git a/ipapython/session_storage.py b/ipapython/session_storage.py
index f208827..6af064c 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,13 @@ 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_pointer = ctypes.c_void_p
+krb5_cc_cursor = krb5_pointer
 
 krb5_init_context = LIBKRB5.krb5_init_context
 krb5_init_context.argtypes = (ctypes.POINTER(krb5_context), )
@@ -61,15 +126,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 +143,79 @@ 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_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 +274,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()
 
     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 of 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()
+        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 f6817cae5e7435ef25be13bddf24a3c33b5623b9 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

Reply via email to