I'd like to introduce a libgnome-keyring-based plugin implementing the support for storing passwords in a fdo Secrets-compatible keyring. Thus, I would use an extension to the claws-mail plugin API providing the necessary functions. The necessary API functions are described below. For the scope of the description I'll assume that 'account information' would stand for the information passed to the keyring to uniquely identify the password (i.e. username, server, port etc. -- will clarify that later). I'll assume that this information is stored in a struct typedefed as 'acctinfo_t'. A keyring plugin would export (at least) the following keyring-specific functions (names, types etc. just for reference): bool keyring_is_available(void) - would check the plugin configuration and try if a connection to the keyring can be established; returns true if keyring can be used to store passwords, false otherwise. char* keyring_password_get(acctinfo_t) - would query the keyring for a password and return either the password or NULL if no keyring entry matches or unable to access the keyring. bool keyring_password_set(acctinfo_t* old, acctinfo_t current, char* newpass) - would query the keyring to store a new password and return true if the request succeeded, false otherwise. The meaning of the particular parameters is described in the keyring querying part. If no keyring plugin is available or keyring_is_available() returns false, claws-mail would use configuration file for password storage and behave as usual. Otherwise, the following remarks would apply: 1) the configuration dialogs are no longer pre-filled with passwords, 2) whenever the account connection is to be established, claws-mail queries the keyring for the password (the password can be changed by another program in runtime). The following changes are necessary to the account password entry fields if the keyring is used: 1) if the entry is empty and non-active, it shows dimmed 'Password not displayed' information, 2) if the entry is non-empty or active, it behaves as usual, allowing user to type in the new password, 3) the password entry fields comes with an additional toolbutton/checkbox named 'Delete password'; if it is enabled, the password entry is cleared and disabled for entry, and shows dimmed 'Password will be deleted' instead. The following changes are necessary to the account configuration dialog itself: 1) the dialog is supposed to store a copy of 'old' account information (acctinfo_t) (i.e. before user modifies them in the dialog), 2) when 'ok' or 'apply' is pressed and either the account information was changed or the password entry is not in 'Password not displayed' state, the keyring is queried as described below, then the 'old' account information is updated and password entries return to the 'Password not displayed' state. For the keyring query description, I assume the following variables are used: acctinfo_t old_acctinfo; // the stored 'old' account information acctinfo_t new_acctinfo; // the account information from the configuration dialog char* new_password; // the new password typed in within the entry Depending on the changes performed in the configuration dialog, the keyring is queried as described below: 1) if the account information isn't changed but a new password was typed in: keyring_password_set(NULL, new_acctinfo, new_password); which is supposed to either update the password associated with a particular account or add a new account to the keyring. 2) if the password entry was put in 'Password will be deleted' state (independently of the account information changes): keyring_password_set(NULL, old_acctinfo, NULL); which is supposed to remove a particular account from the keyring if it's there. 3) if the account information was changed and the password entry was left in 'Password not displayed' state: keyring_password_set(&old_acctinfo, new_acctinfo, NULL); which is supposed to update the account information within the keyring, i.e. associate the old password with the new information. 4) if the account information was changed and a new password was entered: keyring_password_set(&old_acctinfo, new_acctinfo, new_password); which is supposed to remove the old account from the keyring and add a new one with the new password (this can be implemented through 3+1). Such an API should be sufficient to implement the keyring support. Hope I am clear enough.
Created attachment 1073 [details] Patch adding a minimal password-getting functionality A newer version of patch sent to 'devel' list. It supplies 'password plugins' with user, server, proto & port.
Created attachment 1074 [details] Patch adding 2-clause BSD licensed-plugins support (this is the license under which the plugin is written)
Created attachment 1270 [details] Reorder send_message code for further changes The password obtaining code needs to know the port number, so the code guessing it needs to be moved earlier. This was previously done in the password_get patch and made it hardly readable. Splitting it makes it clear that I've just moved the code, not changed it.
Created attachment 1271 [details] Updated main patch
Please revise the patches now. I will start working on integrating the plugin into claws-mail codebase later.
Changes related to this bug have been committed. Please check latest Git and update the bug accordingly. You can also get the patch from: http://git.claws-mail.org/ http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=d04fd18b94a1c6fc8d22afe46367f278370011d3 Author: Colin Leroy <colin@colino.net> Date: Thu May 30 11:35:16 2013 +0200 Reorder send_message's port selection logic, paving the way for bug #2252 "API for external password storage" Patch by Michal Gorny
Hi Michal, Thanks for the patches! I applied the first one. Concerning the second, here are a few points: + /* all have to be set */ + g_assert(user); can you replace all the g_assert() calls with "cm_return_val_if_fail(user != NULL, FALSE)"? We prefer the sanity checks to fail without crashing; also cm_return functions print out backtraces, which help us get good bugreports :) OK, that was a unique point. Rest look great ! :)
Created attachment 1272 [details] Patch updated to use cm_return... Done.
Created attachment 1273 [details] gnome-keyring plugin, part 1 The first patch just adds the directory and configure checks for it.
Created attachment 1274 [details] gnome-keyring plugin, part 2 This one imports the code. I changed the license to GPL3+ to match claws.
Created attachment 1275 [details] gnome-keyring plugin, part 3 And a quick snippet to the README file.
Thanks for the positive feedback. I have attached the patches to integrate the plugin with claws-mail.
Thanks! Attachment #1273 [details] is OK for me; Attachment #1274 [details]: + ret = gnome_keyring_unlock_sync(NULL, kpass); + if (ret != GNOME_KEYRING_RESULT_OK) + debug_print("gnome_keyring_unlock_sync() failed: %s\n", + gnome_keyring_result_to_message(ret)); I'd probably put in an alertpanel_error() in this case, so the user knows it? + if (item->password && *(item->password)) { + enum { + PASS_ANY = 1, + PASS_PROTO = 2, + PASS_PORT = 4 If I understand correctly the plugin will return a password matching the user/server but not necessarily port/protocol. In this case and if the password is incorrect, that will prevent the user from logging in. That'd be annoying :)
(In reply to comment #13) > Thanks! > > Attachment #1273 [details] is OK for me; > > Attachment #1274 [details]: > + ret = gnome_keyring_unlock_sync(NULL, kpass); > + if (ret != GNOME_KEYRING_RESULT_OK) > + debug_print("gnome_keyring_unlock_sync() failed: %s\n", > + gnome_keyring_result_to_message(ret)); > > I'd probably put in an alertpanel_error() in this case, so the user knows it? Generally failure to unlock involves user clicking 'Cancel' in gnome-keyring password dialog; in that particular case, I don't think we should bother user with another error message. However, there can be other cases where error would be appropriate. I plan to take a second look at all the code paths in the future; but until then, I'd rather not bother user with all errors :). > + if (item->password && *(item->password)) { > + enum { > + PASS_ANY = 1, > + PASS_PROTO = 2, > + PASS_PORT = 4 > > If I understand correctly the plugin will return a password matching the > user/server but not necessarily port/protocol. In this case and if the > password is incorrect, that will prevent the user from logging in. That'd be > annoying :) Yes and no. True, it tries from the most specific password to the most general one. However, if the password doesn't work, claws just asks for another one (after the error) *. * I've tried this with IMAP. Not sure if all protos work the same here.
Changes related to this bug have been committed. Please check latest Git and update the bug accordingly. You can also get the patch from: http://git.claws-mail.org/ http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=14b6d48971596385736cfe0cf3fdcfe92a705efd Merge: 6ae1c2d 3f6ee6e Author: Paul <paul@claws-mail.org> Date: Sat Jun 8 21:31:28 2013 +0100 Merge branch 'bugfix-3.9.2' http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=3f6ee6e93089e2a609ff07e97f8fcdb4c76cae71 Author: Paul <paul@claws-mail.org> Date: Thu Jun 6 14:10:21 2013 +0100 fix detection of account in --compose and --compose-from-file where the From value contains a name + email. Patch by Colin. (cherry picked from commit 6ae1c2d29624155d085ea213d1a8431981008c76) http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=7cafeb6405b892a964b2b759e9149bdb79e86586 Author: Ricardo Mones <ricardo@mones.org> Date: Fri May 31 08:29:39 2013 +0200 Fix variable name for the failing case (cherry picked from commit 7f3695d49fba24f50fcb58e2fc7f5991cc9009ea) http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=ee7f626c6b49641605569fc6b717fc75189fabfe Author: Ricardo Mones <ricardo@mones.org> Date: Thu May 30 18:29:07 2013 +0200 Check if detected libpython.so can be dlopen-ed A somewhat better solution for the libpython.so detection saga. Unfortunately it has not presented itself and had to be painfully extracted from a series of trial and error experiments. No living beings were harmed in the process, though. Many thanks to Paul for the help with testing this. (cherry picked from commit df8a1d7e766dea1c48a47d6a7766e20922a78cab) http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=376f87296de50d78fd4d5544cc914d9ef18da6af Author: Colin Leroy <colin@colino.net> Date: Thu May 30 14:37:19 2013 +0200 Get rid of version.in - forgotten files (cherry picked from commit 9375966944e6fef9cc85820b329c46125a93b3f3) http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=29f97d97ca340e92de68c63311b96f7e4b0ae05f Author: Colin Leroy <colin@colino.net> Date: Thu May 30 14:18:10 2013 +0200 Get rid of version.in, generate it ourselves when in a git tree Prevents automake to believe it should remove it on distclean. (cherry picked from commit a0773b18b0651f754b806ce07c61e564ffc21a8c) http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=59cc1d86335ffc052bce0bd94b2828f476ad97d1 Author: Colin Leroy <colin@colino.net> Date: Thu May 30 11:35:16 2013 +0200 Reorder send_message's port selection logic, paving the way for bug #2252 "API for external password storage" Patch by Michal Gorny (cherry picked from commit d04fd18b94a1c6fc8d22afe46367f278370011d3) http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=f1444e9e56554bcb83efe8cf4c98ec5138cf3d35 Author: Paul <paul@claws-mail.org> Date: Wed May 29 18:26:39 2013 +0100 put back this cruft until a better solution presents itself (cherry picked from commit 533722f52efcce1117503d5263efbbd4702c9c09) http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=af939383eb0ec7b279fa3af0b21bb92a7475e280 Author: Paul <paul@claws-mail.org> Date: Mon May 27 13:52:48 2013 +0100 fix locating of pythonXX.so, with thanks to Ricardo (cherry picked from commit 7a8ee419bf23dd5d84275e29cdd04bbb35d47b8d) http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=b94dcc446c466bc41e4f1e5222c516db6d70b60e Author: Colin Leroy <colin@colino.net> Date: Fri May 24 20:27:02 2013 +0200 Fix undoing file insertion. (cherry picked from commit 7e2654802e7eb97090da16bdf0fc3e4adf4fd283) http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=560b2428fbe557c007898c259701db127945dd82 Author: Ricardo Mones <ricardo@mones.org> Date: Fri May 24 14:38:28 2013 +0200 Remove every control char after colon The extraheaderrc format doesn't allow data after the header colon. Other OSes may insert extra characters other than , so, remove them all. (cherry picked from commit 6b978617931b7116397c6c625a26dc638154dd28) http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=5f682354c885a61586db637d42f8bc567564dde8 Author: Colin Leroy <colin@colino.net> Date: Thu May 23 21:52:29 2013 +0200 New big icon for compose windows, by Simon Steinbeiss (cherry picked from commit 94ed99e1acdf6a07c6cb3c01f764ab9f6e97fa77)
Changes related to this bug have been committed. Please check latest Git and update the bug accordingly. You can also get the patch from: http://git.claws-mail.org/ http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=d3c722d3f45e2280384c78994a65bfb4e093eb0a Author: Colin Leroy <colin@colino.net> Date: Thu May 30 15:50:12 2013 +0200 Add Michal to AUTHORS http://git.claws-mail.org/?p=claws.git;a=commitdiff;h=80b136e8b9436e79bfad49a053e2ae1d33521470 Author: Colin Leroy <colin@colino.net> Date: Thu May 30 15:26:00 2013 +0200 Implement a password get hooklist, allowing plugins to provide passwords for various accounts. Patch by Michal Gorki, bug #2252 "API for external password storage"