After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 344839 - Vino should use gnome-keyring to save/get password
Vino should use gnome-keyring to save/get password
Status: RESOLVED FIXED
Product: vino
Classification: Applications
Component: Server
2.13.x
Other All
: Normal enhancement
: ---
Assigned To: Vino Maintainer(s)
Vino Maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2006-06-14 03:21 UTC by Steven Yuchao Zhang
Modified: 2006-10-19 10:29 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
Patch to support gnome-keyring in vino (9.29 KB, patch)
2006-06-30 08:32 UTC, Steven Yuchao Zhang
needs-work Details | Review
New patch (16.56 KB, patch)
2006-07-07 03:54 UTC, Steven Yuchao Zhang
needs-work Details | Review
vino-gnome-keyring.patch (13.02 KB, patch)
2006-10-17 09:16 UTC, Mark McLoughlin
committed Details | Review

Description Steven Yuchao Zhang 2006-06-14 03:21:17 UTC
Currently, Vino use gconf to save password in base64 format, in fact gconf save the password in $HOME/.gconf/desktop/gnome/remote_access/. This is not security. When the users home dir is stored on an NFS server it is VERY VERY easy to over come the UNIX permissions in the default configuration.  Only NFS protected by Kerberos really helps with this.

Gnome-keyring is a good solution to solve this issue. In fact, other gnome applications like Evolution will change from base64+gconf to gnome-keyring. So vino should adopt gnome-keyring too.
Comment 1 Steven Yuchao Zhang 2006-06-30 08:32:21 UTC
Created attachment 68210 [details] [review]
Patch to support gnome-keyring in vino

2006-06-30  Steven Zhang <steven.zhang@sun.com>

        Add support for Gnome-Keyring. This will enchance the
        security. A configure option --enable-gnome-keyring is
        used to specify if gnome-keyring is enabled.
        Fix bug #344839

        * configure.in: add --enable-gnome-keyring option

        * config.h.in: add ENABLE_GNOME_KEYRING

        * server/vino-prefs.c add vino_get_saved_password function
        to retrieve password when Gnome Keyring is enabled.

        * capplet/vino-preferences.c add vino_preferences_dialog_get_password
        and vino_preferences_dialog_set_password function to get/save
        password when Gnome-Keyring is enabled.
Comment 2 Mark McLoughlin 2006-06-30 09:31:16 UTC
Some comments from a 2 second look over the patch:

  + No need for --enable-gnome-keyring, but if there is a password set 
    in GConf and no password in the keyring, we should use the GConf one

  + We shouldn't modify the GConf password ... think about if the 
    user's preferences are shared between different versions of GNOME 
    via an NFS home directory

  + VNC base port is 5900, not 5800

  + Coding style is a little off e.g.:

{
  GnomeKeyringResult  result
  GList              *matches;
  GList              *l;

  matches = NULL;

  result = get (&matches);

  if (result != OK)
    return NULL

  for (l = matches; l; l = l->next)
    {
      GnomeKeyringNetworkPasswordData *item = l->data;

      return g_strdup (item->password);
    }

  return NULL;
}

  + Even better, though, there's no need to iterate the list just to 
    take the first item:

{
  GnomeKeyringResult  result
  GList              *matches;

  matches = NULL;

  result = get (&matches);

  if (result != OK || matches == NULL)
    return NULL;

  item = (GnomeKeyringNetworkPasswordData  *) l->data;

  return g_strdup (item)>password;
}

  + You're leaking the returned matches. Free with 
    gnome_keyring_network_password_list_free()

  + I don't want to use GConf as a notification mechanism. Figure out 
    some other way for vino-server to notice that the password has 
    changed in gnome-keyring.

  + There's no need to base64 encode the password in gnome-keyring
Comment 3 Steven Yuchao Zhang 2006-07-03 02:46:29 UTC
How about when we installed vino, and the first time we run vino-preferences, there will be no password in keyring and no password in gconf? I think to provide a option is not a bad choice. 

How do you think about idea choice? We put the option in gconf, so applications will know if vino will use gnome-keying or gconf for password store.


1, I will use singal to notify server the password si changed. 
2, In gnome-keyring, the password is in clear_text format.
3, I will modify code to use the correct port:5900
Comment 4 Steven Yuchao Zhang 2006-07-04 08:12:51 UTC
Currently, I have 3 questions for this patch.
1, about --enable-gnome-keyring option
As you said, " + No need for --enable-gnome-keyring, but if there is a
password set in GConf and no password in the keyring, we should use the GConf one"

So how about a old user who already have gconf vino password, and now use the new
version. Do you mean he should keep to use gconf?
How about if there is password in gconf and no password in keyring, we save the password 
in keyring, and switch to use keyring?
Let's discuss use case one by one:
	If there is no option, and gnome-keyring is available.
	1.1 no password in gconf, no password in keyring
		we will use keyring to save password
	1.2 no password in gconf, password in keyring 
		we will use keyring to save password
	1.3 password in gconf, no password in keyring
		we will use keyring to save password, copy the password from gconf to 
		keyring OR we just don't use keyring, keep to use gconf?
	1.4 password in gconf, password in keyring
		we will use keyring to save password, and the old password in gconf will 
		keep unchanged.

But if there is an option, we can specify to use gconf or keyring.
How about your opinions?
 
2, for: "+ We shouldn't modify the GConf password ... think about if the 
    user's preferences are shared between different versions of GNOME 
    via an NFS home directory "


How do you think about this idea:
What do you mean, don't modify the GConf password? Do you mean if there
is already a
gconf password, when we use keyring, we should not modify the gconf one,
just keep it
unchanged. Am I right? So this means, when user changed password in
vino-preferences,
the keyring will hold the new password, and gconf will keep
unchanged(the old password)?

3, for notify mechanism,
Currently, I definitely agree with you, that we should not use gconf to
notify. So what
we can do, I believe there are only two choice:signal or dbus. Both
these two choice are not perfect. Do you have any suggestion?
Comment 5 Mark McLoughlin 2006-07-04 13:11:19 UTC
It's simple

  1) When you read the password, try gnome-keyring first and fallback
     to GConf if there's no password in the keyring

     (Note, you shouldn't read from the keyring until we actually need
     the password - e.g. if authentication_method = none)

  2) If the user changes the password, always save it in gnome-keyring
     and don't modify the password in GConf

As for notification, you've lots of options .. in no particular order:

  1) Send a HUP signal to vino-server

  2) Add a password_changed() method to the GNOME::Remote_Desktop CORBA
     interface and use that

  3) Send a D-BUS signal which vino-server would listen for

  4) Use an X ClientMessage based protocol - vino-preferences sends a
     message to the root window and vino-server listens for it. Something
     like panel-action-protocol

  5) Make vino-server poll regularily for the password

  6) Add a notification mechanism to gnome-keyring itself (e.g. in the protocol
     or have the keyring daemon send a D-BUS signal)
Comment 6 Mark McLoughlin 2006-07-04 13:15:38 UTC
Actually, notification is just silly ... we only need notification if the server caches the password. We should just request the password from gnome-keyring each time someone actually tries to authenticate.
Comment 7 Steven Yuchao Zhang 2006-07-04 14:08:53 UTC
Yes, this is a great idea to avoid cache the password in the memory, and every time, there is a client request need to be authenticated, server will fetch password from keyring or gconf.

And for keyring and gconf, by default, firstly, vino will try keyring, if failed, it will call gconf. And only save in keyring. But how about if we failed to save password in keyring? I think if we failed to save password in keyring, we should save it in gconf. How do you think?
Comment 8 Steven Yuchao Zhang 2006-07-06 09:15:20 UTC
1 concern.
I think put the function vino_get_server_password() in vino-util.c or vino-server.c , since there is no gconf client, I have to put the following lines in vino_server_get_password():
  vino_client = gconf_client_get_default ();

  gconf_client_add_dir (vino_client,
                        VINO_PREFS_DIR,
                        GCONF_CLIENT_PRELOAD_ONELEVEL,
                        NULL);

So, this means we have to add the VINO_PREFS_DIR macro in the source file. If we can move vino_get_server_password() into vino_prefs.c, we don't need the above lines and we don't need to add a macro. 

So how do you think about it?
If we get agreement in this, I will upload a new patch for this bug.
Comment 9 Steven Yuchao Zhang 2006-07-07 03:54:09 UTC
Created attachment 68520 [details] [review]
New patch
Comment 10 Steven Yuchao Zhang 2006-07-07 03:56:28 UTC
Comment on attachment 68520 [details] [review]
New patch

According the discussion, I made a new patch. When in saveing password, gconf will be used as fallback. Only when we got failed to save in keyring ,we will use gconf to save password(base64 format). And in keyring, cleartext password will be saved.

        Add support for Gnome-Keyring. This will enchance the
        security. A configure option --enable-gnome-keyring is
        used to specify if gnome-keyring is enabled.
        Fix bug #344839

        * server/vino-util.h export vino_get_server_password() function
        * server/vino-util.c add vino_get_server_password() function
        to retrieve cleartext password.

        * server/vino-prefs.c remove password_changed gconf client handler

        * server/vino-server.c remove vnc_password property from VinoServerPrivate
        and remove related set/get functions. In vino_server_auth_client(), call
        vino_get_server_password() to get cleartext password when auth client.

        * capplet/vino-preferences.c add vino_preferences_dialog_get_password
        and vino_preferences_dialog_set_password function to get/save
        password.
Comment 11 Steven Yuchao Zhang 2006-07-11 06:55:24 UTC
Does anybody can review the new patch and give me some feedback? Is this patch OK to check in? 
Thanks a lot.
Comment 12 Steven Yuchao Zhang 2006-07-17 07:27:41 UTC
Mike, Ping-:)
Comment 13 Steven Yuchao Zhang 2006-07-24 15:04:34 UTC
Does anyone can help me to review this patch and check this patch?
I don't have check in permission.

Mike, I need your help.
Comment 14 Steven Yuchao Zhang 2006-07-25 09:09:12 UTC
Mark Mcloughlin, ping-:)
Comment 15 Steven Yuchao Zhang 2006-07-31 02:14:42 UTC
Mark, Ping-:)
Comment 16 Steven Yuchao Zhang 2006-08-09 09:30:56 UTC
Ping, Mark-:)
Comment 17 Bastien Nocera 2006-08-16 07:56:34 UTC
Stop pinging whoever Steven. A month to add a patch is not uncommon, and it wouldn't be added now anyway, as GNOME 2.15 is in a feature freeze.
Comment 18 Steven Yuchao Zhang 2006-08-16 08:22:59 UTC
Thanks a lot. I don't want to ping anyone if I can get feedback/comments here or by email in two weeks. I don't think 2 weeks is a short time to write feedback or comments. Module owners/maintainers should be active and do bug triage, code review , write feedback in time.

Now, gnome 2.15 is in feature freeze, I can understand currently why this patch can not be put into cvs. 

Comment 19 Mark McLoughlin 2006-10-17 08:55:03 UTC
Okay, talking this over with alex we've concluded it's not such a great idea:

  - gnome-keyring wasn't designed for this kind of usage

  - when a remote user connects, the user may be prompted to unlock the 
    keyring. If this is an unattended terminal, there will be no-one there
    to unlock it.

  - when the user starts the preferences dialog, she may be prompted to unlock
    the keyring. That's very strange.

  - you can't lockdown the password if it can be overidden by the keyring
    password

  - GConf is the most sensible place for the password - storing it in the 
    keyring is only to placate people who worry about it being stored in a
    plaintext file in the users homedir. But since a) vnc authentication is
    inherently insecure and b) only root and the user can read the password
    and c) the password is stored in an obfuscated format ... then I think
    it's not something to worry about

So, I'll add this. But it will be off by default and only enabled with --enable-gnome-keyring
Comment 20 Mark McLoughlin 2006-10-17 09:16:54 UTC
Created attachment 74859 [details] [review]
vino-gnome-keyring.patch

What I committed to HEAD
Comment 21 Mark McLoughlin 2006-10-17 09:17:54 UTC
2006-10-17  Mark McLoughlin  <mark@skynet.ie>

        Add a --enable-gnome-keyring option which causes Vino
        to store its configured password in the user's keyring.
        Disabled by default, because it's not really a good
        idea. See comments in bug #344839

        Based on a patch from Steven Zhang <steven.zhang@sun.com>

        * configure.in: add --enable-gnome-keyring

        * server/vino-server.c:
        (vino_server_get_password_from_keyring): helper to
        read the password from the keyring.
        (vino_server_auth_client): if keyring support is enabled,
        authenticate against the password stored in the keyring.
        If there's no password in the keyring, authenticate against
        the password in GConf.

        * capplet/vino-preferences.c:
        (vino_preferences_dialog_get_password_from_keyring): lookup
        the password from the user's keyring.
        (vino_preferences_dialog_set_password_in_keyring): store
        the password in the user's keyring.
        (vino_preferences_dialog_password_changed): store the new
        password in the keyring, falling back to GConf if that
        fails.
        (vino_preferences_dialog_setup_password_entry): read the
        password from the keyring, falling back to GConf. Only
        watch for changes from GConf if we actually used the
        one from GConf in the first place.
        (vino_preferences_dialog_init): hack to allow variable
        number of listeners.

        * server/Makefile.am, capplet/Makefile.am: build against
        gnome-keyring