Implement gnome-keyring for OSCrypt

Implement gnome-keyring for OSCrypt

OSCrypt can now save its encryption password in gnome-keyring, where
that library exists.

BUG=602624
Committed: https://crrev.com/b013c15b73b3f71e2ec1636ce27b93b4dde04361
Cr-Commit-Position: refs/heads/master@{#416423}

[cfroussios, 2016-08-30 09:42:57]

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

[commit-bot: I haz the power, 2016-08-30 09:43:13]

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-08-30 10:39:31]

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

[commit-bot: I haz the power, 2016-08-30 10:39:32]

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-08-30 11:13:41]

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

[commit-bot: I haz the power, 2016-08-30 11:14:06]

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

[cfroussios, 2016-08-30 11:41:18]

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

[cfroussios, 2016-08-30 11:41:18]

Hi! This is hopefully the last big piece of the linux implementation. Can you
review? Thanks!

[commit-bot: I haz the power, 2016-08-30 11:41:52]

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

[commit-bot: I haz the power, 2016-08-30 11:41:52]

Dry run: Exceeded global retry quota

[Lei Zhang, 2016-08-30 23:19:24]

Looks good. Just some suggestions and nits.

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
File components/os_crypt/key_storage_keyring.cc (right):

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.cc:13: #include
"base/threading/thread.h"
Given there's some threading here, do we want to use
base/threading/thread_checker.h to DCHECK KeyStorageKeyring lives on a single
thread?

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.cc:32: main_thread_runner_ =
main_thread_runner;
Can we use the initializer list?

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
File components/os_crypt/key_storage_keyring.h (right):

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.h:12: #include
"base/synchronization/waitable_event.h"
Forward declare instead? If I previously recommended #includes, I have to
correct myself because I missed the memo here:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.h:18: KeyStorageKeyring(
explicit

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
File components/os_crypt/key_storage_keyring_unittest.cc (right):

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring_unittest.cc:6: 
no blank line

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring_unittest.cc:89: } else {
no else after return

[cfroussios, 2016-08-31 12:03:42]

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

[commit-bot: I haz the power, 2016-08-31 12:03:53]

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

[cfroussios, 2016-08-31 12:18:41]

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
File components/os_crypt/key_storage_keyring.cc (right):

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.cc:13: #include
"base/threading/thread.h"
On 2016/08/30 23:19:24, Lei Zhang wrote:
> Given there's some threading here, do we want to use
> base/threading/thread_checker.h to DCHECK KeyStorageKeyring lives on a single
> thread?

I implemented it, but I think it's a bit meh. The best that it can guarantee is
that GetKeyDelegate() and AddRandomPasswordInKeyring() are called on the same
thread. Given how they are private methods that call each other, hardly anything
is checked.

Ideally, we would just have a check that we are on the main thread, but I didn't
find a way to do this in a component, expect by bring in system libraries.

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.cc:32: main_thread_runner_ =
main_thread_runner;
On 2016/08/30 23:19:24, Lei Zhang wrote:
> Can we use the initializer list?

Done.

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
File components/os_crypt/key_storage_keyring.h (right):

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.h:12: #include
"base/synchronization/waitable_event.h"
On 2016/08/30 23:19:24, Lei Zhang wrote:
> Forward declare instead? If I previously recommended #includes, I have to
> correct myself because I missed the memo here:
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.h:18: KeyStorageKeyring(
On 2016/08/30 23:19:24, Lei Zhang wrote:
> explicit

Done.

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
File components/os_crypt/key_storage_keyring_unittest.cc (right):

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring_unittest.cc:6: 
On 2016/08/30 23:19:24, Lei Zhang wrote:
> no blank line

Done.

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring_unittest.cc:89: } else {
On 2016/08/30 23:19:24, Lei Zhang wrote:
> no else after return

Done.

[commit-bot: I haz the power, 2016-08-31 12:44:43]

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

[commit-bot: I haz the power, 2016-08-31 12:44:44]

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

[cfroussios, 2016-08-31 13:05:39]

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

[commit-bot: I haz the power, 2016-08-31 13:05:46]

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-08-31 14:11:43]

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

[commit-bot: I haz the power, 2016-08-31 14:11:45]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-09-01 07:56:31]

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
File components/os_crypt/key_storage_keyring.cc (right):

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.cc:13: #include
"base/threading/thread.h"
On 2016/08/31 12:18:41, cfroussios wrote:
> On 2016/08/30 23:19:24, Lei Zhang wrote:
> > Given there's some threading here, do we want to use
> > base/threading/thread_checker.h to DCHECK KeyStorageKeyring lives on a
single
> > thread?
> 
> I implemented it, but I think it's a bit meh. The best that it can guarantee
is
> that GetKeyDelegate() and AddRandomPasswordInKeyring() are called on the same
> thread. Given how they are private methods that call each other, hardly
anything
> is checked.

I actually wasn't that interested in checking GetKeyDelegate() and
AddRandomPasswordInKeyring(). As you said, since they are private, if they are
not called on the right thread, then there is nobody to blame but us.

OTOH, the ctor + public methods to KeyStorageKeyring are being called by
external code that may or may not be under other control. That's where we want
to guard against misuse.

https://codereview.chromium.org/2297573002/diff/60001/components/os_crypt/key...
File components/os_crypt/key_storage_keyring.cc (right):

https://codereview.chromium.org/2297573002/diff/60001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.cc:64: void
KeyStorageKeyring::GetKeyDelegate(
Can these be inside an anonymous namespace, instead of (formerly) being static
class methods?

https://codereview.chromium.org/2297573002/diff/60001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.cc:84: if (password_loaded_ptr)
Is this ever false?

[cfroussios, 2016-09-01 10:58:16]

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
File components/os_crypt/key_storage_keyring.cc (right):

https://codereview.chromium.org/2297573002/diff/20001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.cc:13: #include
"base/threading/thread.h"
On 2016/09/01 07:56:31, Lei Zhang wrote:
> On 2016/08/31 12:18:41, cfroussios wrote:
> > On 2016/08/30 23:19:24, Lei Zhang wrote:
> > > Given there's some threading here, do we want to use
> > > base/threading/thread_checker.h to DCHECK KeyStorageKeyring lives on a
> single
> > > thread?
> > 
> > I implemented it, but I think it's a bit meh. The best that it can guarantee
> is
> > that GetKeyDelegate() and AddRandomPasswordInKeyring() are called on the
same
> > thread. Given how they are private methods that call each other, hardly
> anything
> > is checked.
> 
> I actually wasn't that interested in checking GetKeyDelegate() and
> AddRandomPasswordInKeyring(). As you said, since they are private, if they are
> not called on the right thread, then there is nobody to blame but us.
> 
> OTOH, the ctor + public methods to KeyStorageKeyring are being called by
> external code that may or may not be under other control. That's where we want
> to guard against misuse.

I think there might be a misunderstanding about the kind of threading that we
do.

Threading requirements are a quirk of gnome-keyring. I chose not to impose
threading expectations on the callers and/or other children of KeyStorageLinux
just for the sake of this one library, which is already deprecated and we're
just waiting for the users to move on.

The constructor and Init() don't need to be on the main thread. The only method
that makes calls to keyring is GetKey(). We guarantee that those come from the
main thread by switching to it only if we are not already on the main thread
(see line 45). Basically, it can be called from anywhere and it will fix it if
necessary.

Currently, it happens that the first caller (the one which also causes the lazy
instantiation of a KeyStorageLinux) is from the main thread. However, many
clients of OSCrypt are not, so all of this is to ensure that the contract is
maintained if something changes.

Unless there's something I am missing, and since you're not interested in
checking the private methods either, I think we can remove the checks.

https://codereview.chromium.org/2297573002/diff/60001/components/os_crypt/key...
File components/os_crypt/key_storage_keyring.cc (right):

https://codereview.chromium.org/2297573002/diff/60001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.cc:64: void
KeyStorageKeyring::GetKeyDelegate(
On 2016/09/01 07:56:31, Lei Zhang wrote:
> Can these be inside an anonymous namespace, instead of (formerly) being static
> class methods?

AddRandomPasswordInKeyring() needs access to protected fields of
KeyStorageLinux. GetKeyDelegate() needs to call AddRandomPasswordInKeyring(),
which is private.

We could make KeyStorageLinux' stuff public, but making stuff public so that the
children can use them smells to me.

https://codereview.chromium.org/2297573002/diff/60001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.cc:84: if (password_loaded_ptr)
On 2016/09/01 07:56:31, Lei Zhang wrote:
> Is this ever false?

When GetKey() is called on the main thread.

[Lei Zhang, 2016-09-01 17:16:41]

Mmm. Sounds like we should just drop the thread checking then. Sorry for the
trouble. Next time, just say no. :)

https://codereview.chromium.org/2297573002/diff/60001/components/os_crypt/key...
File components/os_crypt/key_storage_keyring.cc (right):

https://codereview.chromium.org/2297573002/diff/60001/components/os_crypt/key...
components/os_crypt/key_storage_keyring.cc:84: if (password_loaded_ptr)
On 2016/09/01 10:58:16, cfroussios wrote:
> On 2016/09/01 07:56:31, Lei Zhang wrote:
> > Is this ever false?
> 
> When GetKey() is called on the main thread.

Right, that's what I get for doing late night reviews.

[cfroussios, 2016-09-02 14:29:52]

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

[commit-bot: I haz the power, 2016-09-02 14:30:07]

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

[cfroussios, 2016-09-02 14:35:33]

I think it is still safer to assume that you know the ways of Chrome better.

Thread checking removed.

[commit-bot: I haz the power, 2016-09-02 15:47:38]

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

[commit-bot: I haz the power, 2016-09-02 15:47:39]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-09-03 01:06:37]

On 2016/09/02 14:35:33, cfroussios wrote:
> I think it is still safer to assume that you know the ways of Chrome better.
> 
> Thread checking removed.

It was just a suggestion. If it comes out looking a bit silly, don't follow it
and push back. :)

[Lei Zhang, 2016-09-03 01:06:45]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-09-03 01:06:45]

lgtm

[commit-bot: I haz the power, 2016-09-03 01:07:13]

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-09-03 01:10:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-03 01:12:35]

Description was changed from

==========
Implement gnome-keyring for OSCrypt

OSCrypt can now save its encryption password in gnome-keyring, where
that library exists.

BUG=602624
==========

to

==========
Implement gnome-keyring for OSCrypt

OSCrypt can now save its encryption password in gnome-keyring, where
that library exists.

BUG=602624

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

[commit-bot: I haz the power, 2016-09-03 01:12:36]

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