[refactor] More post-NSS WebCrypto cleanups (utility functions).

[refactor] More post-NSS WebCrypto cleanups (utility functions).

Started gutting webcrypto_util.{cc,h} and util_openssl.{cc,h}. (I want to eventually kill both of these files, or at least make them rational)

The main reason for webcrypto_util.cc was to share functionality between OpenSSL and NSS versions of algorithms.

With NSS gone, many of these functions can be moved to the single file that uses them.

util_openssl.cc is also a poorly scoped file, and the "openssl" distinction is no longer helpful. I started by splitting out the symmetric key specific stuff, and will find homes for the rest in follow-ups.

Committed: https://crrev.com/d72073d80760cd7cfc41075d3b18ac59b30757d3
Cr-Commit-Position: refs/heads/master@{#349834}

[eroman, 2015-09-18 18:55:30]

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

[davidben, 2015-09-18 22:16:00]

lgtm

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
File components/webcrypto/algorithms/aes.cc (right):

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/aes.cc:59: // BoringSSL does not support 192-bit
AES.
(We actually ended up putting most of it back, FWIW.)

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/aes.cc:181: // BoringSSL does not support
192-bit AES.
(Ditto.)

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
File components/webcrypto/algorithms/hmac.cc (right):

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/hmac.cc:36: *block_size_bits = 1024;
Probably for later, but OpenSSL knows this already. It's EVP_MD_block_size.

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
File components/webcrypto/algorithms/secret_key_util.cc (right):

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/secret_key_util.cc:29: if (keylen_bytes > 0) {
Optional: RAND_bytes does reasonable things if you pass in 0, so you might not
need this check.

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/secret_key_util.cc:30: if
(!(RAND_bytes(&random_bytes[0], keylen_bytes)))
&random_bytes[0] -> vector_as_array?

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/secret_key_util.cc:30: if
(!(RAND_bytes(&random_bytes[0], keylen_bytes)))
Style: unnecessary parens around RAND_byte?

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/jwk.h
File components/webcrypto/jwk.h (left):

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/jw...
components/webcrypto/jwk.h:150: JwkReader* jwk);
Is the intent that this file just be a generic JWK reader and that's why this
got moved? (I don't have an opinion. Just not super familiar with this code's
structure.)

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/we...
File components/webcrypto/webcrypto_util.cc (left):

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/we...
components/webcrypto/webcrypto_util.cc:8: #include "base/numerics/safe_math.h"
Nit: no longer necessary?

[eroman, 2015-09-18 23:13:24]

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
File components/webcrypto/algorithms/aes.cc (right):

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/aes.cc:59: // BoringSSL does not support 192-bit
AES.
On 2015/09/18 22:15:59, David Benjamin wrote:
> (We actually ended up putting most of it back, FWIW.)

Good to know! Moving that discussion to issue 533699.

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/aes.cc:181: // BoringSSL does not support
192-bit AES.
On 2015/09/18 22:15:59, David Benjamin wrote:
> (Ditto.)

I should update these comments, or just support 192-bit. Will follow-up through
the issue I filed.

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
File components/webcrypto/algorithms/hmac.cc (right):

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/hmac.cc:36: *block_size_bits = 1024;
On 2015/09/18 22:15:59, David Benjamin wrote:
> Probably for later, but OpenSSL knows this already. It's EVP_MD_block_size.

Thanks, I will look at that in a follow-up!
For now added a TODO so I don't forget.

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
File components/webcrypto/algorithms/secret_key_util.cc (right):

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/secret_key_util.cc:29: if (keylen_bytes > 0) {
On 2015/09/18 22:16:00, David Benjamin wrote:
> Optional: RAND_bytes does reasonable things if you pass in 0, so you might not
> need this check.

This check predated the use of vector_as_array(), so it was guarding calling
&rand_bytes[0] on an empty vector.

I have left it as > 0 check, since I don't think there is an advantage in
calling RAND_bytes() in the empty case.

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/secret_key_util.cc:30: if
(!(RAND_bytes(&random_bytes[0], keylen_bytes)))
On 2015/09/18 22:15:59, David Benjamin wrote:
> &random_bytes[0] -> vector_as_array?

Done.

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/al...
components/webcrypto/algorithms/secret_key_util.cc:30: if
(!(RAND_bytes(&random_bytes[0], keylen_bytes)))
On 2015/09/18 22:15:59, David Benjamin wrote:
> Style: unnecessary parens around RAND_byte?

Done.

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/jwk.h
File components/webcrypto/jwk.h (left):

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/jw...
components/webcrypto/jwk.h:150: JwkReader* jwk);
On 2015/09/18 22:16:00, David Benjamin wrote:
> Is the intent that this file just be a generic JWK reader and that's why this
> got moved? (I don't have an opinion. Just not super familiar with this code's
> structure.)

Yes. I would like jwk.{h, cc} to just contain the JwkReader/JwkWriter classes.
All other algorithm-specific stuff should be in the algorithm-specific files.

I could see it argued either way, but in my vision jwk.cc is just about the JWK
container format (http://tools.ietf.org/html/draft-ietf-jose-json-web-key), and
all the algorithm specific stuff JWA
(http://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms) falls into
per-algorithm details (hence files in the algorithms/* directory).

Ultimately I want WebCrypto algorithms to be as self-contained as possible, with
as few tendrils as possible into other files. Pretty much everything should be
possible to keep contained in the file defining the AlgorithmImplementation
subclass. Details on say how RSA encodes parameters into a JWK, or how secret
keys use the "oct" attribute are not general stuff worthy of jwk.cc in my mind,
but rather algorithm details to be hidden away into algorithms/*. In this
instance the RSA key parsing code is now part of algorithms/rsa.cc. And with
this change, the shared secret key JWK stuff is part of
algorithms/secret_key_util.cc

Hopefully the abstraction makes sense in more than just my brain.

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/we...
File components/webcrypto/webcrypto_util.cc (left):

https://codereview.chromium.org/1355873002/diff/20001/components/webcrypto/we...
components/webcrypto/webcrypto_util.cc:8: #include "base/numerics/safe_math.h"
On 2015/09/18 22:16:00, David Benjamin wrote:
> Nit: no longer necessary?

Done.

[eroman, 2015-09-19 00:59:05]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-09-19 00:59:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1355873002/#ps40001
(title: "address David's comments")

[commit-bot: I haz the power, 2015-09-19 00:59:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355873002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355873002/40001

[commit-bot: I haz the power, 2015-09-19 01:37:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-19 01:37:49]

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