Bug 164446 - Update SubtleCrypto::importKey to match the latest spec
Summary: Update SubtleCrypto::importKey to match the latest spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 160883
  Show dependency treegraph
 
Reported: 2016-11-04 19:10 PDT by Jiewen Tan
Modified: 2016-11-10 12:26 PST (History)
6 users (show)

See Also:


Attachments
Patch (314.86 KB, patch)
2016-11-04 19:51 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (332.08 KB, patch)
2016-11-05 14:15 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (332.09 KB, patch)
2016-11-05 17:12 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-yosemite (1.90 MB, application/zip)
2016-11-05 18:24 PDT, Build Bot
no flags Details
Patch (333.82 KB, patch)
2016-11-05 18:31 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (336.29 KB, patch)
2016-11-05 18:37 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (335.58 KB, patch)
2016-11-07 15:53 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (335.58 KB, patch)
2016-11-08 12:07 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (349.86 KB, patch)
2016-11-08 22:17 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for landing (349.84 KB, patch)
2016-11-09 18:36 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for landing (354.62 KB, patch)
2016-11-09 18:50 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (349.70 KB, patch)
2016-11-09 21:02 PST, Jiewen Tan
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2016-11-04 19:10:52 PDT
Update SubtleCrypto::importKey to match the latest spec.
Comment 1 Radar WebKit Bug Importer 2016-11-04 19:11:40 PDT
<rdar://problem/29123621>
Comment 2 Jiewen Tan 2016-11-04 19:51:21 PDT
Created attachment 293976 [details]
Patch
Comment 3 Jiewen Tan 2016-11-04 19:52:31 PDT
Comment on attachment 293976 [details]
Patch

The majority of this patch is test cases. Sorry for making a huge patch.
Comment 4 Jiewen Tan 2016-11-05 14:15:04 PDT
Created attachment 294005 [details]
Patch
Comment 5 WebKit Commit Bot 2016-11-05 14:18:12 PDT
Attachment 294005 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:95:  CryptoAlgorithmRSA_OAEP::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::checkUsages is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:91:  CryptoAlgorithmAES_KW::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/JsonWebKey.h:40:  key_ops 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:95:  CryptoAlgorithmRSASSA_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:91:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::checkUsages is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:92:  CryptoAlgorithmAES_CBC::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 8 in 144 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Jiewen Tan 2016-11-05 17:12:51 PDT
Created attachment 294008 [details]
Patch
Comment 7 WebKit Commit Bot 2016-11-05 17:17:02 PDT
Attachment 294008 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:95:  CryptoAlgorithmRSA_OAEP::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::checkUsages is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:91:  CryptoAlgorithmAES_KW::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/JsonWebKey.h:40:  key_ops 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:95:  CryptoAlgorithmRSASSA_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:91:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::checkUsages is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:92:  CryptoAlgorithmAES_CBC::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 8 in 144 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Build Bot 2016-11-05 18:24:18 PDT
Comment on attachment 294008 [details]
Patch

Attachment 294008 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2466915

New failing tests:
imported/w3c/WebCryptoAPI/idlharness.html
crypto/subtle/import-key-malformed-parameters.html
Comment 9 Build Bot 2016-11-05 18:24:21 PDT
Created attachment 294009 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 10 Jiewen Tan 2016-11-05 18:31:25 PDT
Created attachment 294010 [details]
Patch
Comment 11 WebKit Commit Bot 2016-11-05 18:34:07 PDT
Attachment 294010 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:95:  CryptoAlgorithmRSA_OAEP::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::checkUsages is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:91:  CryptoAlgorithmAES_KW::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/JsonWebKey.h:40:  key_ops 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:95:  CryptoAlgorithmRSASSA_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:91:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::checkUsages is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:92:  CryptoAlgorithmAES_CBC::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 8 in 147 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Jiewen Tan 2016-11-05 18:37:28 PDT
Created attachment 294011 [details]
Patch
Comment 13 WebKit Commit Bot 2016-11-05 18:40:35 PDT
Attachment 294011 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:95:  CryptoAlgorithmRSA_OAEP::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:68:  CryptoAlgorithmAES_KW::checkUsages is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:91:  CryptoAlgorithmAES_KW::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/JsonWebKey.h:40:  key_ops 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:95:  CryptoAlgorithmRSASSA_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:91:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:69:  CryptoAlgorithmAES_CBC::checkUsages is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:92:  CryptoAlgorithmAES_CBC::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 8 in 149 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Brent Fulgham 2016-11-07 11:59:51 PST
Comment on attachment 294011 [details]
Patch

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

I think this looks good, but there is still a bit of clean-up needed. Could you consider incorporating some of my comments, especially since the tree is currently closed?

> Source/WebCore/ChangeLog:24
> +           https://bugs.webkit.org/show_bug.cgi?id=151308.

This is a lot for a single bug! Would it make any sense to break this into smaller bits of work?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:71
> +    return !(usages & (CryptoKeyUsageSign | CryptoKeyUsageVerify | CryptoKeyUsageDeriveKey | CryptoKeyUsageDeriveBits));

This could perhaps just be a local static function. It doesn't appear to be needed outside of this implementation file.

Also, a clearer name might be "invalidValidUseForAlgorithm()" or something similar.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:78
> +    if (!checkUsages(usages)) {

Weird that you negate the return value here, and negate it in the implementation. Might be clearer to get rid of both "!" operators since they should cancel out, right?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:94
> +    if (!checkUsages(usages)) {

ditto the negation

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:107
> +            case 128:

Magic number. Might be clearer to make a const called "aes128KeyLength" or something?

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:106
> +            case 128:

Magic numbers

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:106
> +            isUsagesAllowed = isUsagesAllowed || !(usages ^ CryptoKeyUsageDecrypt);

Perhaps use "||=" here?

> Source/WebCore/crypto/keys/CryptoKeyAES.cpp:67
> +inline bool CryptoKeyAES::checkLength(size_t length)

A better name might be "doesHaveValidKeyLength"

> Source/WebCore/crypto/keys/CryptoKeyAES.cpp:69
> +    return (length == 128) || (length == 192) || (length == 256);

It's not clear what these magic numbers refer to. Can you give them useful names?

(length == aes128KeyLength) || (length == aes192KeyLength) || (length == aes256KeyLength)

> Source/WebCore/crypto/keys/CryptoKeyAES.cpp:81
> +    if (!checkLength(keyData.size() * 8))

Why are we multiplying by eight here (and elsewhere). Is that the same of each key block? Maybe it should be given a label we can use everywhere?

> Source/WebCore/crypto/keys/CryptoKeyRSA.cpp:95
> +    for (size_t i = 0; i < keyData.oth.value().size(); ++i) {

Can we not iterate over "keyData.oth.value()" itself?

for (auto value | keyData.oth.value()) ...

?

> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaHashedImportParams.h:37
> +    // FIXME: Consider merging hash and hashIdentifier.

Can you open a Bugzilla bug to merge these two? Then you can get rid of the comments.
Comment 15 Jiewen Tan 2016-11-07 14:41:35 PST
Comment on attachment 294011 [details]
Patch

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

First of all, thanks Brent for reviewing my patch.

>> Source/WebCore/ChangeLog:24
>> +           https://bugs.webkit.org/show_bug.cgi?id=151308.
> 
> This is a lot for a single bug! Would it make any sense to break this into smaller bits of work?

It will be hard to split this patch into multiple ones as 1) we can't test 1 if 2 is not achieved, 2) we don't know which parts of code should be shared among all the other algorithms if we only implement one of them, 3) so minor that itself doesn't worth to be a single patch, 4) and 5) are needed for achieving 2), 6) is just some direct results of implementing 1) and 2) correctly.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:71
>> +    return !(usages & (CryptoKeyUsageSign | CryptoKeyUsageVerify | CryptoKeyUsageDeriveKey | CryptoKeyUsageDeriveBits));
> 
> This could perhaps just be a local static function. It doesn't appear to be needed outside of this implementation file.
> 
> Also, a clearer name might be "invalidValidUseForAlgorithm()" or something similar.

Fixed. Use checkInvalidUsagesForCryptoAlgorithmAES_CBC instead.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:78
>> +    if (!checkUsages(usages)) {
> 
> Weird that you negate the return value here, and negate it in the implementation. Might be clearer to get rid of both "!" operators since they should cancel out, right?

Fixed. Sure.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:94
>> +    if (!checkUsages(usages)) {
> 
> ditto the negation

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:107
>> +            case 128:
> 
> Magic number. Might be clearer to make a const called "aes128KeyLength" or something?

Fixed. Use CryptoKeyAES::s_keyLength128 instead.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:106
>> +            case 128:
> 
> Magic numbers

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:106
>> +            isUsagesAllowed = isUsagesAllowed || !(usages ^ CryptoKeyUsageDecrypt);
> 
> Perhaps use "||=" here?

I tried. Got a compilation error: Expected expression. Not sure why.

>> Source/WebCore/crypto/keys/CryptoKeyAES.cpp:67
>> +inline bool CryptoKeyAES::checkLength(size_t length)
> 
> A better name might be "doesHaveValidKeyLength"

Fixed. Use validateLengthForCryptoKeyAES instead.

>> Source/WebCore/crypto/keys/CryptoKeyAES.cpp:69
>> +    return (length == 128) || (length == 192) || (length == 256);
> 
> It's not clear what these magic numbers refer to. Can you give them useful names?
> 
> (length == aes128KeyLength) || (length == aes192KeyLength) || (length == aes256KeyLength)

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyAES.cpp:81
>> +    if (!checkLength(keyData.size() * 8))
> 
> Why are we multiplying by eight here (and elsewhere). Is that the same of each key block? Maybe it should be given a label we can use everywhere?

In the spec/protocol, all length are in bits while the passed vector has uint8_t(byte) elements. Therefore, I have to multiply 8. Not sure if 8 should be a magic number.

>> Source/WebCore/crypto/keys/CryptoKeyRSA.cpp:95
>> +    for (size_t i = 0; i < keyData.oth.value().size(); ++i) {
> 
> Can we not iterate over "keyData.oth.value()" itself?
> 
> for (auto value | keyData.oth.value()) ...
> 
> ?

Fixed.

>> Source/WebCore/crypto/parameters/CryptoAlgorithmRsaHashedImportParams.h:37
>> +    // FIXME: Consider merging hash and hashIdentifier.
> 
> Can you open a Bugzilla bug to merge these two? Then you can get rid of the comments.

Sure. I am not sure what could be a proper title for the new Bugzilla bug as this seems to be a limitation of our current binding generator. Some of the solutions could be use 1) constructors for initializing dictionaries, or 2) customized setter/getter for dictionary members. I will discuss with Chris to file an appropriate bug.
Comment 16 Jiewen Tan 2016-11-07 15:53:35 PST
Created attachment 294095 [details]
Patch
Comment 17 WebKit Commit Bot 2016-11-07 15:57:48 PST
Attachment 294095 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:95:  CryptoAlgorithmRSA_OAEP::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:41:  checkInvalidUsagesForCryptoAlgorithmAES_KW is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:91:  CryptoAlgorithmAES_KW::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/JsonWebKey.h:40:  key_ops 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:95:  CryptoAlgorithmRSASSA_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:91:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:42:  checkInvalidUsagesForCryptoAlgorithmAES_CBC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:92:  CryptoAlgorithmAES_CBC::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 8 in 149 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Brent Fulgham 2016-11-08 11:00:43 PST
Comment on attachment 294095 [details]
Patch

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

I had a few suggestions to make this code a little clearer. Since we're still blocked from check-in, let's tidy it up one last time! :-)

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:42
> +inline bool checkInvalidUsagesForCryptoAlgorithmAES_CBC(CryptoKeyUsage usages)

I recommend renaming to 'usagesAreInvalidForCryptoAlgorithmAES_CBC'

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:78
> +    if (checkInvalidUsagesForCryptoAlgorithmAES_CBC(usages)) {

I think this would read more clearly with a name like:

if (usagesAreInvalidForCryptoAlgorithmAES_CBC(usages))

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:94
> +    if (checkInvalidUsagesForCryptoAlgorithmAES_CBC(usages)) {

usagesAreInvalidForCryptoAlgorithmAES_CBC

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:41
> +inline bool checkInvalidUsagesForCryptoAlgorithmAES_KW(CryptoKeyUsage usages)

I recommend usagesAreInvalidForCryptoAlgorithmAES_KW

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:77
> +    if (checkInvalidUsagesForCryptoAlgorithmAES_KW(usages)) {

usagesAreInvalidForCryptoAlgorithmAES_KW

> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:93
> +    if (checkInvalidUsagesForCryptoAlgorithmAES_KW(usages)) {

usagesAreInvalidForCryptoAlgorithmAES_KW

> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:42
> +inline bool checkInvalidUsagesForCryptoAlgorithmHMAC(CryptoKeyUsage usages)

I recommend usagesAreInvalidForCryptoAlgorithmHMAC

> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:80
> +    if (checkInvalidUsagesForCryptoAlgorithmHMAC(usages)) {

if (usagesAreInvalidForCryptoAlgorithmHMAC(usages))

> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:103
> +    if (checkInvalidUsagesForCryptoAlgorithmHMAC(usages)) {

usagesAreInvalidForCryptoAlgorithmHMAC

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:97
> +        if ((key.d && (usages ^ CryptoKeyUsageDecrypt)) || (!key.d && (usages ^ CryptoKeyUsageEncrypt))) {

I think this would be clearer with a helper function: usagesAreInvalidForCryptoAlgorithmRSAES_PKCS1_v1_5() that checks both key.d and usages.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:108
> +        }

These two clauses could perhaps be combined into a helper function:

if (usagesAndKeyAreInvalidForCryptoAlgorithmRSAES_PKCS1_v1_5() that checks both

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:104
> +        if ((key.d && (usages ^ CryptoKeyUsageSign)) || (!key.d && (usages ^ CryptoKeyUsageVerify))) {

I think this could be clearer as usagesAreInvalidForCryptoAlgorithmRSASSA_PKCS1_v1_5() that checks both key.d and usages.

> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:114
> +        if (!isUsagesAllowed) {

I think the above would be clearer as a helper function:  usagesAreInvalidForCryptoAlgorithmRSA_OAEP() that does this logic and just returns a bool.

> Source/WebCore/crypto/keys/CryptoKeyAES.cpp:39
> +inline bool validateLengthForCryptoKeyAES(size_t length)

A better name would be "lengthIsValid"
Comment 19 Jiewen Tan 2016-11-08 11:54:40 PST
Comment on attachment 294095 [details]
Patch

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

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:42
>> +inline bool checkInvalidUsagesForCryptoAlgorithmAES_CBC(CryptoKeyUsage usages)
> 
> I recommend renaming to 'usagesAreInvalidForCryptoAlgorithmAES_CBC'

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:78
>> +    if (checkInvalidUsagesForCryptoAlgorithmAES_CBC(usages)) {
> 
> I think this would read more clearly with a name like:
> 
> if (usagesAreInvalidForCryptoAlgorithmAES_CBC(usages))

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:94
>> +    if (checkInvalidUsagesForCryptoAlgorithmAES_CBC(usages)) {
> 
> usagesAreInvalidForCryptoAlgorithmAES_CBC

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:41
>> +inline bool checkInvalidUsagesForCryptoAlgorithmAES_KW(CryptoKeyUsage usages)
> 
> I recommend usagesAreInvalidForCryptoAlgorithmAES_KW

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:77
>> +    if (checkInvalidUsagesForCryptoAlgorithmAES_KW(usages)) {
> 
> usagesAreInvalidForCryptoAlgorithmAES_KW

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:93
>> +    if (checkInvalidUsagesForCryptoAlgorithmAES_KW(usages)) {
> 
> usagesAreInvalidForCryptoAlgorithmAES_KW

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:42
>> +inline bool checkInvalidUsagesForCryptoAlgorithmHMAC(CryptoKeyUsage usages)
> 
> I recommend usagesAreInvalidForCryptoAlgorithmHMAC

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:80
>> +    if (checkInvalidUsagesForCryptoAlgorithmHMAC(usages)) {
> 
> if (usagesAreInvalidForCryptoAlgorithmHMAC(usages))

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp:103
>> +    if (checkInvalidUsagesForCryptoAlgorithmHMAC(usages)) {
> 
> usagesAreInvalidForCryptoAlgorithmHMAC

Fixed.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:97
>> +        if ((key.d && (usages ^ CryptoKeyUsageDecrypt)) || (!key.d && (usages ^ CryptoKeyUsageEncrypt))) {
> 
> I think this would be clearer with a helper function: usagesAreInvalidForCryptoAlgorithmRSAES_PKCS1_v1_5() that checks both key.d and usages.

I agree with you that it will be clearer. However, the reason why I don't make a helper function here is CryptoAlgorithmRSAES_PKCS1_v1_5::generateKey has a completely different usages check. Hence, a helper function seems too specific and unnecessary.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:108
>> +        }
> 
> These two clauses could perhaps be combined into a helper function:
> 
> if (usagesAndKeyAreInvalidForCryptoAlgorithmRSAES_PKCS1_v1_5() that checks both

Probably not a good idea as these two checks check different things and are separate steps in the spec.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:104
>> +        if ((key.d && (usages ^ CryptoKeyUsageSign)) || (!key.d && (usages ^ CryptoKeyUsageVerify))) {
> 
> I think this could be clearer as usagesAreInvalidForCryptoAlgorithmRSASSA_PKCS1_v1_5() that checks both key.d and usages.

Same as above.

>> Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:114
>> +        if (!isUsagesAllowed) {
> 
> I think the above would be clearer as a helper function:  usagesAreInvalidForCryptoAlgorithmRSA_OAEP() that does this logic and just returns a bool.

Same as above.

>> Source/WebCore/crypto/keys/CryptoKeyAES.cpp:39
>> +inline bool validateLengthForCryptoKeyAES(size_t length)
> 
> A better name would be "lengthIsValid"

Fixed.
Comment 20 Jiewen Tan 2016-11-08 12:07:41 PST
Created attachment 294177 [details]
Patch
Comment 21 WebKit Commit Bot 2016-11-08 12:11:43 PST
Attachment 294177 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:95:  CryptoAlgorithmRSA_OAEP::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:41:  usagesAreInvalidForCryptoAlgorithmAES_KW is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:91:  CryptoAlgorithmAES_KW::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/JsonWebKey.h:40:  key_ops 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:95:  CryptoAlgorithmRSASSA_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:91:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:42:  usagesAreInvalidForCryptoAlgorithmAES_CBC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:92:  CryptoAlgorithmAES_CBC::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 8 in 149 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Brent Fulgham 2016-11-08 12:18:06 PST
Comment on attachment 294177 [details]
Patch

Looks good!
Comment 23 Jiewen Tan 2016-11-08 12:38:39 PST
(In reply to comment #22)
> Comment on attachment 294177 [details]
> Patch
> 
> Looks good!

Thanks, Brent.
Comment 24 Jiewen Tan 2016-11-08 20:59:00 PST
Comment on attachment 294177 [details]
Patch

Just notice a very bad bug. cq- it before the next patch.
Comment 25 Jiewen Tan 2016-11-08 22:17:30 PST
Created attachment 294216 [details]
Patch
Comment 26 WebKit Commit Bot 2016-11-08 22:21:22 PST
Attachment 294216 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:95:  CryptoAlgorithmRSA_OAEP::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:41:  usagesAreInvalidForCryptoAlgorithmAES_KW is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:91:  CryptoAlgorithmAES_KW::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:42:  usagesAreInvalidForCryptoAlgorithmAES_CBC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:92:  CryptoAlgorithmAES_CBC::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/JsonWebKey.h:40:  key_ops 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:95:  CryptoAlgorithmRSASSA_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:91:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 8 in 163 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Brent Fulgham 2016-11-09 16:40:25 PST
Comment on attachment 294216 [details]
Patch

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

Looks good! A few small nits but otherwise ready to land!

> Source/WebCore/crypto/keys/CryptoKeyAES.cpp:39
> +inline bool lengthIsValid(size_t length)

These kinds of functions should be 'static' as well, if you don't want them to be accessible outside the compilation unit.

> Source/WebCore/crypto/keys/CryptoKeyHMAC.cpp:75
> +    // We currently only support key length that is a mutipler of 8, which is different from the spec:

"... that is a MULTIPLE of 8"

> Source/WebCore/crypto/keys/CryptoKeyHMAC.cpp:77
> +    if (lengthBits % 8)

Do we need to provide any error feedback that the generate failed due to key length?

> Source/WebCore/crypto/parameters/CryptoAlgorithmHmacKeyGenParams.h:-49
> -#endif // ENABLE(SUBTLE_CRYPTO)

Was this handled as a "svn move CryptoAlgorithmHmacKeyGenParams.h CryptoAlgorithmHmacKeyParams.h"? You should do so if you can, because it will keep the history of this file intact.

> Source/WebCore/crypto/parameters/HmacKeyGenParams.idl:-36
> -};

Was this handled as a "svn move HmacKeyGenParams.idl HmacKeyParams.idl"? You should do so if you can, because it will keep the history of this file intact.
Comment 28 Jiewen Tan 2016-11-09 16:55:24 PST
Comment on attachment 294216 [details]
Patch

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

Thanks, Brent.

>> Source/WebCore/crypto/keys/CryptoKeyAES.cpp:39
>> +inline bool lengthIsValid(size_t length)
> 
> These kinds of functions should be 'static' as well, if you don't want them to be accessible outside the compilation unit.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyHMAC.cpp:75
>> +    // We currently only support key length that is a mutipler of 8, which is different from the spec:
> 
> "... that is a MULTIPLE of 8"

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyHMAC.cpp:77
>> +    if (lengthBits % 8)
> 
> Do we need to provide any error feedback that the generate failed due to key length?

I was actually thinking about providing better feedback. I would like to do it as a followup patch since the spec doesn't require it.

>> Source/WebCore/crypto/parameters/CryptoAlgorithmHmacKeyGenParams.h:-49
>> -#endif // ENABLE(SUBTLE_CRYPTO)
> 
> Was this handled as a "svn move CryptoAlgorithmHmacKeyGenParams.h CryptoAlgorithmHmacKeyParams.h"? You should do so if you can, because it will keep the history of this file intact.

I directly renamed it in Xcode. Xcode does keep the history intact. Awesome, Xcode!

>> Source/WebCore/crypto/parameters/HmacKeyGenParams.idl:-36
>> -};
> 
> Was this handled as a "svn move HmacKeyGenParams.idl HmacKeyParams.idl"? You should do so if you can, because it will keep the history of this file intact.

Ditto.
Comment 29 Jiewen Tan 2016-11-09 18:36:05 PST
Created attachment 294320 [details]
Patch for landing
Comment 30 Jiewen Tan 2016-11-09 18:50:07 PST
Created attachment 294322 [details]
Patch for landing
Comment 31 WebKit Commit Bot 2016-11-09 18:53:59 PST
Attachment 294322 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:95:  CryptoAlgorithmRSA_OAEP::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:41:  usagesAreInvalidForCryptoAlgorithmAES_KW is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:91:  CryptoAlgorithmAES_KW::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:42:  usagesAreInvalidForCryptoAlgorithmAES_CBC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:92:  CryptoAlgorithmAES_CBC::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/JsonWebKey.h:40:  key_ops 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:95:  CryptoAlgorithmRSASSA_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:91:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 8 in 163 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 Jiewen Tan 2016-11-09 21:02:50 PST
Created attachment 294331 [details]
Patch
Comment 33 WebKit Commit Bot 2016-11-09 21:05:47 PST
Attachment 294331 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:95:  CryptoAlgorithmRSA_OAEP::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:41:  usagesAreInvalidForCryptoAlgorithmAES_KW is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:91:  CryptoAlgorithmAES_KW::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:42:  usagesAreInvalidForCryptoAlgorithmAES_CBC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:92:  CryptoAlgorithmAES_CBC::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/JsonWebKey.h:40:  key_ops 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:95:  CryptoAlgorithmRSASSA_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:91:  CryptoAlgorithmRSAES_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 8 in 163 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 34 Brent Fulgham 2016-11-10 10:27:12 PST
Comment on attachment 294331 [details]
Patch

r=me.
Comment 35 Jiewen Tan 2016-11-10 10:39:58 PST
Committed r208548: <http://trac.webkit.org/changeset/208548>