Created attachment 228625 [details] [review]
Clean up temporary accounts (eg., Kerberos) when they are stale

Review of attachment 228625 [details] [review]:

I don't know enough about account staleness, and the way accounts.conf works, to review this effectively.

Review of attachment 228625 [details] [review]:

Fixes make sense to me.  It might be nice if the keyring stuff was a separate commit.  A few more comments below:

::: src/daemon/goadaemon.c
@@ +356,2 @@
 static void
+write_config_file (GKeyFile *key_file, const gchar *path)

having this as a separate function makes sense, I think. Though, it would be good if all the other callers of g_key_file_to_data called this new function.

@@ +452,3 @@
                       goa_debug ("ignoring account \"%s\" in file %s because it's stale",
                                  groups[n], path);
+                      g_key_file_remove_group (key_file, groups[n], NULL);

So, I think my original (wrong) thinking here was that by the end of the load, the hash table that was built up in this function would get used to scrub the keyfile right before serialization in process_config_entries. That was a mistake, the keyfile isn't serialized in process_config_entries, and looking now, that wasn't even the intent of the code, because the code is structured to allow multiple cascading config files (even though, at the moment, there's only one).

g_key_file_remove_group here seems right.

@@ +906,3 @@
               guid = g_dbus_connection_get_guid (daemon->connection);
               g_key_file_set_string (key_file, group, "SessionId", guid);
+              collection = SECRET_COLLECTION_SESSION; /* session keyring */

So this change is interesting.  We don't actually have a password to store in the keyring for temporary accounts (at the moment at least) because they're added implicitly from noticing the ccache has changed.  Certainly, if having a keyring is important, then the keyring should be a transient session keyring for temporary accounts.  I suspect the motivation for this change was stale entries getting added to the keyring? I wonder if it would make more sense to do see if credentials is empty with g_variant_n_children (or so) and avoid calling into the keyring at all?  Or maybe doing what you do here and avoiding when possible makes sense?

(In reply to Ray Strode [halfline] from comment #3)
> ::: src/daemon/goadaemon.c
> @@ +356,2 @@
>  static void
> +write_config_file (GKeyFile *key_file, const gchar *path)
> 
> having this as a separate function makes sense, I think. Though, it would be
> good if all the other callers of g_key_file_to_data called this new function.

Turns out that g_key_file_save_to_file became a thing almost a year after I wrote this code.

Created attachment 327305 [details] [review]
daemon, utils: Simplify saving a GKeyFile to a path

Created attachment 327353 [details] [review]
media-server: Don't touch the keyring

Created attachment 327354 [details] [review]
daemon: Clean up temporary accounts (eg., Kerberos) from accounts.conf

Created attachment 327355 [details] [review]
daemon: Don't store empty entries in the keyring

(In reply to Ray Strode [halfline] from comment #3)
> Review of attachment 228625 [details] [review] [review]:
> @@ +906,3 @@
>                guid = g_dbus_connection_get_guid (daemon->connection);
>                g_key_file_set_string (key_file, group, "SessionId", guid);
> +              collection = SECRET_COLLECTION_SESSION; /* session keyring */
> 
> So this change is interesting.  We don't actually have a password to store
> in the keyring for temporary accounts (at the moment at least) because
> they're added implicitly from noticing the ccache has changed.  Certainly,
> if having a keyring is important, then the keyring should be a transient
> session keyring for temporary accounts.  I suspect the motivation for this
> change was stale entries getting added to the keyring? I wonder if it would
> make more sense to do see if credentials is empty with g_variant_n_children
> (or so) and avoid calling into the keyring at all?  Or maybe doing what you
> do here and avoiding when possible makes sense?

On second thoughts, neither SECRET_COLLECTION_SESSION nor checking for empty credentials will be enough because they won't clean up the stale keyring entries that are already there. I think we need to use libsecret API to actually remove them.

We can certainly avoid storing empty credentials GVariants in the keyring, but that would be more of a cosmetic thing.

Review of attachment 327305 [details] [review]:

This looks straightforward, so I am going to push it to master. Let me know if I made a mistake.

Created attachment 327411 [details] [review]
daemon: Move object_path_to_group to its own area

Created attachment 327412 [details] [review]
daemon: Rename the variable 'group' to 'id'

Created attachment 327413 [details] [review]
daemon: Refactor code to get ID from group name into a function

Created attachment 327414 [details] [review]
daemon, utils: Rename goa_utils_delete_credentials_sync

Created attachment 327415 [details] [review]
utils: Add goa_utils_delete_credentials_for_id_sync helper

Created attachment 327416 [details] [review]
daemon: Clean up temporary accounts (eg., Kerberos)

Created attachment 327417 [details] [review]
daemon: Don't store empty entries in the keyring

I must say that I don't like the various scenarios in which goa_daemon_reload_configuration is called. For example, cleaning up the GKeyFile and committing it to disk while reloading the configuration means the GFileMonitor will call it again. Similarly, we change accounts.conf while adding or removing an account, then call goa_daemon_reload_configuration, which will end up being called once more by the GFileMonitor.

It would be nice to avoid these, but then file I/O is inherently racy. I would rather repeat a few things than miss some event from GFileMonitor, or fail to update the D-Bus objects before completing the add or remove call.

Review of attachment 327411 [details] [review]:

++

Review of attachment 327412 [details] [review]:

should say "Rename the variable 'group' from 'id'"

Review of attachment 327413 [details] [review]:

++

Review of attachment 327414 [details] [review]:

makes sense to me

Review of attachment 327415 [details] [review]:

i like how incremental this patchset is.

Review of attachment 327353 [details] [review]:

yay for cutting out unnecessary code!

Comment on attachment 327353 [details] [review]
media-server: Don't touch the keyring

Thanks for the reviews, halfline. Pushed to master.

(In reply to Ray Strode [halfline] from comment #20)
> Review of attachment 327412 [details] [review] [review]:
> 
> should say "Rename the variable 'group' from 'id'"

Oh, yes. Of course. Thanks for catching it.

Review of attachment 327417 [details] [review]:

seems nice to me.

Review of attachment 327416 [details] [review]:

I think this is okay, but I also agree with comment 18.  Right now it looks like we call reload_configuration in init, which saves the file, which will trigger the file monitor to reload the configuration again.  We could try to prevent the second reload by recording the st_mtim.tv_nsec of the file after save and only only doing the reload if it's different.  That be a little cumbersome since there is more than one config file.

Another idea is we could create the file monitor a little later after we reload configuration in init.  That would help prevent it from firing from our own writes.  Note, that after the first reload, subsequent reloads aren't likely to write to the config file.  

anyway patch looks good to me.

Comment on attachment 327414 [details] [review]
daemon, utils: Rename goa_utils_delete_credentials_sync

Pushed to master.

(In reply to Ray Strode [halfline] from comment #28)

Thanks for the review, halfline!

> Review of attachment 327416 [details] [review] [review]:
> Another idea is we could create the file monitor a little later after we
> reload configuration in init.  That would help prevent it from firing from
> our own writes.

Creating the monitor after the initial goa_daemon_reload_configuration call can cause us to miss some changes. In practice it shouldn't be a problem, but the possibility does exist, no?

> Note, that after the first reload, subsequent reloads
> aren't likely to write to the config file.  

That's a good point. This observation significantly reduces my urge to fix things. :)

> anyway patch looks good to me.

Pushed everything to master.

Created attachment 331704 [details] [review]
daemon: Ensure temporary accounts are really removed from the keyring

Comment on attachment 331704 [details] [review]
daemon: Ensure temporary accounts are really removed from the keyring

Pushed to master.