Store session cookie in a ccache option #546

simo5 · 2017-03-07T13:45:07Z

Instead of using the kernel keyring, store the session cookie within the
ccache. This way kdestroy will really wipe away all crededntials.

Ticket: https://pagure.io/freeipa/issue/6661

MartinBasti · 2017-03-07T14:52:36Z

ipapython/ccache_storage.py

+    pass
+
+
+PY3 = sys.version_info[0] == 3


Please use six.PY3 instead to be consistent with other parts

MartinBasti · 2017-03-07T14:57:20Z

ipapython/ccache_storage.py

+            value = self.value
+            if value is None:
+                return None
+            elif not isinstance(value, str):


probably

elif isinstance(value, bytes):

might be better to handle Py2/Py3. Otherwise you must check Unicode too

MartinBasti · 2017-03-07T15:07:01Z

ipapython/ccache_storage.py

+
+    class _krb5_context(ctypes.Structure):  # noqa
+        """krb5/krb5.h struct _krb5_context"""
+        __slots__ = ()


Why __slots__ are defined? Is it performance improvement?

it's defined empty, but I'll remove them altogether

MartinBasti · 2017-03-07T15:15:21Z

ipapython/ccache_storage.py

+        def from_param(cls, value):
+            if value is None:
+                return None
+            if PY3 and isinstance(value, str):


Can be this replaced with:

if not isinstance(value, bytes): return value.encode('utf-8')

?

turns out I am not actually using c_text_p anywhere in the end, so I'm removing all of this

MartinBasti · 2017-03-07T15:18:34Z

ipapython/ccache_storage.py

+    krb5_cc_get_config.retval = krb5_error
+    krb5_cc_get_config.errcheck = krb5_errcheck
+
+class session_store:


Should be class session_store(object):

MartinBasti · 2017-03-07T15:24:59Z

ipapython/ccache_storage.py

+            krb5_free_context(self.__context)
+            self.__context = None
+
+    def __del__(self):


Don't define __del__ method in python unless you have bulletproof evidence that it will work in your case.
https://docs.python.org/2/reference/datamodel.html#object.__del__

In python, if you want safely do cleanup, it must be done by contextmanager (what you have already implemented), don't use del method

MartinBasti · 2017-03-07T15:27:20Z

ipapython/ccache_storage.py

+            krb5_cc_get_config(self.__context, ccache, principal,
+                               self._hidden_cred_name, ctypes.byref(data))
+
+            return str(data.data)


What is data.data type? If bytes this will return unexpected string in py3. .decode() should be used in case of bytes

it is decalred as c_char_p

MartinBasti · 2017-03-07T15:32:01Z

ipapython/ccache_storage.py

+try:
+    LIBKRB5 = ctypes.CDLL('libkrb5.so.3')
+except OSError as e:  # pragma: no cover
+    LIBKRB5 = e


Shouldn't we rather fail early and raise ImportError ?

I guess we could

MartinBasti · 2017-03-07T15:32:42Z

Pylint failed and I have a few inline comments

************* Module ipapython.ccache_storage
ipapython/ccache_storage.py:234: [C0305(trailing-newlines), ] Trailing newlines)
ipapython/ccache_storage.py:32: [W1612(unicode-builtin), c_text_p.from_param] unicode built-in referenced)
ipapython/ccache_storage.py:45: [E1101(no-member), c_text_p.text] Class 'value' has no 'decode' member)
ipapython/ccache_storage.py:128: [C1001(old-style-class), session_store] Old-style class defined.)
ipapython/ccache_storage.py:132: [E0710(raising-non-exception), session_store.__init__] Raising a new style class which doesn't inherit from BaseException)
ipapython/ccache_storage.py:6: [W0611(unused-import), ] Unused import os)

rcritten · 2017-03-07T15:52:51Z

Should this patch not also remove the keyring code?

Unit tests should be provided.

simo5 · 2017-03-07T16:08:54Z

@rcritten the keyring stuff is still used for detection of keyring in other places, so I did not touch it as those uses are still vaild

simo5 · 2017-03-07T16:09:41Z

Not sure how to provide unit tests, these functions work only if you have a valid ccache in the name of the principal you are trying to store a session cookie for.

simo5 · 2017-03-07T16:34:45Z

Ok removed a bunch of code and made sure pylint passes.

simo5 · 2017-03-07T16:35:07Z

I also renamed the module and the class, makes more sense to me this way around.

tiran · 2017-03-08T10:37:52Z

ipapython/session_storage.py

+
+class ccache_store(object):
+    def __init__(self, name='X-IPA-Session-Cookie'):
+        self.__context = None


Please use single underscore here. Double underscore lead to name mangling, which makes debugging harder.

tiran · 2017-03-08T10:38:21Z

ipapython/session_storage.py

+        self._hidden_cred_name = name
+
+    def __enter__(self):
+        return self


context should be initialized in __enter__, not in __init__

Actually ccache_store is not used together with with statement, what is your goal @simo5?

I just copied example code from krbproxy, I do not have any special goal.
I could even not carry a global context but crate and destroy contexts at each call
Whatever is preferred.

simo5 · 2017-03-09T12:34:42Z

Ok I decide to do away with the whole class stuff, given we never really keep a round the class object for more than one operation at a time in actual use.
As @rcritten requested I also added a test, and I am glad it was asked as I found a failure case we need to handle (see the exception handling in remove_data()

All requested changes have been applied

MartinBasti · 2017-03-09T12:53:40Z

ipatests/test_ipapython/test_session_storage.py

+Test the `session_storage.py` module.
+"""
+
+from nose.tools import raises  # pylint: disable=E0611


it is unused and instead of nose.raises please use pytest raises

MartinBasti · 2017-03-09T12:54:47Z

************* Module ipapython.session_storage
ipapython/session_storage.py:187: [W1624(indexing-exception), remove_data] Indexing exceptions will not work on Python 3)
************* Module ipalib.rpc
ipalib/rpc.py:114: [E1120(no-value-for-parameter), read_persistent_client_session_data] No value for argument 'value' in function call)
************* Module ipatests.test_ipapython.test_session_storage
ipatests/test_ipapython/test_session_storage.py:39: [W0612(unused-variable), test_session_storage.test_03] Unused variable 'e')
ipatests/test_ipapython/test_session_storage.py:9: [W0611(unused-import), ] Unused raises imported from nose.tools)
ipatests/test_ipapython/test_session_storage.py:12: [W0611(unused-import), ] Unused import pytest)

Instead of using the kernel keyring, store the session cookie within the ccache. This way kdestroy will really wipe away all credentials. Ticket: https://pagure.io/freeipa/issue/6661 Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 · 2017-03-09T13:52:48Z

Oops sorry, forgot to run make pylint on my last iteration, should be all fixed now

MartinBasti · 2017-03-09T15:46:34Z

ipatests/test_ipapython/test_session_storage.py

+        self.key = 'X-IPA-test-session-storage'
+        self.data = 'Test Data'
+
+    def test_01(self):


Tests has no description

but they are somehow selfexplanatory

MartinBasti · 2017-03-10T11:40:51Z

master:

7cab959 Store session cookie in a ccache option

MartinBasti previously requested changes Mar 7, 2017

View reviewed changes

simo5 force-pushed the ccachesess branch from 8aac1ae to 1a90c20 Compare March 7, 2017 16:34

simo5 force-pushed the ccachesess branch from 1a90c20 to 77ba575 Compare March 7, 2017 16:39

tiran reviewed Mar 8, 2017

View reviewed changes

simo5 force-pushed the ccachesess branch from 77ba575 to 136f5b4 Compare March 9, 2017 12:33

MartinBasti reviewed Mar 9, 2017

View reviewed changes

Store session cookie in a ccache option

c1ae93a

Instead of using the kernel keyring, store the session cookie within the ccache. This way kdestroy will really wipe away all credentials. Ticket: https://pagure.io/freeipa/issue/6661 Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 force-pushed the ccachesess branch from 136f5b4 to c1ae93a Compare March 9, 2017 13:52

MartinBasti reviewed Mar 9, 2017

View reviewed changes

MartinBasti added the ack Pull Request approved, can be merged label Mar 9, 2017

MartinBasti added the pushed Pull Request has already been pushed label Mar 10, 2017

MartinBasti closed this Mar 10, 2017

Store session cookie in a ccache option #546

Store session cookie in a ccache option #546

simo5 commented Mar 7, 2017 •

edited by MartinBasti

MartinBasti Mar 7, 2017

simo5 Mar 7, 2017

MartinBasti Mar 7, 2017

MartinBasti Mar 7, 2017

simo5 Mar 7, 2017

MartinBasti Mar 7, 2017

simo5 Mar 7, 2017

MartinBasti Mar 7, 2017

MartinBasti Mar 7, 2017

simo5 Mar 7, 2017

MartinBasti Mar 7, 2017

simo5 Mar 7, 2017

MartinBasti Mar 7, 2017

simo5 Mar 7, 2017

MartinBasti commented Mar 7, 2017

rcritten commented Mar 7, 2017

simo5 commented Mar 7, 2017

simo5 commented Mar 7, 2017

simo5 commented Mar 7, 2017

simo5 commented Mar 7, 2017

tiran Mar 8, 2017

tiran Mar 8, 2017

MartinBasti Mar 8, 2017

simo5 Mar 9, 2017

simo5 commented Mar 9, 2017

MartinBasti Mar 9, 2017

MartinBasti commented Mar 9, 2017

simo5 commented Mar 9, 2017

MartinBasti Mar 9, 2017

MartinBasti Mar 9, 2017

MartinBasti commented Mar 10, 2017

Store session cookie in a ccache option #546

Store session cookie in a ccache option #546

Conversation

simo5 commented Mar 7, 2017 • edited by MartinBasti

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinBasti commented Mar 7, 2017

rcritten commented Mar 7, 2017

simo5 commented Mar 7, 2017

simo5 commented Mar 7, 2017

simo5 commented Mar 7, 2017

simo5 commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simo5 commented Mar 9, 2017

Choose a reason for hiding this comment

MartinBasti commented Mar 9, 2017

simo5 commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinBasti commented Mar 10, 2017

simo5 commented Mar 7, 2017 •

edited by MartinBasti