Bug 173696 - [GCrypt] Implement CryptoKeyRSA PKCS#8 imports
Summary: [GCrypt] Implement CryptoKeyRSA PKCS#8 imports
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on: 173694
Blocks: 133122
  Show dependency treegraph
 
Reported: 2017-06-21 23:19 PDT by Zan Dobersek
Modified: 2017-07-14 22:20 PDT (History)
3 users (show)

See Also:


Attachments
WIP patch (11.76 KB, patch)
2017-06-21 23:57 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (13.85 KB, patch)
2017-06-28 09:27 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (11.07 KB, patch)
2017-07-13 03:17 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (1.72 MB, application/zip)
2017-07-13 07:02 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-06-21 23:19:06 PDT
SSIA.
Comment 1 Zan Dobersek 2017-06-21 23:57:18 PDT
Created attachment 313589 [details]
WIP patch
Comment 2 Build Bot 2017-06-22 00:17:15 PDT
Attachment 313589 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Zan Dobersek 2017-06-28 09:27:21 PDT
Created attachment 314029 [details]
Patch
Comment 4 Jiewen Tan 2017-07-12 10:47:45 PDT
Comment on attachment 314029 [details]
Patch

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

Looks good to me. r- because of the duplicated function.

> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:244
> +static bool supportedAlgorithmIdentifier(const uint8_t* data, size_t size)

I believe this part is duplicated with Bug 173694. Please upload another patch after that bug is resolved.

> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:297
> +        if (version->size() != 1 || version->at(0) != 0x00)

See comments on Bug 173647.

> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:328
> +        if (version->size() != 1 || version->at(0) != 0x00)

Ditto.
Comment 5 Zan Dobersek 2017-07-13 02:54:51 PDT
Comment on attachment 314029 [details]
Patch

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

>> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:244
>> +static bool supportedAlgorithmIdentifier(const uint8_t* data, size_t size)
> 
> I believe this part is duplicated with Bug 173694. Please upload another patch after that bug is resolved.

This, and the duplication in bug #173697, was intentional to get the patch building on the EWS bots. I'll upload new patches in both cases.
Comment 6 Zan Dobersek 2017-07-13 03:17:56 PDT
Created attachment 315339 [details]
Patch
Comment 7 Build Bot 2017-07-13 07:02:40 PDT
Comment on attachment 315339 [details]
Patch

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

New failing tests:
storage/indexeddb/modern/new-database-after-user-delete.html
Comment 8 Build Bot 2017-07-13 07:02:41 PDT
Created attachment 315344 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Jiewen Tan 2017-07-14 08:43:29 PDT
Comment on attachment 315339 [details]
Patch

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

Looks good to me. r=me. Please address my comments in a followup patch.

> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:337
> +        if (version->size() != 1 || version->at(0) != 0x00)

Please do a follow up patch to replace the 0x00 with a constant in all the places. Using memcmp here is more ideal.

> Source/WebCore/crypto/gcrypt/CryptoKeyRSAGCrypt.cpp:368
> +        if (version->size() != 1 || version->at(0) != 0x00)

Ditto.
Comment 10 Zan Dobersek 2017-07-14 22:20:35 PDT
Comment on attachment 315339 [details]
Patch

Clearing flags on attachment: 315339

Committed r219535: <http://trac.webkit.org/changeset/219535>
Comment 11 Zan Dobersek 2017-07-14 22:20:39 PDT
All reviewed patches have been landed.  Closing bug.