Bug 175657 - [WebCrypto][Mac] Replace CCECCryptorGetKeyComponents with CCECCryptorExportKey for exporting ECC JWKs
Summary: [WebCrypto][Mac] Replace CCECCryptorGetKeyComponents with CCECCryptorExportKe...
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:
 
Reported: 2017-08-16 21:49 PDT by Jiewen Tan
Modified: 2018-05-24 16:42 PDT (History)
5 users (show)

See Also:


Attachments
Patch (18.79 KB, patch)
2017-08-17 12:41 PDT, Jiewen Tan
bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (18.79 KB, patch)
2017-08-18 12:32 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2017-08-16 21:49:28 PDT
For unknown reasons, CCECCryptorGetKeyComponents sometimes output wrong values. Hence, we need to replace it with CCECCryptorGetKeyComponents for reliable outputs.
Comment 1 Jiewen Tan 2017-08-16 21:49:49 PDT
<rdar://problem/33797150>
Comment 2 Jiewen Tan 2017-08-17 12:41:23 PDT
Created attachment 318401 [details]
Patch
Comment 3 Brent Fulgham 2017-08-18 09:14:42 PDT
Comment on attachment 318401 [details]
Patch

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

I think this looks good, but I would prefer you revise the return code as I outlined above. r=me assuming you make the changes I suggest.

> Source/WebCore/crypto/keys/CryptoKeyEC.cpp:149
> +    return Exception { OperationError };

I think this would be clearer if you did the early return for the failure case, rather than success.

I also think this would be clearer (use 'isEmpty()' rather than a zero-check for size())

if (result.isEmpty())
    return Exception { OperationError };

return WTFMove(result);

> Source/WebCore/crypto/keys/CryptoKeyEC.cpp:168
> +    return Exception { OperationError };

Ditto the early-return case.

> Source/WebCore/crypto/keys/CryptoKeyEC.cpp:179
> +    return Exception { OperationError };

Ditto.

> Source/WebCore/crypto/keys/CryptoKeyEC.cpp:190
> +    return Exception { OperationError };

Ditto.
Comment 4 Jiewen Tan 2017-08-18 12:28:43 PDT
Comment on attachment 318401 [details]
Patch

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

Thanks Brent for r+ my patch.

>> Source/WebCore/crypto/keys/CryptoKeyEC.cpp:149
>> +    return Exception { OperationError };
> 
> I think this would be clearer if you did the early return for the failure case, rather than success.
> 
> I also think this would be clearer (use 'isEmpty()' rather than a zero-check for size())
> 
> if (result.isEmpty())
>     return Exception { OperationError };
> 
> return WTFMove(result);

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyEC.cpp:168
>> +    return Exception { OperationError };
> 
> Ditto the early-return case.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyEC.cpp:179
>> +    return Exception { OperationError };
> 
> Ditto.

Fixed.

>> Source/WebCore/crypto/keys/CryptoKeyEC.cpp:190
>> +    return Exception { OperationError };
> 
> Ditto.

Fixed.
Comment 5 Jiewen Tan 2017-08-18 12:32:58 PDT
Created attachment 318528 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2017-08-18 14:30:49 PDT
Comment on attachment 318528 [details]
Patch for landing

Clearing flags on attachment: 318528

Committed r220933: <http://trac.webkit.org/changeset/220933>