Add a dummy entry with libsecret when initializing OSCrypt.

Add a dummy entry with libsecret when initializing OSCrypt.

Under certain conditions, gnome libsecret methods that perform a lookup
in keyring may ignore a locked keyring and return empty results, instead
of unlocking said keyring. Store operations are unaffected by this quirk
(or bug, presumably).

With this change, we store a dummy entry during the initialisation of
OSCrypt. This ensures that the default keyring gets unlocked and is
available for future reads.

This solution is also applied to Password Manager, before performing
calls to secret_service_search_sync.

This solution should be temporary, until the underlying issue gets fixed
or there is an official workaround.

BUG=660005, 631171
Committed: https://crrev.com/f3a8dfe38fab661729155dac82c8ab7d8422c2e1
Cr-Commit-Position: refs/heads/master@{#428378}

[cfroussios, 2016-10-27 17:25:59]

The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 17:26:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cfroussios, 2016-10-27 17:31:25]

The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 17:32:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 18:50:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 18:50:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[cfroussios, 2016-10-28 08:45:25]

The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 08:45:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cfroussios, 2016-10-28 09:00:16]

Description was changed from

==========
Add a dummy entry with libsecret when initializing OSCrypt.

Under certain conditions, gnome libsecret methods that perform a lookup
in keyring may ignore a locked keyring and return empty results, instead
of unlocking said keyring. Store operations are unaffected by this quirk
(or bug).

With this change, we store a dummy entry during the initialisation of
OSCrypt. This ensures that the default keyring gets unlocked and is
available for future reads.

This solution does not need to be applied to Password Manager, because
its reaction to this situation is to repopulate the apparently empty
keyring. Thus, the store operations will unlock the keyring.

This is a temporary solution, until the underlying issue gets fixed or
until we get feedback on a better way to address the problem.

BUG=660005,631171
==========

to

==========
Add a dummy entry with libsecret when initializing OSCrypt.

Under certain conditions, gnome libsecret methods that perform a lookup
in keyring may ignore a locked keyring and return empty results, instead
of unlocking said keyring. Store operations are unaffected by this quirk
(or bug).

With this change, we store a dummy entry during the initialisation of
OSCrypt. This ensures that the default keyring gets unlocked and is
available for future reads.

This solution does not need to be applied to Password Manager, because
its reaction to this situation is to repopulate the apparently empty
keyring. Thus, the store operations will unlock the keyring.

This solution should be temporary, until the underlying issue gets fixed
or there is an official workaround.

BUG=660005,631171
==========

[cfroussios, 2016-10-28 09:01:13]

Description was changed from

==========
Add a dummy entry with libsecret when initializing OSCrypt.

Under certain conditions, gnome libsecret methods that perform a lookup
in keyring may ignore a locked keyring and return empty results, instead
of unlocking said keyring. Store operations are unaffected by this quirk
(or bug).

With this change, we store a dummy entry during the initialisation of
OSCrypt. This ensures that the default keyring gets unlocked and is
available for future reads.

This solution does not need to be applied to Password Manager, because
its reaction to this situation is to repopulate the apparently empty
keyring. Thus, the store operations will unlock the keyring.

This solution should be temporary, until the underlying issue gets fixed
or there is an official workaround.

BUG=660005,631171
==========

to

==========
Add a dummy entry with libsecret when initializing OSCrypt.

Under certain conditions, gnome libsecret methods that perform a lookup
in keyring may ignore a locked keyring and return empty results, instead
of unlocking said keyring. Store operations are unaffected by this quirk
(or bug, presumably).

With this change, we store a dummy entry during the initialisation of
OSCrypt. This ensures that the default keyring gets unlocked and is
available for future reads.

This solution does not need to be applied to Password Manager, because
its reaction to this situation is to repopulate the apparently empty
keyring. Thus, the store operations will unlock the keyring.

This solution should be temporary, until the underlying issue gets fixed
or there is an official workaround.

BUG=660005,631171
==========

[cfroussios, 2016-10-28 09:04:55]

cfroussios@chromium.org changed reviewers:
+ vasilii@chromium.org

[vasilii, 2016-10-28 09:10:59]

Feedback on the description only:
- http://crbug.com/660005 should link to the external bug for Keyring.
- I don't get the explanation regarding the password manager. What would it
repopulate back if it has read 0 passwords? Keyring isn't a backup or similar.

[cfroussios, 2016-10-28 09:17:59]

On 2016/10/28 09:10:59, vasilii wrote:
> Feedback on the description only:
> - http://crbug.com/660005 should link to the external bug for Keyring.
> - I don't get the explanation regarding the password manager. What would it
> repopulate back if it has read 0 passwords? Keyring isn't a backup or similar.

I will create a bug in gnome and update issue http://crbug.com/660005, once I
clean up the code snippet that demonstrates the problem.

Regarding password manager, my observation was that if it fails to read stuff in
the beginning, it will attempt to write things, thus unlocking the keyring. Did
I misunderstand? It should not be a problem to add
LibsecretLoader::EnsureKeyringUnlocked() in Password Manager too.

[commit-bot: I haz the power, 2016-10-28 09:25:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 09:25:16]

Dry run: This issue passed the CQ dry run.

[vasilii, 2016-10-28 09:36:18]

On 2016/10/28 09:17:59, cfroussios wrote:
> On 2016/10/28 09:10:59, vasilii wrote:
> > Feedback on the description only:
> > - http://crbug.com/660005 should link to the external bug for Keyring.
> > - I don't get the explanation regarding the password manager. What would it
> > repopulate back if it has read 0 passwords? Keyring isn't a backup or
similar.
> 
> I will create a bug in gnome and update issue http://crbug.com/660005, once I
> clean up the code snippet that demonstrates the problem.
> 
> Regarding password manager, my observation was that if it fails to read stuff
in
> the beginning, it will attempt to write things, thus unlocking the keyring.
Did
> I misunderstand? It should not be a problem to add
> LibsecretLoader::EnsureKeyringUnlocked() in Password Manager too.

What things would it write? The Keyring is the only place that had your
passwords.

[vasilii, 2016-10-28 09:36:24]

https://codereview.chromium.org/2461453002/diff/40001/components/os_crypt/key...
File components/os_crypt/key_storage_libsecret.cc (right):

https://codereview.chromium.org/2461453002/diff/40001/components/os_crypt/key...
components/os_crypt/key_storage_libsecret.cc:76:
LibsecretLoader::EnsureKeyringUnlocked();
How often is this method called? Is it enough to unlock it once?

[cfroussios, 2016-10-28 11:08:15]

The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 11:08:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cfroussios, 2016-10-28 11:20:57]

On 2016/10/28 09:36:18, vasilii wrote:
> On 2016/10/28 09:17:59, cfroussios wrote:
> > On 2016/10/28 09:10:59, vasilii wrote:
> > > Feedback on the description only:
> > > - http://crbug.com/660005 should link to the external bug for Keyring.
> > > - I don't get the explanation regarding the password manager. What would
it
> > > repopulate back if it has read 0 passwords? Keyring isn't a backup or
> similar.
> > 
> > I will create a bug in gnome and update issue http://crbug.com/660005, once
I
> > clean up the code snippet that demonstrates the problem.
> > 
> > Regarding password manager, my observation was that if it fails to read
stuff
> in
> > the beginning, it will attempt to write things, thus unlocking the keyring.
> Did
> > I misunderstand? It should not be a problem to add
> > LibsecretLoader::EnsureKeyringUnlocked() in Password Manager too.
> 
> What things would it write? The Keyring is the only place that had your
> passwords.

The passwords, after sync fetches them again? Point taken. I've added ensure to
Password Manager.

Note, EnsureDefaultUnlocked() does not go in Init() in password manager because
it then blocks the Chrome window from starting.

[cfroussios, 2016-10-28 11:21:22]

https://codereview.chromium.org/2461453002/diff/40001/components/os_crypt/key...
File components/os_crypt/key_storage_libsecret.cc (right):

https://codereview.chromium.org/2461453002/diff/40001/components/os_crypt/key...
components/os_crypt/key_storage_libsecret.cc:76:
LibsecretLoader::EnsureKeyringUnlocked();
On 2016/10/28 09:36:24, vasilii wrote:
> How often is this method called? Is it enough to unlock it once?

GetKey() is called once and its result will be cached.

We should only need to unlock the keyring once, and not expect that someone
would re-lock it. I've moved it to Init(). That doesn't have a practical effect,
but should remove the confusion.

[commit-bot: I haz the power, 2016-10-28 11:47:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 11:47:30]

Dry run: This issue passed the CQ dry run.

[vasilii, 2016-10-28 12:55:55]

On 2016/10/28 11:20:57, cfroussios wrote:
> On 2016/10/28 09:36:18, vasilii wrote:
> > On 2016/10/28 09:17:59, cfroussios wrote:
> > > On 2016/10/28 09:10:59, vasilii wrote:
> > > > Feedback on the description only:
> > > > - http://crbug.com/660005 should link to the external bug for Keyring.
> > > > - I don't get the explanation regarding the password manager. What would
> it
> > > > repopulate back if it has read 0 passwords? Keyring isn't a backup or
> > similar.
> > > 
> > > I will create a bug in gnome and update issue http://crbug.com/660005,
once
> I
> > > clean up the code snippet that demonstrates the problem.
> > > 
> > > Regarding password manager, my observation was that if it fails to read
> stuff
> > in
> > > the beginning, it will attempt to write things, thus unlocking the
keyring.
> > Did
> > > I misunderstand? It should not be a problem to add
> > > LibsecretLoader::EnsureKeyringUnlocked() in Password Manager too.
> > 
> > What things would it write? The Keyring is the only place that had your
> > passwords.
> 
> The passwords, after sync fetches them again? Point taken. I've added ensure
to
> Password Manager.
> 
> Note, EnsureDefaultUnlocked() does not go in Init() in password manager
because
> it then blocks the Chrome window from starting.

Sync is optional.
Don't other components block Chrome the same way? On which thread the Keyring is
accessed?

[vasilii, 2016-10-28 12:56:06]

https://codereview.chromium.org/2461453002/diff/60001/components/os_crypt/lib...
File components/os_crypt/libsecret_util_linux.cc (right):

https://codereview.chromium.org/2461453002/diff/60001/components/os_crypt/lib...
components/os_crypt/libsecret_util_linux.cc:27: bool s_ensured_keyring_unlocked
= false;
Are you thread-safe?
I'm not a big fan of globals. Is EnsureKeyringUnlocked called very often?
Also I'd like to protect against the cases when Keyring is locked not on
start-up but later.

https://codereview.chromium.org/2461453002/diff/60001/components/os_crypt/lib...
components/os_crypt/libsecret_util_linux.cc:138: "properly.";
Do you need this print on every startup?

[cfroussios, 2016-10-28 13:50:02]

On 2016/10/28 12:55:55, vasilii wrote:
> On 2016/10/28 11:20:57, cfroussios wrote:
> > On 2016/10/28 09:36:18, vasilii wrote:
> > > On 2016/10/28 09:17:59, cfroussios wrote:
> > > > On 2016/10/28 09:10:59, vasilii wrote:
> > > > > Feedback on the description only:
> > > > > - http://crbug.com/660005 should link to the external bug for Keyring.
> > > > > - I don't get the explanation regarding the password manager. What
would
> > it
> > > > > repopulate back if it has read 0 passwords? Keyring isn't a backup or
> > > similar.
> > > > 
> > > > I will create a bug in gnome and update issue http://crbug.com/660005,
> once
> > I
> > > > clean up the code snippet that demonstrates the problem.
> > > > 
> > > > Regarding password manager, my observation was that if it fails to read
> > stuff
> > > in
> > > > the beginning, it will attempt to write things, thus unlocking the
> keyring.
> > > Did
> > > > I misunderstand? It should not be a problem to add
> > > > LibsecretLoader::EnsureKeyringUnlocked() in Password Manager too.
> > > 
> > > What things would it write? The Keyring is the only place that had your
> > > passwords.
> > 
> > The passwords, after sync fetches them again? Point taken. I've added ensure
> to
> > Password Manager.
> > 
> > Note, EnsureDefaultUnlocked() does not go in Init() in password manager
> because
> > it then blocks the Chrome window from starting.
> 
> Sync is optional.
> Don't other components block Chrome the same way? On which thread the Keyring
is
> accessed?

Password Manager is doing search and store from the DB thread. In the past,
that's where password-manager would block on the prompt (if it does get
triggered).

Init() does call libsecret, but currently it intentionally does not trigger a
prompt. It is not necessary to unlock the keyring that early. If we do, it
blocks the Chrome window from starting.

I do not know how other components initialise themselves. I would assume on the
main thread, but they don't block on prompts.

[cfroussios, 2016-10-28 13:50:12]

https://codereview.chromium.org/2461453002/diff/60001/components/os_crypt/lib...
File components/os_crypt/libsecret_util_linux.cc (right):

https://codereview.chromium.org/2461453002/diff/60001/components/os_crypt/lib...
components/os_crypt/libsecret_util_linux.cc:27: bool s_ensured_keyring_unlocked
= false;
On 2016/10/28 12:56:06, vasilii wrote:
> Are you thread-safe?
> I'm not a big fan of globals. Is EnsureKeyringUnlocked called very often?
> Also I'd like to protect against the cases when Keyring is locked not on
> start-up but later.

EnsureDefaultUnlocked() is not necessary if a keyring is re-locked. The regular
calls will trigger the necessary prompt (I fixed that in
https://codereview.chromium.org/2441653002). EnsureDefaultUnlocked() is needed
specifically to address the problem where secret_service_search will ignore a
keyring if it's never opened since rebooting.

Calling EnsureDefaultUnlocked() more than once is redundant. The reason for the
s_ensured_keyring_unlocked is to reduce unnecessary interactions with libsecret.
We can do the same thing from the side of the Password Manager if you think it's
more appropriate.

[cfroussios, 2016-10-28 13:54:22]

Description was changed from

==========
Add a dummy entry with libsecret when initializing OSCrypt.

Under certain conditions, gnome libsecret methods that perform a lookup
in keyring may ignore a locked keyring and return empty results, instead
of unlocking said keyring. Store operations are unaffected by this quirk
(or bug, presumably).

With this change, we store a dummy entry during the initialisation of
OSCrypt. This ensures that the default keyring gets unlocked and is
available for future reads.

This solution does not need to be applied to Password Manager, because
its reaction to this situation is to repopulate the apparently empty
keyring. Thus, the store operations will unlock the keyring.

This solution should be temporary, until the underlying issue gets fixed
or there is an official workaround.

BUG=660005,631171
==========

to

==========
Add a dummy entry with libsecret when initializing OSCrypt.

Under certain conditions, gnome libsecret methods that perform a lookup
in keyring may ignore a locked keyring and return empty results, instead
of unlocking said keyring. Store operations are unaffected by this quirk
(or bug, presumably).

With this change, we store a dummy entry during the initialisation of
OSCrypt. This ensures that the default keyring gets unlocked and is
available for future reads.

This solution is also applied to Password Manager, before performing
calls to secret_service_search_sync.

This solution should be temporary, until the underlying issue gets fixed
or there is an official workaround.

BUG=660005,631171
==========

[vasilii, 2016-10-28 14:17:48]

https://codereview.chromium.org/2461453002/diff/60001/components/os_crypt/lib...
File components/os_crypt/libsecret_util_linux.cc (right):

https://codereview.chromium.org/2461453002/diff/60001/components/os_crypt/lib...
components/os_crypt/libsecret_util_linux.cc:27: bool s_ensured_keyring_unlocked
= false;
On 2016/10/28 13:50:12, cfroussios wrote:
> On 2016/10/28 12:56:06, vasilii wrote:
> > Are you thread-safe?
> > I'm not a big fan of globals. Is EnsureKeyringUnlocked called very often?
> > Also I'd like to protect against the cases when Keyring is locked not on
> > start-up but later.
> 
> EnsureDefaultUnlocked() is not necessary if a keyring is re-locked. The
regular
> calls will trigger the necessary prompt (I fixed that in
> https://codereview.chromium.org/2441653002). EnsureDefaultUnlocked() is needed
> specifically to address the problem where secret_service_search will ignore a
> keyring if it's never opened since rebooting.
> 
> Calling EnsureDefaultUnlocked() more than once is redundant. The reason for
the
> s_ensured_keyring_unlocked is to reduce unnecessary interactions with
libsecret.
> We can do the same thing from the side of the Password Manager if you think
it's
> more appropriate.

Given that you call the method only once from KeyStorageLibsecret::Init() but
many from the password manager, I'd move the flag there.

[cfroussios, 2016-10-28 14:33:05]

The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 14:33:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cfroussios, 2016-10-28 14:36:39]

https://codereview.chromium.org/2461453002/diff/60001/components/os_crypt/lib...
File components/os_crypt/libsecret_util_linux.cc (right):

https://codereview.chromium.org/2461453002/diff/60001/components/os_crypt/lib...
components/os_crypt/libsecret_util_linux.cc:27: bool s_ensured_keyring_unlocked
= false;
On 2016/10/28 14:17:48, vasilii wrote:
> On 2016/10/28 13:50:12, cfroussios wrote:
> > On 2016/10/28 12:56:06, vasilii wrote:
> > > Are you thread-safe?
> > > I'm not a big fan of globals. Is EnsureKeyringUnlocked called very often?
> > > Also I'd like to protect against the cases when Keyring is locked not on
> > > start-up but later.
> > 
> > EnsureDefaultUnlocked() is not necessary if a keyring is re-locked. The
> regular
> > calls will trigger the necessary prompt (I fixed that in
> > https://codereview.chromium.org/2441653002). EnsureDefaultUnlocked() is
needed
> > specifically to address the problem where secret_service_search will ignore
a
> > keyring if it's never opened since rebooting.
> > 
> > Calling EnsureDefaultUnlocked() more than once is redundant. The reason for
> the
> > s_ensured_keyring_unlocked is to reduce unnecessary interactions with
> libsecret.
> > We can do the same thing from the side of the Password Manager if you think
> it's
> > more appropriate.
> 
> Given that you call the method only once from KeyStorageLibsecret::Init() but
> many from the password manager, I'd move the flag there.

Done.

https://codereview.chromium.org/2461453002/diff/60001/components/os_crypt/lib...
components/os_crypt/libsecret_util_linux.cc:138: "properly.";
On 2016/10/28 12:56:06, vasilii wrote:
> Do you need this print on every startup?

Done.

[vasilii, 2016-10-28 15:08:21]

lgtm

[commit-bot: I haz the power, 2016-10-28 15:17:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 15:17:14]

Dry run: This issue passed the CQ dry run.

[cfroussios, 2016-10-28 15:24:51]

The CQ bit was checked by cfroussios@chromium.org

[commit-bot: I haz the power, 2016-10-28 15:25:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 15:31:00]

Description was changed from

==========
Add a dummy entry with libsecret when initializing OSCrypt.

Under certain conditions, gnome libsecret methods that perform a lookup
in keyring may ignore a locked keyring and return empty results, instead
of unlocking said keyring. Store operations are unaffected by this quirk
(or bug, presumably).

With this change, we store a dummy entry during the initialisation of
OSCrypt. This ensures that the default keyring gets unlocked and is
available for future reads.

This solution is also applied to Password Manager, before performing
calls to secret_service_search_sync.

This solution should be temporary, until the underlying issue gets fixed
or there is an official workaround.

BUG=660005,631171
==========

to

==========
Add a dummy entry with libsecret when initializing OSCrypt.

Under certain conditions, gnome libsecret methods that perform a lookup
in keyring may ignore a locked keyring and return empty results, instead
of unlocking said keyring. Store operations are unaffected by this quirk
(or bug, presumably).

With this change, we store a dummy entry during the initialisation of
OSCrypt. This ensures that the default keyring gets unlocked and is
available for future reads.

This solution is also applied to Password Manager, before performing
calls to secret_service_search_sync.

This solution should be temporary, until the underlying issue gets fixed
or there is an official workaround.

BUG=660005,631171
==========

[commit-bot: I haz the power, 2016-10-28 15:31:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-28 15:49:03]

Description was changed from

==========
Add a dummy entry with libsecret when initializing OSCrypt.

Under certain conditions, gnome libsecret methods that perform a lookup
in keyring may ignore a locked keyring and return empty results, instead
of unlocking said keyring. Store operations are unaffected by this quirk
(or bug, presumably).

With this change, we store a dummy entry during the initialisation of
OSCrypt. This ensures that the default keyring gets unlocked and is
available for future reads.

This solution is also applied to Password Manager, before performing
calls to secret_service_search_sync.

This solution should be temporary, until the underlying issue gets fixed
or there is an official workaround.

BUG=660005,631171
==========

to

==========
Add a dummy entry with libsecret when initializing OSCrypt.

Under certain conditions, gnome libsecret methods that perform a lookup
in keyring may ignore a locked keyring and return empty results, instead
of unlocking said keyring. Store operations are unaffected by this quirk
(or bug, presumably).

With this change, we store a dummy entry during the initialisation of
OSCrypt. This ensures that the default keyring gets unlocked and is
available for future reads.

This solution is also applied to Password Manager, before performing
calls to secret_service_search_sync.

This solution should be temporary, until the underlying issue gets fixed
or there is an official workaround.

BUG=660005,631171

Committed: https://crrev.com/f3a8dfe38fab661729155dac82c8ab7d8422c2e1
Cr-Commit-Position: refs/heads/master@{#428378}
==========

[commit-bot: I haz the power, 2016-10-28 15:49:04]

Patchset 5 (id:??) landed as
https://crrev.com/f3a8dfe38fab661729155dac82c8ab7d8422c2e1
Cr-Commit-Position: refs/heads/master@{#428378}