Change the WebCrypto behavior when importing EC private keys without a public key.

Change the WebCrypto behavior when exporting EC private keys that were imported without a public key.

Before such keys were exported to PKCS8 format, the publicKey would be missing in the ECPrivateKey.

Now instead the publicKey is written to exported bytes even if it was not provided during import.

This is more consistent with how export works for JWK, where the public key is a required parameter.

It also speeds up the structured (de) cloning of such keys, which uses PKCS8 format under the hood (and for which if the publicKey was not provided, BoringSSL computes it).

BUG=445635
Committed: https://crrev.com/6a87d7f37462fe33d0da8b730ce7f81e31063790
Cr-Commit-Position: refs/heads/master@{#311553}

[eroman, 2015-01-02 02:11:36]

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

[Ryan Sleevi, 2015-01-02 22:45:23]

LGTM % a naming nit that I'm not too comfortable with.

https://codereview.chromium.org/835633002/diff/1/content/child/webcrypto/open...
File content/child/webcrypto/openssl/ec_algorithm_openssl.cc (right):

https://codereview.chromium.org/835633002/diff/1/content/child/webcrypto/open...
content/child/webcrypto/openssl/ec_algorithm_openssl.cc:127:
EC_KEY_set_enc_flags(ec.get(), enc_flags);
davidben should review this

https://codereview.chromium.org/835633002/diff/1/content/test/data/webcrypto/...
File content/test/data/webcrypto/ec_private_keys.json (right):

https://codereview.chromium.org/835633002/diff/1/content/test/data/webcrypto/...
content/test/data/webcrypto/ec_private_keys.json:94: "pkcs8":
"308187020100301306072A8648CE3D020106082A8648CE3D030107046D306B02010104201FE33950C5F461124AE992C2BDFDF1C73B1615F571BD567E60D19AA1F48CDF42A144034200047C110C66DCFDA807F6E69E45DDB3C74F69A1484D203E8DC5ADA8E9A9DD7CB3C70DF448986E51BDE5D1576F99901F9C2C6A806A47FD907643A72B835597EFC8C6"
I find naming the inputs "pkcs8" and "pkcs8_input" to be confusing. Is there a
better naming you can suggest? expected_pkcs8 ?

[eroman, 2015-01-02 23:49:39]

https://codereview.chromium.org/835633002/diff/1/content/test/data/webcrypto/...
File content/test/data/webcrypto/ec_private_keys.json (right):

https://codereview.chromium.org/835633002/diff/1/content/test/data/webcrypto/...
content/test/data/webcrypto/ec_private_keys.json:94: "pkcs8":
"308187020100301306072A8648CE3D020106082A8648CE3D030107046D306B02010104201FE33950C5F461124AE992C2BDFDF1C73B1615F571BD567E60D19AA1F48CDF42A144034200047C110C66DCFDA807F6E69E45DDB3C74F69A1484D203E8DC5ADA8E9A9DD7CB3C70DF448986E51BDE5D1576F99901F9C2C6A806A47FD907643A72B835597EFC8C6"
On 2015/01/02 22:45:23, Ryan Sleevi wrote:
> I find naming the inputs "pkcs8" and "pkcs8_input" to be confusing. Is there a
> better naming you can suggest? expected_pkcs8 ?

A couple ideas:

 (1) Rename them to "original_pkcs8" and "exported_pkcs8" for this test case. In
all the other tests a symmetry between the imported and exported data is assumed
and it is simply called "pkcs8"

 (2) Same concept as above, but rather than make this test case special, require
all the other tests to specify separate input/output values. Less excited about
this as it leads to more duplication

 (3) Remove this test data from ec_private_keys and create a separate C++ test
that explores this individually. Less excited about more C++ code though.

 (4) Re-write the whole test so instead of constituting equivalent JWK/PKCS8
pairs (which breaks apart for this case) it instead expresses an input (format,
keydata) and an ouptut (format, keydata). To represent the same set of tests
more pairs will be needed (jwk -> jwk, pkcs8 -> pkcs8, jwk->pkcs8, pkcs8->jwk),
however it will be more readable for this particular instance.

[eroman, 2015-01-02 23:50:16]

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

[eroman, 2015-01-02 23:50:17]

David, could you please review the BoringSSL side of this change?

ec_algorithm_openssl.cc

[eroman, 2015-01-06 20:16:14]

https://codereview.chromium.org/835633002/diff/1/content/child/webcrypto/open...
File content/child/webcrypto/openssl/ec_algorithm_openssl.cc (left):

https://codereview.chromium.org/835633002/diff/1/content/child/webcrypto/open...
content/child/webcrypto/openssl/ec_algorithm_openssl.cc:119: if
(!EC_KEY_check_key(ec.get()))
Hmm, this removal was unintentional from having split CLs, adding it back.

[davidben, 2015-01-06 20:24:41]

lgtm.

Probably worth later as BoringSSL cleanup adding new ECPrivateKey encode/decode
functions, with encode taking enc_flags as parameter rather than burning them
into the EC_KEY and decode taking an EC_GROUP* as explicit parameter rather than
getting an EC_KEY template in the T** parameter. EC_KEYs knowing about encoding
decisions seems weird to me.

https://codereview.chromium.org/835633002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/openssl/ec_algorithm_openssl.cc (right):

https://codereview.chromium.org/835633002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/ec_algorithm_openssl.cc:123: if (enc_flags &
EC_PKEY_NO_PUBKEY) {
I maybe wouldn't bother with the check and drop lines 123-125 and 128, but
whatever.

[eroman, 2015-01-06 20:26:46]

https://codereview.chromium.org/835633002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/openssl/ec_algorithm_openssl.cc (right):

https://codereview.chromium.org/835633002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/ec_algorithm_openssl.cc:123: if (enc_flags &
EC_PKEY_NO_PUBKEY) {
On 2015/01/06 20:24:41, David Benjamin wrote:
> I maybe wouldn't bother with the check and drop lines 123-125 and 128, but
> whatever.

Good idea, will do!

[eroman, 2015-01-06 21:14:36]

https://codereview.chromium.org/835633002/diff/1/content/test/data/webcrypto/...
File content/test/data/webcrypto/ec_private_keys.json (right):

https://codereview.chromium.org/835633002/diff/1/content/test/data/webcrypto/...
content/test/data/webcrypto/ec_private_keys.json:94: "pkcs8":
"308187020100301306072A8648CE3D020106082A8648CE3D030107046D306B02010104201FE33950C5F461124AE992C2BDFDF1C73B1615F571BD567E60D19AA1F48CDF42A144034200047C110C66DCFDA807F6E69E45DDB3C74F69A1484D203E8DC5ADA8E9A9DD7CB3C70DF448986E51BDE5D1576F99901F9C2C6A806A47FD907643A72B835597EFC8C6"
On 2015/01/02 23:49:38, eroman wrote:
> On 2015/01/02 22:45:23, Ryan Sleevi wrote:
> > I find naming the inputs "pkcs8" and "pkcs8_input" to be confusing. Is there
a
> > better naming you can suggest? expected_pkcs8 ?
> 
> A couple ideas:
> 
>  (1) Rename them to "original_pkcs8" and "exported_pkcs8" for this test case.
In
> all the other tests a symmetry between the imported and exported data is
assumed
> and it is simply called "pkcs8"
> 
>  (2) Same concept as above, but rather than make this test case special,
require
> all the other tests to specify separate input/output values. Less excited
about
> this as it leads to more duplication
> 
>  (3) Remove this test data from ec_private_keys and create a separate C++ test
> that explores this individually. Less excited about more C++ code though.
> 
>  (4) Re-write the whole test so instead of constituting equivalent JWK/PKCS8
> pairs (which breaks apart for this case) it instead expresses an input
(format,
> keydata) and an ouptut (format, keydata). To represent the same set of tests
> more pairs will be needed (jwk -> jwk, pkcs8 -> pkcs8, jwk->pkcs8,
pkcs8->jwk),
> however it will be more readable for this particular instance.

Ryan: I have changed to strategy (1). You can see those differences here:
https://codereview.chromium.org/835633002/diff2/20001:40001/content/test/data...

Let me know if this is clearer.

https://codereview.chromium.org/835633002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/openssl/ec_algorithm_openssl.cc (right):

https://codereview.chromium.org/835633002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/ec_algorithm_openssl.cc:123: if (enc_flags &
EC_PKEY_NO_PUBKEY) {
On 2015/01/06 20:26:46, eroman wrote:
> On 2015/01/06 20:24:41, David Benjamin wrote:
> > I maybe wouldn't bother with the check and drop lines 123-125 and 128, but
> > whatever.
> 
> Good idea, will do!

Done.

[Ryan Sleevi, 2015-01-13 05:48:13]

lgtm

[eroman, 2015-01-14 20:39:28]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2015-01-14 20:40:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/835633002/60001

[commit-bot: I haz the power, 2015-01-14 22:32:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-14 22:33:18]

Patchset 4 (id:??) landed as
https://crrev.com/6a87d7f37462fe33d0da8b730ce7f81e31063790
Cr-Commit-Position: refs/heads/master@{#311553}