Linux: detect the native (encrypted) password store to use by default.

Linux: detect the native (encrypted) password store to use by default.
The basic, unencrypted store is still available with --password-store=basic.
BUG=25404
TEST=on gnome, kde 4, and probably xfce we should now use system-level stores

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70431

[Mike Mammarella, 2010-09-09 20:46:01]

ping

[Evan Martin, 2010-09-13 19:03:29]

CC'ing some others who might have feedback.  I think this LGTM for dev channel

http://codereview.chromium.org/3326010/diff/1/2
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/3326010/diff/1/2#newcode970
chrome/browser/profile_impl.cc:970: LOG(WARNING) << "Using basic (unencrypted)
store for password storage.";
Is it worth pointing people to somewhere with more information about this?  For
example, a thread on chromium-dev, or a wiki page?  (Keep in mind this message
will only be seen by advanced users, so getting technical isn't harmful.)

[Evan Stade, 2010-09-13 19:08:10]

lgtm

http://codereview.chromium.org/3326010/diff/1/2
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/3326010/diff/1/2#newcode944
chrome/browser/profile_impl.cc:944: } else {  // Detect the store to use
automatically.
can you add a dcheck or otherwise make sure that store_type is empty (for
future-proofing)

[Mike Mammarella, 2010-09-13 21:49:22]

http://codereview.chromium.org/3326010/diff/1/2
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/3326010/diff/1/2#newcode944
chrome/browser/profile_impl.cc:944: } else {  // Detect the store to use
automatically.
On 2010/09/13 19:08:10, Evan Stade wrote:
> can you add a dcheck or otherwise make sure that store_type is empty (for
> future-proofing)

store_type comes from the command line, that would make us die if you pass
--password-store=unknown to a debug build... I could LOG(WARNING) though, if you
like.

http://codereview.chromium.org/3326010/diff/1/2#newcode970
chrome/browser/profile_impl.cc:970: LOG(WARNING) << "Using basic (unencrypted)
store for password storage.";
On 2010/09/13 19:03:29, Evan Martin wrote:
> Is it worth pointing people to somewhere with more information about this? 
For
> example, a thread on chromium-dev, or a wiki page?  (Keep in mind this message
> will only be seen by advanced users, so getting technical isn't harmful.)

Do you have a suggestion for such a wiki page or thread, or were you implying
that I should create one? :)

[Evan Martin, 2010-09-13 21:51:17]

http://codereview.chromium.org/3326010/diff/1/2
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/3326010/diff/1/2#newcode970
chrome/browser/profile_impl.cc:970: LOG(WARNING) << "Using basic (unencrypted)
store for password storage.";
On 2010/09/13 21:49:22, Mike Mammarella wrote:
> On 2010/09/13 19:03:29, Evan Martin wrote:
> > Is it worth pointing people to somewhere with more information about this? 
> For
> > example, a thread on chromium-dev, or a wiki page?  (Keep in mind this
message
> > will only be seen by advanced users, so getting technical isn't harmful.)
> 
> Do you have a suggestion for such a wiki page or thread, or were you implying
> that I should create one? :)

The latter.  Do you expect this flag to be used by real users?  Or is it for
debugging?  In the former case, maybe it belongs in the manpage (look for a file
named "manpage" in the tree); in the latter, you can leave it.

[Mike Mammarella, 2010-09-13 22:19:49]

On 2010/09/13 21:51:17, Evan Martin wrote:
> http://codereview.chromium.org/3326010/diff/1/2
> File chrome/browser/profile_impl.cc (right):
> 
> http://codereview.chromium.org/3326010/diff/1/2#newcode970
> chrome/browser/profile_impl.cc:970: LOG(WARNING) << "Using basic (unencrypted)
> store for password storage.";
> On 2010/09/13 21:49:22, Mike Mammarella wrote:
> > On 2010/09/13 19:03:29, Evan Martin wrote:
> > > Is it worth pointing people to somewhere with more information about this?

> > For
> > > example, a thread on chromium-dev, or a wiki page?  (Keep in mind this
> message
> > > will only be seen by advanced users, so getting technical isn't harmful.)
> > 
> > Do you have a suggestion for such a wiki page or thread, or were you
implying
> > that I should create one? :)
> 
> The latter.  Do you expect this flag to be used by real users?  Or is it for
> debugging?  In the former case, maybe it belongs in the manpage (look for a
file
> named "manpage" in the tree); in the latter, you can leave it.

I don't really expect most users to use the flag, no. It's mostly for debugging,
but also for the 100 or so people in the world using tiling window managers...
;)

However, this message can show even if you are not using the flag: it shows if
there is an error initializing the detected store, or if there is an error
detecting the store (for example, if you are on an unsupported desktop
environment).

But in those cases it's probably worth having some tidbit of info sort of like
the Linux proxy settings wiki page; I'll look into making such a page.

[Mike Mammarella, 2010-09-15 00:33:07]

http://codereview.chromium.org/3326010/diff/9001/10001
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/3326010/diff/9001/10001#newcode974
chrome/browser/profile_impl.cc:974: "See
http://code.google.com/p/chromium/wiki/LinuxPasswordStorage for "
What do you think of this page?

[Evan Stade, 2010-09-15 00:58:00]

page looks good (I made a couple edits)

[Mike Mammarella, 2010-09-17 01:22:36]

I'm actually going to hold off just a bit more on this since I've discovered a
bug: GNOME Keyring can get into a weird state if you try to use it from a
running deleted binary after an upgrade, because the /proc/.../exe link has "
(deleted)" in it which confuses the keyring daemon. (The browser ends up
crashing as a result.) The solution would seem to be to explicitly create all
keyring entries with both the deleted and non-deleted binary names allowed to
access them, but that'll require some more work.

[Evan Martin, 2010-09-17 22:53:36]

Why are we accessing the keyring from a /proc/.../exe link?

[Mike Mammarella, 2010-09-17 23:03:07]

On 2010/09/17 22:53:36, Evan Martin wrote:
> Why are we accessing the keyring from a /proc/.../exe link?

The gnome keyring daemon receives, via DBus, our PID and looks it up itself in
order to authenticate whether this particular client is allowed to access the
keyring items it's requesting.

[Evan Martin, 2010-09-17 23:07:34]

On 2010/09/17 23:03:07, Mike Mammarella wrote:
> The gnome keyring daemon receives, via DBus, our PID and looks it up itself in
> order to authenticate whether this particular client is allowed to access the
> keyring items it's requesting.

So this problem (deleted executables) happens for any app that is deleted?  I
think we should file a bug with gnome-password and just ignore the problem.

[Mike Mammarella, 2010-09-17 23:20:06]

On 2010/09/17 23:07:34, Evan Martin wrote:
> On 2010/09/17 23:03:07, Mike Mammarella wrote:
> > The gnome keyring daemon receives, via DBus, our PID and looks it up itself
in
> > order to authenticate whether this particular client is allowed to access
the
> > keyring items it's requesting.
> 
> So this problem (deleted executables) happens for any app that is deleted?  I
> think we should file a bug with gnome-password and just ignore the problem.

Yes, this would happen for any app, as far as I can tell. But it does crash the
browser, for a reason I haven't been able to pin down yet. It also sends
gnome-keyring-daemon into a weird state, and the browser continues to crash just
trying to talk to it until you restart gnome-keyring-daemon. We could sidestep
the issue by just setting both binary names as allowed to access the keyring
entries though.

Either way I'll go file a bug, good idea.

[Mike Mammarella, 2010-12-30 01:50:19]

On 2010/09/17 23:20:06, Mike Mammarella wrote:
> On 2010/09/17 23:07:34, Evan Martin wrote:
> > On 2010/09/17 23:03:07, Mike Mammarella wrote:
> > > The gnome keyring daemon receives, via DBus, our PID and looks it up
itself
> in
> > > order to authenticate whether this particular client is allowed to access
> the
> > > keyring items it's requesting.
> > 
> > So this problem (deleted executables) happens for any app that is deleted? 
I
> > think we should file a bug with gnome-password and just ignore the problem.
> 
> Yes, this would happen for any app, as far as I can tell. But it does crash
the
> browser, for a reason I haven't been able to pin down yet. It also sends
> gnome-keyring-daemon into a weird state, and the browser continues to crash
just
> trying to talk to it until you restart gnome-keyring-daemon. We could sidestep
> the issue by just setting both binary names as allowed to access the keyring
> entries though.
> 
> Either way I'll go file a bug, good idea.

So it turns out the bug I found has been fixed since 2009 (they've just removed
ACLs altogether); it affects Ubuntu Hardy and some older systems, but anything
more recent is fine. I've dusted off this CL and it's ready to go again. Want to
have a quick look at it before I submit it?

[Evan Martin, 2011-01-04 17:44:07]

LGTM