Session cookie storage and handling fixes #649

simo5 · 2017-03-23T20:29:49Z

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

Related https://pagure.io/freeipa/issue/6775 Signed-off-by: Simo Sorce <simo@redhat.com>

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 <simo@redhat.com>

simo5 · 2017-03-23T20:30:20Z

Note I am still running tests, but I think the patchset is good for review already.

simo5 · 2017-03-23T21:03:12Z

The FILE ccache is still growing because we keep getting updated cookies (where the only thing that changes is the expiration date.

simo5 · 2017-03-23T22:35:34Z

I aded a 4th patch to address the FILE ccache growth issue.
It is a bit unorthodox but it works. Please review carefully and let me know if you are ok with this

abbra · 2017-03-24T07:40:34Z

I tested the whole patchset. It worked for me first time I've got cookie expired. However, it broke in ~10 minutes afterwards -- apparently, keyring ccache was empty, according to klist. After few more minutes I was able to list TGT from the same ccache and ipa CLI worked again.

I suspect we created something that MIT Kerberos library does not really understand.

[10609] 1490339971.189122: Storing config in KEYRING:persistent:0:krb_ccache_uA6VDOR for admin@XS.IPA.COOL: X-IPA-Session-Cookie: ipa_session=MagBearerToken=NtVuqNjq7jKtuDiw9lDSxHI%2frs5vd4UZ9o1sSZjDAemTImufljlG66i3l6MgA%2fmxtC0kPQgUqUEVcFJ04GWKOzK%2bYeTTEeAXrs59sNUq4VZzmRDTbLW%2by9ccodzlUdoeIiDVKdJsGHlBKyKTtcm1UW0a0LY%2bQLJscOQImQOlNpJ%2bxFs3szGU5w1rFbjQPwp6\x00
[10609] 1490339971.189156: Storing admin@XS.IPA.COOL -> krb5_ccache_conf_data/X-IPA-Session-Cookie/admin\@XS.IPA.COOL@X-CACHECONF: in KEYRING:persistent:0:krb_ccache_uA6VDOR

... some time later, in a different execution of ipa user-show ...

ipa: DEBUG: New HTTP connection (nyx.xs.ipa.cool)
ipa: DEBUG: HTTP connection destroyed (nyx.xs.ipa.cool)
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 676, in single_request
    self.get_auth_info()
  File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 628, in get_auth_info
    self._handle_exception(e, service=service)
  File "/usr/lib/python2.7/site-packages/ipalib/rpc.py", line 585, in _handle_exception
    raise errors.CCacheError()
CCacheError: did not receive Kerberos credentials
ipa: DEBUG: Destroyed connection context.rpcclient_140537682029648
ipa: ERROR: did not receive Kerberos credentials
[root@nyx ~]# klist
Ticket cache: KEYRING:persistent:0:krb_ccache_uA6VDOR
Default principal: admin@XS.IPA.COOL

Valid starting       Expires              Service principal
klist: No credentials cache found while retrieving a ticket

.... some time afterwards, without running kinit ....

[root@nyx ~]# klist
Ticket cache: KEYRING:persistent:0:krb_ccache_uA6VDOR
Default principal: admin@XS.IPA.COOL

Valid starting       Expires              Service principal
03/24/2017 08:07:02  03/25/2017 08:06:56  krbtgt/XS.IPA.COOL@XS.IPA.COOL

.... and running ipa user-show now succeeds in retrieving old cookie, invalidating it, negotiating a new one, and storing it ....

[10747] 1490340689.131026: Storing config in KEYRING:persistent:0:krb_ccache_uA6VDOR for admin@XS.IPA.COOL: X-IPA-Session-Cookie: ipa_session=MagBearerToken=J9aCtYUAsRFpJJhrMu4x4E2gwA2ojJOPdYT7iN7GtTyec7%2fj9lW1LyzgpLhjawaCa9MsK%2btOPDF6mKTsCSJqey3vhgY35ezg8Cwzbln6yGr0kPfDCWoxSQGYWx%2fSSIRVltu8akoXu1NvzP1%2bF0NEFrdzGi2%2bZDZXRFvUC5UpLg%2b3JMg5ZNExYlr%2bLHHQpAJh\x00
[10747] 1490340689.131071: Storing admin@XS.IPA.COOL -> krb5_ccache_conf_data/X-IPA-Session-Cookie/admin\@XS.IPA.COOL@X-CACHECONF: in KEYRING:persistent:0:krb_ccache_uA6VDOR

abbra · 2017-03-24T08:38:53Z

@simo5, I think I found why it happened -- I actually had krbMaxTicketLife set for HTTP/... principal to 300 seconds.

So I think your patches are good. I'd like you to fix fourth patch according to inline comments I left but that's it.

tiran · 2017-03-24T08:55:21Z

ipapython/session_storage.py

+class _krb5_keyblock(ctypes.Structure):  # noqa
+    """krb5/krb5.h struct _krb5_keyblock"""
+    _fields_ = [
+        ("magic", ctypes.c_int32),


You are resolving typedefs all the way down. When I write ctypes code, I usually try to stick to original typedefs as close as possible. Your approach is also fine, just a bit harder to review.

krb5_int32 = ctypes.c_int32 krb5_enctype = krb5_int32 krb5_kvno = ctypes.c_uint krb5_error_code = krb5_int32 krb5_magic = krb5_error_code ... ("magic", krb5_magic), ...

tiran · 2017-03-24T09:01:06Z

ipapython/session_storage.py

+krb5_cc_get_principal.errcheck = krb5_errcheck
+
+krb5_build_principal = LIBKRB5.krb5_build_principal
+krb5_build_principal.argtypes = (krb5_context, ctypes.POINTER(krb5_principal),


Please add a comment that this function is a variadic function and could take more arguments.

tiran · 2017-03-24T09:09:33Z

ipapython/session_storage.py

+                    krb5_free_cred_contents(context,
+                                            ctypes.byref(checkcreds))
+        except KRB5Error:
+            pass


At first I though that the loop could leak checkcred in case krb5_principal_compare fails. Then I noticed that the comparison function only returns true/false. Please add a comment.

tiran · 2017-03-24T09:11:06Z

ipapython/session_storage.py

+        try:
+            while True:
+                checkcreds = krb5_creds()
+                krb5_cc_next_cred(context, ccache, ctypes.byref(cursor),


The while True loop contains neither break nor return. Please add a comment that explains the loop is terminated when krb5_cc_next_cred() raises an exception when it ha reached the end of the list.

tiran · 2017-03-24T09:16:32Z

@simo5 I left some comments.

simo5 · 2017-03-24T12:53:04Z

Thank you @tiran @abbra all very good comments, I'll address soon all of them

simo5 · 2017-03-24T15:44:39Z

I should have addressed all comments.

I did not comment on krb5_principal_compare() because I think that is obvious and the function definition also does not define an errcheck argument for it so it should be clear enough.

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 <simo@redhat.com>

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 <simo@redhat.com>

abbra · 2017-03-27T12:57:08Z

LGTM to me. @simo5 explained that expiry=... substring is part of the actual cookie mod_session adds (it is timestamp in nanonseconds) -- Cookie class does not see it, so it has to be removed separately in the last commit.

tkrizek · 2017-03-28T11:37:11Z

master:

9a6ac74 Avoid growing FILE ccaches unnecessarily
fbbeb13 Handle failed authentication via cookie
e07aefb Work around issues fetching session data
d633266 Prevent churn on ccaches

tkrizek · 2017-03-28T11:37:46Z

@simo5 Please rebase for ipa-4-5.

simo5 · 2017-03-28T12:58:24Z

Should I make a new PR for 4.5 ?

MartinBasti · 2017-03-28T13:00:37Z

Yes please

simo5 added 2 commits March 23, 2017 16:11

Avoid growing FILE ccaches unnecessarily

9fd0b4c

Related https://pagure.io/freeipa/issue/6775 Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 mentioned this pull request Mar 23, 2017

ipalib/rpc.py: Fix session handling for KEYRING: ccaches #638

Closed

tiran reviewed Mar 24, 2017

View reviewed changes

simo5 force-pushed the clisesshandling branch from db0f583 to 75183b0 Compare March 24, 2017 15:42

simo5 added 2 commits March 24, 2017 11:50

simo5 force-pushed the clisesshandling branch from 75183b0 to f6817ca Compare March 24, 2017 15:50

abbra added the ack Pull Request approved, can be merged label Mar 27, 2017

tkrizek added the pushed Pull Request has already been pushed label Mar 28, 2017

tkrizek closed this Mar 28, 2017

Session cookie storage and handling fixes #649

Session cookie storage and handling fixes #649

simo5 commented Mar 23, 2017 •

edited by tkrizek

simo5 commented Mar 23, 2017

simo5 commented Mar 23, 2017

simo5 commented Mar 23, 2017

abbra commented Mar 24, 2017

abbra commented Mar 24, 2017

tiran Mar 24, 2017

tiran Mar 24, 2017

tiran Mar 24, 2017

tiran Mar 24, 2017

tiran commented Mar 24, 2017

simo5 commented Mar 24, 2017

simo5 commented Mar 24, 2017

abbra commented Mar 27, 2017

tkrizek commented Mar 28, 2017

tkrizek commented Mar 28, 2017

simo5 commented Mar 28, 2017

MartinBasti commented Mar 28, 2017

Session cookie storage and handling fixes #649

Session cookie storage and handling fixes #649

Conversation

simo5 commented Mar 23, 2017 • edited by tkrizek

simo5 commented Mar 23, 2017

simo5 commented Mar 23, 2017

simo5 commented Mar 23, 2017

abbra commented Mar 24, 2017

abbra commented Mar 24, 2017

tiran Mar 24, 2017

Choose a reason for hiding this comment

tiran Mar 24, 2017

Choose a reason for hiding this comment

tiran Mar 24, 2017

Choose a reason for hiding this comment

tiran Mar 24, 2017

Choose a reason for hiding this comment

tiran commented Mar 24, 2017

simo5 commented Mar 24, 2017

simo5 commented Mar 24, 2017

abbra commented Mar 27, 2017

tkrizek commented Mar 28, 2017

tkrizek commented Mar 28, 2017

simo5 commented Mar 28, 2017

MartinBasti commented Mar 28, 2017

simo5 commented Mar 23, 2017 •

edited by tkrizek