oops I meant g_get_user_runtime_dir()

yes, please!

Created attachment 184903 [details] [review]
Use runtime dir instead of tmp for sockets

*** Bug 646492 has been marked as a duplicate of this bug. ***

Review of attachment 184903 [details] [review]:

Cool. Thanks! Just needs a few changes before this can go in.

::: daemon/gkd-util.c
@@ +113,3 @@
 	/* Generate a new directory */
 	if (!valid) {
+		master_directory = g_build_filename (g_get_user_runtime_dir (), "keyring", NULL);

I think the string here needs have the "-XXXXXX" ending, since its passed to egg_mkdtemp().

::: daemon/gpg-agent/gkd-gpg-agent-standalone.c
@@ +104,3 @@
 		return 1;
 
+	sock = gkd_gpg_agent_startup (g_get_user_runtime_dir ());

Don't need to make this change, as its just a example program.

::: daemon/ssh-agent/gkd-ssh-agent-standalone.c
@@ +106,3 @@
 		return 1;
 
+	sock = gkd_ssh_agent_startup (g_get_user_runtime_dir ());

Don't need to make this change, as its just a example program.

::: pkcs11/rpc-layer/Makefile.am
@@ +32,2 @@
 	$(GLIB_CFLAGS)
 

Did this accidentally get included in this patch?

@@ +65,3 @@
+
+gkm_rpc_daemon_standalone_CFLAGS = \
+	$(GLIB_CFLAGS)

Good fix, but maybe this should be split off to another commit. You can push this right away.

::: pkcs11/rpc-layer/gkm-rpc-daemon-standalone.c
@@ +115,1 @@
 	if (sock == -1)

This is just a test/example/devel program, so no need to make this change.

(In reply to comment #5)
> Review of attachment 184903 [details] [review]:
> 
> Cool. Thanks! Just needs a few changes before this can go in.
> 
> ::: daemon/gkd-util.c
> @@ +113,3 @@
>      /* Generate a new directory */
>      if (!valid) {
> +        master_directory = g_build_filename (g_get_user_runtime_dir (),
> "keyring", NULL);
> 
> I think the string here needs have the "-XXXXXX" ending, since its passed to
> egg_mkdtemp().

The subdirectory in the user runtime dir doesn't need to use a secure name, since it will go in a user-private directory.  It doesn't hurt to do so, though.

Josh, if keyring daemon is run for multiple login sessions, each will need its own subdirectory of XDG_RUNTIME_DIR.

Hmmm, on another topic, there's also this strangeness in the spec:

    Files in this directory MAY be subjected to periodic clean-up. To
    ensure that your files are not removed, they should have their
    access time timestamp modified at least once every 6 hours of
    monotonic time or the 'sticky' bit should be set on the file.

That's not an annoying feature from the perspective of a daemon writer. But I guess we need to either update the time every so often. or set the sticky bit. Are there security ramifications to setting the sticky bit?

One thing I forgot, William, is that the glib dependency needs to be updated to account for the use of the new g_get_user_runtime_dir() function.

Split this patch into two, and pushed to git. Skipped on changes to the example programs.

Thanks.

This patch doesn't work in practice :S

XDG_RUNTIME_DIR is not yet set when gnome-keyring-daemon is started by PAM.

Unlike other bits of initialization, gnome-keyring-daemon needs to create this directory immediately, so that it get initialization further by the session startup.

When XDG_RUNTIME_DIR is not set, Glib defaults to /data/.cache as the return value from g_get_user_runtime_dir(). And it's easy to have this be a directory that doesn't work with sockets (NFS, etc.)

This is caused by ordering issues in PAM configuration. In particular pam_gnome_keyring.so 'session' directive needs to be after pam_systemd.so

Posted heads up for packagers to desktop-devel-list about this.

Related fix (which will go into gnome-keyring 3.4.1):

commit cc6e1136fb9c98f26d60afee492a60865b0b2b93
Author: Stef Walter <stefw@gnome.org>
Date:   Mon Apr 2 16:22:48 2012 +0200

    Use correct XDG_RUNTIME_DIR when started from PAM
    
     * Accept the XDG_RUNTIME_DIR environment variable from
       the session when initializing gnome-keyring-daemon