Bug 124236 – Implement key generation and JWK import for RSASSA-PKCS1-v1_5

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

[Alexey Proskuryakov]

Implement key generation and JWK import for RSASSA-PKCS1-v1_5.

[Alexey Proskuryakov]

Created attachment 216736 [details]
proposed patch

[WebKit Commit Bot]

Attachment 216736 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/crypto/subtle/resources/common.js', u'LayoutTests/crypto/subtle/rsassa-pkcs1-v1_5-generate-key-expected.txt', u'LayoutTests/crypto/subtle/rsassa-pkcs1-v1_5-generate-key.html', u'LayoutTests/crypto/subtle/rsassa-pkcs1-v1_5-import-jwk-expected.txt', u'LayoutTests/crypto/subtle/rsassa-pkcs1-v1_5-import-jwk.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSCryptoAlgorithmBuilder.cpp', u'Source/WebCore/bindings/js/JSCryptoAlgorithmBuilder.h', u'Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp', u'Source/WebCore/bindings/js/JSCryptoKeySerializationJWK.cpp', u'Source/WebCore/bindings/js/JSCryptoKeySerializationJWK.h', u'Source/WebCore/crypto/CryptoAlgorithmDescriptionBuilder.h', u'Source/WebCore/crypto/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp', u'Source/WebCore/crypto/CryptoKey.h', u'Source/WebCore/crypto/CryptoKeyData.h', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmHMAC.cpp', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp', u'Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.h', u'Source/WebCore/crypto/keys/CryptoKeyAES.h', u'Source/WebCore/crypto/keys/CryptoKeyDataOctetSequence.h', u'Source/WebCore/crypto/keys/CryptoKeyDataRSAComponents.cpp', u'Source/WebCore/crypto/keys/CryptoKeyDataRSAComponents.h', u'Source/WebCore/crypto/keys/CryptoKeyHMAC.h', u'Source/WebCore/crypto/keys/CryptoKeyRSA.h', u'Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp', u'Source/WebCore/crypto/mac/CryptoAlgorithmHMACMac.cpp', u'Source/WebCore/crypto/mac/CryptoAlgorithmRegistryMac.cpp', u'Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp', u'Source/WebCore/crypto/parameters/CryptoAlgorithmHmacKeyParams.h', u'Source/WebCore/crypto/parameters/CryptoAlgorithmRsaKeyGenParams.h', u'Source/WebCore/crypto/parameters/CryptoAlgorithmRsaSsaKeyParams.h', u'Source/WebCore/crypto/parameters/CryptoAlgorithmRsaSsaParams.h']" exit_code: 1
Source/WebCore/crypto/keys/CryptoKeyRSA.h:50:  The parameter name "identifier" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/crypto/keys/CryptoKeyRSA.h:50:  The parameter name "keyData" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/crypto/keys/CryptoKeyRSA.h:50:  The parameter name "usage" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.h:45:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:39:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:46:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:47:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:170:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:176:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:185:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/bindings/js/JSCryptoKeySerializationJWK.cpp:118:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:39:  CryptoAlgorithmRSASSA_PKCS1_v1_5::s_name is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:49:  CryptoAlgorithmRSASSA_PKCS1_v1_5::create is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:54:  CryptoAlgorithmRSASSA_PKCS1_v1_5::identifier is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:59:  CryptoAlgorithmRSASSA_PKCS1_v1_5::importKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:76:  CryptoAlgorithmRSASSA_PKCS1_v1_5::generateKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/crypto/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:35:  CryptoAlgorithmRSASSA_PKCS1_v1_5::sign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/crypto/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:40:  CryptoAlgorithmRSASSA_PKCS1_v1_5::verify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/crypto/CryptoKey.h:44:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/crypto/CryptoKey.h:45:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 21 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 216736 [details]
proposed patch

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

> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:231
> +    RefPtr<Uint8Array> publicExponentArray = toUint8Array(publicExponentValue);

Do you need to type check this?

> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:241
> +    auto result = std::make_unique<CryptoAlgorithmRsaSsaKeyParams>();
> +    return std::move(result);

One line?

> Source/WebCore/bindings/js/JSCryptoKeySerializationJWK.cpp:331
> +        return CryptoKeyDataRSAComponents::createPrivate(modulus, exponent, privateExponent);

This part isn't right, as you told me.

> Source/WebCore/crypto/keys/CryptoKeyAES.h:66
> +    RELEASE_ASSERT(isCryptoKeyAES(key));

This should use ASSERT_WITH_SECURITY_IMPLICATION

> Source/WebCore/crypto/keys/CryptoKeyAES.h:72
> +    RELEASE_ASSERT(isCryptoKeyAES(key));

As should this.

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:79
> +    RELEASE_ASSERT(isCryptoKeyRSA(key));

ASSERT_WITH_SECURITY_IMPLICATION

> Source/WebCore/crypto/keys/CryptoKeyRSA.h:85
> +    RELEASE_ASSERT(isCryptoKeyRSA(key));

ASSERT_WITH_SECURITY_IMPLICATION

[Build Bot]

Comment on attachment 216736 [details]
proposed patch

Attachment 216736 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22749645

[Alexey Proskuryakov]

> This part isn't right, as you told me.

Upon further inspection, it turned out that this is correct - having all or none additional parameters is a requirement for JWK producers, not for JWK consumers. 

Committed <http://trac.webkit.org/r159160>.

[Alexey Proskuryakov]

Release build fix in <http://trac.webkit.org/r159161>.

Looking into the other failure now.

[Alexey Proskuryakov]

Another build fix in <http://trac.webkit.org/r159164>.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 124253

[Alexey Proskuryakov]

Created attachment 216778 [details]
combined fixed patch for re-landing

[WebKit Commit Bot]

Comment on attachment 216778 [details]
combined fixed patch for re-landing

Rejecting attachment 216778 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 216778, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
it_code: 29 cwd: /Volumes/Data/EWS/WebKit

Parsed 36 diffs from patch file(s).
cp: Source/WebCore/crypto/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp: No such file or directory
Failed to copy Source/WebCore/crypto/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp Source/WebCore/crypto/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp. at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 436.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 29 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/22719840

[Alexey Proskuryakov]

Created attachment 216780 [details]
patch for landing

Fixed format for svn-apply to work.

[WebKit Commit Bot]

Comment on attachment 216780 [details]
patch for landing

Clearing flags on attachment: 216780

Committed r159180: <http://trac.webkit.org/changeset/159180>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

Not sure the tests are working: http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK2%20(Tests)/r159181%20(658)/results.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 124280

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/159180>.

[Alexey Proskuryakov]

Oops, I thought that I forgot to close the bug yesterday, but it's opened again.

[Alexey Proskuryakov]

OK, so this wasn't rolled out, just a test was skipped. Marking resolved/fixed again.