ipalib/rpc.py: Fix session handling for KEYRING: ccaches #638
Conversation
MIT Kerberos allows to store configuration entries in the ccache. Unfortunately, there are big differences between ccache types in how these entries behave: - MIT Kerberos FILE: ccache code does always append entries, so we end up with ever growing ccache files. In KEYRING: case we are lucky that add_key syscall actually updates the key with the same name. - MIT Kerberos FILE: and KEYRING: ccache code does not allow to remove cred from ccache. Corresponding functions simply return KRB5_CC_NOSUPP; As result, using FILE: ccache type does not allow us to override our session cookie stored as a config entry in the ccache. Successive runs of ipa CLI create new entries in the ccache and only return the original one. Once we put a cookie in the FILE: ccache, it cannot be removed from there and cannot be replaced. Also, as retrieval code in krb5_cc_get_conf() ends up calling krb5_cc_retrieve_cred() with 0 flags and only has a cred principal name constructed out of a our conf key (X-IPA-Session_Cookie), none of the matching logic for "most recent ticket" could be applied. This commit attempts to improve situation for KEYRING: ccache type by setting the cookie to a predefined 'empty' value when deleting config entry. This avoids non-working 'remove cred' code path in ccache processing in MIT Kerberos. Additionally, when server side denies our cookie, it sends us empty Negotiate value. We errorneously treat it as invalid token. We also must use proper method to initialize our connection, SSLTransport.make_connection knows nothing about setting up GSSAPI client context, KrbTransport does. Unfortunately, with non-removable session cookie the code to initialize session context never triggered properly after expire. Fixes https://pagure.io/freeipa/issue/6775
|
Note: this is WIP, please test it against KEYRING: ccaches. |
| @@ -118,7 +119,10 @@ def read_persistent_client_session_data(principal): | |||
| ''' | |||
|
|
|||
| try: | |||
| return session_storage.get_data(principal, CCACHE_COOKIE_KEY) | |||
| value = session_storage.get_data(principal, CCACHE_COOKIE_KEY) | |||
| if value == CCACHE_COOKIE_EMPTY_VALUE: | |||
There was a problem hiding this comment.
Please move the three lines out of the try/except block. It doesn't make sense to catch and reraise the error.
Under Python 3, does get_data return string or bytes?
There was a problem hiding this comment.
This is exactly the same code pattern as it was before. It is there for a reason and I don't want to change it.
Python 3 should be returning bytes, so comparison needs to change, yes.
| @@ -658,7 +668,7 @@ def _auth_complete(self, response): | |||
| 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) | |||
There was a problem hiding this comment.
This looks unrelated to the issue.
There was a problem hiding this comment.
It is related. But I can move it into a separate commit.
| @@ -1021,7 +1031,7 @@ def create_connection(self, ccache=None, verbose=None, fallback=None, | |||
| except Exception as e: | |||
| # This shouldn't happen if we have a session but it isn't fatal. | |||
| pass | |||
| return self.create_connection(ccache, verbose, fallback, delegate) | |||
| return self.create_connection(ccache, verbose, fallback, delegate, ca_cerfile) | |||
There was a problem hiding this comment.
I can move it into a separate commit. This is a bug.
|
One way to deal with this in the FILE case is to copy the ccache to a tmp file and then rename to the original one. There is a risk of racing and removing a new ticket, but it is low. Luckily this problem should be solved once we have KCM caches ... |
|
Yes, KCM will work. However, I wonder if we could use a different approach by storing cookie in a fake ticket with a proper lifetime set to the cookie expiration. This would still get multiple entries added for FILE: case but at least will allow us to return most recent one. |
|
This PR has been obsoleted by #649 |
MIT Kerberos allows to store configuration entries in the ccache.
Unfortunately, there are big differences between ccache types in how
these entries behave:
MIT Kerberos FILE: ccache code does always append entries, so we end
up with ever growing ccache files. In KEYRING: case we are lucky that
add_key syscall actually updates the key with the same name.
MIT Kerberos FILE: and KEYRING: ccache code does not allow to remove
cred from ccache. Corresponding functions simply return
KRB5_CC_NOSUPP;
As result, using FILE: ccache type does not allow us to override our
session cookie stored as a config entry in the ccache. Successive runs
of ipa CLI create new entries in the ccache and only return the original
one.
Once we put a cookie in the FILE: ccache, it cannot be removed from
there and cannot be replaced. Also, as retrieval code in
krb5_cc_get_conf() ends up calling krb5_cc_retrieve_cred() with 0 flags
and only has a cred principal name constructed out of a our conf key
(X-IPA-Session_Cookie), none of the matching logic for "most recent
ticket" could be applied.
This commit attempts to improve situation for KEYRING: ccache type by
setting the cookie to a predefined 'empty' value when deleting config
entry. This avoids non-working 'remove cred' code path in ccache
processing in MIT Kerberos.
Additionally, when server side denies our cookie, it sends us empty
Negotiate value. We errorneously treat it as invalid token.
We also must use proper method to initialize our connection,
SSLTransport.make_connection knows nothing about setting up GSSAPI
client context, KrbTransport does. Unfortunately, with non-removable
session cookie the code to initialize session context never triggered
properly after expire.
Fixes https://pagure.io/freeipa/issue/6775