Bug 169318

Summary: [WebCrypto] Support SPKI/PKCS8 for Elliptic Curve
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, jiewen_tan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 166746    
Attachments:
Description Flags
Patch
bfulgham: review+
Patch for landing none

Description Jiewen Tan 2017-03-07 16:04:24 PST
We should support SPKI/PKCS8 for Elliptic Curve according to the spec: https://www.w3.org/TR/WebCryptoAPI/#ecdh-operations.
Comment 1 Radar WebKit Bug Importer 2017-03-15 21:21:14 PDT
<rdar://problem/31081956>
Comment 2 Jiewen Tan 2017-03-15 21:34:56 PDT
Created attachment 304603 [details]
Patch
Comment 3 Brent Fulgham 2017-03-16 07:45:03 PDT
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!
Comment 4 Jiewen Tan 2017-03-16 12:18:51 PDT
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.
Comment 5 Jiewen Tan 2017-03-16 12:22:49 PDT
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.
Comment 6 Jiewen Tan 2017-03-16 12:44:56 PDT
Created attachment 304677 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2017-03-16 15:32:25 PDT
Comment on attachment 304677 [details]
Patch for landing

Clearing flags on attachment: 304677

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