GNOME Bugzilla – Bug 704956
asks for keyring password
Last modified: 2016-03-01 17:11:59 UTC
after creating a google account at goa page, a popup is raised asking the user to enter a password for the default keyring.
it seems that when goa is storing user's credentials, there's no keyring created. so, gnome-keyring-daemon creates a default one. I've cooked a patch that works, but I'm not sure if it's the right approach.
Created attachment 250239 [details] [review] proposed patch, draft this patch creates a keyring called 'login' after creating the user, using the user password. it also starts the gnome-keyring-daemon at startup so that the above step works. what do you think? is this approach valid?
Stef, can you comment here?
Jasper and I talked about this at GUADEC. gnome-initial-setup needs to run the equivalent of: $ echo 'password' | gnome-keyring-daemon --login $ gnome-keyring-daemon --start --components=secrets Yes, two commands. This is similar to what pam_gnome_keyring (does the first step) and autostart (does the second step) do on usual startup. Alternatively I'm happy for patches to gnome-keyring-daemon to make this case easier.
I tested the patch in Comment #2; after gnome-initial-setup finishes and the new user's session starts, gnome-initial-setup-copy-worker is started from /etc/xdg/autostart and attempts to copy login.keyring but fails: WARNING **: Unable to move /run/gnome-initial-setup/.local/share/keyrings/login.keyring to /home/XXX/.local/share/keyrings/login.keyring: Target file exists "/usr/bin/gnome-keyring-daemon --start --components=secrets" is also started from /etc/xdg/autostart/; how do we guarantee that gnome-keyring-daemon starts only when gnome-initial-setup-copy-worker finished to move the files?
yep, this is not a final patch, I've attached it here only for comments. this issue happens because on startup, keyring daemon starts before than g-i-s worker copy. we need to pass the flag OVERWRITE in order to make it work. however, I'd remove that stuff from xdg autostart and copy the files immediately when g-i-s finishes...
(In reply to comment #6) > yep, this is not a final patch, I've attached it here only for comments. > > this issue happens because on startup, keyring daemon starts before than g-i-s > worker copy. we need to pass the flag OVERWRITE in order to make it work. No, because we want the keys from the login keyring that pam_gnome_keyring creates as well. What we want is a way to merge the keyring from g-i-s into the keyring created by pam_gnome_keyring.
No need to merge keyrings. pam_gnome_keyring.so won't create a login keyring if one already exits. If you want to use a specific keyring as the login keyring, make sure it's master password is the user's password, and then place it as $XDG_DATA_HOME/keyrings/login.keyring But realistically, you just need to do what I said above, right when the user chose their password. Don't do this at the end, do it right when the user chooses their password. > $ echo 'password' | gnome-keyring-daemon --login > $ gnome-keyring-daemon --start --components=secrets So much of this confusion is because of the way g-i-s tries to perform all the actions at the end. That is broken in various ways, and this is one of them. Start gnome-keyring-daemon (as outlined above) after the user chooses their password, but before they use GOA pages or anything else like that. > "/usr/bin/gnome-keyring-daemon --start --components=secrets" is also started > from /etc/xdg/autostart/; how do we guarantee that gnome-keyring-daemon starts > only when gnome-initial-setup-copy-worker finished to move the files? Ah, so at the very beginning of first boot is /etc/xdg/autostart running? Jasper, does it really make sense to run /etc/xdg/autostart during the gnome-initial-setup user session?
(In reply to comment #8) > No need to merge keyrings. GOA uses gnome-keyring in the gnome-initial-setup session to save service passwords. We need to merge that with the keyring generated by pam_gnome_keyring. > pam_gnome_keyring.so won't create a login keyring if one already exits. If you > want to use a specific keyring as the login keyring, make sure it's master > password is the user's password, and then place it as > $XDG_DATA_HOME/keyrings/login.keyring > > But realistically, you just need to do what I said above, right when the user > chose their password. Don't do this at the end, do it right when the user > chooses their password. I thought we were going to rely on pam_gnome_keyring. Do you want me to create a new login keyring with the user's password by starting gnome-keyring in the gnome-initial-setup session when the user chooses his password, and then have GOA rely on that? > Ah, so at the very beginning of first boot is /etc/xdg/autostart running? > Jasper, does it really make sense to run /etc/xdg/autostart during the > gnome-initial-setup user session? We don't run /etc/xdg/autostart in a greeter session. He's saying that sometimes gnome-session runs gnome-keyring after the copy-worker in the user session, meaning that there's a race where potentially the login.keyring won't be copied in time.
(In reply to comment #9) > (In reply to comment #8) > > No need to merge keyrings. > > GOA uses gnome-keyring in the gnome-initial-setup session to save service > passwords. We need to merge that with the keyring generated by > pam_gnome_keyring. We just want them to use the same keyring. GOA will use whatever is the default keyring. > > pam_gnome_keyring.so won't create a login keyring if one already exits. If you > > want to use a specific keyring as the login keyring, make sure it's master > > password is the user's password, and then place it as > > $XDG_DATA_HOME/keyrings/login.keyring > > > > But realistically, you just need to do what I said above, right when the user > > chose their password. Don't do this at the end, do it right when the user > > chooses their password. > > I thought we were going to rely on pam_gnome_keyring. > Do you want me to create > a new login keyring with the user's password by starting gnome-keyring in the Yes, you can do this by starting gnome-keyring-daemon as I noted above: $ echo 'password' | gnome-keyring-daemon --login $ gnome-keyring-daemon --start --components=secrets > gnome-initial-setup session when the user chooses his password, and then have > GOA rely on that? Yes. GOA will just use the default keyring, which will be the 'login.keyring'
(In reply to comment #9) > I thought we were going to rely on pam_gnome_keyring. Do you want me to create > a new login keyring with the user's password by starting gnome-keyring in the > gnome-initial-setup session when the user chooses his password, and then have > GOA rely on that? In that case, the file will be: /run/gnome-initial-setup/.local/share/keyrings/login.keyring and will belong to the temporary user 'gnome-initial-setup'. The copy-worker does not run as user 'gnome-initial-setup' but as the new user. How does it get read access to the keyring file? Should it be world-readable? It seems reasonable because this part of gnome-initial-setup only runs when there was no users. But in my test, /run/gnome-initial-setup/.local had drwx------ rights.
Created attachment 251771 [details] [review] [PATCH] Options: accept --login with --start or --replace gnome-initial-setup needs to start gnome-keyring-daemon and gives a login password but we are already in the proper environment because gnome-keyring would not be started from PAM. Instead of running the two commands from gnome-initial-setup: $ echo 'password' | gnome-keyring-daemon --login $ gnome-keyring-daemon --start --components=secrets gnome-keyring can accept a single command: $ echo 'password' | gnome-keyring-daemon --login --start --components=secrets
Created attachment 251772 [details] [review] [PATCH] new account: start gnome-keyring and give the password When a new user is created, start gnome-keyring like: $ echo "password" | gnome-keyring-daemon --login --start --foreground --components secrets The last gnome-keyring accepts both '--login' and '--start' at the same time: it was added for gnome-initial-setup's needs. It is useful for the online accounts page: OAuth tokens will be stored in /run/gnome-initial-setup/.local/share/keyrings/login.keyring encrypted with the user's password. --- .../pages/account/gis-account-page.c | 55 ++++++++++++++++++++ 1 file changed, 55 insertions(+)
With the 2 patches above (respectively on gnome-keyring and gnome-initial-setup), there are no popups asking for the password. It still does not fully work because of the 'xdg autostart' race mentioned before, but I verified that the file login.keyring (created during the initial setup) contains the correct credentials by copying it manually instead of relying on gnome-initial-setup-copy-worker.
(In reply to comment #11) > (In reply to comment #9) > > > I thought we were going to rely on pam_gnome_keyring. Do you want me to create > > a new login keyring with the user's password by starting gnome-keyring in the > > gnome-initial-setup session when the user chooses his password, and then have > > GOA rely on that? > > In that case, the file will be: > /run/gnome-initial-setup/.local/share/keyrings/login.keyring > and will belong to the temporary user 'gnome-initial-setup'. > > The copy-worker does not run as user 'gnome-initial-setup' but as the new user. > How does it get read access to the keyring file? Should it be world-readable? > It seems reasonable because this part of gnome-initial-setup only runs when > there was no users. But in my test, /run/gnome-initial-setup/.local had > drwx------ rights. Jasper, are there other places where g-i-s runs into the same problem? Seems like there needs to be a general fix for fixing permissions of files you're handing into the new user account, no?
(In reply to comment #15) > Jasper, are there other places where g-i-s runs into the same problem? Seems > like there needs to be a general fix for fixing permissions of files you're > handing into the new user account, no? I didn't notice that GDM was doing a recursive chown on /run/gnome-initial-setup: daemon/gdm-simple-slave.c: if (slave->priv->doing_initial_setup) { chown_initial_setup_home_dir (); } I suggest to copy the files right after the recursive chown instead of using xdg autostart. Not sure whether to exec gnome-initial-setup-copy-worker or to include ~50 lines. Would it work fine?
(In reply to comment #16) > I suggest to copy the files right after the recursive chown instead of using > xdg autostart. Not sure whether to exec gnome-initial-setup-copy-worker or to > include ~50 lines. Would it work fine? No. That code takes place when setting up the session where gnome-initial-setup runs. gnome-initial-setup-copy-worker takes place in the user session after login.
Created attachment 252287 [details] [review] [PATCHES] gnome-session: add new phase: GSM_MANAGER_PHASE_INITIAL_SETUP gnome-initial-setup-copy-worker and gnome-keyring (and others) used to be started automatically by the session manager during the GSM_MANAGER_PHASE_INITIALIZATION phase. This is racy because gnome-keyring could read its file ~/.local/share/keyrings/login.keyring before it is written by gnome-initial-setup-copy-worker. The correct solution is to wait gnome-initial-setup-copy-worker finishes before starting other components in the initialization phase. I want to enforce this order without adding knowledge of gnome-initial-setup in gnome-keyring and others. It must also work on systems which don't use gnome-initial-setup. So I don't use a flag file in gnome-keyring's AutostartCondition to delay its startup. Instead, I add a new phase GSM_MANAGER_PHASE_INITIAL_SETUP before the initialization phase. If no autostart desktop files use that phase, gnome-session will just go on to the next phase. But if gnome-initial-setup-copy-worker uses this new phase, it will ensure gnome-initial-setup-copy-worker finishes before the newly installed files in $HOME are read by anyone. Relevent documentation: https://wiki.gnome.org/SessionManagement http://standards.freedesktop.org/autostart-spec/autostart-spec-latest.html http://lists.freedesktop.org/archives/xdg/2007-January/007436.html
Created attachment 252290 [details] [review] [PATCH] copy-worker: use gnome-session's new InitialSetup phase With this patch and the previous patches, it does not ask for the keyring password and it installs the login keyring without race condition.
Review of attachment 251772 [details] [review]: Isn't modifying gnome-keyring, then relying on that modification, introducing rather more "moving parts" than we really need? It doesn't look as though it would be difficult to do two separate invocations, which has the advantage of being exactly what would happen "in real life" via PAM + autostart. ::: gnome-initial-setup/pages/account/gis-account-page.c @@ +462,3 @@ + NULL, /* envp */ + G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL + | G_SPAWN_STDERR_TO_DEV_NULL, Shouldn't we be letting gnome-keyring-daemon's stderr (and possibly also its stdout) pass through to the log/journal/wherever our stderr is pointing?
Review of attachment 251771 [details] [review]: This seems fairly confusing to me, but I can't really think of a better way to do it... Previously, we had four orthogonal modes: --start, --login, --replace and "none of the above". --login was the first half of a two-phase start, doing as little as possible for the reasons quoted at the top of main(), and stays in the foreground. --start would try to do the second half of a two-phase start, falling back to normal startup if --login hadn't been done, and daemonizes. "no option" always does normal startup, and daemonizes. --replace tries to replace an existing daemon, falling back to normal startup if there isn't one, and daemonizes. With this patch, we also have: --login --start reads the password from stdin, does not try to interact with an existing instance, and daemonizes. ... which doesn't really behave a whole lot like either --login or --start, which seems odd to me.
Review of attachment 252287 [details] [review]: This seems to be two patches concatenated, which confuses Splinter. Technically, it looks fine to me, but perhaps EARLY_INITIALIZATION (time-based) or POPULATE_HOME (function-based) would be a better name than INITIAL_SETUP - if you asked me out-of-context "which comes first, initial setup or initialization?" or even "what's the difference between initial setup or initialization?", I wouldn't be able to tell you. If INITIALIZATION wasn't already API (via it appearing in .desktop files) I'd suggest renaming it to DAEMONS. Actually, that might not be a bad idea - the old name in .desktop files could still be recognized as a synonym?
(In reply to comment #20) > Review of attachment 251772 [details] [review]: > > Isn't modifying gnome-keyring, then relying on that modification, introducing > rather more "moving parts" than we really need? It doesn't look as though it > would be difficult to do two separate invocations, which has the advantage of > being exactly what would happen "in real life" via PAM + autostart. During the first invocation (with --login), gnome-keyring prints the following on stdout: GNOME_KEYRING_CONTROL=/run/user/xxxx/keyring-xxxxxx GNOME_KEYRING_PID=xxxx The output needs to be parsed and the environment variable set in order to make the second invocation (with --start) successful. It is done by the PAM module: https://git.gnome.org/browse/gnome-keyring/tree/pam/gkr-pam-module.c#n555 When gnome-keyring is started in a single command, it works without setting the environment variables: gnome-initial-setup+libsecrets talks to gnome-keyring through D-Bus without using the environment variables. So I would prefer to avoid copying the code for parsing the environment variables from gnome-keyring's PAM module into gnome-initial-setup. > ::: gnome-initial-setup/pages/account/gis-account-page.c > @@ +462,3 @@ > + NULL, /* envp */ > + G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL > + | G_SPAWN_STDERR_TO_DEV_NULL, > > Shouldn't we be letting gnome-keyring-daemon's stderr (and possibly also its > stdout) pass through to the log/journal/wherever our stderr is pointing? Right, I should do that for stderr. stdout contains the environment variables. If we stay if the single invocation, should we send stdout to log/journal/wherever?
Comment on attachment 252287 [details] [review] [PATCHES] gnome-session: add new phase: GSM_MANAGER_PHASE_INITIAL_SETUP I created Bug #706692 for the two gnome-session patches. I renamed the phase to EARLY_INITIALIZATION as suggested by Simon. I didn't name it DAEMON because it would be confusing if we ever fix Bug #563642.
Review of attachment 251771 [details] [review]: The use case makes sense, but --start is supposed to initialize an already running daemon. We shouldn't need to use --start, if we're starting the daemon directly. Lets use another option, such as --unlock ::: daemon/gkd-main.c @@ +952,2 @@ /* The --replace option */ + } else if (run_for_replace && !run_for_login) { With this patch: $ echo -n "password" | gnome-keyring-daemon --replace --foreground Does not replace previous invocation of daemon. Will make a change.
Bug #710187 has a patch for gnome-keyring. Could you test it in the context of this work? The gnome-keyring-daemon invocation would need to be changed to be run like this: $ echo -n "password" | gnome-keyring-daemon --daemonize --components=secrets --unlock Note the absense of a newline written to stdin.
Thanks for your reviews and new patches. Unfortunately I will not have time to work more on this bug.
*** This bug has been marked as a duplicate of bug 719458 ***
Sorry, marked it the wrong way round.
*** Bug 719458 has been marked as a duplicate of this bug. ***
This bug is going to be shipped in Fedora 20 Final, as it stands. That seems unfortunate. Note that even if you go ahead and create a keyring as g-i-s wants you to do, the account you configure will not work once you're logged into your user session: this bug effectively breaks the feature entirely.
(In reply to comment #31) > This bug is going to be shipped in Fedora 20 Final, as it stands. That seems > unfortunate. Note that even if you go ahead and create a keyring as g-i-s wants > you to do, the account you configure will not work once you're logged into your > user session: this bug effectively breaks the feature entirely. Maybe we should disable the Online Accounts page for F20 if we can not fix this time?
That might be a good idea if we can't fix it; right now that function of g-i-s is essentially a booby trap. You can't possibly get a properly-working online account out of it, and there is at least one relatively easy-to-hit crasher bug in it, https://bugzilla.gnome.org/show_bug.cgi?id=711641 , which gives you a pretty bad experience if you hit it. Anyhow, we should probably discuss that downstream.
Current approach to fixing this is: https://git.gnome.org/browse/gnome-initial-setup/log/?h=keyring
this is included in 3.11.91
I encountered this bug while testing 3.19.x with a GNOME continuous image from four days ago: after adding a Google account, a dialog pops up asking me to create a keyring password.
*** Bug 761754 has been marked as a duplicate of this bug. ***
Probably bug 758592
This shoudl be fixed with the gnome-keyring change to allow empty passwords for --unlock
I just tested an up to date continuous image, and this bug was still present.
I just tried a fresh continuous image, and the bug didn't appear when adding a Google account.
ok, so one of my fixes worked