Bug 170789

Summary: [WebCrypto] Add support for ECDSA
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, 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+, bfulgham: commit-queue-
Patch for landing commit-queue: commit-queue-

Description Jiewen Tan 2017-04-12 14:15:52 PDT
Add support for ECDSA: https://www.w3.org/TR/WebCryptoAPI/#ecdsa
Comment 1 Radar WebKit Bug Importer 2017-04-12 14:16:41 PDT
<rdar://problem/31588604>
Comment 2 Jiewen Tan 2017-04-14 16:47:00 PDT
Created attachment 307168 [details]
Patch
Comment 3 Build Bot 2017-04-14 16:49:57 PDT
Attachment 307168 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:72:  CryptoAlgorithmRSASSA_PKCS1_v1_5::sign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:81:  CryptoAlgorithmRSASSA_PKCS1_v1_5::verify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 88 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Brent Fulgham 2017-04-14 20:14:56 PDT
Comment on attachment 307168 [details]
Patch

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

Looks great! Inching closer to complete WebCrypto!

Please confirm the iso-sim build failure is unrelated to your patch before landing.

> Source/WebCore/crypto/mac/CommonCryptoDERUtilities.h:48
> +size_t bytesNeededForEncodedLength(size_t);

I wonder if these should have been namespaced as "WebCore::crypto:: ..."

> LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/generateKey/failures_ECDSA.worker-expected.txt:397
> +PASS Bad usages: generateKey({name: ECDSA, namedCurve: P-521}, true, [sign, verify, sign, sign, verify, deriveBits]) 

Excellent!

> LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/sign_verify/ecdsa.worker-expected.txt:13
> +FAIL importVectorKeys step: ECDSA P-521 with SHA-512 verification assert_unreached: importVectorKeys failed for ECDSA P-521 with SHA-512. Message: ''Data provided to an operation does not meet requirements'' Reached unreachable code

I wish these passed. Do we have a radar to deal with this? Or a bug on W3C to fix?
Comment 5 Jiewen Tan 2017-04-16 19:24:01 PDT
Comment on attachment 307168 [details]
Patch

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

Thanks Brent for r+ my patch.

>> Source/WebCore/crypto/mac/CommonCryptoDERUtilities.h:48
>> +size_t bytesNeededForEncodedLength(size_t);
> 
> I wonder if these should have been namespaced as "WebCore::crypto:: ..."

I have filed <rdar://problem/31618371> to replace all these customized DER implementations with an Apple platforms' library. Therefore, I don't think we need a namespace for these helper functions since they are not supposed to be used outside Crypto codes and to be used anymore.

>> LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/sign_verify/ecdsa.worker-expected.txt:13
>> +FAIL importVectorKeys step: ECDSA P-521 with SHA-512 verification assert_unreached: importVectorKeys failed for ECDSA P-521 with SHA-512. Message: ''Data provided to an operation does not meet requirements'' Reached unreachable code
> 
> I wish these passed. Do we have a radar to deal with this? Or a bug on W3C to fix?

We have a bug to keep track the addition of P-521: Bug 169231. We should support P-521 if we could, but it is not an urgent need.
Comment 6 Jiewen Tan 2017-04-16 23:09:15 PDT
Created attachment 307255 [details]
Patch for landing
Comment 7 Build Bot 2017-04-16 23:13:00 PDT
Attachment 307255 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:72:  CryptoAlgorithmRSASSA_PKCS1_v1_5::sign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:81:  CryptoAlgorithmRSASSA_PKCS1_v1_5::verify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 88 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 WebKit Commit Bot 2017-04-17 02:10:16 PDT
Comment on attachment 307255 [details]
Patch for landing

Rejecting attachment 307255 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 307255, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ted/w3c/ChangeLog
fatal: multiple stage entries for merged file 'LayoutTests/imported/w3c'

Failed to run "['git', 'commit', '--all', '-F', '-']" exit_code: 128 cwd: /Volumes/Data/EWS/WebKit

You have both LayoutTests/imported/w3c and LayoutTests/imported/w3c/ChangeLog
fatal: multiple stage entries for merged file 'LayoutTests/imported/w3c'

Failed to run "['git', 'commit', '--all', '-F', '-']" exit_code: 128 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/3550135
Comment 9 Jiewen Tan 2017-04-17 13:08:09 PDT
Committed r215423: <http://trac.webkit.org/changeset/215423>