cryptohome: Move Encrypt/DecryptWithSystemSalt() out of CryptohomeLibrary

cryptohome: Move Encrypt/DecryptWithSystemSalt() out of CryptohomeLibrary

The implementation in CryptohomeLibrary is moved to CryptohomeTokenEncryptor
as-is, except that CryptohomeLibrary::GetCachedSystemSalt() is called at the
beginning of LoadSystemSaltKey():

  bool CryptohomeTokenEncryptor::LoadSystemSaltKey() {
    if (system_salt_.empty())
      system_salt_ = CryptohomeLibrary::Get()->GetCachedSystemSalt();

BUG=303474, 298605
TEST=none
R=davidroche@chromium.org, hashimoto@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227631

[satorux1, 2013-10-04 06:11:14]

https://codereview.chromium.org/25975002/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/token_encryptor.cc (right):

https://codereview.chromium.org/25975002/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/token_encryptor.cc:65: system_salt_ =
CryptohomeLibrary::Get()->GetSystemSalt();
As mentioned in the patch description, the two lines are the only difference
from the original code in cryptohome_library.cc

[hashimoto, 2013-10-04 06:22:28]

https://codereview.chromium.org/25975002/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/token_encryptor.cc (right):

https://codereview.chromium.org/25975002/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/token_encryptor.cc:65: system_salt_ =
CryptohomeLibrary::Get()->GetSystemSalt();
On 2013/10/04 06:11:14, satorux1 wrote:
> As mentioned in the patch description, the two lines are the only difference
> from the original code in cryptohome_library.cc

I don't think we should add a new user of CryptohomeLibrary::GetSystemSalt here
because the method may make D-Bus method call synchronously.

[satorux1, 2013-10-04 06:23:01]

https://codereview.chromium.org/25975002/diff/1/chromeos/cryptohome/mock_cryp...
File chromeos/cryptohome/mock_cryptohome_library.h (right):

https://codereview.chromium.org/25975002/diff/1/chromeos/cryptohome/mock_cryp...
chromeos/cryptohome/mock_cryptohome_library.h:20: class MockCryptohomeLibrary :
public CryptohomeLibrary {
Just noticed that this class is no longer in use now
(device_oauth2_token_service_unittest.cc was the last client). Removed it!

[satorux1, 2013-10-04 06:44:00]

https://codereview.chromium.org/25975002/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/token_encryptor.cc (right):

https://codereview.chromium.org/25975002/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/token_encryptor.cc:65: system_salt_ =
CryptohomeLibrary::Get()->GetSystemSalt();
On 2013/10/04 06:22:28, hashimoto wrote:
> On 2013/10/04 06:11:14, satorux1 wrote:
> > As mentioned in the patch description, the two lines are the only difference
> > from the original code in cryptohome_library.cc
> 
> I don't think we should add a new user of CryptohomeLibrary::GetSystemSalt
here
> because the method may make D-Bus method call synchronously.

That's a good point. In the original code, there was a hidden dependency between
LoadSystemSalt() and LoadSystemSaltKey(), where LoadSystemSaltKey() expects that
LoadSystemSalt() is called beforehand. I guess it's guaranteed that
GetSystemSalt() is called beforehand at login time.

That said, I think we need a function that simply returns a cached system salt
here. Added CryptohomeLibrary::GetCachedSystemSalt() and used it here.

https://codereview.chromium.org/25975002/diff/12001/chromeos/cryptohome/crypt...
File chromeos/cryptohome/cryptohome_library.h (right):

https://codereview.chromium.org/25975002/diff/12001/chromeos/cryptohome/crypt...
chromeos/cryptohome/cryptohome_library.h:17: class CHROMEOS_EXPORT
CryptohomeLibrary {
BTW, now this class just manages the system salt. we should rename this to
something like SystemSaltGetter

[hashimoto, 2013-10-04 08:38:55]

lgtm

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:16: #include
"chrome/browser/chromeos/settings/token_encryptor.h"
nit: Can't this include replaced with a forward declaration?

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:73:
TokenEncryptor* token_encryptor);
nit: How about taking scoped_ptr<TokenEncryptor> as an argument to make the
ownership clear?

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/token_encryptor.cc (right):

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/token_encryptor.cc:80: 
nit: Remove this duplicated blank line.

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/token_encryptor.cc:82: // Encrypts (AES) the
token given |key| and |salt|.
nit: Do we need to have the same comment written in the header here?

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/token_encryptor.cc:105: // Decrypts (AES) hex
encoded encrypted token given |key| and |salt|.
ditto.

https://codereview.chromium.org/25975002/diff/12001/chromeos/cryptohome/crypt...
File chromeos/cryptohome/cryptohome_library.cc (right):

https://codereview.chromium.org/25975002/diff/12001/chromeos/cryptohome/crypt...
chromeos/cryptohome/cryptohome_library.cc:79: bool locked_;
nit: Can we remove these two members? It seems they are not used.

[satorux1, 2013-10-07 02:19:14]

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/device_oauth2_token_service.h (right):

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:16: #include
"chrome/browser/chromeos/settings/token_encryptor.h"
On 2013/10/04 08:38:56, hashimoto wrote:
> nit: Can't this include replaced with a forward declaration?

Done.

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/device_oauth2_token_service.h:73:
TokenEncryptor* token_encryptor);
On 2013/10/04 08:38:56, hashimoto wrote:
> nit: How about taking scoped_ptr<TokenEncryptor> as an argument to make the
> ownership clear?

Thought it's a good idea but doing it would make it difficult to set mock
behaviors in the unit test. currently, we do something like

  mock_(new MockTokenEncryptor),
  service_(new DeviceOAuth2TokenService(..., mock_)),

If mock_ is a scoped ptr and Passed(), we cannot set mock expectations. We could
expose it via getter like token_encryptor() but it'd be a bit undesirable. For
now, let me keep it as-is but add a comment about ownership.

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/settings/token_encryptor.cc (right):

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/token_encryptor.cc:80: 
On 2013/10/04 08:38:56, hashimoto wrote:
> nit: Remove this duplicated blank line.

Done.

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/token_encryptor.cc:82: // Encrypts (AES) the
token given |key| and |salt|.
On 2013/10/04 08:38:56, hashimoto wrote:
> nit: Do we need to have the same comment written in the header here?

Done.

https://codereview.chromium.org/25975002/diff/12001/chrome/browser/chromeos/s...
chrome/browser/chromeos/settings/token_encryptor.cc:105: // Decrypts (AES) hex
encoded encrypted token given |key| and |salt|.
On 2013/10/04 08:38:56, hashimoto wrote:
> ditto.

Done.

https://codereview.chromium.org/25975002/diff/12001/chromeos/cryptohome/crypt...
File chromeos/cryptohome/cryptohome_library.cc (right):

https://codereview.chromium.org/25975002/diff/12001/chromeos/cryptohome/crypt...
chromeos/cryptohome/cryptohome_library.cc:79: bool locked_;
On 2013/10/04 08:38:56, hashimoto wrote:
> nit: Can we remove these two members? It seems they are not used.

Good catch. Forgot to remove them earlier. Done!

[satorux1, 2013-10-07 06:00:47]

ping David.

[satorux1, 2013-10-08 01:25:50]

adding fgorski@ in case davidroche is ooo.

[David Roche, 2013-10-08 22:58:22]

LGTM

Sorry for the delay, I was out for 2 days.

[satorux1, 2013-10-08 23:50:35]

On 2013/10/08 22:58:22, David Roche wrote:
> LGTM
> 
> Sorry for the delay, I was out for 2 days.

NP. That's what I thought. Thank you for taking a look as soon as you are back!

[satorux1, 2013-10-09 00:25:02]

Committed patchset #5 manually as r227631 (presubmit successful).