GNOME Bugzilla – Bug 739593
A pile of kerberos changes
Last modified: 2015-05-22 14:04:03 UTC
I've done few kerberos changes for RHEL. The most notable changes are: 1) moving the identity service to a separate process so if it leaks or crashes it won't bring down all of goa 2) fixing a leak in ObjectManager objects by making the ObjectManager be a singleton (versus adding a finalize method and freeing it there) 3) a crasher fix
Created attachment 289940 [details] [review] identity: destroy alarms from main thread, even on dispose These days we defer destroying alarms to the main thread when alarms are reset. We still dispose of alarms in the current thread when disposing their identity object, however. This commit changes the code to always dispose of alarms from the main thread.
Created attachment 289941 [details] [review] identity: add some locking around expiration time We don't want the expiration time to get read while it's getting written.
Created attachment 289942 [details] [review] identity: make verify_identity have one exit path This makes the control flow easier to follow and lays the way for a follow up change.
Created attachment 289943 [details] [review] identity: don't set expiration_time to 0 intermediately while verifying identity The top of verify_identity calls set_expiration_time(self, 0); as a way to make sure the expiration time is 0'd if the identity's credentials are invalidated. In the case, they are still valid, set_expiration_time is called again with the new, most up to date value. The problem is the intermediate 0 isn't invisible, there's a window where the identity can be read and 0 will get returned, even when the identity is valid. This commit defers setting the expiration time until the very end of the function.
Created attachment 289945 [details] [review] kerberos: maintain one long-lasting object manager client to kerberos service At the moment, the kerberos backend creates one object manager to the kerberos identity service per provider object. Provider objects are actually fairly transient and get created and destroyed, in some cases, per operation on an account. The upshot is, object manager clients end up getting created more frequently than they really should be. To make matters worse, the kerberos provider has no finalize function, so these object manager clients are getting leaked. This commit makes the object manager client get created once at start up, and get reused by all providers. Since there's only one object manager, rooted in the main thread, using the main thread's main loop context now, the per-thread synchronous codepaths can't call object manager async functions using a local main loop context. They do this, at the moment, because there are async, main thread code paths that also need to talk to the kerberos service. The local main loop context provides a way to call the async code synchronously, and prevent duplication of logic. This commit gets rid of all the local main loop contexts, and instead uses sync functions. To prevent duplication of logic, the async code now leverages the sync code, in a thread.
Created attachment 289946 [details] [review] identity: separate identity service off into its own process This commit segregates the kerberos specific functionality off into its own helper process. This has a couple of benefits: 1) It is actually a better fit for how the code was initially designed, which was first staged in gnome-settings-daemon with g-o-a talking to it. Right now we have gnome-online-accounts talking to itself, in-process, through d-bus, which is suboptimal. 2) It keeps any leaks or crashes in the kerberos code from bringing down the whole online accounts daemon.
Created attachment 289947 [details] [review] kerberos: zero initialize operation structure We need zero initialize the structure so we don't end up freeing unused members when cleaning up the operation.
Review of attachment 289947 [details] [review]: Makes sense to me. Is this suitable for older branches?
Review of attachment 289940 [details] [review]: Ok. Suitable for older branches?
Review of attachment 289941 [details] [review]: I hope we continue to be careful to not take the lock recursively while refactoring the code in future. Otherwise looks good to me.
Review of attachment 289942 [details] [review]: Sure, looks good.
Review of attachment 289943 [details] [review]: Makes sense. Suitable for older branches?
i'm going to throw another few patches up following our discussion this morning on IRC.
Created attachment 289977 [details] [review] kerberos: drop weird, superfluous assignment in query function goa_kerberos_identity_inquiry_answer_query has this strange code: g_return_if_fail (inquiry == kerberos_query->inquiry); ... inquiry = kerberos_query->inquiry; It's completely redundant and makes no sense so drop it.
Created attachment 289978 [details] [review] kerberos: fail inquiry if provided buffer is too small for password Right now we just truncate the user's response if it's too big for the passed in buffer. That's wrong on a theoretical level; we shouldn't ever use a password that the user didn't enter (even if it's a truncated version of what the user did enter). In practice, it's not actually a problem. The kerberos library provides a full 1024 byte buffer, which is much longer than a user would realistically enter into an entry. Still, this commit addresses the problem by introducing a mechanism for failing the inquiry.
Created attachment 289979 [details] [review] inquiry: fix edge case when answer is full size of reply buffer Right now if the user's answer is 1024 bytes exactly then we'll call strlen() on an unterminated string. This commit addresses the issue by avoiding the strlen() call and instead using the already computed length.
Review of attachment 289977 [details] [review]: Ofcourse
Review of attachment 289978 [details] [review]: Makes sense. ::: src/goaidentity/goakerberosidentityinquiry.c @@ +34,3 @@ int number_of_queries; int number_of_unanswered_queries; + guint is_failed : 1; Is it really worth doing this instead of using a gboolean?
Review of attachment 289979 [details] [review]: Of course.
(In reply to comment #8) > Review of attachment 289947 [details] [review]: > > Makes sense to me. Is this suitable for older branches? yea, definitely.
(In reply to comment #9) > Review of attachment 289940 [details] [review]: > > Ok. Suitable for older branches? at least 3-12. it's sort of a follow up to bug 729718 .
(In reply to comment #12) > Review of attachment 289943 [details] [review]: > > Makes sense. Suitable for older branches? i think so, but it of course depends on the commit before it that restructures the function, so that would need to go too.
(In reply to comment #18) > Review of attachment 289978 [details] [review]: > > Makes sense. > > ::: src/goaidentity/goakerberosidentityinquiry.c > @@ +34,3 @@ > int number_of_queries; > int number_of_unanswered_queries; > + guint is_failed : 1; > > Is it really worth doing this instead of using a gboolean? no, not really. it's a question of coding style. If you prefer gboolean, then gboolean is fine with me.
Comment on attachment 289979 [details] [review] inquiry: fix edge case when answer is full size of reply buffer Attachment 289979 [details] pushed as 524ea19 - inquiry: fix edge case when answer is full size of reply buffer
Created attachment 303819 [details] [review] kerberos: Maintain one long-lasting object manager client
Created attachment 303820 [details] [review] identity: Separate identity service off into its own process
The order in which these patches were generated (with respect to bug 739594), was different from the RHEL patches. I rebased these against master in the same order as RHEL since that is what we have been actually shipping so far. Sorry for having let these slip through the cracks.