GNOME Bugzilla – Bug 784944
Be robust against org.freedesktop.secrets crashes and restarts
Last modified: 2021-07-05 10:58:26 UTC
Libsecret's caching of the SecretService proxy object (bug 765406) prevents it from being robust against the org.freedesktop.secrets service crashing or getting restarted (eg., bug 764029). While it might be possible to fix the underlying problem in libsecret (I have doubts about it), I'd like to workaround it in gnome-online-accounts by caching our own SecretService and avoiding the secret_password* APIs.
Created attachment 355582 [details] [review] imap-smtp, smtp-auth: Remove unused code paths
Created attachment 355584 [details] [review] imap-auth-login, imap-smtp: Remove unused code paths
Created attachment 355587 [details] [review] provider: Clean up the public header
Created attachment 355596 [details] [review] provider: Clean up the public header
Work-in-progress patches at: gnome-online-accounts:wip/rishi/libsecret-workaround
Created attachment 355763 [details] [review] kerberos: Create the account only after the initial sign in
Review of attachment 355763 [details] [review]: So I guess proof is in the pudding if it works! One thing I wonder about...the identify service notices when a user manually runs kinit and creates a temporary account. Changing the order here, where you sign in first before creating the account seems like it has the potential to exercise that kinit-temporary-account-creating code path. Do you think goaidentityservice.c:on_account_added is failing and we don't notice since failure isn't logged? If so, probably harmless, but might make since to put a comment or something. what about the other way, could identity service win the race the account gets stuck as a temporary account ? ::: src/goabackend/goakerberosprovider.c @@ +1211,3 @@ + /* FIXME: we go to great lengths to keep the password in non-pageable memory, + * and then just duplicate it into a gvariant here + */ probably should just delete this fixme. it's not true really and we're probably not going to fix it anytime soon.
(In reply to Ray Strode [halfline] from comment #7) > Review of attachment 355763 [details] [review] [review]: > One thing I wonder about...the identify service notices when a user manually > runs kinit and creates a temporary account. Changing the order here, where > you sign in first before creating the account seems like it has the > potential to exercise that kinit-temporary-account-creating code path. Do > you think goaidentityservice.c:on_account_added is failing and we don't > notice since failure isn't logged? If so, probably harmless, but might make > since to put a comment or something. what about the other way, could > identity service win the race the account gets stuck as a temporary account ? I'll take another look. My initial reading of the code didn't raise any red flags, and both the GUI and kinit seem to work as expected. > ::: src/goabackend/goakerberosprovider.c > @@ +1211,3 @@ > + /* FIXME: we go to great lengths to keep the password in non-pageable > memory, > + * and then just duplicate it into a gvariant here > + */ > > probably should just delete this fixme. it's not true really and we're > probably not going to fix it anytime soon. Ok.
(In reply to Ray Strode [halfline] from comment #7) > Review of attachment 355763 [details] [review] [review]: > One thing I wonder about...the identify service notices when a user manually > runs kinit and creates a temporary account. Changing the order here, where > you sign in first before creating the account seems like it has the > potential to exercise that kinit-temporary-account-creating code path. Do > you think goaidentityservice.c:on_account_added is failing and we don't > notice since failure isn't logged? Yes, you are right. It is not failing, but losing the race with GoaKerberosProvider::add_account because we schedule a refresh only after the polling timeout. > If so, probably harmless, but might make > since to put a comment or something. what about the other way, could > identity service win the race the account gets stuck as a temporary account ? Yes, it could win the race if we use a cache-type with notification support, or if we are right on the edge of the timeout. I wonder if we should add some freeze/thaw call to org.gnome.Identity to skip a particular principal from being added as temporary. The pending_temporary_account_results hash table can be abused to implement it. We could also emit a signal from org.gnome.Identity and move the temporary account creation into goa-daemon. That would reduce the circular dependency between goa-daemon and goa-identity-service a bit. However, that will be more complicated because GoaKerberosProvider::add_account doesn't happen in goa-daemon, but in the UI process (eg., gnome-control-center). So we'd need to convey the freeze/thaw between the UI and goa-daemon. This attempt at being robust against org.freedesktop.secrets crashes/restarts is beginning to feel like shaving a yak. :)
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-online-accounts/-/issues/ Thank you for your understanding and your help.