Bug 169318 – [WebCrypto] Support SPKI/PKCS8 for Elliptic Curve

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jiewen Tan]

We should support SPKI/PKCS8 for Elliptic Curve according to the spec: https://www.w3.org/TR/WebCryptoAPI/#ecdh-operations.

[Radar WebKit Bug Importer]

<rdar://problem/31081956>

[Jiewen Tan]

Created attachment 304603 [details]
Patch

[Brent Fulgham]

Comment on attachment 304603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304603&action=review

Looks good! I had a few minor comments before you land. R=me.

> Source/WebCore/crypto/mac/CommonCryptoDERUtilities.h:43
> +    if (octet < 128)

I think these '128' magic number values should be represented as a constant.

> Source/WebCore/crypto/mac/CryptoKeyECMac.cpp:96
> +    for (size_t i = 0; i < length; i++) {

I'm surprised we don't already have an efficient compare you can use for this. Wouldn't memcmp do what you want here?

> Source/WebCore/crypto/mac/CryptoKeyECMac.cpp:272
> +    Vector<uint8_t> result;

Suggest you hint to Vector how large it will be to avoid reallocations. You seem to have the sizing information already here. (Vector::reserve)

> Source/WebCore/crypto/mac/CryptoKeyECMac.cpp:273
> +    result.append(SequenceMark);

I meant reserveCapacity

> Source/WebCore/crypto/mac/CryptoKeyECMac.cpp:377
> +    Vector<uint8_t> result;

ReserveCapacity

> LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/import_export/ec_importKey.worker-expected.txt:56
> +PASS Good parameters: P-384 bits (pkcs8, buffer(185), {name: ECDH, namedCurve: P-384}, false, [deriveBits]) 

Hooray! So many passes!

[Jiewen Tan]

Comment on attachment 304603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304603&action=review

Thanks Brent for r+ my patch.

>> Source/WebCore/crypto/mac/CommonCryptoDERUtilities.h:43
>> +    if (octet < 128)
> 
> I think these '128' magic number values should be represented as a constant.

Fixed.

>> Source/WebCore/crypto/mac/CryptoKeyECMac.cpp:96
>> +    for (size_t i = 0; i < length; i++) {
> 
> I'm surprised we don't already have an efficient compare you can use for this. Wouldn't memcmp do what you want here?

Oops. Always forget we have memcmp.

>> Source/WebCore/crypto/mac/CryptoKeyECMac.cpp:272
>> +    Vector<uint8_t> result;
> 
> Suggest you hint to Vector how large it will be to avoid reallocations. You seem to have the sizing information already here. (Vector::reserve)

Fixed. I pass the expected volume to the constructor directly.

[Jiewen Tan]

Comment on attachment 304603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304603&action=review

>> Source/WebCore/crypto/mac/CryptoKeyECMac.cpp:273
>> +    result.append(SequenceMark);
> 
> I meant reserveCapacity

Yup. Fixed it.

[Jiewen Tan]

Created attachment 304677 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 304677 [details]
Patch for landing

Clearing flags on attachment: 304677

Committed r214074: <http://trac.webkit.org/changeset/214074>