Bug 175657

Summary: [WebCrypto][Mac] Replace CCECCryptorGetKeyComponents with CCECCryptorExportKey for exporting ECC JWKs
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, jiewen_tan, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
bfulgham: review+, bfulgham: commit-queue-
Patch for landing none

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>