[Freeipa-devel] [freeipa PR#649][opened] Session cookie storage and handling fixes

[simo5]

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

[simo5]

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

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

"""

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-288850417

[simo5]

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

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

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-288859035

[simo5]

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

[simo5]

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

simo5 commented:
"""
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
"""

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-288881336

[abbra]

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

abbra commented:
"""
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.

```text
[10609] 1490339971.189122: Storing config in KEYRING:persistent:0:krb_ccache_uA6VDOR for ***@XS.IPA.COOL: X-IPA-Session-Cookie: ipa_session=MagBearerToken=NtVuqNjq7jKtuDiw9lDSxHI%2frs5vd4UZ9o1sSZjDAemTImufljlG66i3l6MgA%2fmxtC0kPQgUqUEVcFJ04GWKOzK%2bYeTTEeAXrs59sNUq4VZzmRDTbLW%2by9ccodzlUdoeIiDVKdJsGHlBKyKTtcm1UW0a0LY%2bQLJscOQImQOlNpJ%2bxFs3szGU5w1rFbjQPwp6\x00
[10609] 1490339971.189156: Storing ***@XS.IPA.COOL -> krb5_ccache_conf_data/X-IPA-Session-Cookie/admin\@***@X-CACHECONF: in KEYRING:persistent:0:krb_ccache_uA6VDOR
```
... some time later, in a different execution of ipa user-show ...

```text
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
[***@nyx ~]# klist
Ticket cache: KEYRING:persistent:0:krb_ccache_uA6VDOR
Default principal: ***@XS.IPA.COOL

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

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

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

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

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

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


```
"""

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-288954010

[abbra]

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

abbra commented:
"""
@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.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-288963636

[tiran]

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

tiran commented:
"""
@simo5 I left some comments.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-288971205

[simo5]

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

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

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-289014748

[simo5]

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

[simo5]

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

simo5 commented:
"""
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.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-289060068

[simo5]

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

[abbra]

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

abbra commented:
"""
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.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-289445234

[abbra]

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

Label: +ack

[tomaskrizek]

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

Label: +pushed

[tomaskrizek]

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

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

[tomaskrizek]

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

tomaskrizek commented:
"""
@simo5 Please rebase for `ipa-4-5`.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-289742723

[tomaskrizek]

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

tomaskrizek commented:
"""
master:

* 9a6ac74eb4421b9ffa831dc6fed067d2ddc0618e Avoid growing FILE ccaches unnecessarily
* fbbeb132bf37f8a03ef2f2184adb11796ab13d8b Handle failed authentication via cookie
* e07aefb886096a7d419a4f1a2dec287e5ecd1626 Work around issues fetching session data
* d63326632b796a5ec9c6468c5ffe0c5a846501e1 Prevent churn on ccaches
"""

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-289742598

[simo5]

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

simo5 commented:
"""
Should I make a new PR for 4.5 ?

"""

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-289761195

[MartinBasti]

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

MartinBasti commented:
"""
Yes please
"""

See the full comment at https://github.com/freeipa/freeipa/pull/649#issuecomment-289761754