Bug 164446 – Update SubtleCrypto::importKey to match the latest spec

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

[Jiewen Tan]

Update SubtleCrypto::importKey to match the latest spec.

[Radar WebKit Bug Importer]

<rdar://problem/29123621>

[Jiewen Tan]

Created attachment 293976 [details]
Patch

[Jiewen Tan]

Comment on attachment 293976 [details]
Patch

The majority of this patch is test cases. Sorry for making a huge patch.

[Jiewen Tan]

Created attachment 294005 [details]
Patch

[WebKit Commit Bot]

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.

[Jiewen Tan]

Created attachment 294008 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Jiewen Tan]

Created attachment 294010 [details]
Patch

[WebKit Commit Bot]

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.

[Jiewen Tan]

Created attachment 294011 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

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.

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 294095 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

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"

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 294177 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

Comment on attachment 294177 [details]
Patch

Looks good!

[Jiewen Tan]

(In reply to comment #22)
> Comment on attachment 294177 [details]
> Patch
> 
> Looks good!

Thanks, Brent.

[Jiewen Tan]

Comment on attachment 294177 [details]
Patch

Just notice a very bad bug. cq- it before the next patch.

[Jiewen Tan]

Created attachment 294216 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

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.

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 294320 [details]
Patch for landing

[Jiewen Tan]

Created attachment 294322 [details]
Patch for landing

[WebKit Commit Bot]

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.

[Jiewen Tan]

Created attachment 294331 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

Comment on attachment 294331 [details]
Patch

r=me.

[Jiewen Tan]

Committed r208548: <http://trac.webkit.org/changeset/208548>