GNOME Bugzilla – Bug 344839
Vino should use gnome-keyring to save/get password
Last modified: 2006-10-19 10:29:18 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.
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.
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
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
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?
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)
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.
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?
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.
Created attachment 68520 [details] [review] New patch
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.
Does anybody can review the new patch and give me some feedback? Is this patch OK to check in? Thanks a lot.
Mike, Ping-:)
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.
Mark Mcloughlin, ping-:)
Mark, Ping-:)
Ping, Mark-:)
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.
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.
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
Created attachment 74859 [details] [review] vino-gnome-keyring.patch What I committed to HEAD
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