URL: https://github.com/freeipa/freeipa/pull/649
Author: simo5
 Title: #649: Session cookie storage and handling fixes
Action: opened

PR body:
"""
This patchset improves the behavior of the client in various ways.
- Avoids unbounded growth of FILE ccaches
- Fix regression with session cookies updates not being retrievable with FILE 
caches
- Fix client authentication to better handle servers that may decide our cookie 
is not good anymore
"""

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/3] 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/3] 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 3e5b74d38a4a42586898d3f711b430f48dc755b8 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/3] 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 | 177 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 156 insertions(+), 21 deletions(-)

diff --git a/ipapython/session_storage.py b/ipapython/session_storage.py
index f208827..f7201f3 100644
--- a/ipapython/session_storage.py
+++ b/ipapython/session_storage.py
@@ -38,6 +38,43 @@ class krb5_principal_data(ctypes.Structure):  # noqa
     _fields_ = []
 
 
+class _krb5_keyblock(ctypes.Structure):  # noqa
+    """krb5/krb5.h struct _krb5_keyblock"""
+    _fields_ = [
+        ("magic", ctypes.c_int32),
+        ("enctype", ctypes.c_int32),
+        ("length", ctypes.c_uint),
+        ("contents", ctypes.c_void_p)
+    ]
+
+
+class _krb5_ticket_times(ctypes.Structure):  # noqa
+    """krb5/krb5.h struct _krb5_ticket_times"""
+    _fields_ = [
+        ("authtime", ctypes.c_int32),
+        ("starttime", ctypes.c_int32),
+        ("endtime", ctypes.c_int32),
+        ("renew_till", ctypes.c_int32),
+    ]
+
+
+class _krb5_creds(ctypes.Structure):  # noqa
+    """krb5/krb5.h struct _krb5_creds"""
+    _fields_ = [
+        ("magic", ctypes.c_int32),
+        ("client", ctypes.POINTER(krb5_principal_data)),
+        ("server", ctypes.POINTER(krb5_principal_data)),
+        ("keyblock", _krb5_keyblock),
+        ("times", _krb5_ticket_times),
+        ("is_skey", ctypes.c_uint),
+        ("ticket_flags", ctypes.c_int32),
+        ("addresses", ctypes.c_void_p),
+        ("ticket", _krb5_data),
+        ("second_ticket", _krb5_data),
+        ("authdata", ctypes.c_void_p)
+    ]
+
+
 class KRB5Error(Exception):
     pass
 
@@ -52,6 +89,7 @@ def krb5_errcheck(result, func, arguments):
 krb5_context = ctypes.POINTER(_krb5_context)
 krb5_ccache = ctypes.POINTER(_krb5_ccache)
 krb5_data_p = ctypes.POINTER(_krb5_data)
+krb5_creds = _krb5_creds
 krb5_error = ctypes.c_int32
 
 krb5_init_context = LIBKRB5.krb5_init_context
@@ -61,15 +99,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 +116,76 @@ 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 = 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(ctypes.c_void_p), )
+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(ctypes.c_void_p),
+                              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(ctypes.c_void_p), )
+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 = ctypes.c_uint
+
+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 +244,76 @@ 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 = ctypes.c_void_p(0)
+        creds = krb5_creds()
+        got_creds = False
+        krb5_cc_start_seq_get(context, ccache, ctypes.byref(cursor))
+        try:
+            while True:
+                checkcreds = krb5_creds()
+                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)
 
-- 
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