Implement chrome.platformKeys.getKeyPair().

Implement chrome.platformKeys.getKeyPair().

This also makes the chrome.platformKeys.subtleCrypto().sign() working.

Second change after
https://codereview.chromium.org/875373002/
to implement chrome.platformKeys.

Still missing: per extension key permissions and UI changes.

BUG=450167
R=asvitkine@chromium.org, kaliamoorthi@chromium.org, kalman@chromium.org, rsleevi@chromium.org
TBR=arv@chromium.org (for .grd change)

Committed: https://chromium.googlesource.com/chromium/src/+/594333f07c9d287c268f4d04259f74e08fc20bb9

[pneubeck (no reviews), 2015-01-29 13:10:09]

Patchset #1 (id:1) has been deleted

[pneubeck (no reviews), 2015-01-29 13:10:15]

Patchset #1 (id:20001) has been deleted

[pneubeck (no reviews), 2015-01-29 13:10:20]

Patchset #1 (id:40001) has been deleted

[pneubeck (no reviews), 2015-01-29 13:12:13]

Patchset #1 (id:60001) has been deleted

[pneubeck (no reviews), 2015-01-29 13:41:04]

Patchset #2 (id:100001) has been deleted

[pneubeck (no reviews), 2015-01-29 13:45:20]

Patchset #2 (id:120001) has been deleted

[pneubeck (no reviews), 2015-01-29 15:17:23]

pneubeck@chromium.org changed reviewers:
+ eroman@chromium.org

[pneubeck (no reviews), 2015-01-29 15:17:24]

second part of the implementation. ptal

[eroman, 2015-01-30 17:54:12]

eroman@chromium.org changed reviewers:
+ rsleevi@chromium.org

[eroman, 2015-01-30 17:54:13]

TBH I am not very experienced with crypto, probably not the best reviewer.

Ryan, would you be able to review this?

[pneubeck (no reviews), 2015-02-02 10:34:39]

pneubeck@chromium.org changed reviewers:
+ bartfab@chromium.org

[pneubeck (no reviews), 2015-02-02 10:34:40]

@Bartosz, and this one third.

[pneubeck (no reviews), 2015-02-02 12:37:39]

pneubeck@chromium.org changed reviewers:
+ tnagel@chromium.org
- bartfab@chromium.org

[pneubeck (no reviews), 2015-02-02 12:37:39]

-Bartosz, +Thiemo

[Ryan Sleevi, 2015-02-03 01:44:51]

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

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:37: HASH_ALGORITHM_NONE,
Needs documentation about expected behaviours (you've got it on 98/99, but
that's a *use* of this enum rather than an explanation about what this could
mean)

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:57: struct PublicKeyInfo {
NEeds documentation

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:61: std::vector<char>
public_key_spki_der;
Why |char| and not |uint8_t| ?

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:65: // that.
This doesn't always hold true

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:68: unsigned int
modulus_length_bits;
This is a weird interface then, as ec keys have a strength/public key length.

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:99: // signing. PKCS#1
v1.5 padding will still be applied.
OK, so from a documentation standpoint, it's a little weird.

Consider
- If this API is meant to take EC keys, what would HASH_ALGORITHM_NONE mean?
- The maximum size of |data| is limited in the case of PKCS#1 v1.5 signing with
an RSA key (to modulus_len - 11, IIRC).

Maybe this should be split into two APIs - Sign() and SignDirectPKCS1Padded()
[since lines 91-92 no longer apply, and it's not exactly Direct signing], with a
shared under the hood impl?

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:747:
SECKEY_EncodeDERSubjectPublicKeyInfo(public_key.get()));
Why this round trip?

why not just certificate->os_cert_handle()->derPublicKey ?

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:761: //
TODO(pneubeck): Verify that the public exponent equals 65537.
Yes, please do :)

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

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:222:
!permission_check_disabled_) {
I've read this code multiple times and I'm still confused by it.

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/platform_keys/platform_keys_api.cc (right):

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:95:
cert_der.size());
SECURITY BUG: Validate that |cert_x509| is valid here, before doing anything
else. With your code as written, you would end up crashing at a NULL pointer
deref.

https://codereview.chromium.org/884073002/diff/160001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/platform_keys/get_public_key.js
(right):

https://codereview.chromium.org/884073002/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/platform_keys/get_public_key.js:35:
algorithm.publicExponent = new Uint8Array(algorithm.publicExponent);
BUG? The point of WebCrypto is to ensure the import params match. Is this really
correct here?

https://codereview.chromium.org/884073002/diff/160001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/platform_keys_custom_bindings.js
(right):

https://codereview.chromium.org/884073002/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/platform_keys_custom_bindings.js:37: new
Uint8Array(match.keyAlgorithm.publicExponent);
Ditto remarks here

https://codereview.chromium.org/884073002/diff/160001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/platform_keys/basic.js (right):

https://codereview.chromium.org/884073002/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/platform_keys/basic.js:109: ]);
Is there no way to move all this test data into secondary files to make it
easier to maintain/update?

[pneubeck (no reviews), 2015-02-03 10:48:18]

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

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:61: std::vector<char>
public_key_spki_der;
On 2015/02/03 01:44:50, Ryan Sleevi wrote:
> Why |char| and not |uint8_t| ?

Generated API code uses vector<char>, the code that I used from net/ mostly
std::string.
I'll change this back to std::string which is more consistent with the rest of
this file.

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:65: // that.
On 2015/02/03 01:44:50, Ryan Sleevi wrote:
> This doesn't always hold true

Is it acceptable to restrict it to 65537 if I implement the verification of
that?
Or should we allow usage (but not generation) of such certs/keys?

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:99: // signing. PKCS#1
v1.5 padding will still be applied.
On 2015/02/03 01:44:50, Ryan Sleevi wrote:
> OK, so from a documentation standpoint, it's a little weird.
> 
> Consider
> - If this API is meant to take EC keys, what would HASH_ALGORITHM_NONE mean?
> - The maximum size of |data| is limited in the case of PKCS#1 v1.5 signing
with
> an RSA key (to modulus_len - 11, IIRC).
> 
> Maybe this should be split into two APIs - Sign() and SignDirectPKCS1Padded()
> [since lines 91-92 no longer apply, and it's not exactly Direct signing], with
a
> shared under the hood impl?

This function is in namespace subtle and an implementation details of the
PlatformKeysService::Sign (more a SignRSA currently).
PlatformKeysService::Sign itself has some amount of implementation which I'd
hate to duplicate.

The question is how to pass around the Sign parameters "Sign(HashAlgorithm) or
DirectSign".
I got annoyed already by the huge number of arguments of this function that I'd
consider putting them into a struct including a new 'bool direct_sign':

struct SignParams { // or SignRSAParams
  std::string token_id;
  std::string public_key;
  HashAlgorithm hash_algorithm;  // Ignored if direct_sign==true.
  bool direct_sign;
  std::string data;
};

[pneubeck (no reviews), 2015-02-03 20:14:59]

pneubeck@chromium.org changed reviewers:
- tnagel@chromium.org

[pneubeck (no reviews), 2015-02-03 20:15:01]

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

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:57: struct PublicKeyInfo {
On 2015/02/03 01:44:50, Ryan Sleevi wrote:
> NEeds documentation

changed the name to SPKI and documented it.

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:68: unsigned int
modulus_length_bits;
On 2015/02/03 01:44:50, Ryan Sleevi wrote:
> This is a weird interface then, as ec keys have a strength/public key length.

changed the name to key_size_bits

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:747:
SECKEY_EncodeDERSubjectPublicKeyInfo(public_key.get()));
On 2015/02/03 01:44:50, Ryan Sleevi wrote:
> Why this round trip?
> 
> why not just certificate->os_cert_handle()->derPublicKey ?

didn't know about that. changed, although it's totally not obvious that this is
the subjectPublicKeyInfo and not the subjectPublicKey ?!

note that this function relates to the mail I wrote you a while ago.
namely, this could be shared with GetPublicKeyInfo in x509_util_nss/
X509Certificate.
Now with your pointer to derPublicKey, there is actually no modification of that
function required anymore to expose the SPKI and I can reuse it as is.

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

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:222:
!permission_check_disabled_) {
On 2015/02/03 01:44:50, Ryan Sleevi wrote:
> I've read this code multiple times and I'm still confused by it.

reordered to make it clearer.

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/platform_keys/platform_keys_api.cc (right):

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:95:
cert_der.size());
On 2015/02/03 01:44:50, Ryan Sleevi wrote:
> SECURITY BUG: Validate that |cert_x509| is valid here, before doing anything
> else. With your code as written, you would end up crashing at a NULL pointer
> deref.

Done.

https://codereview.chromium.org/884073002/diff/160001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/platform_keys/get_public_key.js
(right):

https://codereview.chromium.org/884073002/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/platform_keys/get_public_key.js:35:
algorithm.publicExponent = new Uint8Array(algorithm.publicExponent);
On 2015/02/03 01:44:50, Ryan Sleevi wrote:
> BUG? The point of WebCrypto is to ensure the import params match. Is this
really
> correct here?

added a comment explaining that this only fixes the type of the publicExponent
returned by the internal API.

regarding whether the import params match the key properties:
I currently don't check for any match and instead overwrite the import params
with the params that I read from the SPKI.

https://codereview.chromium.org/884073002/diff/160001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/platform_keys_custom_bindings.js
(right):

https://codereview.chromium.org/884073002/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/platform_keys_custom_bindings.js:37: new
Uint8Array(match.keyAlgorithm.publicExponent);
On 2015/02/03 01:44:50, Ryan Sleevi wrote:
> Ditto remarks here

added a comment here as well.

https://codereview.chromium.org/884073002/diff/160001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/platform_keys/basic.js (right):

https://codereview.chromium.org/884073002/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/platform_keys/basic.js:109: ]);
On 2015/02/03 01:44:51, Ryan Sleevi wrote:
> Is there no way to move all this test data into secondary files to make it
> easier to maintain/update?

I assume that you mean adding files that contains exactly these byte sequences
in raw.
I could probably do that with an XHR but only if they're relative resources.

Wdyt?

[pneubeck (no reviews), 2015-02-04 20:58:24]

Patchset #7 (id:240001) has been deleted

[pneubeck (no reviews), 2015-02-04 20:59:20]

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:761: //
TODO(pneubeck): Verify that the public exponent equals 65537.
On 2015/02/03 01:44:50, Ryan Sleevi wrote:
> Yes, please do :)

Done.

https://codereview.chromium.org/884073002/diff/160001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/platform_keys/basic.js (right):

https://codereview.chromium.org/884073002/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/platform_keys/basic.js:109: ]);
On 2015/02/03 20:15:00, pneubeck wrote:
> On 2015/02/03 01:44:51, Ryan Sleevi wrote:
> > Is there no way to move all this test data into secondary files to make it
> > easier to maintain/update?
> 
> I assume that you mean adding files that contains exactly these byte sequences
> in raw.
> I could probably do that with an XHR but only if they're relative resources.
> 
> Wdyt?

changed to XHR loading local files.

[pneubeck (no reviews), 2015-02-05 10:41:57]

@Ryan, I should have addressed all of your feedback now. ptal.

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

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:37: HASH_ALGORITHM_NONE,
On 2015/02/03 01:44:49, Ryan Sleevi wrote:
> Needs documentation about expected behaviours (you've got it on 98/99, but
> that's a *use* of this enum rather than an explanation about what this could
> mean)

Done.

https://codereview.chromium.org/884073002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:99: // signing. PKCS#1
v1.5 padding will still be applied.
On 2015/02/03 10:48:18, pneubeck wrote:
> On 2015/02/03 01:44:50, Ryan Sleevi wrote:
> > OK, so from a documentation standpoint, it's a little weird.
> > 
> > Consider
> > - If this API is meant to take EC keys, what would HASH_ALGORITHM_NONE mean?
> > - The maximum size of |data| is limited in the case of PKCS#1 v1.5 signing
> with
> > an RSA key (to modulus_len - 11, IIRC).
> > 
> > Maybe this should be split into two APIs - Sign() and
SignDirectPKCS1Padded()
> > [since lines 91-92 no longer apply, and it's not exactly Direct signing],
with
> a
> > shared under the hood impl?
> 
> This function is in namespace subtle and an implementation details of the
> PlatformKeysService::Sign (more a SignRSA currently).
> PlatformKeysService::Sign itself has some amount of implementation which I'd
> hate to duplicate.
> 
> The question is how to pass around the Sign parameters "Sign(HashAlgorithm) or
> DirectSign".
> I got annoyed already by the huge number of arguments of this function that
I'd
> consider putting them into a struct including a new 'bool direct_sign':
> 
> struct SignParams { // or SignRSAParams
>   std::string token_id;
>   std::string public_key;
>   HashAlgorithm hash_algorithm;  // Ignored if direct_sign==true.
>   bool direct_sign;
>   std::string data;
> };

done.
I addressed your comments by clearer documentation ('optionally digest...',
'hash algorithm ignored if direct_sign_pkcs_padded is true...') and by renaming
the Sign function to SignRSA.

[pneubeck (no reviews), 2015-02-06 09:07:38]

pneubeck@chromium.org changed reviewers:
+ kalman@chromium.org
- eroman@chromium.org

[pneubeck (no reviews), 2015-02-06 09:07:39]

@Ryan, ping

@Benjamin, ptal at
 chrome/common/extensions/api/platform_keys_internal.idl
 chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
 chrome/renderer/extensions/platform_keys_natives.cc
 chrome/renderer/resources/extensions/platform_keys_custom_bindings.js

[pneubeck (no reviews), 2015-02-06 09:07:54]

pneubeck@chromium.org changed required reviewers:
+ kalman@chromium.org, rsleevi@chromium.org

[not at google - send to devlin, 2015-02-06 18:24:52]

extension mechanics lgtm, I can't review the functionality of course.

[Ryan Sleevi, 2015-02-07 02:09:39]

rsleevi@chromium.org changed reviewers:
+ eroman@chromium.org

[Ryan Sleevi, 2015-02-07 02:09:41]

I have not reviewed the JS tests, and I get the feeling from Ben's reply that he
hasn't either. I'm certainly not capable of reviewing them. Perhaps Eric can be
baited into reviewing.

On the matter of DER, uploading binary files is almost universally a pain. Just
base64 them and window.atob/btoa after the XHR?

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

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:55: bool
sign_direct_pkcs_padded = false;
It feels like by moving this into a struct, you're using in class initialization
(
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Initialization
) 

to avoid
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum...

At the least, it seems like SignRSA() should take params "as expected", and if
it needs to bundle into a struct, that should be an internal implementation
detail for marshalling, not a public API contract.

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:90: };
Ditto this. I don't really understand this structure much :(

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:466: case
HASH_ALGORITHM_NONE:
Add a default? Or are we relying on the compiler to yell at us? Are we sure it
will?

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

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:218:
callback.Run(!permission_check_enabled_ || key_was_valid);
For the life of me, I cannot understand what this code is doing :/

It just seems a lot of negations and double negations and such.

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/extensio...
File chrome/browser/extensions/api/platform_keys/platform_keys_api.cc (right):

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:33: // The
default public exponent 65537 is assumed.
This comment needs updating, doesn't it? You no longer assume, but enforce.

https://codereview.chromium.org/884073002/diff/280001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/platform_keys/basic.js (right):

https://codereview.chromium.org/884073002/diff/280001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/platform_keys/basic.js:29: // The public
key of clientCert1 as Subject Public Key Info in DER encoding.
comment: no longer clientCert1 but client_1, right?

https://codereview.chromium.org/884073002/diff/280001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/platform_keys/basic.js:34: // The
distinguished name of the CA that issued clientCert1 in DER encoding.
ditto comment update

[pneubeck (no reviews), 2015-02-08 10:26:08]

New patchsets have been uploaded after l-g-t-m from kalman@chromium.org

[pneubeck (no reviews), 2015-02-08 10:44:11]

Patchset #9 (id:300001) has been deleted

[pneubeck (no reviews), 2015-02-08 10:49:50]

Patchset #9 (id:320001) has been deleted

[pneubeck (no reviews), 2015-02-08 10:52:00]

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

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:55: bool
sign_direct_pkcs_padded = false;
The possibility of using this as default arguments wasn't intentional.
Changed to explicit constructors to prevent that.
(With the side effect of 3-times as much code...)

I'm not convinced about the 'should take params "as expected"' as the number of
arguments is already significant and any passing of this number of arguments is
a huge amount of boilerplate that makes the code less readable.
This class is meant to encapsulate these arguments, so that code that just
passes them around is oblivious to its content.
(And I have already code that just passes it around.)

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:90: };
On 2015/02/07 02:09:40, Ryan Sleevi wrote:
> Ditto this. I don't really understand this structure much :(

I can only guess what you find problematic here.
As this struct seems not to form a reasonable general concept to you, I changed
it's description/intention to only hold the results of the GetPublicKey
function.
That function happens to return several seemingly unrelated properties because I
didn't want unnecessary many calls to CERT_ExtractPublicKey (because of
potential runtime cost).
If a larger number of calls to CERT_ExtractPublicKey is unproblematic, I can
also split the function into several with more conceptually reasonable results.

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:466: case
HASH_ALGORITHM_NONE:
On 2015/02/07 02:09:40, Ryan Sleevi wrote:
> Add a default? Or are we relying on the compiler to yell at us? Are we sure it
> will?

yes it will.
'default' prevents the completeness check by the compiler.

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

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:218:
callback.Run(!permission_check_enabled_ || key_was_valid);
On 2015/02/07 02:09:40, Ryan Sleevi wrote:
> For the life of me, I cannot understand what this code is doing :/
> 
> It just seems a lot of negations and double negations and such.

done.
(note that this code is temporary and will be removed by the CL that adds
permissions.)

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/extensio...
File chrome/browser/extensions/api/platform_keys/platform_keys_api.cc (right):

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:33: // The
default public exponent 65537 is assumed.
On 2015/02/07 02:09:41, Ryan Sleevi wrote:
> This comment needs updating, doesn't it? You no longer assume, but enforce.

Maybe my english understanding is flawed here. Probably it's not correct that
"Propositions that A ensures become assumptions/premises of B".
Anyways, changed.

[pneubeck (no reviews), 2015-02-08 10:54:16]

On 2015/02/07 02:09:41, Ryan Sleevi wrote:
> I have not reviewed the JS tests, and I get the feeling from Ben's reply that
he
> hasn't either. I'm certainly not capable of reviewing them. Perhaps Eric can
be
> baited into reviewing.
I'll find someone, no worries.

> On the matter of DER, uploading binary files is almost universally a pain.
Just
> base64 them and window.atob/btoa after the XHR?
Please clarify what pain you're referring to.
I didn't run into any issues with 'git cl upload' and the XHR seemingly supports
binary payload as well.

[pneubeck (no reviews), 2015-02-08 10:55:11]

@Ryan, ptal whether your comments were addressed. Thanks.

[pneubeck (no reviews), 2015-02-08 15:11:05]

pneubeck@chromium.org changed reviewers:
+ kaliamoorthi@chromium.org
- eroman@chromium.org

[pneubeck (no reviews), 2015-02-08 15:11:06]

-Eric

@Prabhu, ptal at the tests in basic.js & apitest_nss.cc . Thanks.

[pneubeck (no reviews), 2015-02-08 15:11:17]

pneubeck@chromium.org changed required reviewers:
+ kaliamoorthi@chromium.org

[pneubeck (no reviews), 2015-02-08 15:52:07]

https://codereview.chromium.org/884073002/diff/280001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/platform_keys/basic.js (right):

https://codereview.chromium.org/884073002/diff/280001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/platform_keys/basic.js:29: // The public
key of clientCert1 as Subject Public Key Info in DER encoding.
On 2015/02/07 02:09:41, Ryan Sleevi wrote:
> comment: no longer clientCert1 but client_1, right?

Done.

https://codereview.chromium.org/884073002/diff/280001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/platform_keys/basic.js:34: // The
distinguished name of the CA that issued clientCert1 in DER encoding.
On 2015/02/07 02:09:41, Ryan Sleevi wrote:
> ditto comment update

Done.

[pneubeck (no reviews), 2015-02-09 10:31:16]

Patchset #11 (id:380001) has been deleted

[kaliamoorthi, 2015-02-09 13:37:56]

LGTM for basic.js after fixing nit.

https://codereview.chromium.org/884073002/diff/360001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/platform_keys/basic.js (right):

https://codereview.chromium.org/884073002/diff/360001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/platform_keys/basic.js:51: // Reads the
binary file at |path| and passes it as a Uin8Array to |callbac|.
|callback|

[Ryan Sleevi, 2015-02-10 00:59:33]

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

https://codereview.chromium.org/884073002/diff/280001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:90: };
On 2015/02/08 10:52:00, pneubeck wrote:
> On 2015/02/07 02:09:40, Ryan Sleevi wrote:
> > Ditto this. I don't really understand this structure much :(
> 
> I can only guess what you find problematic here.
> As this struct seems not to form a reasonable general concept to you, I
changed
> it's description/intention to only hold the results of the GetPublicKey
> function.
> That function happens to return several seemingly unrelated properties because
I
> didn't want unnecessary many calls to CERT_ExtractPublicKey (because of
> potential runtime cost).
> If a larger number of calls to CERT_ExtractPublicKey is unproblematic, I can
> also split the function into several with more conceptually reasonable
results.

Sorry I wasn't clearer:

I don't understand why you don't do

bool GetPublicKey(scoped_refptr<X509Certificate> certificate,
                  std::string* spki_der,
                  net::X509Certificate::PublicKeyType* key_type,
                  size_t* key_size_in_bits);

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

https://codereview.chromium.org/884073002/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:45: class SignRSAParams {
I still have an issue with the SignRSAParams, which is more or less the same as
the issue with GetPublicKeyResult.

I don't think either of these APIs needs to be explicitly using a struct to
bundle arguments. I have no objection if your internal implementation uses such
a struct for thread marshalling, but I can't help but feel these should be two
explicit methods

SignRSA[PKCS1?]Digest(token_id, data, public_key, hash_algorithm, callback,
browser_context)

SignRSA[PKCS1?]Raw(token_id, data, public_key, callback, browser_context)


The struct created the case where the object could be in an inconsitent state,
which you've now has to resolve by making this object be a class with heap-based
helper functions.

https://codereview.chromium.org/884073002/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:100: const HashAlgorithm
hash_algorithm_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/884073002/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:183: bool
GetPublicKey(scoped_refptr<net::X509Certificate> certificate,
This should be "const scoped_refptr<...>&"

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

https://codereview.chromium.org/884073002/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:220: // it was
removed above).
To make sure I'm understanding: You only allow one signing operation with the
key, right?

That's what line 209-215 cover? You allow it to be used once?

[pneubeck (no reviews), 2015-02-10 10:21:49]

New patchsets have been uploaded after l-g-t-m from kaliamoorthi@chromium.org

[pneubeck (no reviews), 2015-02-10 10:38:52]

Patchset #12 (id:420001) has been deleted

[pneubeck (no reviews), 2015-02-10 10:40:52]

@Ryan, ptal

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

https://codereview.chromium.org/884073002/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:45: class SignRSAParams {
On 2015/02/10 00:59:32, Ryan Sleevi wrote:
> I still have an issue with the SignRSAParams, which is more or less the same
as
> the issue with GetPublicKeyResult.
> 
> I don't think either of these APIs needs to be explicitly using a struct to
> bundle arguments.
If I follow your reasoning correctly, than _no_ API in Chrome should ever expose
arguments bundled in a struct/class.
I find that very surprising for the reasons explained:

- Code using the API will have to pass around individual arguments even if it
should/could be oblivious to the individual arguments. That creates unnecessary
depedencies and thus makes future maintenance and refactorings harder.

- Similarly for the reader, this tends to expose more details than he needs to
see. In some cases it is totally sufficient to know about a 'sign operation with
arguments X' vs. a 'sign operation with public key XY and hash algorithm YZ and
data D, ....'. If details have to be known, he can look back at the source of
the argument.

- Unnamed arguments: On the caller site multiple arguments of the same type are
indistinguishable. A struct/class can clarify that.

- Boilerplate: in every case it increases the amount of code necessary to call
the function.

> I have no objection if your internal implementation uses such
> a struct for thread marshalling,
It complicates the layers on top as well.
As I explained already, the SignRSA function _is_ already in the namespace
'subtle'. The public interface (except the extension API) for signing is in the
PlatformKeysService.

> but I can't help but feel these should be two
> explicit methods
> 
> SignRSA[PKCS1?]Digest(token_id, data, public_key, hash_algorithm, callback,
> browser_context)
> 
> SignRSA[PKCS1?]Raw(token_id, data, public_key, callback, browser_context)
> 
> 
> The struct created the case where the object could be in an inconsitent state,
Inconsistency can be CHECKed without much boilerplate overhead.

> which you've now has to resolve by making this object be a class with
heap-based
> helper functions.
It doesn't have to be heap-based but comes in handy here as it prevents repeated
copies of the data and public_key strings.

https://codereview.chromium.org/884073002/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:100: const HashAlgorithm
hash_algorithm_;
On 2015/02/10 00:59:33, Ryan Sleevi wrote:
> DISALLOW_COPY_AND_ASSIGN

Acknowledged.

https://codereview.chromium.org/884073002/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:183: bool
GetPublicKey(scoped_refptr<net::X509Certificate> certificate,
On 2015/02/10 00:59:33, Ryan Sleevi wrote:
> This should be "const scoped_refptr<...>&"

Done.

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

https://codereview.chromium.org/884073002/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_service.cc:220: // it was
removed above).
On 2015/02/10 00:59:33, Ryan Sleevi wrote:
> To make sure I'm understanding: You only allow one signing operation with the
> key, right?
> 
> That's what line 209-215 cover? You allow it to be used once?

Outside the new platform_keys_apitest_nss.cc the permission check is not
changed, because |permission_check_enabled_| is true (in other tests and in
production).

The existing permission check ensures that an extension, that generates a key,
can use the key _at most once_ for signing.
That's ensured by line 211 (remove the key from the permission list) and line
215 (persist the reduced list).

[not at google - send to devlin, 2015-02-10 20:37:08]

Ryan, why aren't you capable of reviewing the JS tests? I had a go and they're
quite technically focused on the crypto part.

[Ryan Sleevi, 2015-02-10 20:46:03]

On 2015/02/10 20:37:08, kalman wrote:
> Ryan, why aren't you capable of reviewing the JS tests? I had a go and they're
> quite technically focused on the crypto part.

I have zero experience with our JS style guide and zero experience the idioms of
JS code in Chromium or Google. It would be irresponsible of me as a reviewer to
suggest I'm capable of offering meaningful feedback on that code - or alerting
to pitfalls - beyond a simple reading.

[Ryan Sleevi, 2015-02-10 21:25:28]

It seems "git cl" & the CQ now support binary files.

They're a little bit of a pain to deal with (can't grab from the web source
control interface, AFACIT; seems you need a full checkout), so there's a light
preference for encoding those in base64 just so it's easy to poke from the web
interface, but that's not a big deal.

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

https://codereview.chromium.org/884073002/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:45: class SignRSAParams {
On 2015/02/10 10:40:52, pneubeck wrote:
> If I follow your reasoning correctly, than _no_ API in Chrome should ever
expose
> arguments bundled in a struct/class.

Generally speaking, yes. That's correct. Don't make C++ try to look like
Javascript.

> - Code using the API will have to pass around individual arguments even if it
> should/could be oblivious to the individual arguments. That creates
unnecessary
> depedencies and thus makes future maintenance and refactorings harder.

I admit I don't entirely follow this argument, but I consider the line of
reasoning somewhat specious, to put it bluntly.

> - Similarly for the reader, this tends to expose more details than he needs to
> see. In some cases it is totally sufficient to know about a 'sign operation
with
> arguments X' vs. a 'sign operation with public key XY and hash algorithm YZ
and
> data D, ....'. If details have to be known, he can look back at the source of
> the argument.

I _strongly_ disagree with this. I think your proposed change harms, not helps,
readability.
 
> - Unnamed arguments: On the caller site multiple arguments of the same type
are
> indistinguishable. A struct/class can clarify that.

This is C++. This is true for every single method call ever. This is perhaps the
weakest argument.

> > I have no objection if your internal implementation uses such
> > a struct for thread marshalling,
> It complicates the layers on top as well.
> As I explained already, the SignRSA function _is_ already in the namespace
> 'subtle'. The public interface (except the extension API) for signing is in
the
> PlatformKeysService.

Subtle is not an escape clause to make code less readable or to diverge from
common practice. Yours is the first code I've seen in five years on Chrome to
make this argument, and that alone should be somewhat telling. I'm not trying to
pick on you - I think this code, from a JS idiomatic sense, probably makes
sense. But you essentially have two methods, with two common arguments. That
does not make a suitable justification for a struct, with creators, and added
DCHECKs, and further validation being required on methods that accept this.

> Inconsistency can be CHECKed without much boilerplate overhead.

That you have to at all is a design flaw.


It's clear from your reply that you feel this is important that it's an
acceptable pattern. I feel the otherwise - I think it hinders readability, adds
more code than necessary (look at how much just to describe the struct, let
alone add the error checks), and it does not feel idiomatic C++ to me, in
Chromium or at large. I don't feel comfortable approving the change with the
struct, because I don't see any reason for its technical necessity. We can take
it to the mailing list if you'd like additional opinions, because I don't want
to be "Do it my way or else", but I also feel like I need to hold the line on
"Don't do it this way".

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

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:120: // not equal to
65537.
Comment nit: This reads somewhat weird. Perhaps reword

// Obtains information about the public key in |certificate|
// If |certificate| contains an RSA key, sets |key_size_bits| to the modulus
length, |public_key_spki_der|
// to the DER encoding of the X.509 Subject Public Key Info, and |key_type| to
RSA.
// If |certificate| contains any other key type, or if the public exponent of
the RSA key in |certificate| is not F4, returns false and does not update any of
the output parameters.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:128: size_t*
out_key_size_bits);
naming nit: None of these need to be named |out_| for any stylistic reasons. It
does seem somewhat unnecessary (much like hungarian notation for variable
names), so I'd suggest it's just dropped and they become |public_key_spki_der|,
|key_type|, |key_size_bits|.

The pointer types are already strong indicators that they're out params (whether
pure out or in/out), per the Google style guide, so I feel that information is
already there (and in the comment)

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:459: SECItem input =
{siBuffer, vector_as_array(&data_vec), data_vec.size()};
It doesn't seem necessary to make the copy of |data_vec|. Why can't you just

reinterpret_cast<unsigned char*>(const_cast<char*>(state->data_.data()))

While more typing, this does feel more idiomatic with our crypto code.

(The const vs non-const is an artifact of NSS's ABI limitations. WE can make
PK11_Sign take a "const SECItem", but it still means the .data pointer of the
secitem is non-const, even if the data isn't mutated)

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:772: bool
GetPublicKey(const scoped_refptr<net::X509Certificate>& certificate,
API: Don't make the three output parameters optional.

First, it creates a situation where all three parameters may be nullptr, which
is a little weird, because it means you did all this work for nothing.

Second, it doesn't seem like you're saving any significant amount of copying
(even the SPKI copy is lightweight) to be worth omitting it.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:787: if (key_type ==
net::X509Certificate::kPublicKeyTypeRSA) {
And what about ECC keys? Should they be rejected with "return false"?

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
File chrome/browser/extensions/api/platform_keys/platform_keys_api.cc (right):

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:35:
net::X509Certificate::PublicKeyType key_type;
Why not default-assign this to unknown, given that this is a primitive type?

It's weird to have the =0 for key_size_bits, since the default constructor for
PublicKeyInfo that the compiler will generate will default-initialize (ergo,
zero-initialize numerics). The = 0 makes this explicit to the reader, but it
seems more useful to make explicit the default value of the key type (which
would be the enum value equivalent to zero, IIRC)

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:53: const char
defaultPublicExponent[] = {0x01, 0x00, 0x01};
nit: For safety (in the event of future type changes), this should be unsigned
char.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:105: const
std::vector<char>& cert_der = params->certificate;
BUG? What if |cert_der.empty()|

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:125:
std::vector<char>(key_info.public_key_spki_der.begin(),
Why |char| and not |unsigned char|?

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:217: if
(params->hash_algorithm_name == "none") {
Did you mean to do case-sensitive comparisons for all of these?

[pneubeck (no reviews), 2015-02-11 14:37:05]

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

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:120: // not equal to
65537.
On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> Comment nit: This reads somewhat weird. Perhaps reword
> 
> // Obtains information about the public key in |certificate|
> // If |certificate| contains an RSA key, sets |key_size_bits| to the modulus
> length, |public_key_spki_der|
> // to the DER encoding of the X.509 Subject Public Key Info, and |key_type| to
> RSA.
> // If |certificate| contains any other key type, or if the public exponent of
> the RSA key in |certificate| is not F4, returns false and does not update any
of
> the output parameters.

adapted the implementation to ensure that false is returned on non-RSA keys.

Done.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:128: size_t*
out_key_size_bits);
On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> naming nit: None of these need to be named |out_| for any stylistic reasons.
It
> does seem somewhat unnecessary (much like hungarian notation for variable
> names), so I'd suggest it's just dropped and they become
|public_key_spki_der|,
> |key_type|, |key_size_bits|.
> 
> The pointer types are already strong indicators that they're out params
(whether
> pure out or in/out), per the Google style guide, so I feel that information is
> already there (and in the comment)

Done.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:459: SECItem input =
{siBuffer, vector_as_array(&data_vec), data_vec.size()};
On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> It doesn't seem necessary to make the copy of |data_vec|. Why can't you just
> 
> reinterpret_cast<unsigned char*>(const_cast<char*>(state->data_.data()))
> 
> While more typing, this does feel more idiomatic with our crypto code.
> 
> (The const vs non-const is an artifact of NSS's ABI limitations. WE can make
> PK11_Sign take a "const SECItem", but it still means the .data pointer of the
> secitem is non-const, even if the data isn't mutated)

Done.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:772: bool
GetPublicKey(const scoped_refptr<net::X509Certificate>& certificate,
On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> API: Don't make the three output parameters optional.
> 
> First, it creates a situation where all three parameters may be nullptr, which
> is a little weird, because it means you did all this work for nothing.
> 
> Second, it doesn't seem like you're saving any significant amount of copying
> (even the SPKI copy is lightweight) to be worth omitting it.

Done.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:787: if (key_type ==
net::X509Certificate::kPublicKeyTypeRSA) {
On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> And what about ECC keys? Should they be rejected with "return false"?

I didn't see need for that here, as the caller of this function can itself
decide that on the value of |key_type|.

As it doesn't matter for now and simplifies this function, rejecting ECC keys.

Done.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
File chrome/browser/extensions/api/platform_keys/platform_keys_api.cc (right):

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:35:
net::X509Certificate::PublicKeyType key_type;
On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> Why not default-assign this to unknown, given that this is a primitive type?
> 
> It's weird to have the =0 for key_size_bits, since the default constructor for
> PublicKeyInfo that the compiler will generate will default-initialize (ergo,
> zero-initialize numerics). The = 0 makes this explicit to the reader, but it
> seems more useful to make explicit the default value of the key type (which
> would be the enum value equivalent to zero, IIRC)

Looking at the specification of initialization/default constructor which is
different for every C++ version, I'd better not rely on any of this.
Added the default key_type and keeping the size_t initialization which is
consistent with the style guide.

"Use in-class member initialization for simple initializations, especially when
a member variable must be initialized the same way in more than one constructor.

If your class defines member variables that aren't initialized in-class, and if
it has no other constructors, you must define a default constructor (one that
takes no arguments). [...]"

Done.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:53: const char
defaultPublicExponent[] = {0x01, 0x00, 0x01};
On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> nit: For safety (in the event of future type changes), this should be unsigned
> char.

base::BinaryValue::CreateWithCopiedBuffer takes a const char*.
changing to 'const unsigned char defaultPublicExponent[]' does not compile.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:105: const
std::vector<char>& cert_der = params->certificate;
On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> BUG? What if |cert_der.empty()|

net::X509Certificate::CreateFromBytes should likely handle this.
To be on the safe side, I added explicit handling of this case here.

Done.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:125:
std::vector<char>(key_info.public_key_spki_der.begin(),
On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> Why |char| and not |unsigned char|?

api_pki::GetPublicKey::Results::Create is generated from the API's .idl .
The generator always creates the same C++ representation for all occurrences of
the JS ArrayBuffer type, namely std::vector<char> .

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:217: if
(params->hash_algorithm_name == "none") {
On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> Did you mean to do case-sensitive comparisons for all of these?

Yes.
This API function is internal and only called by specific Javascript code which
ensures to pass the right strings.
Once the json_schema_compiler supports it, these strings can be replaced by an
enum / generated constants.

[pneubeck (no reviews), 2015-02-11 15:57:51]

@Ryan, please take a look at the last patch set.

Prabhu reviewed basic.js and platform_keys_apitest_nss.cc .

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

https://codereview.chromium.org/884073002/diff/400001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys.h:45: class SignRSAParams {
Aaalright... the line has to bend to not cross it.

On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> On 2015/02/10 10:40:52, pneubeck wrote:
> > If I follow your reasoning correctly, than _no_ API in Chrome should ever
> expose
> > arguments bundled in a struct/class.
> 
> Generally speaking, yes. That's correct. Don't make C++ try to look like
> Javascript.
> 
> > - Code using the API will have to pass around individual arguments even if
it
> > should/could be oblivious to the individual arguments. That creates
> unnecessary
> > depedencies and thus makes future maintenance and refactorings harder.
> 
> I admit I don't entirely follow this argument, but I consider the line of
> reasoning somewhat specious, to put it bluntly.
> 
> > - Similarly for the reader, this tends to expose more details than he needs
to
> > see. In some cases it is totally sufficient to know about a 'sign operation
> with
> > arguments X' vs. a 'sign operation with public key XY and hash algorithm YZ
> and
> > data D, ....'. If details have to be known, he can look back at the source
of
> > the argument.
> 
> I _strongly_ disagree with this. I think your proposed change harms, not
helps,
> readability.
>  
> > - Unnamed arguments: On the caller site multiple arguments of the same type
> are
> > indistinguishable. A struct/class can clarify that.
> 
> This is C++. This is true for every single method call ever. This is perhaps
the
> weakest argument.
> 
> > > I have no objection if your internal implementation uses such
> > > a struct for thread marshalling,
> > It complicates the layers on top as well.
> > As I explained already, the SignRSA function _is_ already in the namespace
> > 'subtle'. The public interface (except the extension API) for signing is in
> the
> > PlatformKeysService.
> 
I referenced 'namespace subtle' only to explain that this could qualify as
'internal implementation' of the PlatformKeysService because it didn't happen to
be in platform_keys_service_nss.cc .
I mentioned that in reply to your "... I have no objection if your internal
implementation uses such a struct for thread marshalling ...". But apparently I
didn't read your emphasis on 'thread marshalling'.

> Subtle is not an escape clause to make code less readable or to diverge from
> common practice. Yours is the first code I've seen in five years on Chrome to
> make this argument, and that alone should be somewhat telling. I'm not trying
to
> pick on you - I think this code, from a JS idiomatic sense, probably makes
> sense. But you essentially have two methods, with two common arguments. That
> does not make a suitable justification for a struct, with creators, and added
> DCHECKs, and further validation being required on methods that accept this.
> 
> > Inconsistency can be CHECKed without much boilerplate overhead.
> 
> That you have to at all is a design flaw.
> 
> 
> It's clear from your reply that you feel this is important that it's an
> acceptable pattern. I feel the otherwise - I think it hinders readability,
adds
> more code than necessary (look at how much just to describe the struct, let
> alone add the error checks), and it does not feel idiomatic C++ to me, in
> Chromium or at large. I don't feel comfortable approving the change with the
> struct, because I don't see any reason for its technical necessity. We can
take
> it to the mailing list if you'd like additional opinions, because I don't want
> to be "Do it my way or else", but I also feel like I need to hold the line on
> "Don't do it this way".

[pneubeck (no reviews), 2015-02-12 09:32:30]

pneubeck@chromium.org changed reviewers:
+ asvitkine@chromium.org

[pneubeck (no reviews), 2015-02-12 09:32:31]

asvitkine@chromium.org: Please review the histogram changes. Thanks.

[pneubeck (no reviews), 2015-02-12 09:32:46]

pneubeck@chromium.org changed required reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-02-12 15:33:46]

lgtm

[Ryan Sleevi, 2015-02-13 03:24:28]

LGTM. Thanks for putting up with my pedantry.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
File chrome/browser/extensions/api/platform_keys/platform_keys_api.cc (right):

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:53: const char
defaultPublicExponent[] = {0x01, 0x00, 0x01};
On 2015/02/11 14:37:05, pneubeck wrote:
> On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> > nit: For safety (in the event of future type changes), this should be
unsigned
> > char.
> 
> base::BinaryValue::CreateWithCopiedBuffer takes a const char*.
> changing to 'const unsigned char defaultPublicExponent[]' does not compile.

At this risk of pedantry, I'd recommend a reinterpret cast then.

{0xF1, 0x00, 0x01} can cause... problems. Much like {128, 0, 1}. That is, the
compiler may truncate to the maximum range (or may yell). Forcing it through the
reinterpret_cast<> leaves those bytes pristine in memory and unmolested in the
cast.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:105: const
std::vector<char>& cert_der = params->certificate;
On 2015/02/11 14:37:05, pneubeck wrote:
> On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> > BUG? What if |cert_der.empty()|
> 
> net::X509Certificate::CreateFromBytes should likely handle this.

It does not. It's an API violation to supply that. Either way, validating user
input should happen as early as possible, so thanks for adding this.

> To be on the safe side, I added explicit handling of this case here.
> 
> Done.

https://codereview.chromium.org/884073002/diff/500001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/884073002/diff/500001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:460: "Can't
reinterpret data if it's characters are not 8 bit large.");
C++03, Section 5.3.3, p1

sizeof(char), sizeof(signed char), and sizeof(unsigned char) are 1.

C++03, Section 3.9.1, p1

"A char, a signed char, and an unsigned char occupy the same amount of storage
and have the same alignment requirements (3.9), that is, they have the same
object representation"

So you can remove this static assert.

https://codereview.chromium.org/884073002/diff/500001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:785:
CHECK(key_size_bits);
jschuh has advised me in the past to explicitly remove such checks.

If they're NULL, we'll crash on a null-deref, and all is good.

[pneubeck (no reviews), 2015-02-13 05:52:20]

https://codereview.chromium.org/884073002/diff/500001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/884073002/diff/500001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:460: "Can't
reinterpret data if it's characters are not 8 bit large.");
On 2015/02/13 03:24:28, Ryan Sleevi wrote:
> C++03, Section 5.3.3, p1
> 
> sizeof(char), sizeof(signed char), and sizeof(unsigned char) are 1.
> 
> C++03, Section 3.9.1, p1
> 
> "A char, a signed char, and an unsigned char occupy the same amount of storage
> and have the same alignment requirements (3.9), that is, they have the same
> object representation"
> 
> So you can remove this static assert.

This check was meant to trigger in case someone changes |state->data_| to
another type that is not compatible.
Sorry that I didn't make that clearer.

E.g. this could be changed to some vector<uint??> which provides also a ::data()
member function in C++11.

[pneubeck (no reviews), 2015-02-14 13:25:14]

I'll go ahead an commit this.
If you have any further comments I can do a follow up.

Thanks for reviewing.

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
File chrome/browser/extensions/api/platform_keys/platform_keys_api.cc (right):

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:53: const char
defaultPublicExponent[] = {0x01, 0x00, 0x01};
On 2015/02/13 03:24:28, Ryan Sleevi wrote:
> On 2015/02/11 14:37:05, pneubeck wrote:
> > On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> > > nit: For safety (in the event of future type changes), this should be
> unsigned
> > > char.
> > 
> > base::BinaryValue::CreateWithCopiedBuffer takes a const char*.
> > changing to 'const unsigned char defaultPublicExponent[]' does not compile.
> 
> At this risk of pedantry, I'd recommend a reinterpret cast then.
> 
> {0xF1, 0x00, 0x01} can cause... problems. Much like {128, 0, 1}. That is, the
> compiler may truncate to the maximum range (or may yell). Forcing it through
the
> reinterpret_cast<> leaves those bytes pristine in memory and unmolested in the
> cast.

I don't quite follow.
It's {0x01, ...} and not {0xF1, ...} which should not cause issues for any size
of char or any other numeric type.
Do you suggest protecting against an accidental change of
defaultPublicExponent's value (e.g. to {0xF1, ....}) or against a change of its
declared type or against a change of the used size of char?
(your first comment mentioned 'future type changes')

https://codereview.chromium.org/884073002/diff/500001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/884073002/diff/500001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:785:
CHECK(key_size_bits);
On 2015/02/13 03:24:28, Ryan Sleevi wrote:
> jschuh has advised me in the past to explicitly remove such checks.
> 
> If they're NULL, we'll crash on a null-deref, and all is good.

Done.

[pneubeck (no reviews), 2015-02-14 13:31:18]

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
File chrome/browser/extensions/api/platform_keys/platform_keys_api.cc (right):

https://codereview.chromium.org/884073002/diff/460001/chrome/browser/extensio...
chrome/browser/extensions/api/platform_keys/platform_keys_api.cc:53: const char
defaultPublicExponent[] = {0x01, 0x00, 0x01};
On 2015/02/14 13:25:14, pneubeck wrote:
> On 2015/02/13 03:24:28, Ryan Sleevi wrote:
> > On 2015/02/11 14:37:05, pneubeck wrote:
> > > On 2015/02/10 21:25:28, Ryan Sleevi wrote:
> > > > nit: For safety (in the event of future type changes), this should be
> > unsigned
> > > > char.
> > > 
> > > base::BinaryValue::CreateWithCopiedBuffer takes a const char*.
> > > changing to 'const unsigned char defaultPublicExponent[]' does not
compile.
> > 
> > At this risk of pedantry, I'd recommend a reinterpret cast then.
> > 
> > {0xF1, 0x00, 0x01} can cause... problems. Much like {128, 0, 1}. That is,
the
> > compiler may truncate to the maximum range (or may yell). Forcing it through
> the
> > reinterpret_cast<> leaves those bytes pristine in memory and unmolested in
the
> > cast.
> 
> I don't quite follow.
> It's {0x01, ...} and not {0xF1, ...} which should not cause issues for any
size
> of char or any other numeric type.
> Do you suggest protecting against an accidental change of
> defaultPublicExponent's value (e.g. to {0xF1, ....}) or against a change of
its
> declared type or against a change of the used size of char?
> (your first comment mentioned 'future type changes')

added the reinterpert_cast to follow the pattern in net/

[pneubeck (no reviews), 2015-02-14 13:34:53]

New patchsets have been uploaded after l-g-t-m from
asvitkine@chromium.org,rsleevi@chromium.org

[pneubeck (no reviews), 2015-02-14 13:36:17]

pneubeck@chromium.org changed reviewers:
+ arv@chromium.org

[pneubeck (no reviews), 2015-02-14 13:36:18]

arv@chromium.org: Please review changes in 
  chrome/renderer/resources/renderer_resources.grd

[pneubeck (no reviews), 2015-02-14 13:37:04]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2015-02-14 13:37:08]

The CQ bit was unchecked by pneubeck@chromium.org

[commit-bot: I haz the power, 2015-02-14 13:45:17]

Patchset 17 (id:??) landed as
https://crrev.com/594333f07c9d287c268f4d04259f74e08fc20bb9
Cr-Commit-Position: refs/heads/master@{#316386}

[pneubeck (no reviews), 2015-02-14 13:45:26]

Committed patchset #17 (id:540001) manually as
594333f07c9d287c268f4d04259f74e08fc20bb9 (presubmit successful).