Add the Sign-At-Most-Once restriction the enterprise.platformKeys API.

Add the Sign-At-Most-Once restriction the enterprise.platformKeys API.

This change ensures that the extension that generated a key with enterprise.platformKeys can use that key at most once for signing data. Any attempt to sign data afterwards with the same key will fail.

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

[pneubeck (no reviews), 2014-06-10 17:50:11]

@Benjamin, not sure whether you're the right to check the StateStore usage?

@Eric, ptal.

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:27: namespace subtle {
I could move these functions to another file.
But to simplify the review, I put them into a namespace at first.

[not at google - send to devlin, 2014-06-10 17:56:14]

at a casual glance the state store usage looks fine, is there anything else you
want me to look at?

[pneubeck (no reviews), 2014-06-10 18:03:00]

On 2014/06/10 17:56:14, kalman wrote:
> at a casual glance the state store usage looks fine, is there anything else
you
> want me to look at?

Thanks for the quick response.

One more detail maybe. Namely whether the access to KeyedService
'PlatformKeysService' from the extension functions is idiomatic.
Apart from that, no.

[not at google - send to devlin, 2014-06-10 18:15:15]

yes looks fine, lgtm.

[eroman, 2014-06-12 05:50:45]

I am not the best reviewer for this. Looks ok but not sure if these are the
right ways to store the data; will defer to kalman.

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:29: // an extension.
Instead use PlatfromKeysService which enforces restrictions
typeo Platfrom -> Platform

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys_service.cc (right):

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:48: // signing
operation which will call back |callback|. If not allowd, calls
typo: allowd -> allowed

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:182:
DCHECK(platform_keys->end() == platform_keys->Find(*key_value))
This is a DCHECK() because it is assumed keys are always generated and shouldn't
collide?

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:184:
platform_keys->Append(key_value.release());
What sort of bounds checks are done? Is it possible for extensions to exhaust
browser memory by generating lots of keys?

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:235:
value.reset(keys);
why bother with this line, when the next one releases it?

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys_service.h (right):

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:49: // Generates a
RSA key with |modulus_length| and registers the key to allow a
key --> key pair? also specify whether modulus_length is bits or bytes
(preferably by changing its name).

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:65: // Signs
|data| with the private key matching |public_key|, if that key is
why not call public_key -> public_key_spki_der to match other functions

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:107: // for the
given extension. If any error occurrs during key generation or
typo: ocurrs -> occurs

[pneubeck (no reviews), 2014-06-12 09:21:37]

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys.h (right):

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys.h:29: // an extension.
Instead use PlatfromKeysService which enforces restrictions
On 2014/06/12 05:50:45, eroman wrote:
> typeo Platfrom -> Platform

Done.

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys_service.cc (right):

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:48: // signing
operation which will call back |callback|. If not allowd, calls
On 2014/06/12 05:50:45, eroman wrote:
> typo: allowd -> allowed

Done.

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:182:
DCHECK(platform_keys->end() == platform_keys->Find(*key_value))
On 2014/06/12 05:50:45, eroman wrote:
> This is a DCHECK() because it is assumed keys are always generated and
shouldn't
> collide?

Yes, updated the error string for clarification.

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:184:
platform_keys->Append(key_value.release());
On 2014/06/12 05:50:45, eroman wrote:
> What sort of bounds checks are done? Is it possible for extensions to exhaust
> browser memory by generating lots of keys?

No bounds checks so far. And yes an extension can exhaust memory that way. In
the design doc I mentioned a quota policy which we could consider if we see need
for it.

Again, the problem is less serious because only policy pre-installed extensions
can use this API. So we would protect against attackers of such extensions.
I think it's a nice to have (say M38) but not a release blocker.

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:235:
value.reset(keys);
On 2014/06/12 05:50:45, eroman wrote:
> why bother with this line, when the next one releases it?
the intention was that both branches have the same postcondition (keys != NULL)
iff (value != NULL).
Also I don't like to have heap allocation without immediately binding it to a
scoped_ptr.

Still, changed it to make it overall a bit simpler.

Maybe a scoped_ptr method for safe downcasting would be reasonable; something
like:

template<class Base, class Derived>
scoped_ptr<Derived> PassAs(scoped_ptr<Base>& base, Derived* derived) {
  DCHECK_EQ(base.get(), derived);
  ignore_result(base.release());
  return make_scoped_ptr(derived);
}

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/platform_keys/platform_keys_service.h (right):

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:49: // Generates a
RSA key with |modulus_length| and registers the key to allow a
On 2014/06/12 05:50:45, eroman wrote:
> key --> key pair? also specify whether modulus_length is bits or bytes
> (preferably by changing its name).

Done.

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:65: // Signs
|data| with the private key matching |public_key|, if that key is
On 2014/06/12 05:50:45, eroman wrote:
> why not call public_key -> public_key_spki_der to match other functions

Done.

https://codereview.chromium.org/323093003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:107: // for the
given extension. If any error occurrs during key generation or
On 2014/06/12 05:50:45, eroman wrote:
> typo: ocurrs -> occurs

Done.

[not at google - send to devlin, 2014-06-15 18:27:07]

some style comments. like I mentioned the usage of state store looks fine,
though I'm not very familiar with it.

https://codereview.chromium.org/323093003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_service.h (right):

https://codereview.chromium.org/323093003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:58: Profile*
profile);
It's odd to me that several of these methods take a Profile as an argument when
ostensibly this class is bound to a BrowserContext (at least, implicitly via its
dependence on the per-BrowserContext StateStore).

(by the way: the interface implies they need a Profile but do they really? can
they make do with a BrowserContext?)

https://codereview.chromium.org/323093003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:111: static void
GenerateRSAKeyCallback(const std::string& extension_id,
can these static-private methods just be in an anonymous namespace within
platform_keys_service.cc?

https://codereview.chromium.org/323093003/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/323093003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:55:
browser_context());
It's nice to have a wrapper for this lengthy statement, like
PlatformKeysService::Get(..)

(see
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
for example)

https://codereview.chromium.org/323093003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:95:
browser_context());
here too

[pneubeck (no reviews), 2014-06-16 09:11:12]

https://codereview.chromium.org/323093003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_service.h (right):

https://codereview.chromium.org/323093003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:58: Profile*
profile);
On 2014/06/15 18:27:06, kalman wrote:
> It's odd to me that several of these methods take a Profile as an argument
when
> ostensibly this class is bound to a BrowserContext (at least, implicitly via
its
> dependence on the per-BrowserContext StateStore).
> 
> (by the way: the interface implies they need a Profile but do they really? can
> they make do with a BrowserContext?)

Yeah, I realized that Profile isn't required, too. Landing a CL to fix that.

Storing the BrowserContext* and removing the argument.

https://codereview.chromium.org/323093003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_service.h:111: static void
GenerateRSAKeyCallback(const std::string& extension_id,
On 2014/06/15 18:27:07, kalman wrote:
> can these static-private methods just be in an anonymous namespace within
> platform_keys_service.cc?

They're all calling back to the PlatformKeysService (using the provided
weakptr).
As it should be fine to just drop callbacks when the service is destroyed, I
dropped the explicit WeakPtr argument and made the non-static.

https://codereview.chromium.org/323093003/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc
(right):

https://codereview.chromium.org/323093003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:55:
browser_context());
On 2014/06/15 18:27:07, kalman wrote:
> It's nice to have a wrapper for this lengthy statement, like
> PlatformKeysService::Get(..)
> 
> (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> for example)

Oops. I had written GetForBrowserContext to be a static function already, just
missed it here. Removed the unnecessary GetInstance().

https://codereview.chromium.org/323093003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/enterprise_platform_keys/enterprise_platform_keys_api.cc:95:
browser_context());
On 2014/06/15 18:27:07, kalman wrote:
> here too

Done.

[not at google - send to devlin, 2014-06-16 17:01:30]

lgtm

[pneubeck (no reviews), 2014-06-17 12:01:43]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-06-17 12:04:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/323093003/160001

[commit-bot: I haz the power, 2014-06-17 12:25:33]

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

[commit-bot: I haz the power, 2014-06-17 12:25:34]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/17726)

[pneubeck (no reviews), 2014-06-17 13:57:59]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-06-17 13:59:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/323093003/180001

[commit-bot: I haz the power, 2014-06-17 16:57:14]

Change committed as 277784