commit 5ca7387952f99bf318d337df2d853f5bc3c0ddd2
Author: Stef Walter <stefw@collabora.co.uk>
Date:   Mon Dec 19 13:08:51 2011 +0100

    rpc-layer: Correctly handle case where gnome-keyring-daemon not running
    
     * In the gnome-keyring-pkcs11.so module return stand in info
       when the gnome-keyring-daemon info is not running.
     * In addition no slots will be listed
    
    https://bugzilla.gnome.org/show_bug.cgi?id=665961

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

this is apparently not fixed, still seeing these symptoms using gnome-keyring-3.4.1 on fedora 17.

WARNING: gnome-keyring:: couldn't connect to: /home/rdieter1/.cache/keyring-xyQAcY/pkcs11: No such file or directory

Where 
/etc/xdg/autostart/gnome-keyring-pkcs11.desktop
doesn't autostart for all environments

$ grep  Only /etc/xdg/autostart/gnome-keyring-pkcs11.desktop
OnlyShowIn=GNOME;Unity;

(I'm using KDE).

So, what is preferred, to remove/fix the OnlyShowIn or make gnome-keyring handle this case more gracefully?

(In reply to comment #3)
> this is apparently not fixed, still seeing these symptoms using
> gnome-keyring-3.4.1 on fedora 17.
> 
> WARNING: gnome-keyring:: couldn't connect to:
> /home/rdieter1/.cache/keyring-xyQAcY/pkcs11: No such file or directory
> 
> Where 
> /etc/xdg/autostart/gnome-keyring-pkcs11.desktop
> doesn't autostart for all environments

Good point. Thanks for catching, researching the issue.

> So, what is preferred, to remove/fix the OnlyShowIn or make gnome-keyring
> handle this case more gracefully?

The latter. We should check for ENOENT errno here:

http://git.gnome.org/browse/gnome-keyring/tree/pkcs11/rpc-layer/gkm-rpc-module.c#n254

And instead of printing a warning for ENOENT, print a debug (which won't normally be shown).

Are you able to provide a patch?

Created attachment 221533 [details] [review]
proposed patch to print debug on ENOENT case

first quick-n-dirty attempt at a fix as suggested.

Review of attachment 221533 [details] [review]:

Thanks for the patch. Appreciate it.

Could you include a bit more description in the commit message of the case that this fixes?

::: pkcs11/rpc-layer/gkm-rpc-module.c
@@ +259,1 @@
 		return CKR_DEVICE_ERROR;

Hmmm, I think we may need to return CKR_DEVICE_REMOVED for the ENOENT case instead of CKR_DEVICE_ERROR. 

The effect we want is that the PKCS#11 module acts as if it's not present. We don't want there to be a failure during initialization. I think returning CKR_DEVICE_REMOVED will accomplish this the effect we want.

You can verify this by running 'p11-kit -l' before and after the patch. After the patch it shouldn't fail with an error message.

Created attachment 221614 [details] [review]
updated patch

Here's an updated attempt taking the feedback into account.  p11-kit still gives an error, just a different one:

$ p11-kit -l
p11-kit: couldn't initialize registered modules: The device was removed or unplugged

Created attachment 221615 [details] [review]
real updated patch

Oops, the real updated patch that integrates your feedback.

$ p11-kit -l
p11-kit: couldn't initialize registered modules: The device was removed or unplugged

Looks we need to check the results of call_lookup() against CKR_DEVICE_REMOVED in both rpc_C_Initialize() and rpc_C_Finalize(). If call_lookup() returns CKR_DEVICE_REMOVED then we wouldn't run the call (ie: call_prepare() call_run() call_done()) but we would still return CKR_OK.

The goal is to have no error when the daemon isn't running, but just have the PKCS#11 act as if it was empty.

I'm not sure (or am at least a little confused), the code in rpc_C_Initialize already has

ret = call_lookup (&cs);
if (ret == CKR_OK) {
...

which is already != CKR_DEVICE_REMOVED, or should the code just short-circuit and return (or goto to unlock the mutex) at that point?:

oh,  it dawns on me now I guess the point is that these should end up returning CKR_OK, even if the comment about not calling call_prepare(),  call_run(), call_done() may  (or many not?) be valid.

So... would you object if I applied the latest proposed patch in fedora's downstream packaging in the meantime?  imo, it's already better that the status quo even if not fully meeting your requirements to include upstream.

ping, if it wasn't clear before, I need some advice and guidance on how best to proceed here.  I'm stuck on how to improve the patch to achieve the goal(s) mentioned in comment #9

Created attachment 228187 [details] [review]
only print debug message if no pkcs11 socket

Does this do the trick?

Thanks, I'll give it a whirl, but on first glance looks like a winner.

Yep,

$ p11-kit -l
gnome-keyring: /usr/lib64/pkcs11/gnome-keyring-pkcs11.so
    library-description: GNOME Keyring (without daemon)
    library-manufacturer: GNOME Keyring
    library-version: 1.1

Thanks!

So, how about a tentative plan of 
1.  my backporting to fedora's older f16/f17 gnome-keyring packaging
2.  I can include in f18 too, or can wait for next fxied release (whichever you prefer)

??

Okay, committed to git master.

As far as backporting, obviously that is a distro specific choice. I wouldn't
back port it at all because:

 1. This is a rather invasive patch.
 2. The corner case where this shows up is a misconfiguration. This only 
    happens when GNOME_KEYRING_CONTROL env var is set, but the daemon isn't
    running. Either a crash, or someone messing around with their DE.
 3. Easy to work around, just unset the env var.

Attachment 228187 [details] pushed as 370694b - only print debug message if no pkcs11 socket

My interest in this was peeked largely because it seems to me to be not a corner case at all, and occurs on all DE's not in
OnlyShowIn=GNOME;Unity;MATE;
referenced in gnome-keyring's autostart items (gnome-keyring-pkcs11 in particular)

Your characterization of this as a "misconfiguration" seems... inaccurate.


it's unclear to me what implications come from simply unset'ing GNOME_KEYRING_CONTROL.  Will gnome-keyring-using applications still work?

(In reply to comment #18)
> My interest in this was peeked largely because it seems to me to be not a
> corner case at all, and occurs on all DE's not in
> OnlyShowIn=GNOME;Unity;MATE;
> referenced in gnome-keyring's autostart items (gnome-keyring-pkcs11 in
> particular)

Fair enough.

I guess the problem then is having pamgnome-keyring.so in your PAM stack if you're not planning on using gnome-keyring-daemon. GNOME_KEYRING_CONTROL is set by the gnome-keyring PAM module.

> Your characterization of this as a "misconfiguration" seems... inaccurate.

The point is that GNOME_KEYRING_CONTROL is set but gnome-keyring-daemon is not running.

> Will gnome-keyring-using applications still work?

Storing secrets will still work, since that happens over DBus.

Anyway, I'm not really against backporting, to already released distros, just don't see the point. Certainly if Fedora 18 or Ubuntu 12.10 (and others) are not out yet, it may make sense to include this patch. But that's up to the downstream packager. I'm not a packager of gnome-keyring on any distro. 

Distros will need to make their own calls on whether suppressing this benign (but misleading) warning is worth backporting this patch.

> I guess the problem then is having pam_gnome-keyring.so in your PAM stack if
> you're not planning on using gnome-keyring-daemon.

Depends on how you define "planning on using".  Historically, to me, this has meant: do you expect gnome-keying applications to "just work" to store/retrieve secrets.  Are you suggesting this is supposed to work strictly via dbus these days without the pam module?  If so, since when?

It didn't work last I tested using fedora 17 + gnome-keyring-2.4.x + several gnome-keyring-using applications.  The fix/workaround was to add pam_gnome_keyring.so to the pam stack everywhere.