Bug 9098 - winbind does not refresh kerberos tickets
winbind does not refresh kerberos tickets
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: Winbind
3.6.7
x64 Linux
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
: 6743 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-15 12:17 UTC by ian.gordon
Modified: 2012-08-26 20:37 UTC (History)
1 user (show)

See Also:


Attachments
dirty hack to "fix" winbind (579 bytes, patch)
2012-08-15 12:17 UTC, ian.gordon
no flags Details
Test fix (uncompiled :-). (1.15 KB, patch)
2012-08-16 00:51 UTC, Jeremy Allison
no flags Details
Test fix (compiles now :-). (1.26 KB, patch)
2012-08-16 03:57 UTC, Jeremy Allison
no flags Details
Better fix. (3.04 KB, patch)
2012-08-16 04:26 UTC, Jeremy Allison
no flags Details
working patch although it may have other problems (2.74 KB, patch)
2012-08-16 13:19 UTC, ian.gordon
no flags Details
working patch although it may have other problems (2.74 KB, patch)
2012-08-16 13:28 UTC, ian.gordon
no flags Details
Slight update on "working patch" (3.07 KB, patch)
2012-08-16 22:19 UTC, Jeremy Allison
no flags Details
Test patch. (3.14 KB, patch)
2012-08-20 20:54 UTC, Jeremy Allison
no flags Details
git-am fix for 3.6.next. (3.72 KB, patch)
2012-08-21 19:26 UTC, Jeremy Allison
asn: review+
Details
git-am fix for 3.5.next (3.72 KB, patch)
2012-08-21 21:08 UTC, Jeremy Allison
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description ian.gordon 2012-08-15 12:17:09 UTC
Created attachment 7774 [details]
dirty hack to "fix" winbind

Both the Ubuntu 12.04 packages and the real samba 2.6.7 behave the same.

winbindd will renew kerberos tickets until they expire, but it seems unable to refresh them before expiry.

I have the following in smb.conf:

winbind refresh ticket = true

and have cached_login set for pam_winbind

After 7 days ( the renewal limit on AD kerberos tickets) the ticket expires and I lose access to my NFS home directory which uses sec=krb5

I have tried to debug why this is happening and have come to the conclusion that there are two important variables for ticket refreshing to work (both in winbind/winbindd_cred_cache.c):

ccache_list
memory_creds_list

and that the function that stores the password for later refreshing use is called

winbindd_add_memory_creds

This function though requires that the user is in ccache_list before it stores the password in a way it can be used by the rekinit part of the function krb5_ticket_refresh_handler.

The problem as I see it is that winbind forks and the parent populates ccache_list and the child populates memory_creds_list.
This leads to the password not being stored in a way that can be used by the rekinit code in krb5_ticket_refresh_handler.

As a dirty hack (attached) I tried populating memory_creds_list from the same location as ccache_list get populated (winbindd_raw_kerberos_login in winbind/winbindd_pam.c).

This hack "fixes" the problem.
Comment 1 Jeremy Allison 2012-08-16 00:51:30 UTC
Created attachment 7777 [details]
Test fix (uncompiled :-).

Ok, it's close but not quite right I think.

The easy fix is to ensure we store the memory creds in both the parent (for the ntlm-auth external use) and in the child (for krb5 ticket renewal use).

Plus we need to destroy them on logout in the child.

Can you test this instead and see if it fixes the problem ? Sorry, haven't had a chance to compile (but any typos should be easy to fix).

If we're going to store the in-memory creds in only one place we'd need to change the :

       /* Credential cache access */
        { WINBINDD_CCACHE_NTLMAUTH, winbindd_ccache_ntlm_auth, "NTLMAUTH" },
        { WINBINDD_CCACHE_SAVE, winbindd_ccache_save, "CCACHE_SAVE" },

functions in the parent to be async and pass the creds to the child for that domain. Might not be worth the effort (IMHO).

Jeremy.
Comment 2 Jeremy Allison 2012-08-16 03:57:16 UTC
Created attachment 7778 [details]
Test fix (compiles now :-).

Ok, looking more closely at what you did it's pretty much right, with a few tweaks.

Can you test this (latest) attached patch ? If it works for you I'll commit it to master, and get it back-ported to 3.5.next and 3.6.next.

Thanks !

Jeremy.
Comment 3 Jeremy Allison 2012-08-16 04:26:14 UTC
Created attachment 7779 [details]
Better fix.

OK, third time lucky. I think this is the right place to do it as it adds the in-memory creds at the same time the krb5 ticket renewal function is set up.

Let me know if it works for you and we'll get this in the next releases !

Cheers,

Jeremy.
Comment 4 ian.gordon 2012-08-16 13:19:57 UTC
Created attachment 7782 [details]
working patch although it may have other problems
Comment 5 ian.gordon 2012-08-16 13:23:47 UTC
Comment on attachment 7782 [details]
working patch although it may have other problems

Patch doesn't work. Still doesn't refresh tickets.

Hunk 3 to file winbindd_cred_cache.c needs to happen after the ccache_list actuall has the user added to it.

What does hunk 2 to winbindd_cred_cache.c do? Is it just to increase the ref count in memory_creds?
Comment 6 ian.gordon 2012-08-16 13:28:29 UTC
Created attachment 7783 [details]
working patch although it may have other problems


Sorry, please ignore comments 4 and 5.

Patch doesn't work. Still doesn't refresh tickets.

Hunk 3 to file winbindd_cred_cache.c needs to happen after the ccache_list
actuall has the user added to it.

What does hunk 2 to winbindd_cred_cache.c do? Is it just to increase the ref
count in memory_creds?
Comment 7 Jeremy Allison 2012-08-16 22:19:04 UTC
Created attachment 7786 [details]
Slight update on "working patch"

Ok, here's the fix I think will do it - thanks for the testing (and pointing out the error in calling winbindd_add_memory_creds() before the ccache entry was added to the linked list).

Yes, the second hunk is to ensure ref-counting is correct on multiple pam logins.

Can you test this and if you're happy I'll apply to master and get additional Samba Team engineer review to get into 3.6.next.

Cheers and thanks for all your help !

Jeremy.
Comment 8 ian.gordon 2012-08-20 10:04:27 UTC
Patch 7786 work perfectly if I only log in once.

If you log in more than once then the ref_count for the memory_creds doesn't increment. I think this is because there is already a krb5_ticket_refresh_handler event for that user so it never gets to the winbindd_add_memory_creds call to increment the counter.
Comment 9 Jeremy Allison 2012-08-20 16:24:49 UTC
Getting closer :-). I'll take a look and try and fix that issue today.
Comment 10 Jeremy Allison 2012-08-20 20:54:55 UTC
Created attachment 7795 [details]
Test patch.

Move the winbindd_add_memory_creds() hunk to outside the test for entry->event if this is a second or subsequent login.

Let me know if this (finally :-) does the trick, sorry.

Jeremy.
Comment 11 ian.gordon 2012-08-21 08:17:18 UTC
The patch works perfectly :) (well I need to do a proper test with 7 day tickets instead of the short ones I've been using to test it)

Unfortunately there is still one small problem:

If you get your password wrong while logging in the ccache ref_count is wrongly decremented and worse still the actual ccache file is destroyed (see winbindd_raw_kerberos_login in winbindd_pam.c for what happens if anything goes wrong with kerberos authentication). Is this a different bug which should be reported separately?

Thanks for your help.
Comment 12 Jeremy Allison 2012-08-21 18:13:24 UTC
Yes, this one is a different bug - can you open a new bug to track that ?

In the meantime I'll get this fix into master and create a git-am patch for 3.5.next and 3.6.next.

Thanks for your patience !

Jeremy
Comment 13 Jeremy Allison 2012-08-21 19:26:25 UTC
Created attachment 7799 [details]
git-am fix for 3.6.next.

Fix that went into master.
Comment 14 Jeremy Allison 2012-08-21 21:08:25 UTC
Created attachment 7800 [details]
git-am fix for 3.5.next

Backport to 3.5.next.

Jeremy.
Comment 15 Jeremy Allison 2012-08-22 17:10:03 UTC
*** Bug 6743 has been marked as a duplicate of this bug. ***
Comment 16 Andreas Schneider 2012-08-23 08:14:49 UTC
Comment on attachment 7799 [details]
git-am fix for 3.6.next.

Looks fine for me.
Comment 17 Andreas Schneider 2012-08-23 08:54:13 UTC
Comment on attachment 7800 [details]
git-am fix for 3.5.next

looks fine for me too
Comment 18 Jeremy Allison 2012-08-23 15:42:35 UTC
Re-assigning to Karolin for inclusion in 3.5.next and 3.6.next.

Jeremy.
Comment 19 Karolin Seeger 2012-08-23 18:20:52 UTC
Pushed to v3-5-test and v3-6-test.
Closing out bug report.

Thanks!